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

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

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.