Re: [PATCH] hrtimer: Avoid double reprogramming in __hrtimer_start_range_ns()

[Thomas Gleixner]

On Mon, Apr 26 2021 at 11:40, Peter Zijlstra wrote:
> On Mon, Apr 26, 2021 at 10:49:33AM +0200, Thomas Gleixner wrote:
>> If __hrtimer_start_range_ns() is invoked with an already armed hrtimer then
>> the timer has to be canceled first and then added back. If the timer is the
>> first expiring timer then on removal the clockevent device is reprogrammed
>> to the next expiring timer to avoid that the pending expiry fires needlessly.
>>  		/*
>>  		 * Remove the timer and force reprogramming when high
>> @@ -1048,8 +1049,16 @@ remove_hrtimer(struct hrtimer *timer, st
>>  		debug_deactivate(timer);
>>  		reprogram = base->cpu_base == this_cpu_ptr(&hrtimer_bases);
>>  
>> +		/*
>> +		 * If the timer is not restarted then reprogramming is
>> +		 * required if the timer is local. If it is local and about
>> +		 * to be restarted, avoid programming it twice (on removal
>> +		 * and a moment later when it's requeued).
>> +		 */
>>  		if (!restart)
>>  			state = HRTIMER_STATE_INACTIVE;
>> +		else
>> +			reprogram &= !keep_local;
>
> 			reprogram = reprogram && !keep_local;
>
> perhaps?

Maybe

>>  
>>  		__remove_hrtimer(timer, base, state, reprogram);
>>  		return 1;
>> @@ -1103,9 +1112,31 @@ static int __hrtimer_start_range_ns(stru
>>  				    struct hrtimer_clock_base *base)
>>  {
>>  	struct hrtimer_clock_base *new_base;
>> +	bool force_local, first;
>>  
>> -	/* Remove an active timer from the queue: */
>> -	remove_hrtimer(timer, base, true);
>> +	/*
>> +	 * If the timer is on the local cpu base and is the first expiring
>> +	 * timer then this might end up reprogramming the hardware twice
>> +	 * (on removal and on enqueue). To avoid that by prevent the
>> +	 * reprogram on removal, keep the timer local to the current CPU
>> +	 * and enforce reprogramming after it is queued no matter whether
>> +	 * it is the new first expiring timer again or not.
>> +	 */
>> +	force_local = base->cpu_base == this_cpu_ptr(&hrtimer_bases);
>> +	force_local &= base->cpu_base->next_timer == timer;
>
> Using bitwise ops on a bool is cute and all, but isn't that more
> readable when written like:
>
> 	force_local = base->cpu_base == this_cpu_ptr(&hrtimer_bases) &&
> 		      base->cpu_base->next_timer == timer;
>

Which results in an extra conditional branch.

>> +	/*
>> +	 * Timer was forced to stay on the current CPU to avoid
>> +	 * reprogramming on removal and enqueue. Force reprogram the
>> +	 * hardware by evaluating the new first expiring timer.
>> +	 */
>> +	hrtimer_force_reprogram(new_base->cpu_base, 1);
>> +	return 0;
>>  }
>
> There is an unfortunate amount of duplication between
> hrtimer_force_reprogram() and hrtimer_reprogram(). The obvious cleanups
> don't work however :/ Still, does that in_hrtirq optimization make sense
> to have in force_reprogram ?

Yes, no, do not know. Let me have a look.

Thanks,

        tglx