| Message ID | 1907276.tdWV9SEqCh@rjwysocki.net |
|---|---|
| State | New |
| Headers | show |
| Series | cpuidle: teo: Cleanups and very frequent wakeups handling update | expand |
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? > > 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;?
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 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? > >>> >>> 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.
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.
On Wed, Jan 15, 2025 at 9:48 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > 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; Or even if ((!idx || drv->states[idx].target_residency_ns < RESIDENCY_THRESHOLD_NS) && (2 * cpu_data->short_idle >= cpu_data->total || latency_req < A_SMALL_VALUE)) goto out_tick; for that matter. > but that would be a separate patch if you will.
On 1/16/25 12:22, Rafael J. Wysocki wrote: > On Wednesday, January 15, 2025 10:10:11 PM CET Rafael J. Wysocki wrote: >> On Wed, Jan 15, 2025 at 9:48 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >>> >>> 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; >> >> Or even >> >> if ((!idx || drv->states[idx].target_residency_ns < RESIDENCY_THRESHOLD_NS) && >> (2 * cpu_data->short_idle >= cpu_data->total || latency_req < >> A_SMALL_VALUE)) >> goto out_tick; >> >> for that matter. >> >>> but that would be a separate patch if you will. > > So for completeness, it would be a patch like the one below, on top of the [7/9]. > > --- > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Subject: [PATCH v1] cpuidle: teo: Skip sleep length computation for low latency constraints > > If the idle state exit latency constraint is sufficiently low, it > is better to avoid the additional latency related to calling > tick_nohz_get_sleep_length(). It is also not necessary to compute > the sleep length in that case because shallow idle state selection > will be forced then regardless of the recent wakeup history. > > Accordingly, skip the sleep length computation and subsequent > checks of the exit latency constraint is low enough. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Thank you, that makes sense. Reviewed-by: Christian Loehle <christian.loehle@arm.com> > --- > drivers/cpuidle/governors/teo.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > --- a/drivers/cpuidle/governors/teo.c > +++ b/drivers/cpuidle/governors/teo.c > @@ -106,6 +106,12 @@ > #include "gov.h" > > /* > + * Idle state exit latency threshold used for deciding whether or not to check > + * the time till the closest expected timer event. > + */ > +#define LATENCY_THRESHOLD_NS (RESIDENCY_THRESHOLD_NS / 2) > + > +/* > * The PULSE value is added to metrics when they grow and the DECAY_SHIFT value > * is used for decreasing metrics on a regular basis. > */ > @@ -432,9 +438,14 @@ > * duration falls into that range in the majority of cases, assume > * non-timer wakeups to be dominant and skip updating the sleep length > * to reduce latency. > + * > + * Also, if the latency constraint is sufficiently low, it will force > + * shallow idle states regardless of the wakeup type, so the sleep > + * length need not be known in that case. > */ > if ((!idx || drv->states[idx].target_residency_ns < RESIDENCY_THRESHOLD_NS) && > - 2 * cpu_data->short_idle >= cpu_data->total) > + (2 * cpu_data->short_idle >= cpu_data->total || > + latency_req < LATENCY_THRESHOLD_NS)) > goto out_tick; > > duration_ns = tick_nohz_get_sleep_length(&delta_tick); > > > >
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. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Fine with me now, since the posted patch addresses my concern. Reviewed-by: Christian Loehle <christian.loehle@arm.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; > - > - /* > * Skip the timers check if state 0 is the current candidate one, > * because an immediate non-timer wakeup is expected in that case. > */ > > >
--- 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. */