linux-kernel - Re: [PATCH 1/3 v3] hrtimers: Force migrate away hrtimers queued after CPUHP_AP_HRTIMERS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87ikqda2n3.ffs@tglx>
Date: Fri, 17 Jan 2025 16:20:00 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Frederic Weisbecker <frederic@...nel.org>
Cc: LKML <linux-kernel@...r.kernel.org>, vlad.wing@...il.com,
 rcu@...r.kernel.org, boqun.feng@...il.com, joel@...lfernandes.org,
 neeraj.upadhyay@....com, urezki@...il.com, qiang.zhang1211@...il.com,
 Cheng-Jui.Wang@...iatek.com, leitao@...ian.org, kernel-team@...a.com,
 Usama Arif <usamaarif642@...il.com>, paulmck@...nel.org, Anna-Maria
 Behnsen <anna-maria@...utronix.de>
Subject: Re: [PATCH 1/3 v3] hrtimers: Force migrate away hrtimers queued
 after CPUHP_AP_HRTIMERS_DYING

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