Re: [PATCH v1 2/9] cpuidle: teo: Reorder candidate state index checks

[Christian Loehle <christian.loehle@arm.com>]

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);
> 
> 
> 
>