[PATCH v2 00/12] Consolidate hrtimer initialization - Part 5

[PATCH v2 00/12] Consolidate hrtimer initialization - Part 5

[Nam Cao]

All previous users of hrtimer_init() have been converted to use hrtimer_setup()
in the previous series:

  https://lore.kernel.org/lkml/cover.1738746821.git.namcao@linutronix.de
  https://lore.kernel.org/lkml/cover.1738746872.git.namcao@linutronix.de
  https://lore.kernel.org/lkml/cover.1738746904.git.namcao@linutronix.de


This series does the final cleanup:

  - Delete hrtimer_init()
  - Convert users who touch hrtimer::function to use hrtimer_update_function().
    Make hrtimer::function private afterward.
  - Rename the remaining *hrtimer_init*() to *hrtimer_setup*() to keep the
    names consistent

v1 -> v2 https://lore.kernel.org/lkml/cover.1729865740.git.namcao@linutronix.de
  - rebaes onto v6.14-rc1
---
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 Documentation/trace/ftrace.rst                |  4 +-
 .../net/wireless/ralink/rt2x00/rt2800mmio.c   |  2 +-
 .../net/wireless/ralink/rt2x00/rt2800usb.c    |  2 +-
 drivers/tty/serial/xilinx_uartps.c            |  4 +-
 include/linux/hrtimer.h                       |  4 +-
 include/linux/hrtimer_types.h                 |  4 +-
 include/trace/events/timer.h                  |  8 +--
 io_uring/io_uring.c                           |  2 +-
 kernel/time/hrtimer.c                         | 69 +++++--------------
 kernel/time/timer_list.c                      |  2 +-
 tools/perf/tests/shell/trace_btf_enum.sh      |  2 +-
 11 files changed, 35 insertions(+), 68 deletions(-)

-- 
2.39.5

[PATCH v2 01/12] hrtimers: Delete hrtimer_init()

[Nam Cao]

hrtimer_init() is unused. Delete it.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 include/linux/hrtimer.h       |  2 --
 include/linux/hrtimer_types.h |  2 +-
 kernel/time/hrtimer.c         | 20 --------------------
 3 files changed, 1 insertion(+), 23 deletions(-)

diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index acae379541c5..242ceafecde5 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -231,8 +231,6 @@ static inline enum hrtimer_restart hrtimer_dummy_timeout(struct hrtimer *unused)
 /* Exported timer functions: */
 
 /* Initialize timers: */
-extern void hrtimer_init(struct hrtimer *timer, clockid_t which_clock,
-			 enum hrtimer_mode mode);
 extern void hrtimer_setup(struct hrtimer *timer, enum hrtimer_restart (*function)(struct hrtimer *),
 			  clockid_t clock_id, enum hrtimer_mode mode);
 extern void hrtimer_setup_on_stack(struct hrtimer *timer,
diff --git a/include/linux/hrtimer_types.h b/include/linux/hrtimer_types.h
index ad66a3081735..7c5b27daa89d 100644
--- a/include/linux/hrtimer_types.h
+++ b/include/linux/hrtimer_types.h
@@ -34,7 +34,7 @@ enum hrtimer_restart {
  * @is_hard:	Set if hrtimer will be expired in hard interrupt context
  *		even on RT.
  *
- * The hrtimer structure must be initialized by hrtimer_init()
+ * The hrtimer structure must be initialized by hrtimer_setup()
  */
 struct hrtimer {
 	struct timerqueue_node		node;
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 47df6f50e12c..6474089f65fb 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1645,26 +1645,6 @@ static void __hrtimer_setup(struct hrtimer *timer,
 		timer->function = function;
 }
 
-/**
- * hrtimer_init - initialize a timer to the given clock
- * @timer:	the timer to be initialized
- * @clock_id:	the clock to be used
- * @mode:       The modes which are relevant for initialization:
- *              HRTIMER_MODE_ABS, HRTIMER_MODE_REL, HRTIMER_MODE_ABS_SOFT,
- *              HRTIMER_MODE_REL_SOFT
- *
- *              The PINNED variants of the above can be handed in,
- *              but the PINNED bit is ignored as pinning happens
- *              when the hrtimer is started
- */
-void hrtimer_init(struct hrtimer *timer, clockid_t clock_id,
-		  enum hrtimer_mode mode)
-{
-	debug_init(timer, clock_id, mode);
-	__hrtimer_init(timer, clock_id, mode);
-}
-EXPORT_SYMBOL_GPL(hrtimer_init);
-
 /**
  * hrtimer_setup - initialize a timer to the given clock
  * @timer:	the timer to be initialized
-- 
2.39.5

[PATCH v2 02/12] hrtimers: Switch to use __htimer_setup()

[Nam Cao]

__hrtimer_init_sleeper() calls __hrtimer_init() and also setups the
callback function. But there is already __hrtimer_setup() which does both
actions.

Switch to use __hrtimer_setup() to simplify the code.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 kernel/time/hrtimer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 6474089f65fb..d3e02c771b39 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -2066,8 +2066,7 @@ static void __hrtimer_init_sleeper(struct hrtimer_sleeper *sl,
 			mode |= HRTIMER_MODE_HARD;
 	}
 
-	__hrtimer_init(&sl->timer, clock_id, mode);
-	sl->timer.function = hrtimer_wakeup;
+	__hrtimer_setup(&sl->timer, hrtimer_wakeup, clock_id, mode);
 	sl->task = current;
 }
 
-- 
2.39.5

[PATCH v2 03/12] hrtimers: Merge __hrtimer_init() into __hrtimer_setup()

[Nam Cao]

__hrtimer_init() is only called by __hrtimer_setup(). Simplify by merging
__hrtimer_init() into __hrtimer_setup().

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 kernel/time/hrtimer.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index d3e02c771b39..728075f5c9c6 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1597,8 +1597,9 @@ static inline int hrtimer_clockid_to_base(clockid_t clock_id)
 	return HRTIMER_BASE_MONOTONIC;
 }
 
-static void __hrtimer_init(struct hrtimer *timer, clockid_t clock_id,
-			   enum hrtimer_mode mode)
+static void __hrtimer_setup(struct hrtimer *timer,
+			    enum hrtimer_restart (*function)(struct hrtimer *),
+			    clockid_t clock_id, enum hrtimer_mode mode)
 {
 	bool softtimer = !!(mode & HRTIMER_MODE_SOFT);
 	struct hrtimer_cpu_base *cpu_base;
@@ -1631,13 +1632,6 @@ static void __hrtimer_init(struct hrtimer *timer, clockid_t clock_id,
 	timer->is_hard = !!(mode & HRTIMER_MODE_HARD);
 	timer->base = &cpu_base->clock_base[base];
 	timerqueue_init(&timer->node);
-}
-
-static void __hrtimer_setup(struct hrtimer *timer,
-			    enum hrtimer_restart (*function)(struct hrtimer *),
-			    clockid_t clock_id, enum hrtimer_mode mode)
-{
-	__hrtimer_init(timer, clock_id, mode);
 
 	if (WARN_ON_ONCE(!function))
 		timer->function = hrtimer_dummy_timeout;
-- 
2.39.5

[PATCH v2 04/12] serial: xilinx_uartps: Use helper function hrtimer_update_function()

[Nam Cao]

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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/tty/serial/xilinx_uartps.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index efb124a19c01..fe457bf1e15b 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -454,7 +454,7 @@ static void cdns_uart_handle_tx(void *dev_id)
 
 	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED &&
 	    (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(port))) {
-		cdns_uart->tx_timer.function = &cdns_rs485_rx_callback;
+		hrtimer_update_function(&cdns_uart->tx_timer, cdns_rs485_rx_callback);
 		hrtimer_start(&cdns_uart->tx_timer,
 			      ns_to_ktime(cdns_calc_after_tx_delay(cdns_uart)), HRTIMER_MODE_REL);
 	}
@@ -734,7 +734,7 @@ static void cdns_uart_start_tx(struct uart_port *port)
 
 	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
 		if (!cdns_uart->rs485_tx_started) {
-			cdns_uart->tx_timer.function = &cdns_rs485_tx_callback;
+			hrtimer_update_function(&cdns_uart->tx_timer, cdns_rs485_tx_callback);
 			cdns_rs485_tx_setup(cdns_uart);
 			return hrtimer_start(&cdns_uart->tx_timer,
 					     ms_to_ktime(port->rs485.delay_rts_before_send),
-- 
2.39.5

[PATCH v2 05/12] io_uring: Use helper function hrtimer_update_function()

[Nam Cao]

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

Re: [PATCH v2 05/12] io_uring: Use helper function hrtimer_update_function()

[Jens Axboe]

On 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

Re: [PATCH v2 05/12] io_uring: Use helper function hrtimer_update_function()

[Thomas Gleixner]

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

Re: [PATCH v2 05/12] io_uring: Use helper function hrtimer_update_function()

[Jens Axboe]

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

Re: [PATCH v2 05/12] io_uring: Use helper function hrtimer_update_function()

[Thomas Gleixner]

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

Re: [PATCH v2 05/12] io_uring: Use helper function hrtimer_update_function()

[Jens Axboe]

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

[PATCH] hrtimers: Make hrtimer_update_function() less expensive

[Thomas Gleixner]

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;
 }
 

Re: [PATCH] hrtimers: Make hrtimer_update_function() less expensive

[Jens Axboe]

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

Re: [PATCH] hrtimers: Make hrtimer_update_function() less expensive

[Thomas Gleixner]

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

Re: [PATCH] hrtimers: Make hrtimer_update_function() less expensive

[Jens Axboe]

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

[tip: timers/core] hrtimers: Make hrtimer_update_function() less expensive

[tip-bot2 for Thomas Gleixner]

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;
 }
 

[PATCH v2 06/12] wifi: rt2x00: Switch to use hrtimer_update_function()

[Nam Cao]

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: Kalle Valo <kvalo@kernel.org>
---
 drivers/net/wireless/ralink/rt2x00/rt2800mmio.c | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800usb.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c b/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
index 5323acff962a..45775ecdf221 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
@@ -842,7 +842,7 @@ int rt2800mmio_probe_hw(struct rt2x00_dev *rt2x00dev)
 	/*
 	 * Set txstatus timer function.
 	 */
-	rt2x00dev->txstatus_timer.function = rt2800mmio_tx_sta_fifo_timeout;
+	hrtimer_update_function(&rt2x00dev->txstatus_timer, rt2800mmio_tx_sta_fifo_timeout);
 
 	/*
 	 * Overwrite TX done handler
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800usb.c b/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
index 160bef79acdb..b51a23300ba2 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
@@ -618,7 +618,7 @@ static int rt2800usb_probe_hw(struct rt2x00_dev *rt2x00dev)
 	/*
 	 * Set txstatus timer function.
 	 */
-	rt2x00dev->txstatus_timer.function = rt2800usb_tx_sta_fifo_timeout;
+	hrtimer_update_function(&rt2x00dev->txstatus_timer, rt2800usb_tx_sta_fifo_timeout);
 
 	/*
 	 * Overwrite TX done handler
-- 
2.39.5

[PATCH v2 07/12] hrtimers: Make callback function pointer private

[Nam Cao]

Make the field 'function' of struct hrtimer private, to prevent users from
changing this field in an unsafe way. hrtimer_update_function() should be
used if the callback function needs to be changed.

Signed-off-by: Nam Cao <namcao@linutronix.de>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/hrtimer.h       | 2 +-
 include/linux/hrtimer_types.h | 2 +-
 include/trace/events/timer.h  | 4 ++--
 kernel/time/hrtimer.c         | 8 ++++----
 kernel/time/timer_list.c      | 2 +-
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index 242ceafecde5..5facee69b7e2 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -344,7 +344,7 @@ static inline void hrtimer_update_function(struct hrtimer *timer,
 	if (WARN_ON_ONCE(!function))
 		return;
 
-	timer->function = function;
+	ACCESS_PRIVATE(timer, function) = function;
 }
 
 /* Forward a hrtimer so it expires after now: */
diff --git a/include/linux/hrtimer_types.h b/include/linux/hrtimer_types.h
index 7c5b27daa89d..8fbbb6bdf7a1 100644
--- a/include/linux/hrtimer_types.h
+++ b/include/linux/hrtimer_types.h
@@ -39,7 +39,7 @@ enum hrtimer_restart {
 struct hrtimer {
 	struct timerqueue_node		node;
 	ktime_t				_softexpires;
-	enum hrtimer_restart		(*function)(struct hrtimer *);
+	enum hrtimer_restart		(*__private function)(struct hrtimer *);
 	struct hrtimer_clock_base	*base;
 	u8				state;
 	u8				is_rel;
diff --git a/include/trace/events/timer.h b/include/trace/events/timer.h
index 1ef58a04fc57..f8c906be4cd0 100644
--- a/include/trace/events/timer.h
+++ b/include/trace/events/timer.h
@@ -235,7 +235,7 @@ TRACE_EVENT(hrtimer_start,
 
 	TP_fast_assign(
 		__entry->hrtimer	= hrtimer;
-		__entry->function	= hrtimer->function;
+		__entry->function	= ACCESS_PRIVATE(hrtimer, function);
 		__entry->expires	= hrtimer_get_expires(hrtimer);
 		__entry->softexpires	= hrtimer_get_softexpires(hrtimer);
 		__entry->mode		= mode;
@@ -271,7 +271,7 @@ TRACE_EVENT(hrtimer_expire_entry,
 	TP_fast_assign(
 		__entry->hrtimer	= hrtimer;
 		__entry->now		= *now;
-		__entry->function	= hrtimer->function;
+		__entry->function	= ACCESS_PRIVATE(hrtimer, function);
 	),
 
 	TP_printk("hrtimer=%p function=%ps now=%llu",
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 728075f5c9c6..027a63344259 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1326,7 +1326,7 @@ void hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 	struct hrtimer_clock_base *base;
 	unsigned long flags;
 
-	if (WARN_ON_ONCE(!timer->function))
+	if (WARN_ON_ONCE(!ACCESS_PRIVATE(timer, function)))
 		return;
 	/*
 	 * Check whether the HRTIMER_MODE_SOFT bit and hrtimer.is_soft
@@ -1634,9 +1634,9 @@ static void __hrtimer_setup(struct hrtimer *timer,
 	timerqueue_init(&timer->node);
 
 	if (WARN_ON_ONCE(!function))
-		timer->function = hrtimer_dummy_timeout;
+		ACCESS_PRIVATE(timer, function) = hrtimer_dummy_timeout;
 	else
-		timer->function = function;
+		ACCESS_PRIVATE(timer, function) = function;
 }
 
 /**
@@ -1748,7 +1748,7 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base,
 	raw_write_seqcount_barrier(&base->seq);
 
 	__remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE, 0);
-	fn = timer->function;
+	fn = ACCESS_PRIVATE(timer, function);
 
 	/*
 	 * Clear the 'is relative' flag for the TIME_LOW_RES case. If the
diff --git a/kernel/time/timer_list.c b/kernel/time/timer_list.c
index 1c311c46da50..958dd4194684 100644
--- a/kernel/time/timer_list.c
+++ b/kernel/time/timer_list.c
@@ -46,7 +46,7 @@ static void
 print_timer(struct seq_file *m, struct hrtimer *taddr, struct hrtimer *timer,
 	    int idx, u64 now)
 {
-	SEQ_printf(m, " #%d: <%pK>, %ps", idx, taddr, timer->function);
+	SEQ_printf(m, " #%d: <%pK>, %ps", idx, taddr, ACCESS_PRIVATE(timer, function));
 	SEQ_printf(m, ", S:%02x", timer->state);
 	SEQ_printf(m, "\n");
 	SEQ_printf(m, " # expires at %Lu-%Lu nsecs [in %Ld to %Ld nsecs]\n",
-- 
2.39.5

[PATCH v2 08/12] hrtimers: Remove unnecessary NULL check in hrtimer_start_range_ns()

[Nam Cao]

The 'function' field of struct hrtimer can only be changed using
hrtimer_setup*() or hrtimer_update_function(), and both already null-check
'function'. Therefore, null-checking 'function' in hrtimer_start_range_ns()
is not necessary.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 kernel/time/hrtimer.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 027a63344259..4878a2c7e4bd 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1326,8 +1326,6 @@ void hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 	struct hrtimer_clock_base *base;
 	unsigned long flags;
 
-	if (WARN_ON_ONCE(!ACCESS_PRIVATE(timer, function)))
-		return;
 	/*
 	 * Check whether the HRTIMER_MODE_SOFT bit and hrtimer.is_soft
 	 * match on CONFIG_PREEMPT_RT = n. With PREEMPT_RT check the hard
-- 
2.39.5

[PATCH v2 09/12] hrtimers: Rename __hrtimer_init_sleeper() to __hrtimer_setup_sleeper()

[Nam Cao]

All the hrtimer_init*() functions have been renamed to hrtimer_setup*().
Rename __hrtimer_init_sleeper() to __hrtimer_setup_sleeper() as well, to
keep the names consistent.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 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 4878a2c7e4bd..41133ee1e2b3 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -2021,7 +2021,7 @@ void hrtimer_sleeper_start_expires(struct hrtimer_sleeper *sl,
 	 * Make the enqueue delivery mode check work on RT. If the sleeper
 	 * was initialized for hard interrupt delivery, force the mode bit.
 	 * This is a special case for hrtimer_sleepers because
-	 * __hrtimer_init_sleeper() determines the delivery mode on RT so the
+	 * __hrtimer_setup_sleeper() determines the delivery mode on RT so the
 	 * fiddling with this decision is avoided at the call sites.
 	 */
 	if (IS_ENABLED(CONFIG_PREEMPT_RT) && sl->timer.is_hard)
@@ -2031,8 +2031,8 @@ void hrtimer_sleeper_start_expires(struct hrtimer_sleeper *sl,
 }
 EXPORT_SYMBOL_GPL(hrtimer_sleeper_start_expires);
 
-static void __hrtimer_init_sleeper(struct hrtimer_sleeper *sl,
-				   clockid_t clock_id, enum hrtimer_mode mode)
+static void __hrtimer_setup_sleeper(struct hrtimer_sleeper *sl,
+				    clockid_t clock_id, enum hrtimer_mode mode)
 {
 	/*
 	 * On PREEMPT_RT enabled kernels hrtimers which are not explicitly
@@ -2072,7 +2072,7 @@ void hrtimer_setup_sleeper_on_stack(struct hrtimer_sleeper *sl,
 				    clockid_t clock_id, enum hrtimer_mode mode)
 {
 	debug_init_on_stack(&sl->timer, clock_id, mode);
-	__hrtimer_init_sleeper(sl, clock_id, mode);
+	__hrtimer_setup_sleeper(sl, clock_id, mode);
 }
 EXPORT_SYMBOL_GPL(hrtimer_setup_sleeper_on_stack);
 
-- 
2.39.5

[PATCH v2 10/12] hrtimers: Rename debug_init() to debug_setup()

[Nam Cao]

All the hrtimer_init*() functions have been renamed to hrtimer_setup*().
Rename debug_init() to debug_setup() as well, to keep the names consistent.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 kernel/time/hrtimer.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 41133ee1e2b3..0d1b8bb66aeb 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -475,9 +475,7 @@ static inline void debug_hrtimer_activate(struct hrtimer *timer,
 static inline void debug_hrtimer_deactivate(struct hrtimer *timer) { }
 #endif
 
-static inline void
-debug_init(struct hrtimer *timer, clockid_t clockid,
-	   enum hrtimer_mode mode)
+static inline void debug_setup(struct hrtimer *timer, clockid_t clockid, enum hrtimer_mode mode)
 {
 	debug_hrtimer_init(timer);
 	trace_hrtimer_init(timer, clockid, mode);
@@ -1653,7 +1651,7 @@ static void __hrtimer_setup(struct hrtimer *timer,
 void hrtimer_setup(struct hrtimer *timer, enum hrtimer_restart (*function)(struct hrtimer *),
 		   clockid_t clock_id, enum hrtimer_mode mode)
 {
-	debug_init(timer, clock_id, mode);
+	debug_setup(timer, clock_id, mode);
 	__hrtimer_setup(timer, function, clock_id, mode);
 }
 EXPORT_SYMBOL_GPL(hrtimer_setup);
-- 
2.39.5

[PATCH v2 11/12] hrtimers: Rename debug_init_on_stack() to debug_setup_on_stack()

[Nam Cao]

All the hrtimer_init*() functions have been renamed to hrtimer_setup*().
Rename debug_init_on_stack() to debug_setup_on_stack() as well, to keep the
names consistent.

Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 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 0d1b8bb66aeb..a0727a45e8e3 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -481,8 +481,8 @@ static inline void debug_setup(struct hrtimer *timer, clockid_t clockid, enum hr
 	trace_hrtimer_init(timer, clockid, mode);
 }
 
-static inline void debug_init_on_stack(struct hrtimer *timer, clockid_t clockid,
-				       enum hrtimer_mode mode)
+static inline void debug_setup_on_stack(struct hrtimer *timer, clockid_t clockid,
+					enum hrtimer_mode mode)
 {
 	debug_hrtimer_init_on_stack(timer);
 	trace_hrtimer_init(timer, clockid, mode);
@@ -1670,7 +1670,7 @@ void hrtimer_setup_on_stack(struct hrtimer *timer,
 			    enum hrtimer_restart (*function)(struct hrtimer *),
 			    clockid_t clock_id, enum hrtimer_mode mode)
 {
-	debug_init_on_stack(timer, clock_id, mode);
+	debug_setup_on_stack(timer, clock_id, mode);
 	__hrtimer_setup(timer, function, clock_id, mode);
 }
 EXPORT_SYMBOL_GPL(hrtimer_setup_on_stack);
@@ -2069,7 +2069,7 @@ static void __hrtimer_setup_sleeper(struct hrtimer_sleeper *sl,
 void hrtimer_setup_sleeper_on_stack(struct hrtimer_sleeper *sl,
 				    clockid_t clock_id, enum hrtimer_mode mode)
 {
-	debug_init_on_stack(&sl->timer, clock_id, mode);
+	debug_setup_on_stack(&sl->timer, clock_id, mode);
 	__hrtimer_setup_sleeper(sl, clock_id, mode);
 }
 EXPORT_SYMBOL_GPL(hrtimer_setup_sleeper_on_stack);
-- 
2.39.5

[PATCH v2 12/12] tracing/timers: Rename hrtimer_init event to hrtimer_setup

[Nam Cao]

The function hrtimer_init() doesn't exist anymore. It was replaced by
hrtimer_setup().

Thus, rename the hrtimer_init trace event to hrtimer_setup to keep it
consistent.

Signed-off-by: Nam Cao <namcao@linutronix.de>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 Documentation/trace/ftrace.rst           | 4 ++--
 include/trace/events/timer.h             | 4 ++--
 kernel/time/hrtimer.c                    | 4 ++--
 tools/perf/tests/shell/trace_btf_enum.sh | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
index 2b74f96d09d5..c9e88bf65709 100644
--- a/Documentation/trace/ftrace.rst
+++ b/Documentation/trace/ftrace.rst
@@ -3077,7 +3077,7 @@ Notice that we lost the sys_nanosleep.
   # cat set_ftrace_filter
   hrtimer_run_queues
   hrtimer_run_pending
-  hrtimer_init
+  hrtimer_setup
   hrtimer_cancel
   hrtimer_try_to_cancel
   hrtimer_forward
@@ -3115,7 +3115,7 @@ Again, now we want to append.
   # cat set_ftrace_filter
   hrtimer_run_queues
   hrtimer_run_pending
-  hrtimer_init
+  hrtimer_setup
   hrtimer_cancel
   hrtimer_try_to_cancel
   hrtimer_forward
diff --git a/include/trace/events/timer.h b/include/trace/events/timer.h
index f8c906be4cd0..1641ae3e6ca0 100644
--- a/include/trace/events/timer.h
+++ b/include/trace/events/timer.h
@@ -185,12 +185,12 @@ TRACE_EVENT(timer_base_idle,
 		{ HRTIMER_MODE_REL_PINNED_HARD,	"REL|PINNED|HARD" })
 
 /**
- * hrtimer_init - called when the hrtimer is initialized
+ * hrtimer_setup - called when the hrtimer is initialized
  * @hrtimer:	pointer to struct hrtimer
  * @clockid:	the hrtimers clock
  * @mode:	the hrtimers mode
  */
-TRACE_EVENT(hrtimer_init,
+TRACE_EVENT(hrtimer_setup,
 
 	TP_PROTO(struct hrtimer *hrtimer, clockid_t clockid,
 		 enum hrtimer_mode mode),
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index a0727a45e8e3..1fba75f076d0 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -478,14 +478,14 @@ static inline void debug_hrtimer_deactivate(struct hrtimer *timer) { }
 static inline void debug_setup(struct hrtimer *timer, clockid_t clockid, enum hrtimer_mode mode)
 {
 	debug_hrtimer_init(timer);
-	trace_hrtimer_init(timer, clockid, mode);
+	trace_hrtimer_setup(timer, clockid, mode);
 }
 
 static inline void debug_setup_on_stack(struct hrtimer *timer, clockid_t clockid,
 					enum hrtimer_mode mode)
 {
 	debug_hrtimer_init_on_stack(timer);
-	trace_hrtimer_init(timer, clockid, mode);
+	trace_hrtimer_setup(timer, clockid, mode);
 }
 
 static inline void debug_activate(struct hrtimer *timer,
diff --git a/tools/perf/tests/shell/trace_btf_enum.sh b/tools/perf/tests/shell/trace_btf_enum.sh
index 8d1e6bbeac90..3b2a82e08807 100755
--- a/tools/perf/tests/shell/trace_btf_enum.sh
+++ b/tools/perf/tests/shell/trace_btf_enum.sh
@@ -6,7 +6,7 @@ err=0
 set -e
 
 syscall="landlock_add_rule"
-non_syscall="timer:hrtimer_init,timer:hrtimer_start"
+non_syscall="timer:hrtimer_setup,timer:hrtimer_start"
 
 TESTPROG="perf test -w landlock"
 
-- 
2.39.5