| Message ID | 1907276.tdWV9SEqCh@rjwysocki.net |
|---|---|
| State | New |
| Headers | show |
| Series | cpuidle: teo: Cleanups and very frequent wakeups handling update | expand |
On Wed, Jan 15, 2025 at 3:46 PM Christian Loehle <christian.loehle@arm.com> wrote: > > On 1/13/25 18:36, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Since constraint_idx may be 0, the candidate state index may change to 0 > > after assigning constraint_idx to it, so first check if it is greater > > than constraint_idx (and update it if so) and then check it against 0. > > So the reason I've left this where it was is because the prev_intercept_idx > was supposed to query the sleep length if we're in an majority-intercept > period and then it makes sense to query the sleep length (to detect such > a period being over). > A constraint_idx == 0 scenario doesn't need the intercept-machinery to > work at all, why are we querying the sleep length then? In case the constraint is different next time and it's better to know the sleep length to properly classify the wakeup. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > > > This is a rebased variant of > > > > https://lore.kernel.org/linux-pm/8476650.T7Z3S40VBb@rjwysocki.net/ > > > > --- > > drivers/cpuidle/governors/teo.c | 15 ++++++++------- > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > --- a/drivers/cpuidle/governors/teo.c > > +++ b/drivers/cpuidle/governors/teo.c > > @@ -428,6 +428,14 @@ > > break; > > } > > } > > + > > + /* > > + * If there is a latency constraint, it may be necessary to select an > > + * idle state shallower than the current candidate one. > > + */ > > + if (idx > constraint_idx) > > + idx = constraint_idx; > > + > > if (!idx && prev_intercept_idx) { > > /* > > * We have to query the sleep length here otherwise we don't > > @@ -439,13 +447,6 @@ > > } > > > > /* > > - * If there is a latency constraint, it may be necessary to select an > > - * idle state shallower than the current candidate one. > > - */ > > - if (idx > constraint_idx) > > - idx = constraint_idx; > > - > > - /* > > We could leave this here and just do goto end;? Why would this be better?
On Wed, Jan 15, 2025 at 8:20 PM Christian Loehle <christian.loehle@arm.com> wrote: > > On 1/15/25 15:54, Rafael J. Wysocki wrote: > > On Wed, Jan 15, 2025 at 3:46 PM Christian Loehle > > <christian.loehle@arm.com> wrote: > >> > >> On 1/13/25 18:36, Rafael J. Wysocki wrote: > >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>> > >>> Since constraint_idx may be 0, the candidate state index may change to 0 > >>> after assigning constraint_idx to it, so first check if it is greater > >>> than constraint_idx (and update it if so) and then check it against 0. > >> > >> So the reason I've left this where it was is because the prev_intercept_idx > >> was supposed to query the sleep length if we're in an majority-intercept > >> period and then it makes sense to query the sleep length (to detect such > >> a period being over). > >> A constraint_idx == 0 scenario doesn't need the intercept-machinery to > >> work at all, why are we querying the sleep length then? > > > > In case the constraint is different next time and it's better to know > > the sleep length to properly classify the wakeup. > > I would hope constraints change nowhere near as frequently as > idle entry / exit happen, is your experience different? They don't, but they may change at any time and it is kind of good to have history in case this happens. > > > >>> > >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>> --- > >>> > >>> This is a rebased variant of > >>> > >>> https://lore.kernel.org/linux-pm/8476650.T7Z3S40VBb@rjwysocki.net/ > >>> > >>> --- > >>> drivers/cpuidle/governors/teo.c | 15 ++++++++------- > >>> 1 file changed, 8 insertions(+), 7 deletions(-) > >>> > >>> --- a/drivers/cpuidle/governors/teo.c > >>> +++ b/drivers/cpuidle/governors/teo.c > >>> @@ -428,6 +428,14 @@ > >>> break; > >>> } > >>> } > >>> + > >>> + /* > >>> + * If there is a latency constraint, it may be necessary to select an > >>> + * idle state shallower than the current candidate one. > >>> + */ > >>> + if (idx > constraint_idx) > >>> + idx = constraint_idx; > >>> + > >>> if (!idx && prev_intercept_idx) { > >>> /* > >>> * We have to query the sleep length here otherwise we don't > >>> @@ -439,13 +447,6 @@ > >>> } > >>> > >>> /* > >>> - * If there is a latency constraint, it may be necessary to select an > >>> - * idle state shallower than the current candidate one. > >>> - */ > >>> - if (idx > constraint_idx) > >>> - idx = constraint_idx; > >>> - > >>> - /* > >> > >> We could leave this here and just do goto end;? > > > > Why would this be better? > > Saves querying the sleep length in case of constraint_idx == 0, i.e. > qos request to be very latency-sensitive and us actually adding latency > here. Fair enough, but before patch [7/9] leaving it where it is doesn't really cause it to skip the sleep length check unless state 0 is "polling". After patch [7/9] it is possible to add a constraint_idx check against 0 to the "goto out_tick" condition before the tick_nohz_get_sleep_length() call, that is if ((!idx || drv->states[idx].target_residency_ns < RESIDENCY_THRESHOLD_NS) && (2 * cpu_data->short_idle >= cpu_data->total || !constraint_idx)) goto out_tick; but that would be a separate patch if you will.
--- a/drivers/cpuidle/governors/teo.c +++ b/drivers/cpuidle/governors/teo.c @@ -428,6 +428,14 @@ break; } } + + /* + * If there is a latency constraint, it may be necessary to select an + * idle state shallower than the current candidate one. + */ + if (idx > constraint_idx) + idx = constraint_idx; + if (!idx && prev_intercept_idx) { /* * We have to query the sleep length here otherwise we don't @@ -439,13 +447,6 @@ } /* - * If there is a latency constraint, it may be necessary to select an - * idle state shallower than the current candidate one. - */ - if (idx > constraint_idx) - idx = constraint_idx; - - /* * Skip the timers check if state 0 is the current candidate one, * because an immediate non-timer wakeup is expected in that case. */