Re: [patch 2/2] timekeeping: Always check for negative motion

[Guenter Roeck]

On 11/28/24 09:13, Guenter Roeck wrote:
> On 11/28/24 07:57, Guenter Roeck wrote:
> > On 11/28/24 06:51, Thomas Gleixner wrote:
> > > On Wed, Nov 27 2024 at 15:02, Guenter Roeck wrote:
> > > > On 11/27/24 14:08, John Stultz wrote:
> > > > An example log is at [1]. It says
> > > >
> > > > clocksource: npcm7xx-timer1: mask: 0xffffff max_cycles: 0xffffff, max_idle_ns: 597268854 ns
> > >
> > > That's a 24bit counter. So negative motion happens when the readouts are
> > > more than (1 << 23) apart. AFAICT the counter runs with about 14MHz, but
> > > I'd like to have that confirmed.
> > >
> > > > clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
> > > > ...
> > > > clocksource: Switched to clocksource npcm7xx-timer1
> > > >
> > > > I don't know where exactly it stalls; sometime after handover to userspace.
> > > > I'll be happy to do some more debugging, but you'll nee to let me know what
> > > > to look for.
> > >
> > > On that platform max_idle_ns should correspond to 50% of the counter
> > > width. So if both CPUs go deep idle for max_idle_ns, then the next timer
> > > interrupt doing the timeeeping advancement sees a delta of > (1 << 23)
> > > and timekeeping stalls.
> > >
> > > If my ssumption is correct, then the below should fix it.
> >
> > While that didn't work, the following code does.
> >
> > Guenter
> >
> > ---
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index 0ca85ff4fbb4..bd88c04ae357 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -2190,7 +2190,7 @@ static u64 logarithmic_accumulation(struct timekeeper *tk, u64 offset,
> >          /* Accumulate one shifted interval */
> >          offset -= interval;
> >          tk->tkr_mono.cycle_last += interval;
> > -       tk->tkr_raw.cycle_last  += interval;
> > +       tk->tkr_raw.cycle_last  = (tk->tkr_raw.cycle_last + interval) & tk->tkr_mono.mask;
>                                           ^^^^^^^                              ^^^^^^^^
>
> No idea what I was testing earlier, but that obviously doesn't work either, and masking
> both tkr_raw.cycle_last and tk->tkr_mono.cycle_last also doesn't work. Sorry for the noise.

The patch below works better, but it still stalls as soon as the counter wraps,
i.e., when the function returns 0.

Guenter

---
diff --git a/kernel/time/timekeeping_internal.h b/kernel/time/timekeeping_internal.h
index 63e600e943a7..3fe10407b1d5 100644
--- a/kernel/time/timekeeping_internal.h
+++ b/kernel/time/timekeeping_internal.h
@@ -32,7 +32,7 @@ static inline void timekeeping_inc_mg_floor_swaps(void)

 static inline u64 clocksource_delta(u64 now, u64 last, u64 mask)
 {
-       u64 ret = (now - last) & mask;
+       u64 ret = (now - (last & mask)) & mask;

        /*
         * Prevent time going backwards by checking the MSB of mask in