lkml.org 
[lkml]   [2015]   [May]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH V3] cpuidle: Handle tick_broadcast_enter() failure gracefully
Date
On Saturday, May 09, 2015 11:19:16 AM Preeti U Murthy wrote:
> Hi Rafael,
>
> On 05/08/2015 07:48 PM, Rafael J. Wysocki wrote:
> >> +/*
> >> + * find_tick_valid_state - select a state where tick does not stop
> >> + * @dev: cpuidle device for this cpu
> >> + * @drv: cpuidle driver for this cpu
> >> + */
> >> +static int find_tick_valid_state(struct cpuidle_device *dev,
> >> + struct cpuidle_driver *drv)
> >> +{
> >> + int i, ret = -1;
> >> +
> >> + for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
> >> + struct cpuidle_state *s = &drv->states[i];
> >> + struct cpuidle_state_usage *su = &dev->states_usage[i];
> >> +
> >> + /*
> >> + * We do not explicitly check for latency requirement
> >> + * since it is safe to assume that only shallower idle
> >> + * states will have the CPUIDLE_FLAG_TIMER_STOP bit
> >> + * cleared and they will invariably meet the latency
> >> + * requirement.
> >> + */
> >> + if (s->disabled || su->disable ||
> >> + (s->flags & CPUIDLE_FLAG_TIMER_STOP))
> >> + continue;
> >> +
> >> + ret = i;
> >> + }
> >> + return ret;
> >> +}
> >> +
> >> /**
> >> * cpuidle_enter_state - enter the state and update stats
> >> * @dev: cpuidle device for this cpu
> >> @@ -168,10 +199,17 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
> >> * CPU as a broadcast timer, this call may fail if it is not available.
> >> */
> >> if (broadcast && tick_broadcast_enter()) {
> >> - default_idle_call();
> >> - return -EBUSY;
> >> + index = find_tick_valid_state(dev, drv);
> >
> > Well, the new state needs to be deeper than the old one or you may violate the
> > governor's choice and this doesn't guarantee that.
>
> The comment above in find_tick_valid_state() explains why we are bound
> to choose a shallow idle state. I think its safe to assume that any
> state deeper than this one, would have the CPUIDLE_FLAG_TIMER_STOP flag
> set and hence would be skipped.
>
> Your patch relies on the assumption that the idle states are arranged in
> the increasing order of exit_latency/in the order of shallow to deep.
> This is not guaranteed, is it?

No, it isn't, which is a good point. There's no reason to rely on that
assumption, so appended is an updated version of the patch using a latency
limit instead of an index limit.

>
> >
> > Also I don't quite see a reason to duplicate the find_deepest_state() functionality
> > here.
>
> Agreed. We could club them like in your patch.
>
> >
> >> + if (index < 0) {
> >> + default_idle_call();
> >> + return -EBUSY;
> >> + }
> >> + target_state = &drv->states[index];
> >> }
> >>
> >> + /* Take note of the planned idle state. */
> >> + idle_set_state(smp_processor_id(), target_state);
> >
> > And I wouldn't do this either.
> >
> > The behavior here is pretty much as though the driver demoted the state chosen
> > by the governor and we don't call idle_set_state() again in those cases.
>
> Why is this wrong?

Because it is inconsistent, but let me reply to this in a separate message.

Anyway, it is a different problem and should be addressed by a separate
patch IMO.




\
 
 \ /
  Last update: 2015-05-09 20:41    [W:0.079 / U:1.592 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site