[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a79b27d5-ae85-4b7c-bd08-ac7b345f3a20@roeck-us.net>
Date: Sat, 30 Nov 2024 10:21:49 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Thomas Gleixner <tglx@...utronix.de>, John Stultz <jstultz@...gle.com>
Cc: LKML <linux-kernel@...r.kernel.org>,
Anna-Maria Behnsen <anna-maria@...utronix.de>,
Frederic Weisbecker <frederic@...nel.org>, Stephen Boyd <sboyd@...nel.org>,
Peter Zijlstra <peterz@...radead.org>
Subject: Re: [patch 2/2] timekeeping: Always check for negative motion
On 11/30/24 03:09, Thomas Gleixner wrote:
> On Fri, Nov 29 2024 at 08:09, Guenter Roeck wrote:
>> On 11/29/24 04:16, Thomas Gleixner wrote:
>> [ 13.860000] WARNING: CPU: 0 PID: 0 at kernel/time/timekeeping_internal.h:44 timekeeping_advance+0x844/0x9d0
>> [ 13.860000] clocksource_delta() time going backward: now=0xd60127 last=0x85170f4 mask=0xffffff ret=0x849033
>
> So this is a idle sleep which took longer than max_idle_ns. The rest is
> the consequence of this as timekeeping does not advance and the timers
> are rearmed on the stale time.
>
> Can you try the patch below?
>
That one works. I booted kudo-bmc 60 times without failure. I also tested the
patch twice with all my qemu emulations, for a total of 1000+ boots with
different machines/configurations, and did not see a single failure.
Tested-by: Guenter Roeck <linux@...ck-us.net>
Thanks,
Guenter
> Thanks,
>
> tglx
> ---
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -49,6 +49,7 @@ struct module;
> * @archdata: Optional arch-specific data
> * @max_cycles: Maximum safe cycle value which won't overflow on
> * multiplication
> + * @max_raw_delta: Maximum safe delta value for negative motion detection
> * @name: Pointer to clocksource name
> * @list: List head for registration (internal)
> * @freq_khz: Clocksource frequency in khz.
> @@ -109,6 +110,7 @@ struct clocksource {
> struct arch_clocksource_data archdata;
> #endif
> u64 max_cycles;
> + u64 max_raw_delta;
> const char *name;
> struct list_head list;
> u32 freq_khz;
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -24,7 +24,7 @@ static void clocksource_enqueue(struct c
>
> static noinline u64 cycles_to_nsec_safe(struct clocksource *cs, u64 start, u64 end)
> {
> - u64 delta = clocksource_delta(end, start, cs->mask);
> + u64 delta = clocksource_delta(end, start, cs->mask, cs->max_raw_delta);
>
> if (likely(delta < cs->max_cycles))
> return clocksource_cyc2ns(delta, cs->mult, cs->shift);
> @@ -993,6 +993,15 @@ static inline void clocksource_update_ma
> cs->max_idle_ns = clocks_calc_max_nsecs(cs->mult, cs->shift,
> cs->maxadj, cs->mask,
> &cs->max_cycles);
> +
> + /*
> + * Threshold for detecting negative motion in clocksource_delta().
> + *
> + * Allow for 0.875 of the mask value so that overly long idle
> + * sleeps which go slightly over mask/2 do not trigger the negative
> + * motion detection.
> + */
> + cs->max_raw_delta = (cs->mask >> 1) + (cs->mask >> 2) + (cs->mask >> 3);
> }
>
> static struct clocksource *clocksource_find_best(bool oneshot, bool skipcur)
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -755,7 +755,8 @@ static void timekeeping_forward_now(stru
> u64 cycle_now, delta;
>
> cycle_now = tk_clock_read(&tk->tkr_mono);
> - delta = clocksource_delta(cycle_now, tk->tkr_mono.cycle_last, tk->tkr_mono.mask);
> + delta = clocksource_delta(cycle_now, tk->tkr_mono.cycle_last, tk->tkr_mono.mask,
> + tk->tkr_mono.clock->max_raw_delta);
> tk->tkr_mono.cycle_last = cycle_now;
> tk->tkr_raw.cycle_last = cycle_now;
>
> @@ -2230,7 +2231,8 @@ static bool timekeeping_advance(enum tim
> return false;
>
> offset = clocksource_delta(tk_clock_read(&tk->tkr_mono),
> - tk->tkr_mono.cycle_last, tk->tkr_mono.mask);
> + tk->tkr_mono.cycle_last, tk->tkr_mono.mask,
> + tk->tkr_mono.clock->max_raw_delta);
>
> /* Check if there's really nothing to do */
> if (offset < real_tk->cycle_interval && mode == TK_ADV_TICK)
> --- a/kernel/time/timekeeping_internal.h
> +++ b/kernel/time/timekeeping_internal.h
> @@ -30,15 +30,15 @@ static inline void timekeeping_inc_mg_fl
>
> #endif
>
> -static inline u64 clocksource_delta(u64 now, u64 last, u64 mask)
> +static inline u64 clocksource_delta(u64 now, u64 last, u64 mask, u64 max_delta)
> {
> u64 ret = (now - last) & mask;
>
> /*
> - * Prevent time going backwards by checking the MSB of mask in
> - * the result. If set, return 0.
> + * Prevent time going backwards by checking against the result
> + * against @max_delta. If greater, return 0.
> */
> - return ret & ~(mask >> 1) ? 0 : ret;
> + return ret > max_delta ? 0 : ret;
> }
>
> /* Semi public for serialization of non timekeeper VDSO updates. */
>
>
Powered by blists - more mailing lists