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:
>
>>>> But further complicating the type system to prevent concurrent restart sounds
>>>> like a bad approach to me.
>>>
>>> My intent is the opposite: I think this makes the design simpler.
>>>
>>> All four implementations of start() already take self by value. For
>>> Pin<Box<T, A>> and Pin<&mut T> that means what it says -- the box is moved
>>> into the handle, the exclusive borrow is consumed -- so "no arming while
>>> armed" is already the design there. For Arc<T> and Pin<&T> the same
>>> signature meant nothing, because Clone and Copy let you build another
>>> pointer and call start() again.
>>>
>>> So the contract depended on which pointer type you picked, and the module
>>> documentation had to spell that out: "When a type implements both
>>> HrTimerPointer and Clone, it is possible to issue the start operation
>>> while the timer is in the started state." After the series there is one
>>> rule for all four types, and that paragraph is gone together with the
>>> restart operation it described.
>> 
>> 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.
>
> 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.

I should also add that cancel and then restart can be problematic from a lock
order POV.

If there is a shared lock between the handler and the restart like the perf
core's case, this lock cannot be held when cancelling the hrtimer. Otherwise you
can have a deadlock by having

    restart (with lock held) --wait--> handler --wait--> lock

which can be quite subtle.

Of course, there is an argument that by cancelling and restart, locks as seen in
perf core's use case wouldn't be necessary as the cancellation becomes the
synnchronization mechanism.

Best,
Gary