Re: [PATCH v9 22/32] timers: Keep the pinned timers separate from the others

Anna-Maria Behnsen
2023-12-06T10:23:44Z

Sebastian Siewior <bigeasy@linutronix.de> writes:

> On 2023-12-01 10:26:44 [+0100], Anna-Maria Behnsen wrote:
>> --- a/kernel/time/timer.c
>> +++ b/kernel/time/timer.c
>> @@ -1985,10 +1998,31 @@ static inline u64 __get_next_timer_interrupt(unsigned long basej, u64 basem,
>> return expires;
>> }
>>
>> - raw_spin_lock(&base->lock);
>> - nextevt = next_timer_interrupt(base, basej);
>> + base_local = this_cpu_ptr(&timer_bases[BASE_LOCAL]);
>> + base_global = this_cpu_ptr(&timer_bases[BASE_GLOBAL]);
>> +
>> + raw_spin_lock(&base_local->lock);
>> + raw_spin_lock_nested(&base_global->lock, SINGLE_DEPTH_NESTING);
>> +
>> + nextevt_local = next_timer_interrupt(base_local, basej);
>> + nextevt_global = next_timer_interrupt(base_global, basej);
>>
>> - if (base->timers_pending) {
>> + /*
>> + * Check whether the local event is expiring before or at the same
>> + * time as the global event.
>> + *
>> + * Note, that nextevt_global and nextevt_local might be based on
>> + * different base->clk values. So it's not guaranteed that
>> + * comparing with empty bases results in a correct local_first.
>
> This ends like an unsolved mystery case. Could you add why one should
> not worry about an incorrect local_first?
>
> But seriously, how far apart can they get and what difference does it
> make? At timer enqueue time clk equals jiffies. At this point one clk
> base could be at jiffies and the other might be a few jiffies before
> that.
> The next event (as in next_expiry) should be valid for both compare
> wise. Both must be larger than jiffies. The delta between jiffies and
> next event has to be less than NEXT_TIMER_MAX_DELTA for each base.
>
>> + */
>> + if (base_local->timers_pending && base_global->timers_pending)
>> + local_first = time_before_eq(nextevt_local, nextevt_global);
>> + else
>> + local_first = base_local->timers_pending;
>> +
>> + nextevt = local_first ? nextevt_local : nextevt_global;
>> +
>> + if (base_local->timers_pending || base_global->timers_pending) {
>> /* If we missed a tick already, force 0 delta */
>> if (time_before(nextevt, basej))
>> nextevt = basej;
>
> So if nextevt_local missed a tick and nextevt_global is
> NEXT_TIMER_MAX_DELTA-1 (so we get the largest difference possible
> between those two) then the time_before_eq() should still come out
> right. We could still miss more than one tick.
>

This problem was only there when comparing _empty_ bases
(!timer_base::timers_pending) because of the different base clocks and
the stale next_expiry.

But I didn't update the check and the comment after introducing the
forward of the next_expiry when !timer_base::timers_pending in
next_timer_interrupt(). So now it is sufficient to replace the
local_first detection by simply doing:

local_first = time_before_eq(nextevt_local, nextevt_global);

Will fix it and will also add a comment to next_timer_interrupt() where
the next_expiry is updated when !timer_base::timers_pending.

Thanks,

Anna-Maria