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

On Mon, 17 Aug 2026 16:40:56 +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:
>>
>>>>> 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/[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.
>>
>> 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.

Right, hrtimer_try_to_cancel() does not have that restriction but
hrtimer_cancel() must not be called while holding a lock that the
callback takes.