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

Groups > linux.kernel > #1650113 > unrolled thread

Re: [PATCH 5/7] RISC-V: arch/riscv/lib

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

  Re: [PATCH 5/7] RISC-V: arch/riscv/lib Palmer Dabbelt <palmer@dabbelt.com> - 2017-05-25 04:00 +0200
    Re: [PATCH 5/7] RISC-V: arch/riscv/lib Arnd Bergmann <arnd@arndb.de> - 2017-05-26 11:20 +0200
      Re: [PATCH 5/7] RISC-V: arch/riscv/lib Palmer Dabbelt <palmer@dabbelt.com> - 2017-06-06 07:00 +0200
        Re: [PATCH 5/7] RISC-V: arch/riscv/lib Arnd Bergmann <arnd@arndb.de> - 2017-06-06 11:40 +0200
          Re: [PATCH 5/7] RISC-V: arch/riscv/lib Palmer Dabbelt <palmer@dabbelt.com> - 2017-06-06 23:00 +0200
            Re: [PATCH 5/7] RISC-V: arch/riscv/lib Arnd Bergmann <arnd@arndb.de> - 2017-06-07 09:40 +0200

#1650113 — Re: [PATCH 5/7] RISC-V: arch/riscv/lib

On Tue, 23 May 2017 04:19:42 PDT (-0700), Arnd Bergmann wrote:
> On Tue, May 23, 2017 at 2:41 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
>> new file mode 100644
>> index 000000000000..f644e582f4b8
>> --- /dev/null
>> +++ b/arch/riscv/lib/Makefile
>
>> +
>> +void __delay(unsigned long cycles)
>> +{
>> +       u64 t0 = get_cycles();
>> +
>> +       while ((unsigned long)(get_cycles() - t0) < cycles)
>> +               cpu_relax();
>> +}
>> +
>> +void udelay(unsigned long usecs)
>> +{
>> +       u64 ucycles = (u64)usecs * timebase;
>> +       do_div(ucycles, 1000000U);
>> +       __delay((unsigned long)ucycles);
>> +}
>> +EXPORT_SYMBOL(udelay);
>> +
>> +void ndelay(unsigned long nsecs)
>> +{
>> +       u64 ncycles = (u64)nsecs * timebase;
>> +       do_div(ncycles, 1000000000U);
>> +       __delay((unsigned long)ncycles);
>> +}
>
> I'd be slightly worried about a global 'timebase' identifier that
> might conflict with a variable in some random driver.

Makes sense.  I've renamed it to riscv_timebase

  https://github.com/riscv/riscv-linux/commit/ed7d769e2c14e8809c3c125e0bba2978cb6fd37b

> Also, it would be good to replace the multiply+div64
> with a single multiplication here, see how x86 and arm do it
> (for the tsc/__timer_delay case).

Makes sense.  I think this should do it

  https://github.com/riscv/riscv-linux/commit/d397332f6ebff42f3ecb385e9cf3284fdeda6776

but I'm finding this hard to test as this only works for 2ms sleeps.  It seems
at least in the right ballpark

  [    0.048000] before 1000x usleep 1000
  [    1.060000] before 1000x nsleep 1000000
  [    2.072000] done

>       Arnd

Thanks for the feedback.  I'll incorporate this along with all the other
feedback into a v2.

[toc] | [next] | [standalone]

#1651247

On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> On Tue, 23 May 2017 04:19:42 PDT (-0700), Arnd Bergmann wrote:
>> On Tue, May 23, 2017 at 2:41 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:

>> Also, it would be good to replace the multiply+div64
>> with a single multiplication here, see how x86 and arm do it
>> (for the tsc/__timer_delay case).
>
> Makes sense.  I think this should do it
>
>   https://github.com/riscv/riscv-linux/commit/d397332f6ebff42f3ecb385e9cf3284fdeda6776
>
> but I'm finding this hard to test as this only works for 2ms sleeps.  It seems
> at least in the right ballpark

+ if (usecs > MAX_UDELAY_US) {
+ __delay((u64)usecs * riscv_timebase / 1000000ULL);
+ return;
+ }

You still do the 64-bit division here. What I meant is to completely
avoid the division and use a multiply+shift.

Also, you don't need to base anything on HZ, as you do not rely
on the delay calibration but always use a timer.

       Arnd

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

#1658344

On Fri, 26 May 2017 02:06:58 PDT (-0700), Arnd Bergmann wrote:
> On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> On Tue, 23 May 2017 04:19:42 PDT (-0700), Arnd Bergmann wrote:
>>> On Tue, May 23, 2017 at 2:41 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
>>> Also, it would be good to replace the multiply+div64
>>> with a single multiplication here, see how x86 and arm do it
>>> (for the tsc/__timer_delay case).
>>
>> Makes sense.  I think this should do it
>>
>>   https://github.com/riscv/riscv-linux/commit/d397332f6ebff42f3ecb385e9cf3284fdeda6776
>>
>> but I'm finding this hard to test as this only works for 2ms sleeps.  It seems
>> at least in the right ballpark
>
> + if (usecs > MAX_UDELAY_US) {
> + __delay((u64)usecs * riscv_timebase / 1000000ULL);
> + return;
> + }
>
> You still do the 64-bit division here. What I meant is to completely
> avoid the division and use a multiply+shift.

The goal here was to avoid the error case that ARM has on overflow and instead
just delay for the requested time.  This should only divide when the delay is
>=2ms, so the division won't cost much in comparison.

The normal case should have no division in it.

I can copy ARM's error handling if you think that's better, but it seemed more
complicated than just computing the correct answer.

> Also, you don't need to base anything on HZ, as you do not rely
> on the delay calibration but always use a timer.

That makes sense, I just based this blindly off the ARM version.  I'll see if
that lets me avoid unnecessary overflow for ndelay.  If it doesn't then I'd
prefer to just keep exactly the same constraints ARM has to avoid unexpected
behavior.

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

#1658543

On Tue, Jun 6, 2017 at 6:56 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> On Fri, 26 May 2017 02:06:58 PDT (-0700), Arnd Bergmann wrote:
>> On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>> On Tue, 23 May 2017 04:19:42 PDT (-0700), Arnd Bergmann wrote:
>>>> On Tue, May 23, 2017 at 2:41 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>>>> Also, it would be good to replace the multiply+div64
>>>> with a single multiplication here, see how x86 and arm do it
>>>> (for the tsc/__timer_delay case).
>>>
>>> Makes sense.  I think this should do it
>>>
>>>   https://github.com/riscv/riscv-linux/commit/d397332f6ebff42f3ecb385e9cf3284fdeda6776
>>>
>>> but I'm finding this hard to test as this only works for 2ms sleeps.  It seems
>>> at least in the right ballpark
>>
>> + if (usecs > MAX_UDELAY_US) {
>> + __delay((u64)usecs * riscv_timebase / 1000000ULL);
>> + return;
>> + }
>>
>> You still do the 64-bit division here. What I meant is to completely
>> avoid the division and use a multiply+shift.
>
> The goal here was to avoid the error case that ARM has on overflow and instead
> just delay for the requested time.  This should only divide when the delay is
>>=2ms, so the division won't cost much in comparison.
>
> The normal case should have no division in it.
>
> I can copy ARM's error handling if you think that's better, but it seemed more
> complicated than just computing the correct answer.

I think the intention originally was to avoid overflowing the 32-bit
argument in

 void __delay(unsigned long cycles)

If you need to delay for more than 4 billion clocksource cycles,
your code is still broken.

>> Also, you don't need to base anything on HZ, as you do not rely
>> on the delay calibration but always use a timer.
>
> That makes sense, I just based this blindly off the ARM version.  I'll see if
> that lets me avoid unnecessary overflow for ndelay.  If it doesn't then I'd
> prefer to just keep exactly the same constraints ARM has to avoid unexpected
> behavior.

Right, I should have been more specific here, as ARM has two implementations
(loop and timer) and it gets much easier if you know you can rely on the
timer to be available.

       Arnd

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

#1659200

On Tue, 06 Jun 2017 02:31:02 PDT (-0700), Arnd Bergmann wrote:
> On Tue, Jun 6, 2017 at 6:56 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> On Fri, 26 May 2017 02:06:58 PDT (-0700), Arnd Bergmann wrote:
>>> On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>>> On Tue, 23 May 2017 04:19:42 PDT (-0700), Arnd Bergmann wrote:
>>>>> On Tue, May 23, 2017 at 2:41 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>>
>>>>> Also, it would be good to replace the multiply+div64
>>>>> with a single multiplication here, see how x86 and arm do it
>>>>> (for the tsc/__timer_delay case).
>>>>
>>>> Makes sense.  I think this should do it
>>>>
>>>>   https://github.com/riscv/riscv-linux/commit/d397332f6ebff42f3ecb385e9cf3284fdeda6776
>>>>
>>>> but I'm finding this hard to test as this only works for 2ms sleeps.  It seems
>>>> at least in the right ballpark
>>>
>>> + if (usecs > MAX_UDELAY_US) {
>>> + __delay((u64)usecs * riscv_timebase / 1000000ULL);
>>> + return;
>>> + }
>>>
>>> You still do the 64-bit division here. What I meant is to completely
>>> avoid the division and use a multiply+shift.
>>
>> The goal here was to avoid the error case that ARM has on overflow and instead
>> just delay for the requested time.  This should only divide when the delay is
>>>=2ms, so the division won't cost much in comparison.
>>
>> The normal case should have no division in it.
>>
>> I can copy ARM's error handling if you think that's better, but it seemed more
>> complicated than just computing the correct answer.
>
> I think the intention originally was to avoid overflowing the 32-bit
> argument in
>
>  void __delay(unsigned long cycles)
>
> If you need to delay for more than 4 billion clocksource cycles,
> your code is still broken.

Maybe I'm crazy, but I thought the goal was to avoid overflowing on the
multiply.  Specifically, the code looks like

  udelay(long input) {
    long a = input * MUL_VAL;
    long b = a >> SHIFT_VAL;
    __delay(b);
  }

so the place there's extra overflow is at computing a, not b (the input to
__delay).  When I modified the ARM code I went and recalculated the point at
which the multiply would overflow and it matched the value from the ARM code,
which is 2000us.

While I can buy the argument that 2000us is still too long, the real reason I
wrote the code this way is because I thought it was easier than having an error
case.  If you think the error is better then I'll do it that way.

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

#1659480

On Tue, Jun 6, 2017 at 10:53 PM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> On Tue, 06 Jun 2017 02:31:02 PDT (-0700), Arnd Bergmann wrote:
>> On Tue, Jun 6, 2017 at 6:56 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>> On Fri, 26 May 2017 02:06:58 PDT (-0700), Arnd Bergmann wrote:
>>>> On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>>>> On Tue, 23 May 2017 04:19:42 PDT (-0700), Arnd Bergmann wrote:
>>>>>> On Tue, May 23, 2017 at 2:41 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>>>
>>>>>> Also, it would be good to replace the multiply+div64
>>>>>> with a single multiplication here, see how x86 and arm do it
>>>>>> (for the tsc/__timer_delay case).
>>>>>
>>>>> Makes sense.  I think this should do it
>>>>>
>>>>>   https://github.com/riscv/riscv-linux/commit/d397332f6ebff42f3ecb385e9cf3284fdeda6776
>>>>>
>>>>> but I'm finding this hard to test as this only works for 2ms sleeps.  It seems
>>>>> at least in the right ballpark
>>>>
>>>> + if (usecs > MAX_UDELAY_US) {
>>>> + __delay((u64)usecs * riscv_timebase / 1000000ULL);
>>>> + return;
>>>> + }
>>>>
>>>> You still do the 64-bit division here. What I meant is to completely
>>>> avoid the division and use a multiply+shift.
>>>
>>> The goal here was to avoid the error case that ARM has on overflow and instead
>>> just delay for the requested time.  This should only divide when the delay is
>>>>=2ms, so the division won't cost much in comparison.
>>>
>>> The normal case should have no division in it.
>>>
>>> I can copy ARM's error handling if you think that's better, but it seemed more
>>> complicated than just computing the correct answer.
>>
>> I think the intention originally was to avoid overflowing the 32-bit
>> argument in
>>
>>  void __delay(unsigned long cycles)
>>
>> If you need to delay for more than 4 billion clocksource cycles,
>> your code is still broken.
>
> Maybe I'm crazy, but I thought the goal was to avoid overflowing on the
> multiply.  Specifically, the code looks like
>
>   udelay(long input) {
>     long a = input * MUL_VAL;
>     long b = a >> SHIFT_VAL;
>     __delay(b);
>   }
>
> so the place there's extra overflow is at computing a, not b (the input to
> __delay).  When I modified the ARM code I went and recalculated the point at
> which the multiply would overflow and it matched the value from the ARM code,
> which is 2000us.

Ah, that's right. But then you might run into the next overflow at a larger
delay interval.

> While I can buy the argument that 2000us is still too long, the real reason I
> wrote the code this way is because I thought it was easier than having an error
> case.  If you think the error is better then I'll do it that way.

It's probably ok as long as the complexity is not in the inline wrapper, and you
avoid the division for small delays.

     Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web