Yang, On Tue, 27 Oct 2015, Yang Yingliang wrote:
> Mask the cycle values before subtraction. So we can use this > validation while the clocksource mask is not 64-bits. > > Signed-off-by: Yang Yingliang <[email protected]> > Cc: Thomas Gleixner <[email protected]> > --- > kernel/time/timekeeping_internal.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/time/timekeeping_internal.h > b/kernel/time/timekeeping_internal.h > index 4ea005a..984f02e 100644 > --- a/kernel/time/timekeeping_internal.h > +++ b/kernel/time/timekeeping_internal.h > @@ -15,7 +15,7 @@ extern void tk_debug_account_sleep_time(struct timespec64 > *t); > #ifdef CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE > static inline cycle_t clocksource_delta(cycle_t now, cycle_t last, cycle_t > mask) > { > - cycle_t ret = (now - last) & mask; > + cycle_t ret = (now & mask) - (last & mask); I agree the original code is broken for all masks which are != CLOCKSOURCE_MASK(64). But your change does not work for actual wraparounds. You probably cannot trigger it for the 56bits of the arm architected timer, but that does not make it more correct. Assume a CLOCKSOURCE_MASK(32) and that the timer wrapped around since we last read it. last = 0xffffffff now = 0x01 So: ret = (0x01 & 0xffffffff) - (0xffffffff & 0xffffffff); --> ret = 0x01 - 0xffffffff; --> ret = ffffffff00000002; --> (s64) ret is < 0 !!! This is wrong as the clocksource legitimately wrapped around since we accessed it last. The correct solution to this is: ret = (now - last) & mask; negative = ret & ~(mask >> 1); return negative ? 0 : ret; So in the above case this will be: --> ret = (0x01 - 0xffffffff) & 0xffffffff; --> ret = 0x02; --> negative = 0x02 & ~(0x0fffffff); --> negative = 0; But for last = 0x1000 now = 0x01 --> ret = (0x01 - 0x1000) & 0xffffffff; --> ret = 0xfffff001; --> negative = 0xfffff001 & ~(0x0fffffff); --> negative = 0x80000000; And this is the case which we actually need to catch. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
