| Message ID | 1445952073-7260-2-git-send-email-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <yangyingliang@huawei.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > 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
On 2015/10/30 22:56, Thomas Gleixner wrote: > 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 <yangyingliang@huawei.com> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> --- >> 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; Thanks for your advise. I will resend this patch. Regards, Yang
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); return (s64) ret > 0 ? ret : 0; }
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 <yangyingliang@huawei.com> Cc: Thomas Gleixner <tglx@linutronix.de> --- kernel/time/timekeeping_internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)