Re: [PATCH 0/4] Fix forward()/expires() racing with concurrent arming

On Fri, 14 Aug 2026 15:24:10 +0100
"Gary Guo" <[email protected]> wrote:

> On Fri Aug 14, 2026 at 2:48 PM BST, FUJITA Tomonori wrote:
>> On Fri, 14 Aug 2026 01:54:25 +0100
>> "Gary Guo" <[email protected]> wrote:
>>> Let's ignore the implementation detail of all various Rust pointers. It is
>>> something that I plan to overhaul and doesn't matter to the core issue here.
>>> 
>>> The change you're making is to remove the ability to concurrently start a timer
>>> in Rust. So if you have a timer might be running, you'd need to first cancel it
>>> before you can arm it again.
>>> 
>>> I do think it is conceptually cleaner -- however given this is explicitly added
>>> in
>>> https://lore.kernel.org/all/[email protected]/
>>> and the pattern is what perf core uses; so I wouldn't just dismiss the existence
>>> of this pattern. Perhaps cancelling before restarting is considered too
>>> expensive and has to be avoided?
>>
>> The pattern perf core uses is "no arming while armed".
>>
>> While perf_mux_hrtimer_handler() returns HRTIMER_RESTART -- while the
>> timer is active -- perf_mux_hrtimer_restart() does nothing. Only once
>> the handler has cleared cpc->hrtimer_active and returned
>> HRTIMER_NORESTART does perf_mux_hrtimer_restart() arm it again.
>>
>> That flag was added precisely to implement "no arming while armed", in
>> 4cfafd3082af ("sched,perf: Fix periodic timers"):
>>
>>     We do not want to race such that the handler has already decided
>>     to stop, but the (external) restart sees the timer still active and we
>>     end up with a 'lost' timer.
>>
>>     The problem with the current code is that the re-start can come before
>>     the callback does the forward, at which point the forward from the
>>     callback will WARN about forwarding an enqueued timer.
>>
>>
>> With cpc->hrtimer_active in place, neither of the two conditions that
>> 5de2755c8c8b touches is reachable in perf's usage.
> 
> So are we okay saying that concurrent restart is problematic because apparently
> it cannot be used correctly (because you don't have a way to synchronize it)?
> Perhaps we should just revert 5de2755c8c8b or at least do
> 
>     if (restart != HRTIMER_NORESTART) {
>         WARN_ON(timer->state != HRTIMER_STATE_CALLBACK);
>         if (!(timer->state & HRTIMER_STATE_ENQUEUED))
>             enqueue_hrtimer(timer, base);
>     }
> 
> ?

HRTIMER_STATE_CALLBACK is gone; the state machine was reworked, so
that WARN is now WARN_ON(timer->is_queued): the timer was armed again
while the callback was running. And the

if (!(timer->state & HRTIMER_STATE_ENQUEUED))

part is what the tree does already; __run_hrtimer() checks
!timer->is_queued. So against current mainline the proposal is just
the WARN.

That is only a diagnostic and does not change behaviour, so I'm not
sure whether it is worth it.


> That said, the perf core's pattern is still different from the API that you're
> designing. When perf_mux_hrtimer_handler unlocks cpc->hrtimer_lock at that point
> perf_mux_hrtimer_restart can already kick in and restart the timer. From hrtimer
> core's perspective, it is starting a timer that is still have running callback
> -- but that callback shall only return NORESTART.

Yes. The difference is that perf can restart a timer that is stopped, and
also one that is still in its callback, before the callback returns
HRTIMER_NORESTART.

The second case is a short window, but if it matters, I think that
there are two ways to cover it. One is an unsafe restart, where the
user of the hrtimer synchronizes with a spinlock and a flag as perf
does. The other is to add a type that does that synchronization.