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
