Re: [PATCH v2 0/5] s390/idle: CPU idle driver

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

On 6/10/26 21:23, Mete Durlu wrote:
> On 6/9/26 5:47 PM, Christian Loehle wrote:
>> On 6/9/26 14:24, Mete Durlu wrote:
>>> v1 -> v2:
>>>
>>> * Add idle driver enteries to MAINTAINERS file (Christian Loehle)
>>> * Remove extra line break left in drivers/cpuidle/Kconfig.s390
>>>    (Christian Loehle)
>>>
>>> This patch series introduces a CPU idle driver for s390
>>> architecture that leverages the existing cpu idle infrastructure and
>>> TEO (Timer Events Oriented) governor to optimize idle state selection
>>> based on timer events and interrupt patterns.
>>
>> So if (according to v1) there's no data (yet) that teo is much preferred,
>> I would remove all mentions of it in code and patch descriptions?
>> A cpuidle driver requiring a specific governor sort-of violates the
>> abstraction.
> 
> True, s/TEO governor/idle governor/g + s/TEO/idle governor/g should
> be good enough.
> 
>> In case teo does work much better for you, which I wouldn't doubt for
>> a second from what your system looks like, it would be nice to present
>> some data on it. Additionally I think setting it in the defconfig alone
>> is probably enough?
>>
> 
> I ran a quick menu vs teo run on an LPAR.
> There was no significant change between the results of the benchmarks
> except for slightly higher cpu utilization when using menu governor. I
> can put it down to the cover letter. The only noticeable change is on my
> micro benchmark where I use epoll_wait() to make two threads on
> different cores ping each other.
> 
> micro-benchmark |   teo    |   menu
> -------------------------------------
> avg time        | ~21.6sec | ~24.3sec
> 
> Idle framework tries to find the specified governor in the driver struct
> but if it can't find it or no governor was specified it uses the next
> available one, so setting teo on defconfig and disabling rest of the
> governors can be good enough to pick teo.
> 

Thanks for testing!

> 
>>> - Configuration
>>> -----------------------------------------------------------------------
>>>
>>> Idle state parameters are tuned per hypervisor type after benchmarks:
>>>
>>> **LPAR:**
>>> - Polling: 5us target residency, 0us exit latency
>>> - EW: 5us target residency, 5us exit latency
>>>
>>> **KVM/z/VM:**
>>> - Polling: 1us target residency, 0us exit latency
>>> - EW: 1us target residency, 1us exit latency
>>>
>> I think this would also be useful in cpuidle-s390.c in particular the
>> different residency+latency values for LPAR and KVM/z/VM and what they aim
>> to achieve for you.
> 
> We can put down a comment like below;
> 
> /*
>  * After various benchmark runs the tuneables for idle driver has shown
>  * the best performance with the following values;
>  * for LPAR:
>  * - Polling: 5us target residency, 0us exit latency
>  * - EW: 5us target residency, 5us exit latency
>  *
>  * for KVM/z/VM:
>  * - Polling: 1us target residency, 0us exit latency
>  * - EW: 1us target residency, 1us exit latency
>  */
> 
> Is that what you are looking for or something more extensive to cover
> what sort of behavior it causes and why it benefits the performance?
> I wouldn't really like to put down lengthy comments here to be honest.

Why not?
At least to me the values are hard to follow and without knowing what they
represent.
For example is this actually the worst case exit latency for these systems?
Also from a cpuidle perspective residency == latency isn't outright wrong
but certainly always a little suspicious.

> 
>> Additionally polling is initialised to 0/0 by poll_state.c, so I don't know
>> where you're taking these values from?
> 
> Having a look at the implementation of poll_idle() in poll_state.c.
> The polling time limit (target_residency) is acquired from
> cpuidle_poll_time(), which tries to find an enabled state deeper than
> polling state and returns its target_residency. Since we only have two
> states, it automatically means EW state's target_residency, or
> IDLE_POLL_MAX if it is disabled.

I agree that cpuidle_poll_time() can derive the polling time limit from a
deeper enabled state, but it only does that for states with
target_residency_ns >= CPUIDLE_POLL_MIN (10us). With the proposed s390 EW
values of 1us/5us, that state is skipped and the poll limit falls
back to CPUIDLE_POLL_MAX (TICK_NSEC / 16).


> 
> If I am not mistaken governor also uses the next enabled state
> to calculate the target_residency of the polling state.
> 

With just two state that is practically what happens in the selection
algorithm anyway, yes.