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

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Nov 28 2024 at 07:30, Guenter Roeck wrote:
> On 11/28/24 06:51, Thomas Gleixner wrote:
> [   19.090000] ###### now: b5ac62 last: d447e38 mask: ffffff return: 712e2a
> [   19.110000] ###### now: b9ebc3 last: db506d0 mask: ffffff return: 4e4f3
> [   19.120000] ###### now: bb842f last: db8d760 mask: ffffff return: 2accf
> [   19.160000] ###### now: c43f2e last: dbabfa8 mask: ffffff return: 97f86
>
> 'last' advances beyond 'mask', and after that the result is always bad. The call to
> clocksource_delta() is from timekeeping_advance().

This does not make any sense. The bits above the mask in cycle_last are
irrelevant:

        delta = (now - last) & mask;

> [    3.350000] ###### now:  6c4f last:  fe6a84 mask: ffffff return: 201cb    <---
> [    3.360000] ###### now: 40427 last: 10052cc mask: ffffff return: 3b15b    <---

       0x40427 - 0x10052cc = 0xffffffffff03b15b
       0xffffffffff03b15b & 0xffffff = 0x3b15b

That's not any different than before. The only difference is that the
return value is checked:

       return delta & ~(mask >> 1) ? 0 : delta;

But clearly none of the resulting deltas (after masking) has bit 23
set. So the function can't return 0, right?

Coming back to my earlier assumption vs. the max idle time. Here is a
long idle sleep:

> [   18.500000] ###### now: 45b0a2 last: d1c7050 mask: ffffff return: 294052
> [   19.090000] ###### now: b5ac62 last: d447e38 mask: ffffff return: 712e2a

The cycle interval is 125000 clock cycles per tick. That's a HZ=100
kernel, so the nominal clock frequency is 12.5 MHz.

  0x712e2a/12.5e6 = 0.593391s

which is close to the 597268854ns max_idle_ns value.

That's about 0.0776978s away from the point where the delta becomes >
mask/2. So there is a valid concern vs. these long idle sleeps on
machines with a small counter width.

But none of this explains the problems you are observing.

Can you instrument clocksource_delta() to print the values only when the
negative motion detection triggers?

         if (delta & ~(mask >> 1))
         	pr_info(....);

Thanks,

        tglx