* Too much error in __const_udelay() ?
@ 2004-06-05 7:12 john stultz
2004-06-05 15:23 ` Dominik Brodowski
0 siblings, 1 reply; 12+ messages in thread
From: john stultz @ 2004-06-05 7:12 UTC (permalink / raw)
To: lkml; +Cc: george anzinger, Dominik Brodowski, greg kh, Chris McDermott
[-- Attachment #1: Type: text/plain, Size: 2700 bytes --]
[Resending due to lkml bouncing me for having html attachments]
Hey all,
I've been hunting a bug on one of our systems and it seems to closely
resemble the __delay() issues we saw w/ the ACPI PM time source.
Earlier when implementing the ACPI PM time source, we found that we
couldn't use the actual ACPI PM time source for the __delay function, or
else various subsystems would break. I chalked it up to the 8Mhz counter
being too low res for some callers of delay(), causing the waits to be
far too long. We backed off to just using the TSC for delay() and things
seemed to get better.
However I've started to see some problems w/ 2.6 and USB on x440/x445s,
both of which use the 100Mhz cyclone time source. Further digging has
pointed to the fact that certain important udelay()s in the USB
subsystem aren't actually waiting long enough.
So far I've narrowed it down to the scaled math bits in
__const_udelay() causing too much error for loops_per_jiffy values
around the 100,000 level the cyclone timesource uses.
To demonstrate this, I wrote the attached demo app using the
__const_udelay code. For those not wanting to run it themselves, its
output (for HZ=1000) looks like:
1 usec: LPJ: 100000 __udelay: 0 vs my_udelay: 100
1 usec: LPJ: 1500000 __udelay: 1000 vs my_udelay: 1500
2 usec: LPJ: 100000 __udelay: 0 vs my_udelay: 200
2 usec: LPJ: 1500000 __udelay: 2000 vs my_udelay: 3000
5 usec: LPJ: 100000 __udelay: 0 vs my_udelay: 500
5 usec: LPJ: 1500000 __udelay: 7000 vs my_udelay: 7500
10 usec: LPJ: 100000 __udelay: 0 vs my_udelay: 1000
10 usec: LPJ: 1500000 __udelay: 14000 vs my_udelay: 15000
20 usec: LPJ: 100000 __udelay: 1000 vs my_udelay: 2000
20 usec: LPJ: 1500000 __udelay: 29000 vs my_udelay: 30000
50 usec: LPJ: 100000 __udelay: 4000 vs my_udelay: 5000
50 usec: LPJ: 1500000 __udelay: 74000 vs my_udelay: 75000
100 usec: LPJ: 100000 __udelay: 9000 vs my_udelay: 10000
100 usec: LPJ: 1500000 __udelay: 149000 vs my_udelay: 150000
Here you can see __udelay() fails to be even close to accurate until
~50usec and returns zero for values less then 20usec.
I then went and measured the same udelay() values via rdtsc in the
kernel for both the cyclone based delay() as well as the tsc based
delay. The results are attached in the html file. You'll notice this
closely matches the results from the demo app.
This issue hasn't bitten me before w/ 2.4 because (as you can show w/
the demo app) __const_udelay() is more accurate w/ HZ=100.
I tried replacing __const_udelay w/ my_delay() but it didn't boot
(overflow issues, I'm guessing).
So I'm no math wiz. What's the proper fix here?
thanks
-john
[-- Attachment #2: test.c --]
[-- Type: text/x-c, Size: 1291 bytes --]
#include <stdio.h>
#define MILLION 1000000
#define HZ 1000
unsigned long tsc_freq = 1500000000UL;
unsigned long cyclone_freq = 100000000UL;
unsigned long tsc_lpj = 1500000000UL/HZ;
unsigned long cyclone_lpj = 100000000UL/HZ;
unsigned long LPJ;
unsigned long __delay(unsigned long loops)
{
return loops;
}
unsigned long __const_udelay(unsigned long xloops)
{
int d0;
__asm__("mull %0"
:"=d" (xloops), "=&a" (d0)
:"1" (xloops),"0" (LPJ));
return __delay(xloops * HZ);
}
unsigned long my_udelay(unsigned long usec)
{
unsigned long cyc_per_usec = (LPJ*HZ)/MILLION;
return __delay(usec*cyc_per_usec);
}
unsigned long __udelay(unsigned long usecs)
{
return __const_udelay(usecs * 0x000010c6); /* 2**32 / 1000000 */
}
unsigned long __ndelay(unsigned long nsecs)
{
return __const_udelay(nsecs * 0x00005); /* 2**32 / 1000000000 (rounded up) */
}
int main(void)
{
unsigned long time[7] = {1,2,5,10,20,50,100};
int i;
for(i=0;i < 7; i++){
LPJ=cyclone_lpj;
printf("%3i usec: LPJ: %7lu __udelay: %5lu vs my_udelay: %5lu\n",
time[i], LPJ, __udelay(time[i]), my_udelay(time[i]));
LPJ=tsc_lpj;
printf("%3i usec: LPJ: %lu __udelay: %5lu vs my_udelay: %5lu\n",
time[i], LPJ, __udelay(time[i]), my_udelay(time[i]));
printf("\n");
}
return 0;
}
[-- Attachment #3: delay-differences.html.gz --]
[-- Type: application/x-gzip, Size: 1343 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Too much error in __const_udelay() ?
2004-06-05 7:12 Too much error in __const_udelay() ? john stultz
@ 2004-06-05 15:23 ` Dominik Brodowski
2004-06-06 19:46 ` Pavel Machek
2004-06-07 19:12 ` john stultz
0 siblings, 2 replies; 12+ messages in thread
From: Dominik Brodowski @ 2004-06-05 15:23 UTC (permalink / raw)
To: john stultz; +Cc: lkml, george anzinger, greg kh, Chris McDermott
Hi,
> However I've started to see some problems w/ 2.6 and USB on x440/x445s,
> both of which use the 100Mhz cyclone time source. Further digging has
> pointed to the fact that certain important udelay()s in the USB
> subsystem aren't actually waiting long enough.
Certain? AFAICS _no_ call to a delay routine actually passed a big enough
argument. Or am I missing something? Also, __ndelay seems to be affected
as well: it returns zero for 550 nsec even for the TSC variant in your
test.c.
> So I'm no math wiz. What's the proper fix here?
Below are three changes I'd like to discuss. I'll build a fresh kernel with
all three changes enabled + PM_TIMER soon.
Change 1:
Move the multiplication with HZ up into the mull instruction:
unsigned long __const_udelay(unsigned long xloops)
{
int d0;
__asm__("mull %0"
:"=d" (xloops), "=&a" (d0)
:"1" (xloops),"0" (LPJ * HZ));
return __delay(xloops);
}
1 usec: LPJ: 100000 __udelay: 0 vs my_udelay: 99
1 usec: LPJ: 1500000 __udelay: 1000 vs my_udelay: 1499
2 usec: LPJ: 100000 __udelay: 0 vs my_udelay: 199
2 usec: LPJ: 1500000 __udelay: 2000 vs my_udelay: 2999
5 usec: LPJ: 100000 __udelay: 0 vs my_udelay: 499
5 usec: LPJ: 1500000 __udelay: 7000 vs my_udelay: 7498
10 usec: LPJ: 100000 __udelay: 0 vs my_udelay: 999
10 usec: LPJ: 1500000 __udelay: 14000 vs my_udelay: 14996
20 usec: LPJ: 100000 __udelay: 1000 vs my_udelay: 1999
20 usec: LPJ: 1500000 __udelay: 29000 vs my_udelay: 29993
50 usec: LPJ: 100000 __udelay: 4000 vs my_udelay: 4998
50 usec: LPJ: 1500000 __udelay: 74000 vs my_udelay: 74983
100 usec: LPJ: 100000 __udelay: 9000 vs my_udelay: 9997
100 usec: LPJ: 1500000 __udelay: 149000 vs my_udelay: 149966
20000 usec: LPJ: 100000 __udelay: 1999000 vs my_udelay: 1999549
20000 usec: LPJ: 1500000 __udelay: 29993000 vs my_udelay: 29993243
Change 2:
Round up in __udelay. While it can be argued that some time is also
spent in the delay functions, it's better to spend _at least_ the specified
time sleeping, in my humble opinion.
- return __const_udelay2(usecs * 0x000010c6); /* 2**32 / 1000000 */
+ return __const_udelay2(usecs * 0x000010c7); /* 2**32 / 1000000 (rounded up)*/
1 usec: LPJ: 100000 __udelay: 0 vs my_udelay: 100
1 usec: LPJ: 1500000 __udelay: 1000 vs my_udelay: 1500
2 usec: LPJ: 100000 __udelay: 0 vs my_udelay: 200
2 usec: LPJ: 1500000 __udelay: 2000 vs my_udelay: 3000
5 usec: LPJ: 100000 __udelay: 0 vs my_udelay: 500
5 usec: LPJ: 1500000 __udelay: 7000 vs my_udelay: 7500
10 usec: LPJ: 100000 __udelay: 0 vs my_udelay: 1000
10 usec: LPJ: 1500000 __udelay: 14000 vs my_udelay: 15000
20 usec: LPJ: 100000 __udelay: 1000 vs my_udelay: 2000
20 usec: LPJ: 1500000 __udelay: 29000 vs my_udelay: 30000
50 usec: LPJ: 100000 __udelay: 4000 vs my_udelay: 5000
50 usec: LPJ: 1500000 __udelay: 74000 vs my_udelay: 75000
100 usec: LPJ: 100000 __udelay: 9000 vs my_udelay: 10000
100 usec: LPJ: 1500000 __udelay: 149000 vs my_udelay: 150001
20000 usec: LPJ: 100000 __udelay: 1999000 vs my_udelay: 2000015
20000 usec: LPJ: 1500000 __udelay: 29993000 vs my_udelay: 30000228
Change 3:
Asserting at least 1 loop is spent: in really small ndelay() calls to
low-mhz timers, this might be better.
return __delay(xloops ? xloops : 1);
Before:
1 nsec: LPJ: 100000 __ndelay: 0 vs my_udelay: 0
2 nsec: LPJ: 100000 __ndelay: 0 vs my_udelay: 0
5 nsec: LPJ: 100000 __ndelay: 0 vs my_udelay: 0
10 nsec: LPJ: 100000 __ndelay: 0 vs my_udelay: 1
20 nsec: LPJ: 100000 __udelay: 0 vs my_udelay: 2
50 nsec: LPJ: 100000 __ndelay: 0 vs my_udelay: 5
After:
1 nsec: LPJ: 100000 __udelay: 0 vs my_udelay: 1
2 nsec: LPJ: 100000 __udelay: 0 vs my_udelay: 1
5 nsec: LPJ: 100000 __udelay: 0 vs my_udelay: 1
10 nsec: LPJ: 100000 __udelay: 0 vs my_udelay: 1
20 nsec: LPJ: 100000 __udelay: 0 vs my_udelay: 2
50 nsec: LPJ: 100000 __udelay: 0 vs my_udelay: 5
Dominik
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Too much error in __const_udelay() ?
2004-06-05 15:23 ` Dominik Brodowski
@ 2004-06-06 19:46 ` Pavel Machek
2004-06-07 19:12 ` john stultz
1 sibling, 0 replies; 12+ messages in thread
From: Pavel Machek @ 2004-06-06 19:46 UTC (permalink / raw)
To: Dominik Brodowski, john stultz, lkml, george anzinger, greg kh,
Chris McDermott
Hi!
> Change 3:
>
> Asserting at least 1 loop is spent: in really small ndelay() calls to
> low-mhz timers, this might be better.
>
> return __delay(xloops ? xloops : 1);
Should not you always round up? If user asks you to delay 1900 usec,
delaying 1000 usec is a bug. If you do this, make-it-one-when-its-zero
hack should be unneccessary.
Pavel
--
934a471f20d6580d5aad759bf0d97ddc
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Too much error in __const_udelay() ?
2004-06-05 15:23 ` Dominik Brodowski
2004-06-06 19:46 ` Pavel Machek
@ 2004-06-07 19:12 ` john stultz
2004-06-07 20:27 ` john stultz
` (4 more replies)
1 sibling, 5 replies; 12+ messages in thread
From: john stultz @ 2004-06-07 19:12 UTC (permalink / raw)
To: Dominik Brodowski; +Cc: lkml, george anzinger, greg kh, Chris McDermott
On Sat, 2004-06-05 at 08:23, Dominik Brodowski wrote:
> Hi,
>
> > However I've started to see some problems w/ 2.6 and USB on x440/x445s,
> > both of which use the 100Mhz cyclone time source. Further digging has
> > pointed to the fact that certain important udelay()s in the USB
> > subsystem aren't actually waiting long enough.
>
> Certain? AFAICS _no_ call to a delay routine actually passed a big enough
> argument. Or am I missing something? Also, __ndelay seems to be affected
> as well: it returns zero for 550 nsec even for the TSC variant in your
> test.c.
Indeed its likely.
> > So I'm no math wiz. What's the proper fix here?
>
> Below are three changes I'd like to discuss. I'll build a fresh kernel with
> all three changes enabled + PM_TIMER soon.
Ah, your test output is a bit confusing (changes to __const_udealy
affect the output of my_udelay), but I think I understand it. Forgive me
if I miss-interpret.
> Change 1:
>
> Move the multiplication with HZ up into the mull instruction:
>
> unsigned long __const_udelay(unsigned long xloops)
> {
> int d0;
> __asm__("mull %0"
> :"=d" (xloops), "=&a" (d0)
> :"1" (xloops),"0" (LPJ * HZ));
> return __delay(xloops);
> }
This does make a good bit of difference! Good catch!
> Change 2:
>
> Round up in __udelay. While it can be argued that some time is also
> spent in the delay functions, it's better to spend _at least_ the specified
> time sleeping, in my humble opinion.
>
>
> - return __const_udelay2(usecs * 0x000010c6); /* 2**32 / 1000000 */
> + return __const_udelay2(usecs * 0x000010c7); /* 2**32 / 1000000 (rounded up)*/
>
This change looks right to me.
> Change 3:
>
> Asserting at least 1 loop is spent: in really small ndelay() calls to
> low-mhz timers, this might be better.
>
> return __delay(xloops ? xloops : 1);
I agree w/ Pavel that rounding up sounds better, but I can't get the
math to work, so this may be the best solution.
I'm also spinning up a patch w/ these changes to test, let me know how
your testing went and I'll do the same.
-john
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Too much error in __const_udelay() ?
2004-06-07 19:12 ` john stultz
@ 2004-06-07 20:27 ` john stultz
2004-06-07 21:20 ` [PATCH 1/3] mull'ify multiplication with HZ in __const_udelay() [Was: Re: Too much error in __const_udelay() ?] Dominik Brodowski
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: john stultz @ 2004-06-07 20:27 UTC (permalink / raw)
To: Dominik Brodowski; +Cc: lkml, george anzinger, greg kh, Chris McDermott
On Mon, 2004-06-07 at 12:12, john stultz wrote:
> I'm also spinning up a patch w/ these changes to test, let me know how
> your testing went and I'll do the same.
The following patch of your suggestions resolves the USB problem for me.
I'd be interested if anyone else could test this to insure it doesn't
open up any other issues.
thanks
-john
===== arch/i386/lib/delay.c 1.5 vs edited =====
--- 1.5/arch/i386/lib/delay.c Wed Jul 2 21:21:32 2003
+++ edited/arch/i386/lib/delay.c Mon Jun 7 12:17:26 2004
@@ -33,13 +33,13 @@
int d0;
__asm__("mull %0"
:"=d" (xloops), "=&a" (d0)
- :"1" (xloops),"0" (current_cpu_data.loops_per_jiffy));
- __delay(xloops * HZ);
+ :"1" (xloops),"0" (HZ*current_cpu_data.loops_per_jiffy));
+ __delay(xloops?xloops:1);
}
void __udelay(unsigned long usecs)
{
- __const_udelay(usecs * 0x000010c6); /* 2**32 / 1000000 */
+ __const_udelay(usecs * 0x000010c7); /* 2**32 / 1000000 */
}
void __ndelay(unsigned long nsecs)
===== include/asm-i386/delay.h 1.2 vs edited =====
--- 1.2/include/asm-i386/delay.h Tue Feb 18 06:40:31 2003
+++ edited/include/asm-i386/delay.h Mon Jun 7 12:16:42 2004
@@ -16,7 +16,7 @@
extern void __delay(unsigned long loops);
#define udelay(n) (__builtin_constant_p(n) ? \
- ((n) > 20000 ? __bad_udelay() : __const_udelay((n) * 0x10c6ul)) : \
+ ((n) > 20000 ? __bad_udelay() : __const_udelay((n) * 0x10c7ul)) : \
__udelay(n))
#define ndelay(n) (__builtin_constant_p(n) ? \
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] mull'ify multiplication with HZ in __const_udelay() [Was: Re: Too much error in __const_udelay() ?]
2004-06-07 19:12 ` john stultz
2004-06-07 20:27 ` john stultz
@ 2004-06-07 21:20 ` Dominik Brodowski
2004-06-07 22:00 ` john stultz
2004-06-15 6:11 ` Dominik Brodowski
2004-06-07 21:22 ` [PATCH 2/3] round up in __udelay() " Dominik Brodowski
` (2 subsequent siblings)
4 siblings, 2 replies; 12+ messages in thread
From: Dominik Brodowski @ 2004-06-07 21:20 UTC (permalink / raw)
To: john stultz; +Cc: lkml, george anzinger, greg kh, Chris McDermott
Move the multiplication of (loops_per_jiffy * xloops) with HZ into
the "mull" asm operation. This increases the accuracy of the delay functions
largely:
n usec delay on a 1500000 BogoMIPS system:
n before after
1 1000 ticks 1499 ticks
10 14000 ticks 14999 ticks
n usec delay on a 100000 BogoMIPS system:
n before after
1 0 ticks 99 ticks
10 0 ticks 999 ticks
100 9000 ticks 9999 ticks
Signed-off-by: Dominik Brodowski <linux@brodo.de>
diff -ruN linux-original/arch/i386/lib/delay.c linux/arch/i386/lib/delay.c
--- linux-original/arch/i386/lib/delay.c 2004-06-07 22:01:46.608351088 +0200
+++ linux/arch/i386/lib/delay.c 2004-06-07 22:05:03.299449496 +0200
@@ -33,8 +33,8 @@
int d0;
__asm__("mull %0"
:"=d" (xloops), "=&a" (d0)
- :"1" (xloops),"0" (current_cpu_data.loops_per_jiffy));
- __delay(xloops * HZ);
+ :"1" (xloops),"0" (current_cpu_data.loops_per_jiffy * HZ));
+ __delay(xloops);
}
void __udelay(unsigned long usecs)
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] round up in __udelay() [Was: Re: Too much error in __const_udelay() ?]
2004-06-07 19:12 ` john stultz
2004-06-07 20:27 ` john stultz
2004-06-07 21:20 ` [PATCH 1/3] mull'ify multiplication with HZ in __const_udelay() [Was: Re: Too much error in __const_udelay() ?] Dominik Brodowski
@ 2004-06-07 21:22 ` Dominik Brodowski
2004-06-07 21:23 ` [PATCH 3/3] fix for small xloops " Dominik Brodowski
2004-06-07 21:23 ` Too much error in __const_udelay() ? Dominik Brodowski
4 siblings, 0 replies; 12+ messages in thread
From: Dominik Brodowski @ 2004-06-07 21:22 UTC (permalink / raw)
To: john stultz; +Cc: lkml, george anzinger, greg kh, Chris McDermott
Round up in __udelay(): 2**32 / 100000 is 4294.97, so it's more intuitive
to round up, and it causes more predictable results:
n usec delay on a 1500000 BogoMIPS system:
n before -mull after
1 1000 ticks 1499 ticks 1500 ticks
10 14000 ticks 14999 ticks 15000 ticks
n usec delay on a 100000 BogoMIPS system:
n before -mull after
1 0 ticks 99 ticks 100 ticks
10 0 ticks 999 ticks 1000 ticks
100 9000 ticks 9999 ticks 10000 ticks
While it can be argued that some time is also spent in the delay
functions, it's better to spend _at least_ the specified time sleeping,
in my humble opinion. And the overhead of a specific ->delay() implementation
should be substracted in the specific ->delay() implementation.
Signed-off-by: Dominik Brodowski <linux@brodo.de>
diff -ruN linux-original/arch/i386/lib/delay.c linux/arch/i386/lib/delay.c
--- linux-original/arch/i386/lib/delay.c 2004-06-07 22:10:42.053950984 +0200
+++ linux/arch/i386/lib/delay.c 2004-06-07 22:12:58.789164072 +0200
@@ -39,7 +39,7 @@
void __udelay(unsigned long usecs)
{
- __const_udelay(usecs * 0x000010c6); /* 2**32 / 1000000 */
+ __const_udelay(usecs * 0x000010c7); /* 2**32 / 1000000 (rounded up) */
}
void __ndelay(unsigned long nsecs)
diff -ruN linux-original/include/asm-i386/delay.h linux/include/asm-i386/delay.h
--- linux-original/include/asm-i386/delay.h 2004-06-07 22:01:48.901002552 +0200
+++ linux/include/asm-i386/delay.h 2004-06-07 22:14:00.393798744 +0200
@@ -16,7 +16,7 @@
extern void __delay(unsigned long loops);
#define udelay(n) (__builtin_constant_p(n) ? \
- ((n) > 20000 ? __bad_udelay() : __const_udelay((n) * 0x10c6ul)) : \
+ ((n) > 20000 ? __bad_udelay() : __const_udelay((n) * 0x10c7ul)) : \
__udelay(n))
#define ndelay(n) (__builtin_constant_p(n) ? \
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] fix for small xloops [Was: Re: Too much error in __const_udelay() ?]
2004-06-07 19:12 ` john stultz
` (2 preceding siblings ...)
2004-06-07 21:22 ` [PATCH 2/3] round up in __udelay() " Dominik Brodowski
@ 2004-06-07 21:23 ` Dominik Brodowski
2004-06-09 10:03 ` Pavel Machek
2004-06-07 21:23 ` Too much error in __const_udelay() ? Dominik Brodowski
4 siblings, 1 reply; 12+ messages in thread
From: Dominik Brodowski @ 2004-06-07 21:23 UTC (permalink / raw)
To: john stultz; +Cc: lkml, george anzinger, greg kh, Chris McDermott
The const_udelay calculation relies on the "overflow" of the lower 32 bits
of the mull operation. What's in the lower 32 bits is "cut off", so that a
"rounding down" phenomenon exists. For large arguments to {n,u}delay, this does
not matter, as udelay and ndelay round _up_ themselves. However, for small
delays (for cyclone timer: up to 20ns; for pmtmr-based delay timer it's even
up to 1500ns or 1us) it _is_ a critical error. Empirical testing has shown that
it happens only (for usual values of loops_per_jiffies) if xloops is lower or
equal to six. Let's be safe, and double that value, and add one xloop if
xloop is smaller than 13.
Signed-off-by: Dominik Brodowski <linux@brodo.de>
diff -ruN linux-original/arch/i386/lib/delay.c linux/arch/i386/lib/delay.c
--- linux-original/arch/i386/lib/delay.c 2004-06-07 23:02:02.472656160 +0200
+++ linux/arch/i386/lib/delay.c 2004-06-07 22:55:40.063791144 +0200
@@ -34,6 +34,8 @@
__asm__("mull %0"
:"=d" (xloops), "=&a" (d0)
:"1" (xloops),"0" (current_cpu_data.loops_per_jiffy * HZ));
+ if (unlikely(xloops < 13))
+ xloops++;
__delay(xloops);
}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Too much error in __const_udelay() ?
2004-06-07 19:12 ` john stultz
` (3 preceding siblings ...)
2004-06-07 21:23 ` [PATCH 3/3] fix for small xloops " Dominik Brodowski
@ 2004-06-07 21:23 ` Dominik Brodowski
4 siblings, 0 replies; 12+ messages in thread
From: Dominik Brodowski @ 2004-06-07 21:23 UTC (permalink / raw)
To: john stultz; +Cc: lkml, george anzinger, greg kh, Chris McDermott
On Mon, Jun 07, 2004 at 12:12:48PM -0700, john stultz wrote:
> I agree w/ Pavel that rounding up sounds better, but I can't get the
> math to work, so this may be the best solution.
It's some strange sort of rounding, see my patch "3"...
> I'm also spinning up a patch w/ these changes to test, let me know how
> your testing went and I'll do the same.
Testing went fine -- even for the PMTMR-based delay case [*].
Dominik
[*] though I noticed the cpufreq notifier breaks then: it updates
loops_per_jiffy without evaluating if it's indeed TSC- or even
frequency-based. It'll fail on cyclone, too, I think...
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] mull'ify multiplication with HZ in __const_udelay() [Was: Re: Too much error in __const_udelay() ?]
2004-06-07 21:20 ` [PATCH 1/3] mull'ify multiplication with HZ in __const_udelay() [Was: Re: Too much error in __const_udelay() ?] Dominik Brodowski
@ 2004-06-07 22:00 ` john stultz
2004-06-15 6:11 ` Dominik Brodowski
1 sibling, 0 replies; 12+ messages in thread
From: john stultz @ 2004-06-07 22:00 UTC (permalink / raw)
To: Dominik Brodowski
Cc: lkml, george anzinger, greg kh, Chris McDermott, garloff
On Mon, 2004-06-07 at 14:20, Dominik Brodowski wrote:
> Move the multiplication of (loops_per_jiffy * xloops) with HZ into
> the "mull" asm operation. This increases the accuracy of the delay functions
> largely:
>
[snip]
> diff -ruN linux-original/arch/i386/lib/delay.c linux/arch/i386/lib/delay.c
> --- linux-original/arch/i386/lib/delay.c 2004-06-07 22:01:46.608351088 +0200
> +++ linux/arch/i386/lib/delay.c 2004-06-07 22:05:03.299449496 +0200
> @@ -33,8 +33,8 @@
> int d0;
> __asm__("mull %0"
> :"=d" (xloops), "=&a" (d0)
> - :"1" (xloops),"0" (current_cpu_data.loops_per_jiffy));
> - __delay(xloops * HZ);
> + :"1" (xloops),"0" (current_cpu_data.loops_per_jiffy * HZ));
> + __delay(xloops);
> }
Kurt Garloff brought up a good point that loops_per_jiffy*HZ is only
good up to 4Ghz time sources. The workaround he suggested was to
multiply xloops by 4 first and divide HZ by 4. This will allow for
frequencies up to 16Ghz.
So something like:
xloops *= 4;
__asm__("mull %0"
:"=d" (xloops), "=&a" (d0)
:"1" (xloops),"0" (LPJ*(HZ/4)));
-john
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] fix for small xloops [Was: Re: Too much error in __const_udelay() ?]
2004-06-07 21:23 ` [PATCH 3/3] fix for small xloops " Dominik Brodowski
@ 2004-06-09 10:03 ` Pavel Machek
0 siblings, 0 replies; 12+ messages in thread
From: Pavel Machek @ 2004-06-09 10:03 UTC (permalink / raw)
To: Dominik Brodowski, john stultz, lkml, george anzinger, greg kh,
Chris McDermott
Hi!
> The const_udelay calculation relies on the "overflow" of the lower 32 bits
> of the mull operation. What's in the lower 32 bits is "cut off", so that a
> "rounding down" phenomenon exists. For large arguments to {n,u}delay, this does
> not matter, as udelay and ndelay round _up_ themselves. However, for small
> delays (for cyclone timer: up to 20ns; for pmtmr-based delay timer it's even
> up to 1500ns or 1us) it _is_ a critical error. Empirical testing has shown that
> it happens only (for usual values of loops_per_jiffies) if xloops is lower or
> equal to six. Let's be safe, and double that value, and add one xloop if
> xloop is smaller than 13.
Should not you just xloops++, always? Better safe than sorry. Plus you
have one less test and branch...
Pavel
> Signed-off-by: Dominik Brodowski <linux@brodo.de>
>
> diff -ruN linux-original/arch/i386/lib/delay.c linux/arch/i386/lib/delay.c
> --- linux-original/arch/i386/lib/delay.c 2004-06-07 23:02:02.472656160 +0200
> +++ linux/arch/i386/lib/delay.c 2004-06-07 22:55:40.063791144 +0200
> @@ -34,6 +34,8 @@
> __asm__("mull %0"
> :"=d" (xloops), "=&a" (d0)
> :"1" (xloops),"0" (current_cpu_data.loops_per_jiffy * HZ));
> + if (unlikely(xloops < 13))
> + xloops++;
> __delay(xloops);
> }
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
934a471f20d6580d5aad759bf0d97ddc
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] mull'ify multiplication with HZ in __const_udelay() [Was: Re: Too much error in __const_udelay() ?]
2004-06-07 21:20 ` [PATCH 1/3] mull'ify multiplication with HZ in __const_udelay() [Was: Re: Too much error in __const_udelay() ?] Dominik Brodowski
2004-06-07 22:00 ` john stultz
@ 2004-06-15 6:11 ` Dominik Brodowski
1 sibling, 0 replies; 12+ messages in thread
From: Dominik Brodowski @ 2004-06-15 6:11 UTC (permalink / raw)
To: akpm, torvalds, john stultz
Cc: lkml, george anzinger, greg kh, Chris McDermott
John Stultz mentioned on lkml ( http://lkml.org/lkml/2004/6/5/15 ) that
calls to udelay() don't delay long enough, causing trouble e.g. in the USB
subsystem. The following patches address this issue.
Move the multiplication of (loops_per_jiffy * xloops) with HZ into
the "mull" asm operation. This increases the accuracy of the delay functions
largely:
n usec delay on a system with loops_per_jiffy = 1500000 :
n before after
1 1000 ticks 1499 ticks
10 14000 ticks 14999 ticks
n usec delay on a system with loops_per_jiffy = 100000 :
n before after
1 0 ticks 99 ticks
10 0 ticks 999 ticks
100 9000 ticks 9999 ticks
As noted by Kurt Garloff, it's necessary to adjust for large loops_per_jiffies,
as the multiplication of it with HZ fails for 4GHz or larger. So, John Stultz
suggested multiplying xloops with 4 first, and multiplying with (HZ/4).
Signed-off-by: Dominik Brodowski <linux@brodo.de>
diff -ruN linux-original/arch/i386/lib/delay.c linux/arch/i386/lib/delay.c
--- linux-original/arch/i386/lib/delay.c 2004-06-14 18:20:27.000000000 +0200
+++ linux/arch/i386/lib/delay.c 2004-06-15 07:48:57.302279400 +0200
@@ -31,10 +31,11 @@
inline void __const_udelay(unsigned long xloops)
{
int d0;
+ xloops *= 4;
__asm__("mull %0"
:"=d" (xloops), "=&a" (d0)
- :"1" (xloops),"0" (current_cpu_data.loops_per_jiffy));
- __delay(xloops * HZ);
+ :"1" (xloops),"0" (current_cpu_data.loops_per_jiffy * (HZ/4)));
+ __delay(xloops);
}
void __udelay(unsigned long usecs)
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2004-06-15 6:13 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-06-05 7:12 Too much error in __const_udelay() ? john stultz
2004-06-05 15:23 ` Dominik Brodowski
2004-06-06 19:46 ` Pavel Machek
2004-06-07 19:12 ` john stultz
2004-06-07 20:27 ` john stultz
2004-06-07 21:20 ` [PATCH 1/3] mull'ify multiplication with HZ in __const_udelay() [Was: Re: Too much error in __const_udelay() ?] Dominik Brodowski
2004-06-07 22:00 ` john stultz
2004-06-15 6:11 ` Dominik Brodowski
2004-06-07 21:22 ` [PATCH 2/3] round up in __udelay() " Dominik Brodowski
2004-06-07 21:23 ` [PATCH 3/3] fix for small xloops " Dominik Brodowski
2004-06-09 10:03 ` Pavel Machek
2004-06-07 21:23 ` Too much error in __const_udelay() ? Dominik Brodowski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®