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

[Anna-Maria Behnsen]

Sebastian Siewior <bigeasy@xxxxxxxxxxxxx> 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