Skip to contentSkip to conversation

x86: disable preemption in delay_tsc()

PATCH In review Arjan van de Ven 5 participants 11 messages Nov 16, 2007 1 message not in the archive x86

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Arjan van de Ven

author
#1 lore raw
Quoted 5 lines · On Thu, 15 Nov 2007 04:00:47 GMT Linux Kernel Mailing List…

Gitweb:
http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=35d5d08a085c56f153458c3f5d8ce24123617faf
Commit: 35d5d08a085c56f153458c3f5d8ce24123617faf Parent:
7eea436433b7b18045f272562e256976f593f7c0 Author: Andrew Morton
<[email protected]> AuthorDate: Wed Nov 14 17:00:41 2007 -0800

Quoted 9 lines
Committer:  Linus Torvalds <[email protected]>
CommitDate: Wed Nov 14 18:45:44 2007 -0800

x86: disable preemption in delay_tsc()

Marin Mitov points out that delay_tsc() can misbehave if it is
preempted and rescheduled on a different CPU which has a skewed TSC. Fix it by disabling preemption.

this worries me.. this appears to effectively disable preemption during udelay() and mdelay() loops... which are very obvious latency inducers.

Now you can argue that if you're preemptible you should have used msleep() and co, and I'll totally buy that.

Maybe we should just check if we're still on the same cpu or something, or have a cheap way to pin a process to a cpu.... but both are longer term solutions.

Signature · 3 lines
If you want to reach me at my work email, use [email protected]
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org
#2 lore raw
Quoted 21 lines · On Thu, 15 Nov 2007 19:41:16 -0800 Arjan van de Ven <[email protected]> wrote:
Quoted 5 lines · On Thu, 15 Nov 2007 04:00:47 GMT Linux Kernel Mailing List…

Gitweb:
http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=35d5d08a085c56f153458c3f5d8ce24123617faf
Commit: 35d5d08a085c56f153458c3f5d8ce24123617faf Parent:
7eea436433b7b18045f272562e256976f593f7c0 Author: Andrew Morton
<[email protected]> AuthorDate: Wed Nov 14 17:00:41 2007 -0800

Quoted 9 lines
Committer:  Linus Torvalds <[email protected]>
CommitDate: Wed Nov 14 18:45:44 2007 -0800

x86: disable preemption in delay_tsc()

Marin Mitov points out that delay_tsc() can misbehave if it is
preempted and rescheduled on a different CPU which has a skewed TSC. Fix it by disabling preemption.

this worries me.. this appears to effectively disable preemption during udelay() and mdelay() loops... which are very obvious latency inducers.

Quoted 8 lines

Now you can argue that if you're preemptible you should have used msleep() and co, and I'll totally buy that.

Maybe we should just check if we're still on the same cpu or something, or have a cheap way to pin a process to a cpu.... but both are longer term solutions.

Yes, we can do better.

But this bug can cause very rare failures in probably a large number of device drivers on a minorty of machines. Ugly. So I felt it best to plug it fast while people think about more sophisticated fixes.

#3 lore raw
Quoted 19 lines · Andrew Morton
Quoted 11 lines
Quoted 6 lines

x86: disable preemption in delay_tsc()

Marin Mitov points out that delay_tsc() can misbehave if it is
preempted and rescheduled on a different CPU which has a skewed TSC. Fix it by disabling preemption.

this worries me.. this appears to effectively disable preemption during udelay() and mdelay() loops... which are very obvious latency inducers.

Quoted 7 lines

Now you can argue that if you're preemptible you should have used msleep() and co, and I'll totally buy that.

Maybe we should just check if we're still on the same cpu or something, or have a cheap way to pin a process to a cpu.... but both are longer term solutions.

Quoted 5 lines

Yes, we can do better.

But this bug can cause very rare failures in probably a large number of device drivers on a minorty of machines. Ugly. So I felt it best to plug it fast while people think about more sophisticated fixes.

how about using usleep() transparently if high-res timers are active and we have !preempt_count()? That would be a sufficient solution and would avoid all the calibration and per-cpu-ness problems.

Ingo

#4 lore raw
Quoted 22 lines · On Fri, 16 Nov 2007 07:13:32 +0100 Ingo Molnar <[email protected]> wrote:
Quoted 19 lines · Andrew Morton
Quoted 18 lines
Quoted 6 lines

x86: disable preemption in delay_tsc()

Marin Mitov points out that delay_tsc() can misbehave if it is
preempted and rescheduled on a different CPU which has a skewed TSC. Fix it by disabling preemption.

this worries me.. this appears to effectively disable preemption during udelay() and mdelay() loops... which are very obvious latency inducers.

Now you can argue that if you're preemptible you should have used msleep() and co, and I'll totally buy that.

Maybe we should just check if we're still on the same cpu or something, or have a cheap way to pin a process to a cpu.... but both are longer term solutions.

Quoted 8 lines
Quoted 5 lines

Yes, we can do better.

But this bug can cause very rare failures in probably a large number of device drivers on a minorty of machines. Ugly. So I felt it best to plug it fast while people think about more sophisticated fixes.

how about using usleep() transparently if high-res timers are active and we have !preempt_count()?

And CONFIG_PREEMPT, of course

Quoted 2 lines

That would be a sufficient solution and would avoid all the calibration and per-cpu-ness problems.

It sounds like it would work OK. What is the setup cost for a usleep? I'd have thought that code which does something like

	while (i++ < 1000) {
		foo();
		udelay(1);
	}

would take qiute a bit longer with such a change?

#5 lore raw
Quoted 9 lines · Andrew Morton

It sounds like it would work OK. What is the setup cost for a usleep? I'd have thought that code which does something like

	while (i++ < 1000) {
		foo();
		udelay(1);
	}

would take qiute a bit longer with such a change?

full roundtrip cost ought to be below 10 usecs, depending on the system. There's no problem doing a non-preemptible udelay up to 10 usecs and we could use usleep above that.

Ingo

#6 lore raw
Quoted 6 lines · On Fri, 16 Nov 2007 08:17:08 +0100 Ingo Molnar <[email protected]> wrote:
Quoted 3 lines · Andrew Morton

It sounds like it would work OK. What is the setup cost for a usleep? I'd have thought that code which does something like

Quoted 8 lines
Quoted 6 lines
	while (i++ < 1000) {
		foo();
		udelay(1);
	}

would take qiute a bit longer with such a change?

full roundtrip cost ought to be below 10 usecs, depending on the system.

Ow. So the above timeout would take 10x longer. That probably won't break anything, but quite a few drivers do udelay(1) for post-IO settling times and they might not like it.

Quoted 2 lines

There's no problem doing a non-preemptible udelay up to 10 usecs and we could use usleep above that.

Yup, with a few smarts in there we could work out which is the best to use, and also compensate for the setup costs.

It doesn't sound very 2.6.24ish though.

As a quicky things perhaps we could only do the preempt_disable()/preempt_enable() if the TSCs are unsynced? Do we reliably know that? I guess not..

#7 lore raw
In reply to Arjan van de Ven
Quoted 23 lines · Arjan van de Ven
Quoted 5 lines · On Thu, 15 Nov 2007 04:00:47 GMT Linux Kernel Mailing List…

Gitweb:
http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=35d5d08a085c56f153458c3f5d8ce24123617faf
Commit: 35d5d08a085c56f153458c3f5d8ce24123617faf Parent:
7eea436433b7b18045f272562e256976f593f7c0 Author: Andrew Morton
<[email protected]> AuthorDate: Wed Nov 14 17:00:41 2007 -0800

Quoted 10 lines
Committer:  Linus Torvalds <[email protected]>
CommitDate: Wed Nov 14 18:45:44 2007 -0800

x86: disable preemption in delay_tsc()

Marin Mitov points out that delay_tsc() can misbehave if it is
preempted and rescheduled on a different CPU which has a skewed TSC. Fix it by disabling preemption.

this worries me.. this appears to effectively disable preemption during udelay() and mdelay() loops... which are very obvious latency inducers.

Quoted 9 lines

Now you can argue that if you're preemptible you should have used msleep() and co, and I'll totally buy that.

Maybe we should just check if we're still on the same cpu or something, or have a cheap way to pin a process to a cpu.... but both are longer term solutions.

You can use preemption notifiers to get a callback when you are preempted. Not sure what you'd to with that callback, though.

Signature · 1 line
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
#8 lore raw
Quoted 2 lines · Avi Kivity

You can use preemption notifiers to get a callback when you are preempted. Not sure what you'd to with that callback, though.

but that should not be needed in this case. Why doesnt the TSC using delay loop simply poll the CPU it is on and fix up the TSC?

Ingo

#9 lore raw

PATCH 1/1 x86: make delay_tsc() preemptible again

Quoted 2 lines · Ingo Molnar

but that should not be needed in this case. Why doesnt the TSC using delay loop simply poll the CPU it is on and fix up the TSC?

something like the patch below.

Ingo

--------------->
Subject: x86: make delay_tsc() preemptible again
From: Ingo Molnar <[email protected]>

make delay_tsc() preemptible again.

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/lib/delay_32.c |   28 +++++++++++++++++++++++-----
 arch/x86/lib/delay_64.c |   30 ++++++++++++++++++++++++------
 2 files changed, 47 insertions(+), 11 deletions(-)
arch/x86/lib/delay_32.c +23 -5
Index: linux/arch/x86/lib/delay_32.c
===================================================================
@@ -38,17 +38,35 @@ static void delay_loop(unsigned long loo
 		:"0" (loops));
 }
 
-/* TSC based delay: */
+/*
+ * TSC based delay:
+ *
+ * We are careful about preemption as TSC's are per-CPU.
+ */
 static void delay_tsc(unsigned long loops)
 {
-	unsigned long bclock, now;
+	unsigned long prev, now;
+	long left = loops;
+	int prev_cpu, cpu;
 
-	preempt_disable();		/* TSC's are per-cpu */
-	rdtscl(bclock);
+	preempt_disable();
+	rdtscl(prev);
 	do {
+		prev_cpu = smp_processor_id();
 		rep_nop();
+		preempt_enable();
+
+		preempt_disable();
+		cpu = smp_processor_id();
 		rdtscl(now);
-	} while ((now-bclock) < loops);
+		/*
+		 * If we preempted we skip this small amount of time:
+		 */
+		if (prev_cpu != cpu)
+			prev = now;
+		left -= now - prev;
+		prev = now;
+	} while (left > 0);
 	preempt_enable();
 }
arch/x86/lib/delay_64.c +24 -6
Index: linux/arch/x86/lib/delay_64.c
===================================================================
@@ -26,17 +26,35 @@ int read_current_timer(unsigned long *ti
 	return 0;
 }
 
+/*
+ * TSC based delay:
+ *
+ * We are careful about preemption as TSC's are per-CPU.
+ */
 void __delay(unsigned long loops)
 {
-	unsigned bclock, now;
+	unsigned long prev, now;
+	long left = loops;
+	int prev_cpu, cpu;
 
-	preempt_disable();		/* TSC's are pre-cpu */
-	rdtscl(bclock);
+	preempt_disable();
+	rdtscl(prev);
 	do {
-		rep_nop(); 
+		prev_cpu = smp_processor_id();
+		rep_nop();
+		preempt_enable();
+
+		preempt_disable();
+		cpu = smp_processor_id();
 		rdtscl(now);
-	}
-	while ((now-bclock) < loops);
+		/*
+		 * If we preempted we skip this small amount of time:
+		 */
+		if (prev_cpu != cpu)
+			prev = now;
+		left -= now - prev;
+		prev = now;
+	} while (left > 0);
 	preempt_enable();
 }
 EXPORT_SYMBOL(__delay);
#10 lore raw

Re: [patch] x86: make delay_tsc() preemptible again

Quoted 40 lines · Ingo Molnar
Quoted 2 lines · Ingo Molnar

but that should not be needed in this case. Why doesnt the TSC using delay loop simply poll the CPU it is on and fix up the TSC?

something like the patch below.

Ingo

--------------->
Subject: x86: make delay_tsc() preemptible again
From: Ingo Molnar <[email protected]>

make delay_tsc() preemptible again.

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/lib/delay_32.c |   28 +++++++++++++++++++++++-----
 arch/x86/lib/delay_64.c |   30 ++++++++++++++++++++++++------
 2 files changed, 47 insertions(+), 11 deletions(-)

Index: linux/arch/x86/lib/delay_32.c
===================================================================

arch/x86/lib/delay_32.c +7 -2
@@ -38,17 +38,35 @@ static void delay_loop(unsigned long loo
 		:"0" (loops));
 }
 
-/* TSC based delay: */
+/*
+ * TSC based delay:
+ *
+ * We are careful about preemption as TSC's are per-CPU.
+ */
 static void delay_tsc(unsigned long loops)
 {
-	unsigned long bclock, now;
+	unsigned long prev, now;
+	long left = loops;

arch/x86/lib/delay_32.c static void delay_loop(unsigned long loo · line 57

Quoted 10 lines
arch/x86/lib/delay_32.c +5 -2
+	int prev_cpu, cpu;
 
-	preempt_disable();		/* TSC's are per-cpu */
-	rdtscl(bclock);
+	preempt_disable();
+	rdtscl(prev);
 	do {
+		prev_cpu = smp_processor_id();
 		rep_nop();
+		preempt_enable();

Why not have the rep_nop() here between the enable, and disable ?

arch/x86/lib/delay_32.c static void delay_loop(unsigned long loo · line 63

Quoted 7 lines
arch/x86/lib/delay_32.c +5 -1
+
+		preempt_disable();
+		cpu = smp_processor_id();
 		rdtscl(now);
-	} while ((now-bclock) < loops);
+		/*
+		 * If we preempted we skip this small amount of time:

^ migrated, perhaps?

arch/x86/lib/delay_32.c static void delay_loop(unsigned long loo · line 71

Quoted 8 lines
arch/x86/lib/delay_32.c +6 -0
+		 */
+		if (prev_cpu != cpu)
+			prev = now;
+		left -= now - prev;
+		prev = now;
+	} while (left > 0);
 	preempt_enable();
 }

Otherwise, looks like a very nice patch :-)

#11 lore raw

Re: [patch] x86: make delay_tsc() preemptible again

Quoted 5 lines · Peter Zijlstra

Review

Quoted 3 lines
(quoted diff) +2 -0
+		prev_cpu = smp_processor_id();
 		rep_nop();
+		preempt_enable();

Why not have the rep_nop() here between the enable, and disable ?

yes, indeed - fixed.

Quoted 3 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+		/*
+		 * If we preempted we skip this small amount of time:

^ migrated, perhaps?

yeah, fixed.

updated patch below.

Ingo

---------------->
Subject: x86: make delay_tsc() preemptible again
From: Ingo Molnar <[email protected]>

make delay_tsc() preemptible again.

Signed-off-by Ingo Molnar <[email protected]>
Signed-off-by Peter Zijlstra <[email protected]>
---
 arch/x86/lib/delay_32.c |   27 ++++++++++++++++++++++-----
 arch/x86/lib/delay_64.c |   29 +++++++++++++++++++++++------
 2 files changed, 45 insertions(+), 11 deletions(-)
arch/x86/lib/delay_32.c +22 -5
Index: linux/arch/x86/lib/delay_32.c
===================================================================
@@ -38,17 +38,34 @@ static void delay_loop(unsigned long loo
 		:"0" (loops));
 }
 
-/* TSC based delay: */
+/*
+ * TSC based delay:
+ *
+ * We are careful about preemption as TSC's are per-CPU.
+ */
 static void delay_tsc(unsigned long loops)
 {
-	unsigned long bclock, now;
+	unsigned long prev, now;
+	long left = loops;
+	int prev_cpu, cpu;
 
-	preempt_disable();		/* TSC's are per-cpu */
-	rdtscl(bclock);
+	preempt_disable();
+	rdtscl(prev);
 	do {
+		prev_cpu = smp_processor_id();
+		preempt_enable();
 		rep_nop();
+		preempt_disable();
+		cpu = smp_processor_id();
 		rdtscl(now);
-	} while ((now-bclock) < loops);
+		/*
+		 * If we migrated we skip this small amount of time:
+		 */
+		if (prev_cpu != cpu)
+			prev = now;
+		left -= now - prev;
+		prev = now;
+	} while (left > 0);
 	preempt_enable();
 }
arch/x86/lib/delay_64.c +23 -6
Index: linux/arch/x86/lib/delay_64.c
===================================================================
@@ -26,17 +26,34 @@ int read_current_timer(unsigned long *ti
 	return 0;
 }
 
+/*
+ * TSC based delay:
+ *
+ * We are careful about preemption as TSC's are per-CPU.
+ */
 void __delay(unsigned long loops)
 {
-	unsigned bclock, now;
+	unsigned long prev, now;
+	long left = loops;
+	int prev_cpu, cpu;
 
-	preempt_disable();		/* TSC's are pre-cpu */
-	rdtscl(bclock);
+	preempt_disable();
+	rdtscl(prev);
 	do {
-		rep_nop(); 
+		prev_cpu = smp_processor_id();
+		preempt_enable();
+		rep_nop();
+		preempt_disable();
+		cpu = smp_processor_id();
 		rdtscl(now);
-	}
-	while ((now-bclock) < loops);
+		/*
+		 * If we migrated we skip this small amount of time:
+		 */
+		if (prev_cpu != cpu)
+			prev = now;
+		left -= now - prev;
+		prev = now;
+	} while (left > 0);
 	preempt_enable();
 }
 EXPORT_SYMBOL(__delay);