[PATCH v2] hrtimer: Use hard expiry when updating timers on the same base

[PATCH v2] hrtimer: Use hard expiry when updating timers on the same base

[Andrea Parri]

Rearming a queued timer with nonzero slack can leave the timerqueue out
of order. remove_and_enqueue_same_base() checks the new soft expiry
against its neighbours' hard expiries, then stores the new hard expiry
in the node without requeueing it.

For example, with A at 10 and B at 20, rearming A at 11 with slack 30
passes the neighbour check but leaves A's hard expiry of 41 before B's
20. The same function also caches the soft expiry in base->expires_next
when updating or inserting the first timer, giving next-event selection
an earlier deadline than the queue head's hard expiry.

Set the timer expiry before handling the queue. Use its stored hard
expiry for the in-place ordering check and both updates to
base->expires_next.

The early update should be safe because remove_and_enqueue_same_base()
runs with base->cpu_base->lock held. The lock keeps the queue stable
while hrtimer_can_update_in_place() checks the new expiry against both
neighbours.  If the check fails, timerqueue_linked_del() removes the
node without comparing expiry values before it is reinserted.

Fixes: eddffab8282e3 ("hrtimer: Keep track of first expiring timer per clock base")
Fixes: 343f2f4dc5425 ("hrtimer: Try to modify timers in place")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
---
Changes in v2:
- Set the timer expiry before handling the queue and reuse its stored
  hard expiry, as suggested by Peter Zijlstra.

Link to v1: https://lore.kernel.org/r/20260907211134.3854-1-parri.andrea@gmail.com/
---
 kernel/time/hrtimer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 530d61257b9a0..af22a2fec4904 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1263,13 +1263,16 @@ remove_and_enqueue_same_base(struct hrtimer *timer, struct hrtimer_clock_base *b
 {
 	bool was_first = false;
 
+	/* Set the new expiry time */
+	hrtimer_set_expires_range_ns(timer, expires, delta_ns);
+	expires = hrtimer_get_expires(timer);
+
 	/* Remove it from the timer queue if active */
 	if (timer->is_queued) {
 		was_first = !timerqueue_linked_prev(&timer->node);
 
 		/* Try to update in place to avoid the de/enqueue dance */
 		if (hrtimer_can_update_in_place(timer, base, expires)) {
-			hrtimer_set_expires_range_ns(timer, expires, delta_ns);
 			trace_hrtimer_start(timer, mode, true);
 			if (was_first)
 				base->expires_next = expires;
@@ -1280,9 +1283,6 @@ remove_and_enqueue_same_base(struct hrtimer *timer, struct hrtimer_clock_base *b
 		timerqueue_linked_del(&base->active, &timer->node);
 	}
 
-	/* Set the new expiry time */
-	hrtimer_set_expires_range_ns(timer, expires, delta_ns);
-
 	debug_activate(timer, mode, timer->is_queued);
 	base->cpu_base->active_bases |= 1 << base->index;
 
-- 
2.53.0

Re: [PATCH v2] hrtimer: Use hard expiry when updating timers on the same base

[Thomas Gleixner]

On Wed, Sep 09 2026 at 12:27, Andrea Parri wrote:
> @@ -1263,13 +1263,16 @@ remove_and_enqueue_same_base(struct hrtimer *timer, struct hrtimer_clock_base *b
>  {
>  	bool was_first = false;
>  
> +	/* Set the new expiry time */
> +	hrtimer_set_expires_range_ns(timer, expires, delta_ns);
> +	expires = hrtimer_get_expires(timer);

My knee jerk reaction when reading this the first time was something
non-printable because the obvious rule is that you can't change the sort
key of a tree entry when that can make the tree inconsistent.

As the base lock is held and nothing can observe the potentially
resulting inconsistency at that point and the timer either stays in
place or is dequeued, this is safe, but non obvious.

So this really wants to have a big fat comment explaining why this is
safe to do.

Thanks,

        tglx

Re: [PATCH v2] hrtimer: Use hard expiry when updating timers on the same base

[Andrea Parri]

> So this really wants to have a big fat comment explaining why this is
> safe to do.

Agreed. The existing comment only describes the update and misses the
temporary tree inconsistency. I'll spell out that the base lock excludes
other queue operations, that the neighbour check proves the node can
remain in place, and that removal does not consult the expiry otherwise.

Thanks,
Andrea