Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1523657 > unrolled thread

[PATCH] clocksource_cyc2ns: avoid overflowing 64 bits

Back to article view | Back to linux.kernel

Contents

  [PATCH] clocksource_cyc2ns: avoid overflowing 64 bits Chris Metcalf <cmetcalf@mellanox.com> - 2016-11-16 18:00 +0100
    Re: [PATCH] clocksource_cyc2ns: avoid overflowing 64 bits John Stultz <john.stultz@linaro.org> - 2016-11-16 19:10 +0100
      Re: [PATCH] clocksource_cyc2ns: avoid overflowing 64 bits John Stultz <john.stultz@linaro.org> - 2016-11-16 20:50 +0100
        Re: [PATCH] clocksource_cyc2ns: avoid overflowing 64 bits John Stultz <john.stultz@linaro.org> - 2016-11-16 21:10 +0100
      Re: [PATCH] clocksource_cyc2ns: avoid overflowing 64 bits John Stultz <john.stultz@linaro.org> - 2016-11-16 20:50 +0100

#1523657 — [PATCH] clocksource_cyc2ns: avoid overflowing 64 bits

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@mellanox.com>
---
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 :-)

 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);
 }
 
 
-- 
2.7.2

[toc] | [next] | [standalone]

#1523712

On Wed, Nov 16, 2016 at 8:57 AM, Chris Metcalf <cmetcalf@mellanox.com> 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@mellanox.com>
> ---
> 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

[toc] | [prev] | [next] | [standalone]

#1523782

On Wed, Nov 16, 2016 at 11:30 AM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index 08398182f56e..5444429884b8 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -171,6 +171,10 @@ static inline u32 clocksource_hz2mult(u32 hz, u32
> shift_constant)
>   *
>   * Converts cycles to nanoseconds, using the given mult and shift.
>   *
> + * The code is optimized for performance and not intended to work
> + * with absolute clocksource cycles, as it will easily overflow,
> + * but just intended for relative (delta) clocksource cycles.
> + *
>   * XXX - This could use some mult_lxl_ll() asm optimization

Just as a heads up, it seems your working against an older kernel, as
this didn't apply. Its simple enough to fix up, so I'll do so, but in
the future, please submit patches against something close to Linus
HEAD.

thanks
-john

[toc] | [prev] | [next] | [standalone]

#1523797

On Wed, Nov 16, 2016 at 11:56 AM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
> On 11/16/2016 2:45 PM, John Stultz wrote:
>>
>> On Wed, Nov 16, 2016 at 11:30 AM, Chris Metcalf <cmetcalf@mellanox.com>
>> wrote:
>>>
>>> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
>>> index 08398182f56e..5444429884b8 100644
>>> --- a/include/linux/clocksource.h
>>> +++ b/include/linux/clocksource.h
>>> @@ -171,6 +171,10 @@ static inline u32 clocksource_hz2mult(u32 hz, u32
>>> shift_constant)
>>>    *
>>>    * Converts cycles to nanoseconds, using the given mult and shift.
>>>    *
>>> + * The code is optimized for performance and not intended to work
>>> + * with absolute clocksource cycles, as it will easily overflow,
>>> + * but just intended for relative (delta) clocksource cycles.
>>> + *
>>>    * XXX - This could use some mult_lxl_ll() asm optimization
>>
>> Just as a heads up, it seems your working against an older kernel, as
>> this didn't apply. Its simple enough to fix up, so I'll do so, but in
>> the future, please submit patches against something close to Linus
>> HEAD.
>
>
> Oops, sorry; it wasn't version skew (I'm at v4.9-rc4) but whitespace damage.
> I assumed if I just pasted the patch into Thunderbird it would work, since
> it had
> no tabs.  But bizarrely, if I look at the patch in the mailer, it shows a
> two-space
> prefix, but when I save the email to a file, it has a three-space prefix.
> WTF?

Yea. Not many mailers can be trusted with sending patches. I'd
recommend git-send-email. :)

thanks
-john

[toc] | [prev] | [next] | [standalone]

#1523784

On Wed, Nov 16, 2016 at 11:30 AM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
> On 11/16/2016 1:04 PM, John Stultz wrote:
>>
>> On Wed, Nov 16, 2016 at 8:57 AM, Chris Metcalf <cmetcalf@mellanox.com>
>> wrote:
>>>
>>>   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).
>
>
> Well, the interface does just say "convert clocksource cycles to
> nanoseconds". :-)

Right, and I can understand the confusion, but its not being used with
a struct clocksource. Its just being used to convert get_cycles().

> If you think it's more important that it be a little faster, we should
> adjust the
> documentation to say it is only appropriate for delta-cycles, not absolute
> cycles.
> I've appended a commit that does this if you'd like to take it.

That's fair. Thanks for sending that, II'll queue that in my tree here
in a moment.

>> 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.
>
>
> Yes, since tile has a full 64-bit cycle counter, the best thing is to just
> directly
> open-code the mult_frac() in tile's sched_clock().  I'll push that change.
> Steven Miao, I assume you should do the same for blackfin.

thanks
-john

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web