Re: [PATCH] tick/nohz: Avoid unused timekeeping_max_deferment() calls

[Usama Arif]

On 18/09/2026 13:08, Frederic Weisbecker wrote:
> Le Thu, Sep 17, 2026 at 09:39:29AM -0700, Usama Arif a écrit :
>> tick_nohz_next_event() limits a CPU's sleep interval to the maximum
>> deferment supported by the current clocksource when that CPU owns the
>> do_timer() duty. If the duty is unassigned, the limit also applies when
>> the CPU's TS_FLAG_DO_TIMER_LAST flag is set.
>>
>> After the early timer checks, the function currently reads the maximum
>> deferment unconditionally. It then replaces the result with KTIME_MAX
>> unless one of the two conditions above applies.
>>
>> timekeeping_max_deferment() performs a seqcount-protected read of the
>> shared timekeeper and follows its clocksource pointer. Check the do_timer
>> state first and avoid this work when the result would be discarded. This
>> leaves the resulting expiry unchanged and reduces accesses to timekeeper
>> data that is modified regularly.
>>
>> On x86-64 this removes 18-20 dynamically executed instructions, including
>> the call, from the common non-owner path when the seqcount does not retry.
>>
>> Signed-off-by: Usama Arif <usama.arif@xxxxxxxxx>
>> ---
>>  kernel/time/tick-sched.c | 23 +++++++++++++++--------
>>  1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
>> index c8f2c4a503b08..e7c7c28311156 100644
>> --- a/kernel/time/tick-sched.c
>> +++ b/kernel/time/tick-sched.c
>> @@ -872,18 +872,25 @@ static ktime_t tick_nohz_next_event(struct tick_sched *ts, int cpu)
>>  	 * If this CPU is the one which had the do_timer() duty last, we limit
>>  	 * the sleep time to the timekeeping 'max_deferment' value.
>>  	 * Otherwise we can sleep as long as we want.
>> +	 *
>> +	 * Only read the max deferment in the former case: it is a seqcount
>> +	 * read of the globally shared timekeeper, and on a large machine
>> +	 * almost every caller is not the do_timer() CPU and would throw the
>> +	 * value away.
> 
> I don't think that needs a comment.
> 
>>  	 */
>> -	delta = timekeeping_max_deferment();
>>  	tick_cpu = READ_ONCE(tick_do_timer_cpu);
>>  	if (tick_cpu != cpu &&
>> -	    (tick_cpu != TICK_DO_TIMER_NONE || !tick_sched_flag_test(ts, TS_FLAG_DO_TIMER_LAST)))
>> -		delta = KTIME_MAX;
>> -
>> -	/* Calculate the next expiry time */
>> -	if (delta < (KTIME_MAX - basemono))
>> -		expires = basemono + delta;
>> -	else
>> +	    (tick_cpu != TICK_DO_TIMER_NONE || !tick_sched_flag_test(ts, TS_FLAG_DO_TIMER_LAST))) {
>>  		expires = KTIME_MAX;
>> +	} else {
>> +		delta = timekeeping_max_deferment();
>> +
>> +		/* Calculate the next expiry time */
>> +		if (delta < (KTIME_MAX - basemono))
>> +			expires = basemono + delta;
>> +		else
>> +			expires = KTIME_MAX;
>> +	}
> 
> If anything, it also makes the code more readable. You can even remove the delta
> variable because even in its former use in the function, it's an unecessary
> middle step.
> 
> Thanks.


Thanks for the review! I have sent a v2 to address above in [1].

[1] https://lore.kernel.org/all/20260918133408.2834751-1-usama.arif@xxxxxxxxx/

> 
>>  
>>  	ts->timer_expires = min_t(u64, expires, next_tick);
>>  
>> -- 
>> 2.53.0-Meta
>>
>