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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a44d8a62-8753-4efb-8c3a-f9c3cdc1dabc@arm.com>
Date: Wed, 15 Jan 2025 19:20:30 +0000
From: Christian Loehle <christian.loehle@....com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>,
 Linux PM <linux-pm@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>,
 Daniel Lezcano <daniel.lezcano@...aro.org>,
 Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>
Subject: Re: [PATCH v1 2/9] cpuidle: teo: Reorder candidate state index checks

On 1/15/25 15:54, Rafael J. Wysocki wrote:
> On Wed, Jan 15, 2025 at 3:46 PM Christian Loehle
> <christian.loehle@....com> wrote:
>>
>> On 1/13/25 18:36, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@...el.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@...el.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.