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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/