Received-SPF: pass (google.com: domain of linux-kernel+bounces-205729-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <edffb2d4-a13d-4973-ae6f-af993a6225de@arm.com>
Date: Fri, 7 Jun 2024 10:29:01 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 4/6] cpuidle: teo: Increase minimum time to stop tick
To: Dietmar Eggemann <dietmar.eggemann@arm.com>, linux-pm@vger.kernel.org,
 linux-kernel@vger.kernel.org, rafael@kernel.org
Cc: vincent.guittot@linaro.org, qyousef@layalina.io, peterz@infradead.org,
 daniel.lezcano@linaro.org, anna-maria@linutronix.de,
 kajetan.puchalski@arm.com, lukasz.luba@arm.com
References: <20240606090050.327614-1-christian.loehle@arm.com>
 <20240606090050.327614-5-christian.loehle@arm.com>
 <c11acac7-085a-4041-a1f3-8b4f46e4b691@arm.com>
Content-Language: en-US
From: Christian Loehle <christian.loehle@arm.com>
In-Reply-To: <c11acac7-085a-4041-a1f3-8b4f46e4b691@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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@arm.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:
>