Re: [PATCH] ia64: disable preemption in udelay()

["Luck, Tony" <tony.luck@intel.com>]

On Wed, Dec 14, 2005 at 03:25:26PM -0800, hawkes@sgi.com wrote:
> Sending this to a wider audience:
> 
> The udelay() inline for ia64 uses the ITC.  If CONFIG_PREEMPT is enabled
> and the platform has unsynchronized ITCs and the calling task migrates
> to another CPU while doing the udelay loop, then the effective delay may
> be too short or very, very long.
> 
> The most simple fix is to disable preemption around the udelay looping.
> The downside is that this inhibits realtime preemption for cases of long
> udelays.  One datapoint:  an SGI realtime engineer reports that if
> CONFIG_PREEMPT is turned off, that no significant holdoffs are
> are attributed to udelay().
> 
> I am reluctant to propose a much more complicated patch (that disables
> preemption only for "short" delays, and uses the global RTC as the time
> base for longer, preemptible delays) unless this patch introduces
> significant and unacceptable preemption delays.

Stuck between a rock and the proverbial hard place.

I think that the more complex patch is needed though.  If some crazy
driver has a pre-emptible udelay(10000), then you really don't want
to spin for that long without allowing preemption.

What is the threshold between a "long" and a "short" delay?  Presumably
related to whatever your maximum hold-off value is.  Also depends on
how long it takes to read the global RTC.

If you don't want to get the global RTC involved, then perhaps
you can break long delays up inside udelay()?  Something like
this:

#define	SMALLUSECS 250 /* randomly picked, needs some thought! */

static __inline__ void
udelay (unsigned long usecs)
{
	unsigned long start;
	unsigned long cycles;
	unsigned long smallusecs;

	while (usecs > 0) {
		smallusecs = (usecs > SMALLUSECS) ? SMALLUSECS : usecs;
		preempt_disable();
		cycles = smallusecs*local_cpu_data->cyc_per_usec;
		start = ia64_get_itc();

		while (ia64_get_itc() - start < cycles)
			cpu_relax();

		preempt_enable();
		usecs -= smallusecs;
	}
}

This does make the function a bit big for an "inline" though.  Does
it really need to be inline?  Do we care how fast our delay loops
are?

-Tony