* Re: [PATCH] ia64: simplify and fix udelay()
@ 2006-02-14 18:40 hawkes
2006-02-15 9:08 ` Chen, Kenneth W
0 siblings, 1 reply; 6+ messages in thread
From: hawkes @ 2006-02-14 18:40 UTC (permalink / raw)
To: Tony Luck, Andrew Morton, linux-ia64, linux-kernel
Cc: Jack Steiner, hawkes, Robin Holt, Dimitri Sivanich, Jes Sorensen
Version #2, merging Andreas Schwab's suggestion:
The original ia64 udelay() was simple, but flawed for platforms without
synchronized ITCs: a preemption and migration to another CPU during the
while-loop likely resulted in too-early termination or very, very
lengthy looping.
The first fix (now in 2.6.15) broke the delay loop into smaller,
non-preemptible chunks, reenabling preemption between the chunks. This
fix is flawed in that the total udelay is computed to be the sum of just
the non-premptible while-loop pieces, i.e., not counting the time spent
in the interim preemptible periods. If an interrupt or a migration
occurs during one of these interim periods, then that time is invisible
and only serves to lengthen the effective udelay().
This new fix backs out the current flawed fix and returns to a simple
udelay(), fully preemptible and interruptible. It implements two simple
alternative udelay() routines: one a default generic version that uses
ia64_get_itc(), and the other an sn-specific version that uses that
platform's RTC.
Signed-off-by: John Hawkes <hawkes@sgi.com>
Index: linux/arch/ia64/kernel/time.c
===================================================================
--- linux.orig/arch/ia64/kernel/time.c 2006-02-14 10:09:25.000000000 -0800
+++ linux/arch/ia64/kernel/time.c 2006-02-14 10:22:56.000000000 -0800
@@ -250,32 +250,27 @@ time_init (void)
set_normalized_timespec(&wall_to_monotonic, -xtime.tv_sec, -xtime.tv_nsec);
}
-#define SMALLUSECS 100
+/*
+ * Generic udelay assumes that if preemption is allowed and the thread
+ * migrates to another CPU, that the ITC values are synchronized across
+ * all CPUs.
+ */
+static void
+ia64_itc_udelay (unsigned long usecs)
+{
+ unsigned long start = ia64_get_itc();
+ unsigned long end = start + usecs*local_cpu_data->cyc_per_usec;
+
+ while (time_before(ia64_get_itc(), end))
+ cpu_relax();
+}
+
+void (*ia64_udelay)(unsigned long usecs) = &ia64_itc_udelay;
void
udelay (unsigned long usecs)
{
- unsigned long start;
- unsigned long cycles;
- unsigned long smallusecs;
-
- /*
- * Execute the non-preemptible delay loop (because the ITC might
- * not be synchronized between CPUS) in relatively short time
- * chunks, allowing preemption between the chunks.
- */
- 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;
- }
+ (*ia64_udelay)(usecs);
}
EXPORT_SYMBOL(udelay);
Index: linux/arch/ia64/sn/kernel/sn2/timer.c
===================================================================
--- linux.orig/arch/ia64/sn/kernel/sn2/timer.c 2006-01-02 19:21:10.000000000 -0800
+++ linux/arch/ia64/sn/kernel/sn2/timer.c 2006-02-14 10:27:26.000000000 -0800
@@ -14,6 +14,7 @@
#include <asm/hw_irq.h>
#include <asm/system.h>
+#include <asm/timex.h>
#include <asm/sn/leds.h>
#include <asm/sn/shub_mmr.h>
@@ -28,9 +29,27 @@ static struct time_interpolator sn2_inte
.source = TIME_SOURCE_MMIO64
};
+/*
+ * sn udelay uses the RTC instead of the ITC because the ITC is not
+ * synchronized across all CPUs, and the thread may migrate to another CPU
+ * if preemption is enabled.
+ */
+static void
+ia64_sn_udelay (unsigned long usecs)
+{
+ unsigned long start = rtc_time();
+ unsigned long end = start +
+ usecs * sn_rtc_cycles_per_second / 1000000;
+
+ while (time_before((unsigned long)rtc_time(), end))
+ cpu_relax();
+}
+
void __init sn_timer_init(void)
{
sn2_interpolator.frequency = sn_rtc_cycles_per_second;
sn2_interpolator.addr = RTC_COUNTER_ADDR;
register_time_interpolator(&sn2_interpolator);
+
+ ia64_udelay = &ia64_sn_udelay;
}
Index: linux/include/asm-ia64/timex.h
===================================================================
--- linux.orig/include/asm-ia64/timex.h 2006-01-02 19:21:10.000000000 -0800
+++ linux/include/asm-ia64/timex.h 2006-02-14 10:27:46.000000000 -0800
@@ -15,6 +15,8 @@
typedef unsigned long cycles_t;
+extern void (*ia64_udelay)(unsigned long usecs);
+
/*
* For performance reasons, we don't want to define CLOCK_TICK_TRATE as
* local_cpu_data->itc_rate. Fortunately, we don't have to, either: according to George
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH] ia64: simplify and fix udelay()
2006-02-14 18:40 [PATCH] ia64: simplify and fix udelay() hawkes
@ 2006-02-15 9:08 ` Chen, Kenneth W
2006-02-15 14:56 ` Jes Sorensen
2006-02-15 15:08 ` Jack Steiner
0 siblings, 2 replies; 6+ messages in thread
From: Chen, Kenneth W @ 2006-02-15 9:08 UTC (permalink / raw)
To: hawkes, Tony Luck, Andrew Morton, linux-ia64, linux-kernel
Cc: Jack Steiner, Robin Holt, Dimitri Sivanich, Jes Sorensen
hawkes@sgi.com wrote on Tuesday, February 14, 2006 10:40 AM
> a preemption and migration to another CPU during the
> while-loop
Off topic from the subject line a bit, but related: how many Altix
SN2 customers in the field turn on CONFIG_PREEMPT? Redhat EL4 doesn't
turn on preempt, SuSE SLES9 and SLES10 beta don't turn it on either.
Is there a real benefit of turning that option on for SN2?
- Ken
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ia64: simplify and fix udelay()
2006-02-15 9:08 ` Chen, Kenneth W
@ 2006-02-15 14:56 ` Jes Sorensen
2006-02-15 15:08 ` Jack Steiner
1 sibling, 0 replies; 6+ messages in thread
From: Jes Sorensen @ 2006-02-15 14:56 UTC (permalink / raw)
To: Chen, Kenneth W
Cc: hawkes, Tony Luck, Andrew Morton, linux-ia64, linux-kernel,
Jack Steiner, Robin Holt, Dimitri Sivanich
>>>>> "Ken" == Chen, Kenneth W <kenneth.w.chen@intel.com> writes:
Ken> hawkes@sgi.com wrote on Tuesday, February 14, 2006 10:40 AM
>> a preemption and migration to another CPU during the while-loop
Ken> Off topic from the subject line a bit, but related: how many
Ken> Altix SN2 customers in the field turn on CONFIG_PREEMPT? Redhat
Ken> EL4 doesn't turn on preempt, SuSE SLES9 and SLES10 beta don't
Ken> turn it on either. Is there a real benefit of turning that
Ken> option on for SN2?
Ken,
Not sure if any do, however as long as it's a supported kernel option
then we ought to make sure the kernel is reliable under it. Who knows,
at some point some distro might even decide to switch it on as well
(as much as I would discourage doing so ;).
Cheers,
Jes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ia64: simplify and fix udelay()
2006-02-15 9:08 ` Chen, Kenneth W
2006-02-15 14:56 ` Jes Sorensen
@ 2006-02-15 15:08 ` Jack Steiner
1 sibling, 0 replies; 6+ messages in thread
From: Jack Steiner @ 2006-02-15 15:08 UTC (permalink / raw)
To: Chen, Kenneth W
Cc: hawkes, Tony Luck, Andrew Morton, linux-ia64, linux-kernel,
Robin Holt, Dimitri Sivanich, Jes Sorensen
On Wed, Feb 15, 2006 at 01:08:41AM -0800, Chen, Kenneth W wrote:
> hawkes@sgi.com wrote on Tuesday, February 14, 2006 10:40 AM
> > a preemption and migration to another CPU during the
> > while-loop
>
> Off topic from the subject line a bit, but related: how many Altix
> SN2 customers in the field turn on CONFIG_PREEMPT? Redhat EL4 doesn't
> turn on preempt, SuSE SLES9 and SLES10 beta don't turn it on either.
> Is there a real benefit of turning that option on for SN2?
AFAICT, no one at SGI uses or plans to use CONFIG_PREEMPT. Most of
our customers use kernels from one of the distros & none at this point
enables preemption.
The realtime folks here have experimented with CONFIG_PREEMPT but so
far have not seen any significant benefit.
Regardless, we should fix udelay() to handle unsync'ed ITCs. It would
be nice to have it working.
-- Jack
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ia64: simplify and fix udelay()
@ 2006-02-13 18:33 hawkes
2006-02-13 21:28 ` Andreas Schwab
0 siblings, 1 reply; 6+ messages in thread
From: hawkes @ 2006-02-13 18:33 UTC (permalink / raw)
To: Tony Luck, Andrew Morton, linux-ia64, linux-kernel
Cc: Jack Steiner, hawkes, Robin Holt, Dimitri Sivanich, Jes Sorensen
The original ia64 udelay() was simple, but flawed for platforms without
synchronized ITCs: a preemption and migration to another CPU during the
while-loop likely resulted in too-early termination or very, very
lengthy looping.
The first fix (now in 2.6.15) broke the delay loop into smaller,
non-preemptible chunks, reenabling preemption between the chunks. This
fix is flawed in that the total udelay is computed to be the sum of just
the non-premptible while-loop pieces, i.e., not counting the time spent
in the interim preemptible periods. If an interrupt or a migration
occurs during one of these interim periods, then that time is invisible
and only serves to lengthen the effective udelay().
This new fix backs out the current flawed fix and returns to a simple
udelay(), fully preemptible and interruptible. It implements two simple
alternative udelay() routines: one a default generic version that uses
ia64_get_itc(), and the other an sn-specific version that uses that
platform's RTC.
Signed-off-by: John Hawkes <hawkes@sgi.com>
Index: linux/arch/ia64/kernel/time.c
===================================================================
--- linux.orig/arch/ia64/kernel/time.c 2006-02-08 17:59:42.000000000 -0800
+++ linux/arch/ia64/kernel/time.c 2006-02-13 10:03:59.000000000 -0800
@@ -250,31 +250,26 @@ time_init (void)
set_normalized_timespec(&wall_to_monotonic, -xtime.tv_sec, -xtime.tv_nsec);
}
-#define SMALLUSECS 100
-
-void
-udelay (unsigned long usecs)
+/*
+ * Generic udelay assumes that if preemption is allowed and the thread
+ * migrates to another CPU, that the ITC values are synchronized across
+ * all CPUs.
+ */
+static void
+ia64_itc_udelay (unsigned long usecs)
{
- unsigned long start;
- unsigned long cycles;
- unsigned long smallusecs;
+ unsigned long start = ia64_get_itc();
+ unsigned long end = start + usecs*local_cpu_data->cyc_per_usec;
- /*
- * Execute the non-preemptible delay loop (because the ITC might
- * not be synchronized between CPUS) in relatively short time
- * chunks, allowing preemption between the chunks.
- */
- while (usecs > 0) {
- smallusecs = (usecs > SMALLUSECS) ? SMALLUSECS : usecs;
- preempt_disable();
- cycles = smallusecs*local_cpu_data->cyc_per_usec;
- start = ia64_get_itc();
+ while (time_before(ia64_get_itc(), end))
+ cpu_relax();
+}
- while (ia64_get_itc() - start < cycles)
- cpu_relax();
+void (*ia64_udelay)(unsigned long usecs) = &ia64_itc_udelay;
- preempt_enable();
- usecs -= smallusecs;
- }
+void
+udelay (unsigned long usecs)
+{
+ (*ia64_udelay)(usecs);
}
EXPORT_SYMBOL(udelay);
Index: linux/arch/ia64/sn/kernel/sn2/timer.c
===================================================================
--- linux.orig/arch/ia64/sn/kernel/sn2/timer.c 2006-02-08 17:59:42.000000000 -0800
+++ linux/arch/ia64/sn/kernel/sn2/timer.c 2006-02-09 09:02:31.000000000 -0800
@@ -28,9 +28,29 @@ static struct time_interpolator sn2_inte
.source = TIME_SOURCE_MMIO64
};
+extern void (*ia64_udelay)(unsigned long usecs);
+
+/*
+ * sn udelay uses the RTC instead of the ITC because the ITC is not
+ * synchronized across all CPUs, and the thread may migrate to another CPU
+ * if preemption is enabled.
+ */
+static void
+ia64_sn_udelay (unsigned long usecs)
+{
+ unsigned long start = rtc_time();
+ unsigned long end = start +
+ usecs * sn_rtc_cycles_per_second / 1000000;
+
+ while (time_before((unsigned long)rtc_time(), end))
+ cpu_relax();
+}
+
void __init sn_timer_init(void)
{
sn2_interpolator.frequency = sn_rtc_cycles_per_second;
sn2_interpolator.addr = RTC_COUNTER_ADDR;
register_time_interpolator(&sn2_interpolator);
+
+ ia64_udelay = &ia64_sn_udelay;
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ia64: simplify and fix udelay()
2006-02-13 18:33 hawkes
@ 2006-02-13 21:28 ` Andreas Schwab
0 siblings, 0 replies; 6+ messages in thread
From: Andreas Schwab @ 2006-02-13 21:28 UTC (permalink / raw)
To: hawkes
Cc: Tony Luck, Andrew Morton, linux-ia64, linux-kernel, Jack Steiner,
Robin Holt, Dimitri Sivanich, Jes Sorensen
hawkes@sgi.com writes:
> Index: linux/arch/ia64/sn/kernel/sn2/timer.c
> ===================================================================
> --- linux.orig/arch/ia64/sn/kernel/sn2/timer.c 2006-02-08 17:59:42.000000000 -0800
> +++ linux/arch/ia64/sn/kernel/sn2/timer.c 2006-02-09 09:02:31.000000000 -0800
> @@ -28,9 +28,29 @@ static struct time_interpolator sn2_inte
> .source = TIME_SOURCE_MMIO64
> };
>
> +extern void (*ia64_udelay)(unsigned long usecs);
Please put the declaration in a header.
Andreas.
--
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-02-15 15:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-02-14 18:40 [PATCH] ia64: simplify and fix udelay() hawkes
2006-02-15 9:08 ` Chen, Kenneth W
2006-02-15 14:56 ` Jes Sorensen
2006-02-15 15:08 ` Jack Steiner
-- strict thread matches above, loose matches on Subject: below --
2006-02-13 18:33 hawkes
2006-02-13 21:28 ` Andreas Schwab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox