linux-kernel - Re: [PATCH 0/1] cpuidle/menu: Address performance drop from favoring physical over polling cpuidle state

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] [day] [month] [year] [list]

Message-ID: <a91881c5-ec98-41fb-8742-661d1ee001d4@arm.com>
Date: Mon, 21 Oct 2024 10:03:22 +0100
From: Christian Loehle <christian.loehle@....com>
To: Aboorva Devarajan <aboorvad@...ux.ibm.com>, rafael@...nel.org,
 daniel.lezcano@...aro.org, linux-pm@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: gautam@...ux.ibm.com
Subject: Re: [PATCH 0/1] cpuidle/menu: Address performance drop from favoring
 physical over polling cpuidle state

On 10/21/24 09:02, Aboorva Devarajan wrote:
> On Thu, 2024-09-19 at 15:38 +0100, Christian Loehle wrote:
>> [...]
>> And of course I realise right after sending I did indeed make a mistake.
>> RESIDENCY_THRESHOLD_NS is 15us so timer is actually polled and the fields
>> populated accurately, but also in the for loop it checks polling for idx == 0.
>> predicted_ns should still be the value from get_typical_interval(), but
>> that is unable to ever enforce a polling state, making your proposed code
>> change the only obvious fix.
> 
>> Essentially with only one non-polling state the get_typical_interval()
>> logic might as well be skipped, it never affects anything.
>>
>> If I'm correct this time then I'm not really sure how to proceed.
>> On the one hand, if we trust predicted_ns we should indeed select a state
>> with predicted_ns < s->target_residency_ns, no matter if polling or not,
>> but that would be quite a behaviour change for x86 users too.
> 
> That's right, so far this is the only solution I can think of that
> improves performance, But, I'm open to trying alternative suggestions
> if any.

It's the most reasonable I can think of anyway.

>> I'm still curious on why teo wouldn't solve your problem, it's intercept
>> logic doesn't care if it selects a polling state or not because of it.
>>
> I confirmed that "teo" doesn't have the reported issue, and when compared
> to the  vanilla menu governor switching to "teo" does improve the performance
> of some workloads, though not the original workload I tested. I'll look into
> why that's the case.

I'd be really curious about the workload that improves with your patch but
doesn't perform well on teo.

> However, we would still want to resolve the issue with the menu governor,
> as it's the default governor currently enabled and have been tested so
> far. While switching to "teo" could be an option in the future.

Understandable and I think your change makes sense, although that logic is
quite old as I dug up.
The modern arm64 but also x86 platforms AFAICS are unlikely to be affected by
your change as they have efficient/responsive physical idle states (or for
arm64 don't have any non-polling state usually) so the prediction logic
would have to spit out something like <1000 ns to take affect and at that
point getting over that statistically significance is quite unlikely.
OTOH if there were power regressions they might be with platforms where they
would never be reported.
Anyway, Rafael's call here.
If he's willing to take it I would include the historical part in the commit
message so that doesn't need to be dug up again.

Regards,
Christian