Re: [PATCH 1/6] hrtimer: add expiry injecting callback variant

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

On Wed Sep 30, 2026 at 10:43 PM BST, Thomas Gleixner wrote:
> Gary!
>
> On Wed, Sep 30 2026 at 14:14, Gary Guo wrote:
>> On Tue Sep 29, 2026 at 9:56 PM BST, Thomas Gleixner wrote:
>>> hrtimer_forward_safe_from_callback(timer)
>>> {
>>>         base = lock_running_timer_base(timer);
>>>         if (!hrtimer_is_queued(timer))
>>>         	hrtimer_forward(timer);
>>>         unlock_timer_base(base);
>>> }
>>
>> FWIW we did discuss about this option. Here's the list of all options that we
>> come up with:
>>
>> 1. Take base lock before calling forward and release it afterward (the one you
>>    mentioned above). This one requires exposing the base lock (at least to Rust
>>    abstraction).
>
> Why?
>
> With the above C core function Rust does not even know that the
> base lock exists. Rust invokes the function and relies on the guarantees
> it provides.

Sure, if all expiry read/update functions have their _safe_from_callback
variant.

>
> And the function is not Rust specific at all. You can use it to paper
> over the i915 bugs too, no?
>
>> 2. Prevent timer operation from within the callback and have hrtimer core doing
>>    it (Andreas's patch).
>
> Which adds overhead into the hotpath and creates yet another weird
> "scratch my itch" API.
>
>> 3. Force users to stop the timer before re-arming. This was dismissed because
>>    stopping the timer will require waiting for the callback, and this is prone to
>>    deadlock condition.
>
> Obviously and that's one of the reasons that the code is implemented the
> way it is, which puts some reasonable responsibility to the users for
> the sake of simplicity and performance.
>
> Now you want to reverse that and add overhead and complexity to the core
> to cater for the potential stupidity of users without even solving _all_
> related problems:

It's not our initiative to reverse that design. That was changed long time ago:
https://lore.kernel.org/all/tip-5de2755c8c8b3a6b8414870e2c284914a2b42e4d@git.kernel.org/

Enforcing this would be more favourable to Rust API's design. FUJITA proposed an
API which would require user to cancel the timer first before re-arming.

The issue is that for users that do want a concurrent restart, like in perf
core's use case, special care would need to be taken to avoid deadlock, as
cancellation code path must not have any lock held that is shared with the
callback.

>
>    You again forgot that simply setting the new expiry time from the
>    callback without invoking forward() has exactly the same issue.
>
> Maybe you can prevent that on the Rust side, but the C side still allows
> that so your magic new callback is just providing a false sense of safety.

That is indeed something we didn't consider, as Rust abstraction currently does
not provide a way to do it. We could change the forward return value to just be
the new expire time.

    typedef enum hrtimer_restart (*hrtimer_ext_func_t)(struct hrtimer *timer, ktime_t *expires);

I've kept Andreas's name to not have to bikeshed on it.

With such callbacks, users would be abel to use a helper like:

    u64 hrtimer_compute_forward(ktime_t *expires, ktime_t now, ktime_t interval)

to forward the timer.

Alternatively, instead of putting this into the callback signature, struct
hrtimer could stores an additional expires_for_callback field which gets updated
from callbacks. I do think existing C callbacks can be converted to use such API
without too much churn.

>
>> 4. Add a lock to Rust side that all users must take to forward / start. We don't
>>    want to add a new lock just for this, and taking base lock would be more
>>    favourable.
>
> There is a reason why it is documented that this needs external
> serialization.
>
> Neither this magic inject callback variant nor what I proposed solve the
> underlying problem of two competing contexts which try to rearm the
> timer to a context dependent expiry time.
>
> All they can do is prevent inconsistent state, but the price to pay in
> terms of overhead and complexity are very different. And as I said above
> neither one of them solves the 'set expiry directly from the callback'
> problem.
>
>> We consider (2) the best option because it avoids several pitfalls with (1):
>>
>> - It is impossible to forget to forward and return restart, or forward but
>>   return norestart.
>
> Forward and forget to return RESTART is harmless. All what happens is that the
> timer won't fire so some device won't work as expected. There are a
> gazillion of other ways to achieve the same result.
>
> Forget forward and return RESTART will result in a hrtimer interrupt
> overrun warning and if you fail to figure that out when implementing
> your callback then you (and the AI you are relying on) should go and
> resort to HTML coding.
>
>> - The base lock is not unlocked before relocked, so the forward and restarting
>
> What means unlocked before relocked?

It's the example that you've given. If one calls
hrtimer_forward_safe_from_callback and then immediately return RESTART, it would
unlock the base lock within hrtimer_forward_safe_from_callback, and upon
returning hrtimer core relocks the base lock.

>
>>   is atomic. So it's impossible to have the condition where the forward occurs,
>>   but before returning RESTART, the hrtimer is concurrently queued.
>
> What's the problem with that?
>
> [snip]
>
> To prevent that you'd need a start_if_not_queued() function:
>
> [snip]
>
> Even that would not solve all possible problems either. That's an
> application problem. The core can only provide tools to avoid damage but
> it cannot prevent application logic bugs at all.

Ack. FWIW we're not trying to prevent all application logic bug. It's not
possible. But what's important that we try to achieve is for a user bug to be
contained and not cause hrtimer core to complete breakdown.

A buggy hrtimer user that never refires? If we can somehow prevent that, great,
but that's not required.

A buggy hrtimer user that breaks the timer wheel completely by making its data
structure internally inconsistent? That's what we're trying to prevent, at
least for Rust users. As it currently stands, an badly timed hrtimer_forward (or
hrtimer_set_expires) within callback can cause hrtimer core rbtree to violate
its invariant. And that's what we want to avoid.

> Adding complexity to prevent that has been pointed out to be the wrong
> solution by Dijkstra long ago:
>
>   "Complexity breeds bugs. Simplicity is the prerequisite for
>    reliability."

It's a trade-off, really. You can have simple code and complex rules, that's one
way of complexity. More complex code and simpler rule is a different way of
complexity.

The way I see this is that hrtimer code will be less complex to reason about if
expires modification is not possible without the base lock held.

- "timer->expires may only be touched with base lock held" is very simple rule,
  and yes the code would need some slight additional complexity.

- "timer->expires" may be touched without the base lock held, if the hrtimer is
  not queued, and no code can possibly queue it while it is touched." results in
  simpler code, but the rule is more complex, and it's _non-local_ meaning that
  the callback code may look innocent, and all it takes is a hrtimer_start from
  outside the callback to mess things up.

> Don't get me wrong. I'm a great fan of Rust, but I have a background in
> ADA programming (admittedly from decades ago) and I studied the
> limitations of provided safety measures in practice enough to know that
> Dijkstra is absolutely right. There is a reason why ADA grew a massive
> static analysis toolset around it which is sadly not easily exploitable
> due to the design decisions of Rust which borrowed a lot of the
> incomplete ADA concepts... But that's a different discussion to have.
>
>> - Unlock/relock have an overhead (we didn't quantify how much impact this will
>>   be, though).
>
> The extra lock operations won't be measurable except for situations
> which have high lock contention on the base lock independent of the
> problematic scenario. If that's the case then the extra lock/unlock pair
> will just add to the noise.

Ack.

I shall add that this is more than a single lock/unlock. hrtimer_get_expires
would need to take that lock as well.

>
>> I supposed another alternative is to add a mode in hrtimer core where the base
>> lock is not unlocked before calling the callback, and expose base lock APIs
>> (like option 1) so that the Rust hrtimer abstraction can unlock it before
>> calling the callbacks.
>
> That's even worse and Option 1, i.e. the core function I proposed is
> _NOT_ exposing base lock at all.
>
> So what's your actual argument that you can't build a "safe" Rust API
> around this?

Sure, if all expiry read/update functions have their _safe_from_callback
variant.

Best,
Gary

>
> Thanks,
>
>         tglx