linux-kernel - Re: [PATCH 4/6] cpuidle: teo: Increase minimum time to stop tick

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: <edffb2d4-a13d-4973-ae6f-af993a6225de@arm.com>
Date: Fri, 7 Jun 2024 10:29:01 +0100
From: Christian Loehle <christian.loehle@....com>
To: Dietmar Eggemann <dietmar.eggemann@....com>, linux-pm@...r.kernel.org,
 linux-kernel@...r.kernel.org, rafael@...nel.org
Cc: vincent.guittot@...aro.org, qyousef@...alina.io, peterz@...radead.org,
 daniel.lezcano@...aro.org, anna-maria@...utronix.de,
 kajetan.puchalski@....com, lukasz.luba@....com
Subject: Re: [PATCH 4/6] cpuidle: teo: Increase minimum time to stop tick

On 6/7/24 09:14, Dietmar Eggemann wrote:
> On 06/06/2024 11:00, Christian Loehle wrote:
>> Since stopping the tick isn't free, add at least some minor constant
>> (1ms) for the threshold to stop the tick.
> 
> Sounds pretty arbitrary to me? 'duration_ns' is either based on
> target_residency_ns or tick_nohz_get_sleep_length() or even set to
> TICK_NSEC/2. Does adding 1ms makes sense to all these cases? But then
> why 1ms?

It definitely is arbitrary, you're correct.
Feel free to treat this as RFC. I'll probably just drop this from the
serie and issue separately (to get the actual fixes merged more quickly).
Anyway I'd like to hear comments on this.

We are only interested in the cost of stopping the tick, which doesn't
really depend on the selected state residency nor the expected sleep length.
1ms works fine (for me!!), making it depend on TICK_NSEC would be natural,
too, but using TICK_NSEC is far too long for CONFIG_HZ=100 (and TICK_NSEC/2
too short for CONFIG_HZ=1000).
The cost of stopping the tick depends on a number of factors and knowing all
of them is probably not on the table anytime soon and until then I'd consider
this an improvement over 0.

> 
>> Signed-off-by: Christian Loehle <christian.loehle@....com>
>> ---
>>  drivers/cpuidle/governors/teo.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
>> index 216d34747e3b..ca9422bbd8db 100644
>> --- a/drivers/cpuidle/governors/teo.c
>> +++ b/drivers/cpuidle/governors/teo.c
>> @@ -622,10 +622,10 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>>  	/*
>>  	 * Allow the tick to be stopped unless the selected state is a polling
>>  	 * one or the expected idle duration is shorter than the tick period
>> -	 * length.
>> +	 * length plus some constant (1ms) to account for stopping it.
>>  	 */
>>  	if ((!(drv->states[idx].flags & CPUIDLE_FLAG_POLLING) &&
>> -	    duration_ns >= TICK_NSEC) || tick_nohz_tick_stopped())
>> +	    duration_ns > NSEC_PER_MSEC + TICK_NSEC) || tick_nohz_tick_stopped())
>>  		return idx;
>>  
>>  out_tick_state:
>