diff mbox series

[v9,19/32] timers: add_timer_on(): Make sure TIMER_PINNED flag is set

Message ID 20231201092654.34614-20-anna-maria@linutronix.de
State New
Headers
Series timers: Move from a push remote at enqueue to a pull at expiry model |

Commit Message

Anna-Maria Behnsen Dec. 1, 2023, 9:26 a.m. UTC 
  When adding a timer to the timer wheel using add_timer_on(), it is an
implicitly pinned timer. With the timer pull at expiry time model in place,
TIMER_PINNED flag is required to make sure timers end up in proper base.

Add TIMER_PINNED flag unconditionally when add_timer_on() is executed.

Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/time/timer.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Sebastian Andrzej Siewior Dec. 5, 2023, 6:29 p.m. UTC | #1 
On 2023-12-01 10:26:41 [+0100], Anna-Maria Behnsen wrote:
> When adding a timer to the timer wheel using add_timer_on(), it is an
> implicitly pinned timer. With the timer pull at expiry time model in place,
> TIMER_PINNED flag is required to make sure timers end up in proper base.
> 
> Add TIMER_PINNED flag unconditionally when add_timer_on() is executed.

This is odd. I have some vague memory that this was already the case.
Otherwise all add_timer_on() users without TIMER_PINNED may get it wrong
due to optimisation. Looking at git history it was never the case and I
can't confuse it with hrtimer since it never supported the "_on()"
feature…
At least the mix timer in drivers/char/random.c is not using the PINNED
flag with _on(). So this was wrong then?…

Sebastian
Anna-Maria Behnsen Dec. 6, 2023, 9:57 a.m. UTC | #2 
Sebastian Siewior <bigeasy@linutronix.de> writes:

> On 2023-12-01 10:26:41 [+0100], Anna-Maria Behnsen wrote:
>> When adding a timer to the timer wheel using add_timer_on(), it is an
>> implicitly pinned timer. With the timer pull at expiry time model in place,
>> TIMER_PINNED flag is required to make sure timers end up in proper base.
>> 
>> Add TIMER_PINNED flag unconditionally when add_timer_on() is executed.
>
> This is odd. I have some vague memory that this was already the case.
> Otherwise all add_timer_on() users without TIMER_PINNED may get it wrong
> due to optimisation.

Which optimisation are you talking about? Are you talking about the
heuristic for finding the best CPU in get_target_base()? This heuristic
is not used for add_timer_on().

> Looking at git history it was never the case and I
> can't confuse it with hrtimer since it never supported the "_on()"
> feature…
> At least the mix timer in drivers/char/random.c is not using the PINNED
> flag with _on(). So this was wrong then?…

No, this it is not wrong, as at the moment timers expires always on the
CPU where they have been queued. So when a timer should be queued on a
dedicated CPU several approaches are valid:

- using add_timer_on() with or without TIMER_PINNED flag set to enqueue
  timers on any specified CPU

- use add_timer()/mod_timer()/... with TIMER_PINNED flag set - but this
  only works to enqueue timer on this CPU!

When using the add_timer()/mod_timer()/... functions without
TIMER_PINNED flag, the heuristic is used for finding the best CPU.

So without the timer pull model the change doesn't hurt.

But with the timer pull model in place, it is required to keep the
pinned and non pinned timers in separate per CPU wheels (local wheel =
TIMER_PINNED is set; global wheel = TIMER_PINNED is not set). So without
this change but with the timer pull model, the mix timer of random.c
would be enqueued on the dedicated CPU, but it would end up in the wrong
wheel (global wheel). And then the timer could also expire on another
CPUs as the global wheels are handled by the migrator when the CPU is
idle.

Does this makes it a little more clear, why the change is required and
why it is also valid right now?

Thanks,

	Anna-Maria
Sebastian Andrzej Siewior Dec. 6, 2023, 10:26 a.m. UTC | #3 
On 2023-12-06 10:57:57 [+0100], Anna-Maria Behnsen wrote:
> Sebastian Siewior <bigeasy@linutronix.de> writes:
> 
> > On 2023-12-01 10:26:41 [+0100], Anna-Maria Behnsen wrote:
> >> When adding a timer to the timer wheel using add_timer_on(), it is an
> >> implicitly pinned timer. With the timer pull at expiry time model in place,
> >> TIMER_PINNED flag is required to make sure timers end up in proper base.
> >> 
> >> Add TIMER_PINNED flag unconditionally when add_timer_on() is executed.
> >
> > This is odd. I have some vague memory that this was already the case.
> > Otherwise all add_timer_on() users without TIMER_PINNED may get it wrong
> > due to optimisation.
> 
> Which optimisation are you talking about? Are you talking about the
> heuristic for finding the best CPU in get_target_base()? This heuristic
> is not used for add_timer_on().

Yes, true. And nobody probably mixes add_timer_on() and mod_timer().

> Does this makes it a little more clear, why the change is required and
> why it is also valid right now?

Yes, all good. Thanks.

> Thanks,
> 
> 	Anna-Maria

Sebastian
Anna-Maria Behnsen Dec. 6, 2023, 10:46 a.m. UTC | #4 
Sebastian Siewior <bigeasy@linutronix.de> writes:

> On 2023-12-06 10:57:57 [+0100], Anna-Maria Behnsen wrote:
>> Sebastian Siewior <bigeasy@linutronix.de> writes:
>> 
>> > On 2023-12-01 10:26:41 [+0100], Anna-Maria Behnsen wrote:
>> >> When adding a timer to the timer wheel using add_timer_on(), it is an
>> >> implicitly pinned timer. With the timer pull at expiry time model in place,
>> >> TIMER_PINNED flag is required to make sure timers end up in proper base.
>> >> 
>> >> Add TIMER_PINNED flag unconditionally when add_timer_on() is executed.
>> >
>> > This is odd. I have some vague memory that this was already the case.
>> > Otherwise all add_timer_on() users without TIMER_PINNED may get it wrong
>> > due to optimisation.
>> 
>> Which optimisation are you talking about? Are you talking about the
>> heuristic for finding the best CPU in get_target_base()? This heuristic
>> is not used for add_timer_on().
>
> Yes, true. And nobody probably mixes add_timer_on() and mod_timer().

Workqueue mixes add_timer_on() and add_timer(). But therefore the
add_timer_global() function is introduced in patch 17 which drops the
TIMER_PINNED flag. In patch 18 the add_timer() in workqueue code is
replaced by the new function.

Thanks,

        Anna-Maria
diff mbox series

Patch

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 0ce0e6b25482..ea94479ee7e2 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1284,7 +1284,10 @@  EXPORT_SYMBOL(add_timer_global);
  * @timer:	The timer to be started
  * @cpu:	The CPU to start it on
  *
- * Same as add_timer() except that it starts the timer on the given CPU.
+ * Same as add_timer() except that it starts the timer on the given CPU and
+ * the TIMER_PINNED flag is set. When timer shouldn't be a pinned timer in
+ * the next round, add_timer_global() should be used instead as it unsets
+ * the TIMER_PINNED flag.
  *
  * See add_timer() for further details.
  */
@@ -1298,6 +1301,9 @@  void add_timer_on(struct timer_list *timer, int cpu)
 	if (WARN_ON_ONCE(timer_pending(timer)))
 		return;
 
+	/* Make sure timer flags have TIMER_PINNED flag set */
+	timer->flags |= TIMER_PINNED;
+
 	new_base = get_timer_cpu_base(timer->flags, cpu);
 
 	/*