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

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

On Fri Aug 14, 2026 at 12:47 AM BST, FUJITA Tomonori wrote:
> On Thu, 13 Aug 2026 15:16:38 +0100
> "Gary Guo" <gary@garyguo.net> wrote:
>
>> I am thinking about this and I wonder about a different approach: the only
>> reason that we're having this issue, is that `expires()` call and
>> `forward`/`forward_now` is executed outside the protection of the base lock.
>> 
>> The fix is easy -- to ensure that they are executed with the base lock held.
>> The callback wants either:
>> * Do not restart the timer
>> * Call hrtimer_forward[_now] and restart the timer
>> 
>> So, if we change the order from
>> 
>>     unlock base
>>     restart = fn(timer)
>>     lock base
>>     if restart {
>>         queue
>>     }
>> 
>> to
>> 
>>     get expires
>>     unlock base
>>     restart = fn(timer, expires)
>>     lock base
>>     match restart {
>>         Restart(now, interval) => {
>>             hrtimer_forward(timer, now, interval);
>>             queue
>>         }
>>         NoRestart => (),
>>     }
>> 
>> then we completely eradicate this issue.
>
> If I understood the proposal correctly: hrtimer_forward[_now]() would no
> longer be called by drivers at all. It becomes internal to the core and
> runs with the base lock held, and every hrtimer callback in the tree is
> updated to take the expiry as an argument and return the interval, with
> the ones that use the overrun computing it from what they were passed.

Right, that the idea. I think patching all hrtimer callback is probably a bit
excessive, but one way would be add a mode where cpu_base->lock is not unlocked,
and the Rust hrtimer abstraction would read the expiry, unlock it, run the
callback and re-lock the base lock.

>
> That could remove the need for the rule on the Rust side.
>
> Anna-Maria, Frederic, Thomas: does this direction look reasonable to you?
>
>
>> Alternatively, we can add another spinlock to protect `expires` from race
>> condition from within callback and concurrent restart -- that is what perf core
>> does: perf_mux_hrtimer_handler and perf_mux_hrtimer_restart uses the same
>> hrtimer_lock to prevent race.
>
> Right, with the lock and cpc->hrtimer_active flag together, perf
> implements the same "do not arm while armed" rule that these patches
> implement in the type system.
>
>
>> 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?

Best,
Gary