Re: [PATCH 1/3 v3] hrtimers: Force migrate away hrtimers queued after CPUHP_AP_HRTIMERS_DYING

[Thomas Gleixner <tglx@linutronix.de>]

On Fri, Jan 17 2025 at 15:11, Frederic Weisbecker wrote:
> Le Thu, Jan 16, 2025 at 11:59:48AM +0100, Thomas Gleixner a écrit :
>> > +	if (enqueue_hrtimer(timer, new_base, mode))
>> > +		smp_call_function_single_async(cpu, &new_cpu_base->csd);
>> 
>> Duh. This reimplementation of switch_hrtimer_base() is really aweful. We
>> can be smarter than that. Untested patch below.
>
> Indeed, and I tried to "fix" switch_hrtimer_base() but didn't managed to do
> it properly. Looks like you did :-)
>
> But I have a few comments:
>
>> @@ -208,6 +211,13 @@ struct hrtimer_cpu_base *get_target_base
>>  	if (static_branch_likely(&timers_migration_enabled) && !pinned)
>>  		return &per_cpu(hrtimer_bases, get_nohz_timer_target());
>>  #endif
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +	if (unlikely(!base->online)) {
>> +		int cpu = cpumask_any_and(cpu_online_mask, housekeeping_cpumask(HK_TYPE_TIMER));
>> +
>> +		return &per_cpu(hrtimer_bases, cpu);
>> +	}
>> +#endif
>
> This should be at the beginning of get_target_base(), otherwise the target may
> end up being the local one.

Indeed. As I said, it's untested.

>> -		if (new_cpu_base != this_cpu_base &&
>> +		if (new_cpu_base != this_cpu_base && this_cpu_base->online &&
>>  		    hrtimer_check_target(timer, new_base)) {
>>  			raw_spin_unlock(&new_base->cpu_base->lock);
>>  			raw_spin_lock(&base->cpu_base->lock);
>
> This forget the case where the elected remote target is the same as the old one,
> but its next event is after the timer. In that case this default to local, even
> if it is offline.

*blink*

> How about this? (untested yet)
> -static int
> -hrtimer_check_target(struct hrtimer *timer, struct hrtimer_clock_base *new_base)
> +static bool
> +hrtimer_suitable_target(struct hrtimer *timer,
> +			struct hrtimer_clock_base *new_base,
> +			struct hrtimer_cpu_base *new_cpu_base,
> +			struct hrtimer_cpu_base *this_cpu_base)
>  {
>  	ktime_t expires;
>  
> +	/* The local CPU clockevent can be reprogrammed */
> +	if (new_cpu_base == this_cpu_base)
> +		return true;

That needs a comment explaining that @new_cpu_base is guaranteed not to
be @this_cpu_base if @this_cpu_base->online == false due to the magic in
get_target_base(). That's far from obvious.

I had to stare at it 5 times to convince myself that this is correct.

Other than that, this looks about right.

Thanks,

        tglx