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

[Thomas Gleixner <tglx@kernel.org>]

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.

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:

   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.

> 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?

>   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?

If the callback invokes hrtimer_forward_safe_from_callback() then you
have these scenarios:

#A
        callback()
          safe_forward()
            lock()
            if (!queued)  // true
               forward()
            unlock()
                                        lock()
                                        start()
                                        unlock()
            return RESTART
          lock()
          if (!queued)    // false
             start()
          unlock()

#B
        callback()
          safe_forward()
                                        lock()
                                        start()
                                        unlock()
            lock()
            if (!queued)  // false
               forward()
            unlock()
            return RESTART

          lock()
          if (!queued)    // true
             start()
          unlock()

In both cases the external start() wins.

Sure in #A it overwrites the forward result, but that's what the user
asked for and it is not at all different from this sequence:

        callback()
          safe_forward()
            lock()
            if (!queued)  // true
               forward()
            unlock()
            return RESTART
          lock()
          if (!queued)    // true
             start()
          unlock()
                                        lock()
                                        start()
                                        unlock()

IOW, you cannot prevent the forward result from being overwritten and it
does not matter at all whether the overwrite happens before or after the
callback returned RESTART. The outcome is exactly the same. No?

And no, checking hrtimer_is_queued() before invoking the start sequence
does not prevent that simply because that check is lockless:

        callback()
          safe_forward()
            lock()
            if (!queued)  // true
               forward()
            unlock()
            return RESTART
          lock()
          if (!queued)    // true	if (!queued)      // true
             start()
          unlock()
                                          lock()
                                          start()
                                          unlock()

Even if it would be protected by the base lock it would result in a
temporary state which can be invalid when invoking the function:

        callback()
          safe_forward()
            lock()
            if (!queued) // true
               forward()
            unlock()
            return RESTART
            				lock()
            				cond = !queued  // true
                                        unlock()
          lock()
          if (!queued)  // true	      
             start()
          unlock()
          				if (cond)       // true
                                          lock()
                                          start()
                                          unlock()

No?

To prevent that you'd need a start_if_not_queued() function:

        callback()
          safe_forward()
            lock()
            if (!queued) // true
               forward()
            unlock()
            return RESTART
          lock()
          if (!queued)  // true	      
             start()
          unlock()
                                        lock()
          				if (!queued)    // false
                                          start()
                                        unlock()

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.

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."

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.

> 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?

Thanks,

        tglx