udelay() & preemption & drifty ITCs

udelay() & preemption & drifty ITCs

[Jack Steiner]

I may be overlooking something, but isnt there a problem
with the udelay() function on IA64 platforms with drifty ITCs
when preemption is enabled.

The function uses the ITC for timing. If preemption occurs & the process
migrates to a different cpu with a much larger ITC value, it
seems like the delay may be satisfied too quickly.

What am I missing.....


-- 
Thanks

Jack Steiner (steiner@sgi.com)          651-683-5302
Principal Engineer                      SGI - Silicon Graphics, Inc.

Re: udelay() & preemption & drifty ITCs

[Jesse Barnes]

On Sat, Nov 22, 2003 at 09:45:25PM -0600, Jack Steiner wrote:
> I may be overlooking something, but isnt there a problem
> with the udelay() function on IA64 platforms with drifty ITCs
> when preemption is enabled.
> 
> The function uses the ITC for timing. If preemption occurs & the process
> migrates to a different cpu with a much larger ITC value, it
> seems like the delay may be satisfied too quickly.

Yep, it appears so.  I guess we need a 'preempt_disable/disable()' pair
around the itc access.  Of course, callers under a spinlock are already
protected, so maybe exposure to this problem isn't that large?

Jesse

Re: udelay() & preemption & drifty ITCs

[Jesse Barnes]

On Sun, Nov 23, 2003 at 02:39:29PM -0800, Jesse Barnes wrote:
> Yep, it appears so.  I guess we need a 'preempt_disable/disable()' pair
> around the itc access.  Of course, callers under a spinlock are already
> protected, so maybe exposure to this problem isn't that large?

Here's the patch.  Compiles and boots, but I haven't made a test module
that illustrates this bug, so I haven't tested it.

Jesse

diff -Nru a/include/asm-ia64/delay.h b/include/asm-ia64/delay.h
--- a/include/asm-ia64/delay.h	Mon Nov 24 13:59:12 2003
+++ b/include/asm-ia64/delay.h	Mon Nov 24 13:59:12 2003
@@ -15,6 +15,7 @@
 #include <linux/config.h>
 #include <linux/kernel.h>
 #include <linux/sched.h>
+#include <linux/preempt.h>
 #include <linux/compiler.h>
 
 #include <asm/intrinsics.h>
@@ -32,8 +33,10 @@
 {
 	unsigned long result;
 
+	preempt_disable();
 	result = ia64_getreg(_IA64_REG_CR_ITM);
 	ia64_srlz_d();
+	preempt_enable();
 	return result;
 }
 

Re: udelay() & preemption & drifty ITCs

[Jesse Barnes]

On Mon, Nov 24, 2003 at 01:00:43PM -0800, Jesse Barnes wrote:
> On Sun, Nov 23, 2003 at 02:39:29PM -0800, Jesse Barnes wrote:
> > Yep, it appears so.  I guess we need a 'preempt_disable/disable()' pair
> > around the itc access.  Of course, callers under a spinlock are already
> > protected, so maybe exposure to this problem isn't that large?
> 
> Here's the patch.  Compiles and boots, but I haven't made a test module
> that illustrates this bug, so I haven't tested it.

Ugg, where did I keep my brown paper bags?  Ignore this patch, I'll post
another in a minute.

Jesse

Re: udelay() & preemption & drifty ITCs

[Jesse Barnes]

On Mon, Nov 24, 2003 at 01:00:43PM -0800, Jesse Barnes wrote:
> On Sun, Nov 23, 2003 at 02:39:29PM -0800, Jesse Barnes wrote:
> > Yep, it appears so.  I guess we need a 'preempt_disable/disable()' pair
> > around the itc access.  Of course, callers under a spinlock are already
> > protected, so maybe exposure to this problem isn't that large?
> 
> Here's the patch.  Compiles and boots, but I haven't made a test module
> that illustrates this bug, so I haven't tested it.

Here's one that actually patches the affected routine.  Same caveat
applies.

Jesse

diff -Nru a/include/asm-ia64/delay.h b/include/asm-ia64/delay.h
--- a/include/asm-ia64/delay.h	Mon Nov 24 14:19:19 2003
+++ b/include/asm-ia64/delay.h	Mon Nov 24 14:19:19 2003
@@ -15,6 +15,7 @@
 #include <linux/config.h>
 #include <linux/kernel.h>
 #include <linux/sched.h>
+#include <linux/preempt.h>
 #include <linux/compiler.h>
 
 #include <asm/intrinsics.h>
@@ -81,11 +82,14 @@
 static __inline__ void
 udelay (unsigned long usecs)
 {
-	unsigned long start = ia64_get_itc();
-	unsigned long cycles = usecs*local_cpu_data->cyc_per_usec;
+	unsigned long start, cycles;
 
+	preempt_disable();
+	start = ia64_get_itc();
+	cycles = usecs*local_cpu_data->cyc_per_usec;
 	while (ia64_get_itc() - start < cycles)
 		/* skip */;
+	preempt_enable();
 }
 
 #endif /* _ASM_IA64_DELAY_H */

Re: udelay() & preemption & drifty ITCs

[Jack Steiner]

On Mon, Nov 24, 2003 at 01:25:15PM -0800, Jesse Barnes wrote:
> On Mon, Nov 24, 2003 at 01:00:43PM -0800, Jesse Barnes wrote:
> > On Sun, Nov 23, 2003 at 02:39:29PM -0800, Jesse Barnes wrote:
> > > Yep, it appears so.  I guess we need a 'preempt_disable/disable()' pair
> > > around the itc access.  Of course, callers under a spinlock are already
> > > protected, so maybe exposure to this problem isn't that large?
> > 
> > Here's the patch.  Compiles and boots, but I haven't made a test module
> > that illustrates this bug, so I haven't tested it.
> 
> Here's one that actually patches the affected routine.  Same caveat
> applies.

I dislike disabling preemption in udelay(). Although most delays are
short, there are some delays that are 100s or 1000s of usec long. Users running
soft-realtime really dont want preemption disabled unnecessarily.

On platforms with synchronized clocks, disabling preemption is not required. 

Even on platforms with drifty ITCs, preemption in udelay() is ok as long 
as migration is disabled. AFAIK, there is no standard API for disabling 
migration aside from saving/changing/restoring the cpus_allowed mask.




> 
> Jesse
> 
> diff -Nru a/include/asm-ia64/delay.h b/include/asm-ia64/delay.h
> --- a/include/asm-ia64/delay.h	Mon Nov 24 14:19:19 2003
> +++ b/include/asm-ia64/delay.h	Mon Nov 24 14:19:19 2003
> @@ -15,6 +15,7 @@
>  #include <linux/config.h>
>  #include <linux/kernel.h>
>  #include <linux/sched.h>
> +#include <linux/preempt.h>
>  #include <linux/compiler.h>
>  
>  #include <asm/intrinsics.h>
> @@ -81,11 +82,14 @@
>  static __inline__ void
>  udelay (unsigned long usecs)
>  {
> -	unsigned long start = ia64_get_itc();
> -	unsigned long cycles = usecs*local_cpu_data->cyc_per_usec;
> +	unsigned long start, cycles;
>  
> +	preempt_disable();
> +	start = ia64_get_itc();
> +	cycles = usecs*local_cpu_data->cyc_per_usec;
>  	while (ia64_get_itc() - start < cycles)
>  		/* skip */;
> +	preempt_enable();
>  }
>  
>  #endif /* _ASM_IA64_DELAY_H */

-- 
Thanks

Jack Steiner (steiner@sgi.com)          651-683-5302
Principal Engineer                      SGI - Silicon Graphics, Inc.

Re: udelay() & preemption & drifty ITCs

[Jesse Barnes]

On Mon, Nov 24, 2003 at 04:01:31PM -0600, Jack Steiner wrote:
> I dislike disabling preemption in udelay(). Although most delays are
> short, there are some delays that are 100s or 1000s of usec long. Users running
> soft-realtime really dont want preemption disabled unnecessarily.
>
> On platforms with synchronized clocks, disabling preemption is not required. 

Yeah, I thought about this too, but unless we have a global flag telling
us if itcs drift or not, it seems lik we have to unconditionally disable
preemption.

> Even on platforms with drifty ITCs, preemption in udelay() is ok as long 
> as migration is disabled. AFAIK, there is no standard API for disabling 
> migration aside from saving/changing/restoring the cpus_allowed mask.

Then we'd have to check if drifty itcs were present and also check to
see if the process was pinned...

If we found enough code that depended on having preemption disabled
because of itc drift, it might be worthwhile to add another inline to
deal with it.  As for CPU pinning, maybe we could check in
preempt_disable() whether the cpus_allowed mask only had one bit
set--that would prevent us from disabling preemption at all for pinned
processes, right?

Jesse