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

[Mete Durlu <meted@linux.ibm.com>]

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.


>> - 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.

> 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.

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