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

["Gary Guo" <gary@garyguo.net>]

On Fri Aug 14, 2026 at 2:48 PM BST, FUJITA Tomonori wrote:
> On Fri, 14 Aug 2026 01:54:25 +0100
> "Gary Guo" <gary@garyguo.net> 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/tip-5de2755c8c8b3a6b8414870e2c284914a2b42e4d@git.kernel.org/
>> 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);
    }

?

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.

Best,
Gary

>
> v1 is missing the ability to restart a stopped timer: once the
> callback has returned NoRestart, the handle owns the right to arm and
> never gives it back. I'll add it in v2, including a non-blocking
> variant built on hrtimer_try_to_cancel(), so that a caller which
> cannot sleep can re-arm the way perf does. That makes perf's model
> expressible in the Rust abstraction.