Re: [PATCH] clocksource_cyc2ns: avoid overflowing 64 bits

[John Stultz]

On Wed, Nov 16, 2016 at 8:57 AM, Chris Metcalf <cmetcalf@xxxxxxxxxxxx> wrote:
> For large values of "mult" and long uptimes, the intermediate
> result of "cycles * mult" can overflow 64 bits.  For example,
> the tile platform uses this helper function; for a 1.2 GHz clock,
> we have mult = 853, and after 208.5 days, we overflow 64 bits.
>
> The fix is basically the same as the fix for arch/x86 __cycles_2_ns()
> in commit 4cecf6d401a0 ("sched, x86: Avoid unnecessary overflow in
> sched_clock"), using the new mult_frac() helper.
>
> In addition to tile, arm/plat-omap and blackfin also use this helper
> function, so will presumably hit similar issues.
>
> Signed-off-by: Chris Metcalf <cmetcalf@xxxxxxxxxxxx>
> ---
> By the way, this is the bug that I was looking for when I tripped over
> the missing bugfix for timekeeping_delta_to_ns() a couple of days ago :-)

Glad you found your bug! :)

>  include/linux/clocksource.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index 08398182f56e..b2a022acf232 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -175,7 +175,7 @@ static inline u32 clocksource_hz2mult(u32 hz, u32 shift_constant)
>   */
>  static inline s64 clocksource_cyc2ns(cycle_t cycles, u32 mult, u32 shift)
>  {
> -       return ((u64) cycles * mult) >> shift;
> +       return mult_frac(cycles, mult, 1ULL << shift);
>  }


So clocksource_cyc2ns() was never intended to be used with
indefinitely large cycle values, and it looks like tile and blackfin
are abusing the interface (the omap usage provide cycle deltas rather
then just the current counter value).

I'd suggest instead to move tile/blackfin to using the generic
sched_clock implementation that most of the architectures use, or
special case the code in the arch specific sched_clock
implementations(as x86 does) instead of modifying the common interface
to better handle a use case its not intended for.

thanks
-john