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

Groups > linux.kernel > #1523776 > unrolled thread

[PATCH v2] tile: avoid using clocksource_cyc2ns with absolute cycle count

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2] tile: avoid using clocksource_cyc2ns with absolute cycle count Chris Metcalf <cmetcalf@mellanox.com> - 2016-11-16 20:40 +0100
    Re: [PATCH v2] tile: avoid using clocksource_cyc2ns with absolute  cycle count John Stultz <john.stultz@linaro.org> - 2016-11-16 21:00 +0100
      Re: [PATCH v2] tile: avoid using clocksource_cyc2ns with absolute  cycle count John Stultz <john.stultz@linaro.org> - 2016-11-16 21:30 +0100
        Re: [PATCH v2] tile: avoid using clocksource_cyc2ns with absolute  cycle count John Stultz <john.stultz@linaro.org> - 2016-11-16 21:40 +0100
      Re: [PATCH v2] tile: avoid using clocksource_cyc2ns with absolute  cycle count Peter Zijlstra <peterz@infradead.org> - 2016-11-17 11:00 +0100
        Re: [PATCH v2] tile: avoid using clocksource_cyc2ns with absolute  cycle count Peter Zijlstra <peterz@infradead.org> - 2016-11-18 11:40 +0100
          Re: [PATCH v2] tile: avoid using clocksource_cyc2ns with absolute  cycle count Peter Zijlstra <peterz@infradead.org> - 2016-11-18 16:00 +0100

#1523776 — [PATCH v2] tile: avoid using clocksource_cyc2ns with absolute cycle count

For large values of "mult" and long uptimes, the intermediate
result of "cycles * mult" can overflow 64 bits.  For example,
the tile platform calls clocksource_cyc2ns with a 1.2 GHz clock;
we have mult = 853, and after 208.5 days, we overflow 64 bits.

Since clocksource_cyc2ns() is intended to be used for relative
cycle counts, not absolute cycle counts, performance is more
importance than accepting a wider range of cycle values.
So, just use mult_frac() directly in tile's sched_clock().

Signed-off-by: Chris Metcalf <cmetcalf@mellanox.com>
---
Blackfin should make a similar change in their sched_clock().

 arch/tile/kernel/time.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/tile/kernel/time.c b/arch/tile/kernel/time.c
index 178989e6d3e3..ea960d660917 100644
--- a/arch/tile/kernel/time.c
+++ b/arch/tile/kernel/time.c
@@ -218,8 +218,8 @@ void do_timer_interrupt(struct pt_regs *regs, int fault_num)
  */
 unsigned long long sched_clock(void)
 {
-	return clocksource_cyc2ns(get_cycles(),
-				  sched_clock_mult, SCHED_CLOCK_SHIFT);
+	return mult_frac(get_cycles(),
+			 sched_clock_mult, 1ULL << SCHED_CLOCK_SHIFT);
 }
 
 int setup_profiling_timer(unsigned int multiplier)
-- 
2.7.2

[toc] | [next] | [standalone]

#1523788 — Re: [PATCH v2] tile: avoid using clocksource_cyc2ns with absolute cycle count

On Wed, Nov 16, 2016 at 11:35 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 calls clocksource_cyc2ns with a 1.2 GHz clock;
> we have mult = 853, and after 208.5 days, we overflow 64 bits.
>
> Since clocksource_cyc2ns() is intended to be used for relative
> cycle counts, not absolute cycle counts, performance is more
> importance than accepting a wider range of cycle values.
> So, just use mult_frac() directly in tile's sched_clock().
>
> Signed-off-by: Chris Metcalf <cmetcalf@mellanox.com>
> ---
> Blackfin should make a similar change in their sched_clock().
>
>  arch/tile/kernel/time.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/tile/kernel/time.c b/arch/tile/kernel/time.c
> index 178989e6d3e3..ea960d660917 100644
> --- a/arch/tile/kernel/time.c
> +++ b/arch/tile/kernel/time.c
> @@ -218,8 +218,8 @@ void do_timer_interrupt(struct pt_regs *regs, int fault_num)
>   */
>  unsigned long long sched_clock(void)
>  {
> -       return clocksource_cyc2ns(get_cycles(),
> -                                 sched_clock_mult, SCHED_CLOCK_SHIFT);
> +       return mult_frac(get_cycles(),
> +                        sched_clock_mult, 1ULL << SCHED_CLOCK_SHIFT);
>  }

So... looking closer at mult_frac(), its a really slow implementation,
doing 2 divs and a mod and a mult. Hopefully the compiler can sort out
the divs are power of two, and optimize it out, but I'm still
hesitant.

sched_clock() is normally a very hot-path call, so this might have a
real performance impact, especially compared to what its replacing.

In your earlier patch, you mentioned this was similar to 4cecf6d401a0
("sched, x86: Avoid unnecessary overflow in
sched_clock"). It might be better to actually try to use similar logic
there, to make sure the performance impact is minimal.

thanks
-john

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

#1523806 — Re: [PATCH v2] tile: avoid using clocksource_cyc2ns with absolute cycle count

On Wed, Nov 16, 2016 at 12:16 PM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
> On 11/16/2016 2:59 PM, John Stultz wrote:
>>
>> In your earlier patch, you mentioned this was similar to 4cecf6d401a0
>> ("sched, x86: Avoid unnecessary overflow in
>> sched_clock"). It might be better to actually try to use similar logic
>> there, to make sure the performance impact is minimal.
>
>
> This was the first thing I looked at when I saw the mult_frac()
> implementation.  The modulus operations are indeed converted to
> bitmasks and the divides to shifts.  We do have to do two multiplies
> instead of one, but that's basically the worst of the cost.
>
> Change 4cecf6d401a0 results in essentially identical code for x86 as
> this proposed change does for tile.  In fact a follow-on change by
> Salman introduced mult_frac() and switched to using it, so it was
> identical at that point.
>
> PeterZ (cc'ed) then improved it to use __int128 math via
> mul_u64_u32_shr(), but that doesn't help tile; we only do one multiply
> instead of two, but the multiply is handled by an out-of-line call to
> __multi3, and the sched_clock() function ends up about 2.5x slower as
> a result.
>
> Thanks for thinking about this!

Heh. Thanks for the history lesson and apologies for my forgetfulness. :)

thanks
-john

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

#1523813 — Re: [PATCH v2] tile: avoid using clocksource_cyc2ns with absolute cycle count

On Wed, Nov 16, 2016 at 12:29 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Wed, Nov 16, 2016 at 12:16 PM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
>> Change 4cecf6d401a0 results in essentially identical code for x86 as
>> this proposed change does for tile.  In fact a follow-on change by
>> Salman introduced mult_frac() and switched to using it, so it was
>> identical at that point.
>>
>> PeterZ (cc'ed) then improved it to use __int128 math via
>> mul_u64_u32_shr(), but that doesn't help tile; we only do one multiply
>> instead of two, but the multiply is handled by an out-of-line call to
>> __multi3, and the sched_clock() function ends up about 2.5x slower as
>> a result.
>>
>> Thanks for thinking about this!
>
> Heh. Thanks for the history lesson and apologies for my forgetfulness. :)

Oh.. and some of these details might be useful to have in the commit message!

thanks
-john

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

#1524219 — Re: [PATCH v2] tile: avoid using clocksource_cyc2ns with absolute cycle count

On Wed, Nov 16, 2016 at 03:16:59PM -0500, Chris Metcalf wrote:
> PeterZ (cc'ed) then improved it to use __int128 math via
> mul_u64_u32_shr(), but that doesn't help tile; we only do one multiply
> instead of two, but the multiply is handled by an out-of-line call to
> __multi3, and the sched_clock() function ends up about 2.5x slower as
> a result.

Well, only if you set CONFIG_ARCH_SUPPORTS_INT128, otherwise it reduces
to 2 32x23->64 multiplications, of which one if conditional on there
actually being bits set in the high word of the u64 argument.

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

#1525166 — Re: [PATCH v2] tile: avoid using clocksource_cyc2ns with absolute cycle count

On Thu, Nov 17, 2016 at 03:00:14PM -0500, Chris Metcalf wrote:
> On 11/17/2016 4:53 AM, Peter Zijlstra wrote:
> >On Wed, Nov 16, 2016 at 03:16:59PM -0500, Chris Metcalf wrote:
> >>PeterZ (cc'ed) then improved it to use __int128 math via
> >>mul_u64_u32_shr(), but that doesn't help tile; we only do one multiply
> >>instead of two, but the multiply is handled by an out-of-line call to
> >>__multi3, and the sched_clock() function ends up about 2.5x slower as
> >>a result.
> >Well, only if you set CONFIG_ARCH_SUPPORTS_INT128, otherwise it reduces
> >to 2 32x23->64 multiplications, of which one if conditional on there
> >actually being bits set in the high word of the u64 argument.
> 
> I didn't notice that.  It took me down an interesting rathole.
> 
> Obviously the branch optimization won't help on cycle counter values,
> since we blow out of the low 32 bits in the first few seconds of
> uptime.  So the conditional test won't help, but the 32x32
> multiply optimizations should.

Now, I don't quite remember things, but isn't it the idea to convert
cycle deltas and accumulate in ns? That way you most always convert
small values.

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

#1525423 — Re: [PATCH v2] tile: avoid using clocksource_cyc2ns with absolute cycle count

On Fri, Nov 18, 2016 at 09:24:52AM -0500, Chris Metcalf wrote:
> I would think you would also unnecessarily accumulate small errors.

True..

> The x86 sched_clock() seems to purely scale the current TSC value,
> so what tile is doing is consistent with that, at least.

Right, this comes apart the moment TSC goes faster than 1GHz though.
Which might actually be the case, because then the mult-and-shift
reduces resolution and we'd wrap before the 64bit are done.

That would be something I ought to look at some time..

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web