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

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

On 6/10/26 10:23 PM, 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.
> 
> 
>>> - 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.
> 
> 

Friendly ping.