The field 'function' of struct hrtimer should not be changed directly, as
the write is lockless and a concurrent timer expiry might end up using the
wrong function pointer.
Switch to use hrtimer_update_function() which also performs runtime checks
that it is safe to modify the callback.
Signed-off-by: Nam Cao <namcao@linutronix.de>
Cc: Jens Axboe <axboe@kernel.dk>
---
io_uring/io_uring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index ceacf6230e34..936f8b4106cf 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -2421,7 +2421,7 @@ static enum hrtimer_restart io_cqring_min_timer_wakeup(struct hrtimer *timer)
goto out_wake;
}
- iowq->t.function = io_cqring_timer_wakeup;
+ hrtimer_update_function(&iowq->t, io_cqring_timer_wakeup);
hrtimer_set_expires(timer, iowq->timeout);
return HRTIMER_RESTART;
out_wake:
--
2.39.5On 2/5/25 3:55 AM, Nam Cao wrote: > The field 'function' of struct hrtimer should not be changed directly, as > the write is lockless and a concurrent timer expiry might end up using the > wrong function pointer. > > Switch to use hrtimer_update_function() which also performs runtime checks > that it is safe to modify the callback. > > Signed-off-by: Nam Cao <namcao@linutronix.de> > Cc: Jens Axboe <axboe@kernel.dk> > --- > io_uring/io_uring.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c > index ceacf6230e34..936f8b4106cf 100644 > --- a/io_uring/io_uring.c > +++ b/io_uring/io_uring.c > @@ -2421,7 +2421,7 @@ static enum hrtimer_restart io_cqring_min_timer_wakeup(struct hrtimer *timer) > goto out_wake; > } > > - iowq->t.function = io_cqring_timer_wakeup; > + hrtimer_update_function(&iowq->t, io_cqring_timer_wakeup); > hrtimer_set_expires(timer, iowq->timeout); > return HRTIMER_RESTART; > out_wake: The timer is known expired here, it's inside the callback. Is this really necessary or useful? -- Jens Axboe
On Wed, Feb 05 2025 at 11:45, Jens Axboe wrote:
> On 2/5/25 3:55 AM, Nam Cao wrote:
>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>> index ceacf6230e34..936f8b4106cf 100644
>> --- a/io_uring/io_uring.c
>> +++ b/io_uring/io_uring.c
>> @@ -2421,7 +2421,7 @@ static enum hrtimer_restart io_cqring_min_timer_wakeup(struct hrtimer *timer)
>> goto out_wake;
>> }
>>
>> - iowq->t.function = io_cqring_timer_wakeup;
>> + hrtimer_update_function(&iowq->t, io_cqring_timer_wakeup);
>> hrtimer_set_expires(timer, iowq->timeout);
>> return HRTIMER_RESTART;
>> out_wake:
>
> The timer is known expired here, it's inside the callback. Is this
> really necessary or useful?
It's not strictly required here, but in the end this allows to make the
.function member private, which prevents stupid code fiddling with it
without proper sanity checks in the debug case.
Thanks,
tglx
On 2/6/25 9:18 AM, Thomas Gleixner wrote: > On Wed, Feb 05 2025 at 11:45, Jens Axboe wrote: >> On 2/5/25 3:55 AM, Nam Cao wrote: >>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c >>> index ceacf6230e34..936f8b4106cf 100644 >>> --- a/io_uring/io_uring.c >>> +++ b/io_uring/io_uring.c >>> @@ -2421,7 +2421,7 @@ static enum hrtimer_restart io_cqring_min_timer_wakeup(struct hrtimer *timer) >>> goto out_wake; >>> } >>> >>> - iowq->t.function = io_cqring_timer_wakeup; >>> + hrtimer_update_function(&iowq->t, io_cqring_timer_wakeup); >>> hrtimer_set_expires(timer, iowq->timeout); >>> return HRTIMER_RESTART; >>> out_wake: >> >> The timer is known expired here, it's inside the callback. Is this >> really necessary or useful? > > It's not strictly required here, but in the end this allows to make the > .function member private, which prevents stupid code fiddling with it > without proper sanity checks in the debug case. While that makes sense, this is also a potentially hot path for min timeout usage, which with small timeouts for batch/latency reasons can be run tens of thousand times per second. Adding a lock and IRQ dance would be counter productive. How about we just add a comment on why this is fine, rather than slow down a case that's perfectly fine by wrapping it in something much more expensive than a simple memory write? Or perhaps have a basic helper to set it that doesn't do the unnecessary irq lock guard? That would still allow you to make .function private. -- Jens Axboe
On Thu, Feb 06 2025 at 13:05, Jens Axboe wrote:
> On 2/6/25 9:18 AM, Thomas Gleixner wrote:
>> On Wed, Feb 05 2025 at 11:45, Jens Axboe wrote:
>>> On 2/5/25 3:55 AM, Nam Cao wrote:
>>>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>>>> index ceacf6230e34..936f8b4106cf 100644
>>>> --- a/io_uring/io_uring.c
>>>> +++ b/io_uring/io_uring.c
>>>> @@ -2421,7 +2421,7 @@ static enum hrtimer_restart io_cqring_min_timer_wakeup(struct hrtimer *timer)
>>>> goto out_wake;
>>>> }
>>>>
>>>> - iowq->t.function = io_cqring_timer_wakeup;
>>>> + hrtimer_update_function(&iowq->t, io_cqring_timer_wakeup);
>>>> hrtimer_set_expires(timer, iowq->timeout);
>>>> return HRTIMER_RESTART;
>>>> out_wake:
>>>
>>> The timer is known expired here, it's inside the callback. Is this
>>> really necessary or useful?
>>
>> It's not strictly required here, but in the end this allows to make the
>> .function member private, which prevents stupid code fiddling with it
>> without proper sanity checks in the debug case.
>
> While that makes sense, this is also a potentially hot path for min
> timeout usage, which with small timeouts for batch/latency reasons
> can be run tens of thousand times per second. Adding a lock and IRQ
> dance would be counter productive.
I fundamentally hate the fact that C does not enforce encapsulation in
the first place. :)
> How about we just add a comment on why this is fine, rather than
Comments are the worst of a solution as you know.
> slow down a case that's perfectly fine by wrapping it in something
> much more expensive than a simple memory write? Or perhaps have
> a basic helper to set it that doesn't do the unnecessary irq lock
> guard? That would still allow you to make .function private.
The right solution is to add a hotpath helper, which falls back to the
more expensive variant with some anyway expensive debug option, or make
the expensive part of hrtimer_update_function() depend on that debug
option in the first place and otherwise fall back to a simple store.
The latter is the right thing to do. Let me fix that up in mainline.
Thanks,
tglx
On 2/6/25 2:48 PM, Thomas Gleixner wrote: > On Thu, Feb 06 2025 at 13:05, Jens Axboe wrote: >> On 2/6/25 9:18 AM, Thomas Gleixner wrote: >>> On Wed, Feb 05 2025 at 11:45, Jens Axboe wrote: >>>> On 2/5/25 3:55 AM, Nam Cao wrote: >>>>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c >>>>> index ceacf6230e34..936f8b4106cf 100644 >>>>> --- a/io_uring/io_uring.c >>>>> +++ b/io_uring/io_uring.c >>>>> @@ -2421,7 +2421,7 @@ static enum hrtimer_restart io_cqring_min_timer_wakeup(struct hrtimer *timer) >>>>> goto out_wake; >>>>> } >>>>> >>>>> - iowq->t.function = io_cqring_timer_wakeup; >>>>> + hrtimer_update_function(&iowq->t, io_cqring_timer_wakeup); >>>>> hrtimer_set_expires(timer, iowq->timeout); >>>>> return HRTIMER_RESTART; >>>>> out_wake: >>>> >>>> The timer is known expired here, it's inside the callback. Is this >>>> really necessary or useful? >>> >>> It's not strictly required here, but in the end this allows to make the >>> .function member private, which prevents stupid code fiddling with it >>> without proper sanity checks in the debug case. >> >> While that makes sense, this is also a potentially hot path for min >> timeout usage, which with small timeouts for batch/latency reasons >> can be run tens of thousand times per second. Adding a lock and IRQ >> dance would be counter productive. > > I fundamentally hate the fact that C does not enforce encapsulation in > the first place. :) > >> How about we just add a comment on why this is fine, rather than > > Comments are the worst of a solution as you know. Sure I don't disagree - this is just my way of attempting to be nice and suggest alternatives, as I don't like this patch. >> slow down a case that's perfectly fine by wrapping it in something >> much more expensive than a simple memory write? Or perhaps have >> a basic helper to set it that doesn't do the unnecessary irq lock >> guard? That would still allow you to make .function private. > > The right solution is to add a hotpath helper, which falls back to the > more expensive variant with some anyway expensive debug option, or make > the expensive part of hrtimer_update_function() depend on that debug > option in the first place and otherwise fall back to a simple store. > > The latter is the right thing to do. Let me fix that up in mainline. Yep exactly, this is the way. Thanks! -- Jens Axboe
The sanity checks in hrtimer_update_function() are expensive for high
frequency usage like in the io/uring code due to locking.
Hide the sanity checks behind CONFIG_PROVE_LOCKING, which has a decent
chance to be enabled on a regular basis for testing.
Fixes: 8f02e3563bb5 ("hrtimers: Introduce hrtimer_update_function()")
Reported-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
include/linux/hrtimer.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -333,6 +333,7 @@ static inline int hrtimer_callback_runni
static inline void hrtimer_update_function(struct hrtimer *timer,
enum hrtimer_restart (*function)(struct hrtimer *))
{
+#ifdef CONFIG_PROVE_LOCKING
guard(raw_spinlock_irqsave)(&timer->base->cpu_base->lock);
if (WARN_ON_ONCE(hrtimer_is_queued(timer)))
@@ -340,7 +341,7 @@ static inline void hrtimer_update_functi
if (WARN_ON_ONCE(!function))
return;
-
+#endif
timer->function = function;
}On 2/7/25 2:16 PM, Thomas Gleixner wrote: > The sanity checks in hrtimer_update_function() are expensive for high > frequency usage like in the io/uring code due to locking. > > Hide the sanity checks behind CONFIG_PROVE_LOCKING, which has a decent > chance to be enabled on a regular basis for testing. Looks good to me, thanks Thomas. On my side, I always have a debug run done with PROVE_LOCKING and KASAN, fwiw. -- Jens Axboe
On Sat, Feb 08 2025 at 08:18, Jens Axboe wrote:
> On 2/7/25 2:16 PM, Thomas Gleixner wrote:
>> The sanity checks in hrtimer_update_function() are expensive for high
>> frequency usage like in the io/uring code due to locking.
>>
>> Hide the sanity checks behind CONFIG_PROVE_LOCKING, which has a decent
>> chance to be enabled on a regular basis for testing.
>
> Looks good to me, thanks Thomas. On my side, I always have a debug run
> done with PROVE_LOCKING and KASAN, fwiw.
I assume that with that your objections against the conversion of ioring
to hrtimer_update_function() is gone too.
Thanks,
tglx
On 2/10/25 12:52 PM, Thomas Gleixner wrote: > On Sat, Feb 08 2025 at 08:18, Jens Axboe wrote: > >> On 2/7/25 2:16 PM, Thomas Gleixner wrote: >>> The sanity checks in hrtimer_update_function() are expensive for high >>> frequency usage like in the io/uring code due to locking. >>> >>> Hide the sanity checks behind CONFIG_PROVE_LOCKING, which has a decent >>> chance to be enabled on a regular basis for testing. >> >> Looks good to me, thanks Thomas. On my side, I always have a debug run >> done with PROVE_LOCKING and KASAN, fwiw. > > I assume that with that your objections against the conversion of ioring > to hrtimer_update_function() is gone too. Certainly, with the expensive bits under PROVE_LOCKING, I have no objections to the patch. -- Jens Axboe
The following commit has been merged into the timers/core branch of tip:
Commit-ID: 2ea97b76d6712bfb0408e5b81ffd7bc4551d3153
Gitweb: https://git.kernel.org/tip/2ea97b76d6712bfb0408e5b81ffd7bc4551d3153
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Fri, 07 Feb 2025 22:16:09 +01:00
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 10 Feb 2025 20:51:12 +01:00
hrtimers: Make hrtimer_update_function() less expensive
The sanity checks in hrtimer_update_function() are expensive for high
frequency usage like in the io/uring code due to locking.
Hide the sanity checks behind CONFIG_PROVE_LOCKING, which has a decent
chance to be enabled on a regular basis for testing.
Fixes: 8f02e3563bb5 ("hrtimers: Introduce hrtimer_update_function()")
Reported-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/all/87ikpllali.ffs@tglx
---
include/linux/hrtimer.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index f7bfdcf..bdd55c1 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -333,6 +333,7 @@ static inline int hrtimer_callback_running(struct hrtimer *timer)
static inline void hrtimer_update_function(struct hrtimer *timer,
enum hrtimer_restart (*function)(struct hrtimer *))
{
+#ifdef CONFIG_PROVE_LOCKING
guard(raw_spinlock_irqsave)(&timer->base->cpu_base->lock);
if (WARN_ON_ONCE(hrtimer_is_queued(timer)))
@@ -340,7 +341,7 @@ static inline void hrtimer_update_function(struct hrtimer *timer,
if (WARN_ON_ONCE(!function))
return;
-
+#endif
timer->function = function;
}© 2016 - 2025 Red Hat, Inc.