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

On Thu, 20 Aug 2026 11:00:21 +0200
Andreas Hindborg <[email protected]> wrote:

> FUJITA Tomonori <[email protected]> writes:
> 
>> On Tue, 18 Aug 2026 13:59:30 +0200
>> Andreas Hindborg <[email protected]> wrote:
>>
>>> "FUJITA Tomonori" <[email protected]> writes:
>>> 
>>>> On Tue, 18 Aug 2026 11:02:27 +0200
>>>> Andreas Hindborg <[email protected]> wrote:
>>>>
>>>>>> perf and CFS bandwidth have a flag as well as a lock. The flag is "do
>>>>>> not arm while armed", which is the same rule the types enforce
>>>>>> here. rtc and the softlockup watchdog look like they cancel first and
>>>>>> then start instead. None of them arms a timer that is active, so I
>>>>>> would rather the abstraction did not allow it either. Does that seem
>>>>>> reasonable?
>>>>>
>>>>> I am fine with preventing starting a timer that is Started or Running,
>>>>> but I am not liking the `UniqueArc` requirement.
>>>>>
>>>>> I have a use case in `rnull` where I have to start a timer behind an
>>>>> `Arc` with no way to obtain a `UniqueArc`, so I would prefer if that use
>>>>> case keeps on working. Without this, I would have to allocate a box and
>>>>> put it behind a lock, leading to double indirection.
>>>>
>>>> Before the UniqueArc requirement, I would like to check which timer
>>>> you have in mind? The bandwidth timer, the per-command timer, or
>>>> something else? The two seem to need different things, so I would
>>>> rather not guess.
>>> 
>>> The bandwidth timer. It is started from an `Arc<NullBlkDevice>:
>>> HasHrTimer`. If we make the suggested change to `ArcTimerHandle`, I
>>> think I would need to change the `NullBlkDevice::bandwidth_timer` from
>>> an embedded `HrTimer` to a `SpinLock<Box<HrTimer>>` or something like
>>> that.
>>> 
>>> Maybe this is fine. I don't think it will affect performance for `rnull`
>>> - this is already a throttled path. But it gives slightly more
>>> convoluted code in the caller by reducing the way we can use the API.
>>
>> I think we can allow creating HrTimerArc<T> from Arc<T>, so that UniqueArc is
>> not required. ListArc does the same with AtomicTracker, an atomic bool in the
>> object that records whether a ListArc exists:
>>
>>     HrTimerArc::try_from_arc(Arc<T>) -> Result<HrTimerArc<T>, Arc<T>>
>>
>> It fails when there is already another HrTimerArc for the object.
>>
>>> For the completion timer, the change you propose would work fine I
>>> think. I would just start the timer via the unique request reference
>>> rather than the shared one. This is probably a better way to do it
>>> anyway.
>>
>> Agreed.
> 
> We discussed this at the call last night. We came to the conclusion that
> we would like to experiment with the solution outlined by Gary, where we
> inject `expires` into the callback handler, and the callback handler
> returns a forward duration in addition to a restart value. Because with
> that approach, we can avoid adding complexity to the Arc end of the API.
> 
> For the best implementation of this scheme, we probably need to change
> some bits in the C code, add an additional path. Down the line, we could
> also see how man callers of the C code can be changed to use this
> pattern.
> 
> Do you want to send a patch based on this solution Tomo?

https://lore.kernel.org/rust-for-linux/[email protected]/

The solution that we discussed before, right? It changes how the
hrtimer core calls the callback. If the C maintainers take that, I
will do the Rust side for it.

So we leave this race as it is until we know whether the solution is
accepted?