Re: [PATCH 1/2] clocksource: replace cycle_last validation with an equal way

[Thomas Gleixner]

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@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
>  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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/