Re: [External] : Re: [PATCH 1/2] timer: add add_timer_active_cpu()

[Partha Satapathy <partha.satapathy@oracle.com>]

On 19-06-2026 21:50, Thomas Gleixner wrote:
> On Mon, Jun 08 2026 at 19:22, Partha Satapathy wrote:
>> RDS already checks cpu_online() before calling add_timer_on().
>> However, that still races with CPU hotplug and can leave a timer
>> queued on a CPU which goes offline before the enqueue completes.
> I can't find any invocation of add_timer_on() in the RDS code and the
> commit you mentioned does not exist and there is no trace of it on
> lore. But Gemini told me it's Oracle private hackery. So what the heck
> are you telling me about RDS checking something and having a race
> condition?
> 
>> The current workaround is to serialize the operation with
>> cpus_read_lock(), but doing so in the networking path adds overhead
>> to every timer enqueue and increases CPU hotplug latency.
> I buy the cpus_read_lock() overhead, but who cares about hotplug latency?
> 
> Aside of that if you just want to temporary prevent a CPU from vanishing
> while queueing the timer, then preempt_disable() does the trick too and
> that's definitely more lightweight than cpus_read_lock().
> 
>>> On Thu, Apr 23 2026 at 09:19, Partha Satapathy wrote:
>>>>  /**
>>>>   * __timer_delete - Internal function: Deactivate a timer
>>>>   * @timer:	The timer to be deactivated
>>>> @@ -2507,6 +2543,7 @@ int timers_prepare_cpu(unsigned int cpu)
>>>>  		base->next_expiry_recalc = false;
>>>>  		base->timers_pending = false;
>>>>  		base->is_idle = false;
>>>> +		base->is_active = true;
>>> That's just wrong. The base is active only when the CPU reaches online state.
>> Setting base->is_active = true in timers_prepare_cpu() was
>> intentional as a small optimization to avoid an additional hotplug
>> callback. Timers queued in this window will be serviced as the CPU
>> completes bring-up.
> It's still intentionally wrong.
> 
>> However, to keep the state strictly aligned with CPU online state, I
>> can move this activation logic to a dedicated callback under
>> CPUHP_AP_ONLINE_DYN instead if you prefer.
> What's wrong with checking cpu_online() under the CPU's base lock? When
> the lock is held the CPU can't go away.
> 
>> The RDS use case treats the target CPU primarily as a locality hint
>> rather than a strict execution requirement.
>>
>> Given your outline above, this may be better addressed by a generic
>> placement-hint interface than by strict CPU placement semantics.
> Well, a placement hint interface still needs to ensure that the target
> is online and does not vanish before the timer or work has been queued,
> which means the whole operation needs to be protected against hotplug no
> matter what. So you end up with something like this:
> 
>        scoped_guard(preempt) {
>           if (!cpu_online(cpu))
>        	  	cpu = pick_online_cpu();
>           add_timer_on(.....);
>        }
> 
> The simplest variant for pick_online_cpu() is obviously
> cpumask_first(onlinemask), but you can go wild there and find the
> nearest one.
> 
> But making this a generic thing is not necessarily a good idea because
> pick_online_cpu() could turn out to be expensive and you might want to
> cache the result once your preferred CPU vanished.
> 
> Alternatively you implement a hotplug callback for RDS which changes the
> preferred CPU before the CPU goes offline. That's cheap and a one time
> effort and you don't have to do anything on the timer/workqueue
> side. There you can even justify to search for the nearest neighbor or
> whatever preference you have.
> 
> Thanks,
> 
>         tglx
> 
> 

Thanks for the detailed explanation.

I'll drop this timer core change and work on the RDS-side fix instead,
most likely by updating the preferred CPU from a CPU hotplug callback.

Sorry for the noise.

Thanks,
Partha