[RFC -mm] more cpu_relax() places?

[RFC -mm] more cpu_relax() places?

[Andreas Mohr]

Hi all,

while reviewing 2.6.17-rc6-mm1, I found some places that might
want to make use of cpu_relax() in order to not block secondary
pipelines while busy-polling (probably especially useful on SMT CPUs):

arch/i386/kernel/smp.c/flush_tlb_others():

        while (!cpus_empty(flush_cpumask))
                /* nothing. lockup detection does not belong here */
                mb();

should probably be made

        while (!cpus_empty(flush_cpumask)) {
		cpu_relax();
                /* nothing. lockup detection does not belong here */
                mb();
	}

(to have memory barrier directly before flush_cpumask is read).


Second,
include/asm-i386/apic.h/apic_wait_icr_idle() does use cpu_relax(),
but the version in asm-x86_64/ NOT!
Is this because there's not much use doing cpu_relax() on SMP non-SMT
machines, and x86_64 are always non-SMT? Or rather because someone
screwed up?


And what about include/asm-i386/acpi.h/__acpi_acquire/release_global_lock()?

static inline int
__acpi_acquire_global_lock (unsigned int *lock)
{
        unsigned int old, new, val;
        do {
                old = *lock;
                new = (((old & ~0x3) + 2) + ((old >> 1) & 0x1));
                val = cmpxchg(lock, old, new);
        } while (unlikely (val != old));
        return (new < 3) ? -1 : 0;
}

could probably be made

static inline int
__acpi_acquire_global_lock (unsigned int *lock)
{
        unsigned int old, new, val;
        while (1) {
                old = *lock;
                new = (((old & ~0x3) + 2) + ((old >> 1) & 0x1));
                val = cmpxchg(lock, old, new);
		if (likely(val == old))
			break;
		cpu_relax();
        }
        return (new < 3) ? -1 : 0;
}

Andreas Mohr

[PATCH -mm] i386: cpu_relax() smp.c (was: [RFC -mm] more cpu_relax() places?)

[Andreas Mohr]

Hi,

On Mon, Jun 12, 2006 at 08:37:43PM +0200, Andreas Mohr wrote:
> Hi all,
> 
> while reviewing 2.6.17-rc6-mm1, I found some places that might
> want to make use of cpu_relax() in order to not block secondary
> pipelines while busy-polling (probably especially useful on SMT CPUs):

OK, no replies arguing against anything, thus patch follow-up. ;)
(no. 1 of 3)

Signed-off-by: Andreas Mohr <andi@lisas.de>


diff -urN linux-2.6.17-rc6-mm2.orig/arch/i386/kernel/smp.c linux-2.6.17-rc6-mm2.my/arch/i386/kernel/smp.c
--- linux-2.6.17-rc6-mm2.orig/arch/i386/kernel/smp.c	2006-06-08 10:38:04.000000000 +0200
+++ linux-2.6.17-rc6-mm2.my/arch/i386/kernel/smp.c	2006-06-13 19:33:22.000000000 +0200
@@ -388,9 +388,11 @@
 	 */
 	send_IPI_mask(cpumask, INVALIDATE_TLB_VECTOR);
 
-	while (!cpus_empty(flush_cpumask))
+	while (!cpus_empty(flush_cpumask)) {
+		cpu_relax();
 		/* nothing. lockup detection does not belong here */
 		mb();
+	}
 
 	flush_mm = NULL;
 	flush_va = 0;

Re: [PATCH -mm] i386: cpu_relax() smp.c

[Nick Piggin]

Andreas Mohr wrote:

>Hi,
>
>On Mon, Jun 12, 2006 at 08:37:43PM +0200, Andreas Mohr wrote:
>
>>Hi all,
>>
>>while reviewing 2.6.17-rc6-mm1, I found some places that might
>>want to make use of cpu_relax() in order to not block secondary
>>pipelines while busy-polling (probably especially useful on SMT CPUs):
>>
>
>OK, no replies arguing against anything, thus patch follow-up. ;)
>(no. 1 of 3)
>

The other two look fine. This one should remove the mb(). cpu_relax
IIRC already includes a barrier(), and we are not concerned about
consistency here, only coherency, which the hardware takes care of
for us.

The flush_cpumask is guaranteed to be cleared *after* all other
variables (eg. flush_mm) have been used... that happens in the IPI
handler of course.

Aside, if we *were* worried about consistency here, smp_mb would
have been the more correct barrier to use.

>
>Signed-off-by: Andreas Mohr <andi@lisas.de>
>
>
>diff -urN linux-2.6.17-rc6-mm2.orig/arch/i386/kernel/smp.c linux-2.6.17-rc6-mm2.my/arch/i386/kernel/smp.c
>--- linux-2.6.17-rc6-mm2.orig/arch/i386/kernel/smp.c	2006-06-08 10:38:04.000000000 +0200
>+++ linux-2.6.17-rc6-mm2.my/arch/i386/kernel/smp.c	2006-06-13 19:33:22.000000000 +0200
>@@ -388,9 +388,11 @@
> 	 */
> 	send_IPI_mask(cpumask, INVALIDATE_TLB_VECTOR);
> 
>-	while (!cpus_empty(flush_cpumask))
>+	while (!cpus_empty(flush_cpumask)) {
>+		cpu_relax();
> 		/* nothing. lockup detection does not belong here */
> 		mb();
>+	}
> 
> 	flush_mm = NULL;
> 	flush_va = 0;
>

Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH -mm] i386: cpu_relax() smp.c

[Andreas Mohr]

Hi,

On Wed, Jun 14, 2006 at 12:10:16PM +1000, Nick Piggin wrote:
> Andreas Mohr wrote:
> 
> >Hi,
> >
> >On Mon, Jun 12, 2006 at 08:37:43PM +0200, Andreas Mohr wrote:
> >
> >>Hi all,
> >>
> >>while reviewing 2.6.17-rc6-mm1, I found some places that might
> >>want to make use of cpu_relax() in order to not block secondary
> >>pipelines while busy-polling (probably especially useful on SMT CPUs):
> >>
> >
> >OK, no replies arguing against anything, thus patch follow-up. ;)
> >(no. 1 of 3)
> >
> 
> The other two look fine. This one should remove the mb(). cpu_relax
> IIRC already includes a barrier(), and we are not concerned about
> consistency here, only coherency, which the hardware takes care of
> for us.
> 
> The flush_cpumask is guaranteed to be cleared *after* all other
> variables (eg. flush_mm) have been used... that happens in the IPI
> handler of course.
> 
> Aside, if we *were* worried about consistency here, smp_mb would
> have been the more correct barrier to use.

Thanks, updated patch to omit mb().

Signed-off-by: Andreas Mohr <andi@lisas.de>


diff -urN linux-2.6.17-rc6-mm2.orig/arch/i386/kernel/smp.c linux-2.6.17-rc6-mm2.my/arch/i386/kernel/smp.c
--- linux-2.6.17-rc6-mm2.orig/arch/i386/kernel/smp.c	2006-06-08 10:38:04.000000000 +0200
+++ linux-2.6.17-rc6-mm2.my/arch/i386/kernel/smp.c	2006-06-14 20:30:46.000000000 +0200
@@ -388,9 +388,10 @@
 	 */
 	send_IPI_mask(cpumask, INVALIDATE_TLB_VECTOR);
 
-	while (!cpus_empty(flush_cpumask))
+	while (!cpus_empty(flush_cpumask)) {
 		/* nothing. lockup detection does not belong here */
-		mb();
+		cpu_relax();
+	}
 
 	flush_mm = NULL;
 	flush_va = 0;

[PATCH -mm] ACPI lock: cpu_relax() (was: [RFC -mm] more cpu_relax() places?)

[Andreas Mohr]

Hi all,

On Mon, Jun 12, 2006 at 08:37:43PM +0200, Andreas Mohr wrote:
> Hi all,
> 
> while reviewing 2.6.17-rc6-mm1, I found some places that might
> want to make use of cpu_relax() in order to not block secondary
> pipelines while busy-polling (probably especially useful on SMT CPUs):

Patch no. 2 of 3.

This could be considered overkill given the previous unlikely(),
but it is busy-looping on failure after all...

Signed-off-by: Andreas Mohr <andi@lisas.de>


diff -urN linux-2.6.17-rc6-mm2.orig/include/asm-i386/acpi.h linux-2.6.17-rc6-mm2.my/include/asm-i386/acpi.h
--- linux-2.6.17-rc6-mm2.orig/include/asm-i386/acpi.h	2006-06-08 10:38:10.000000000 +0200
+++ linux-2.6.17-rc6-mm2.my/include/asm-i386/acpi.h	2006-06-13 19:35:41.000000000 +0200
@@ -61,11 +61,14 @@
 __acpi_acquire_global_lock (unsigned int *lock)
 {
 	unsigned int old, new, val;
-	do {
+	while (1) {
 		old = *lock;
 		new = (((old & ~0x3) + 2) + ((old >> 1) & 0x1));
 		val = cmpxchg(lock, old, new);
-	} while (unlikely (val != old));
+		if (likely(val == old))
+			break;
+		cpu_relax();
+	}
 	return (new < 3) ? -1 : 0;
 }
 
@@ -73,11 +76,14 @@
 __acpi_release_global_lock (unsigned int *lock)
 {
 	unsigned int old, new, val;
-	do {
+	while (1) {
 		old = *lock;
 		new = old & ~0x3;
 		val = cmpxchg(lock, old, new);
-	} while (unlikely (val != old));
+		if (likely(val == old))
+			break;
+		cpu_relax();
+	}
 	return old & 0x1;
 }
 

Re: [PATCH -mm] ACPI lock: cpu_relax() (was: [RFC -mm] more cpu_relax() places?)

[Andreas Mohr]

Hi all,

just realized that x86_64 has the same thing. (patch untested here!)

Signed-off-by: Andreas Mohr <andi@lisas.de>


--- linux-2.6.17-rc6-mm2.orig/include/asm-x86_64/acpi.h	2006-06-13 19:28:16.000000000 +0200
+++ linux-2.6.17-rc6-mm2.my/include/asm-x86_64/acpi.h	2006-06-14 21:21:15.000000000 +0200
@@ -59,11 +59,14 @@
 __acpi_acquire_global_lock (unsigned int *lock)
 {
 	unsigned int old, new, val;
-	do {
+	while (1) {
 		old = *lock;
 		new = (((old & ~0x3) + 2) + ((old >> 1) & 0x1));
 		val = cmpxchg(lock, old, new);
-	} while (unlikely (val != old));
+		if (likely(val == old))
+			break;
+		cpu_relax();
+	}
 	return (new < 3) ? -1 : 0;
 }
 
@@ -71,11 +74,14 @@
 __acpi_release_global_lock (unsigned int *lock)
 {
 	unsigned int old, new, val;
-	do {
+	while (1) {
 		old = *lock;
 		new = old & ~0x3;
 		val = cmpxchg(lock, old, new);
-	} while (unlikely (val != old));
+		if (likely(val == old))
+			break;
+		cpu_relax();
+	}
 	return old & 0x1;
 }
 

[PATCH -mm] x86_64 apic.h cpu_relax() (was: [RFC -mm] more cpu_relax() places?)

[Andreas Mohr]

Hi all,

On Mon, Jun 12, 2006 at 08:37:43PM +0200, Andreas Mohr wrote:
> Hi all,
> 
> while reviewing 2.6.17-rc6-mm1, I found some places that might
> want to make use of cpu_relax() in order to not block secondary
> pipelines while busy-polling (probably especially useful on SMT CPUs):

Patch no. 3 of 3.

This one is adding a cpu_relax() that already existed in the i386 version.
Any reason this wasn't there, too?

Signed-off-by: Andreas Mohr <andi@lisas.de>


diff -urN linux-2.6.17-rc6-mm2.orig/include/asm-x86_64/apic.h linux-2.6.17-rc6-mm2.my/include/asm-x86_64/apic.h
--- linux-2.6.17-rc6-mm2.orig/include/asm-x86_64/apic.h	2006-06-13 19:28:16.000000000 +0200
+++ linux-2.6.17-rc6-mm2.my/include/asm-x86_64/apic.h	2006-06-13 19:34:08.000000000 +0200
@@ -49,7 +49,8 @@
 
 static __inline__ void apic_wait_icr_idle(void)
 {
-	while ( apic_read( APIC_ICR ) & APIC_ICR_BUSY );
+	while ( apic_read( APIC_ICR ) & APIC_ICR_BUSY )
+		cpu_relax();
 }
 
 static inline void ack_APIC_irq(void)

Re: [PATCH -mm] x86_64 apic.h cpu_relax() (was: [RFC -mm] more cpu_relax() places?)

[Andi Kleen]

Andreas Mohr <andi@rhlx01.fht-esslingen.de> writes:

> Hi all,
> 
> On Mon, Jun 12, 2006 at 08:37:43PM +0200, Andreas Mohr wrote:
> > Hi all,
> > 
> > while reviewing 2.6.17-rc6-mm1, I found some places that might
> > want to make use of cpu_relax() in order to not block secondary
> > pipelines while busy-polling (probably especially useful on SMT CPUs):
> 
> Patch no. 3 of 3.
> 
> This one is adding a cpu_relax() that already existed in the i386 version.
> Any reason this wasn't there, too?
> 
> Signed-off-by: Andreas Mohr <andi@lisas.de>

I merged the patch thanks.

-Andi

[PATCH -mm] i386: add cpu_relax() to cmos_lock()

[Andreas Mohr]

Add cpu_relax() to cmos_lock() inline function
for faster operation on SMT CPUs and less power consumption
on others in case of lock contention (which probably doesn't
happen too often, so admittedly this patch is not too exciting).

This is a small followup to my cpu_relax() patch series last year.

A different, possibly more readable and efficient way to write this loop
would be something like:

	while ((cmos_lock) ||
           (__cmpxchg(&cmos_lock, 0, new, sizeof(cmos_lock)))) {
		cpu_relax();
	}

Compiled and runtime-tested on linux-2.6.22-rc4-mm2, Athlon.

Signed-off-by: Andreas Mohr <andi@lisas.de>


--- linux-2.6.22-rc4-mm2.orig/include/asm-i386/mc146818rtc.h	2007-06-10 22:00:53.000000000 +0200
+++ linux-2.6.22-rc4-mm2/include/asm-i386/mc146818rtc.h	2007-06-09 21:40:21.000000000 +0200
@@ -43,8 +43,10 @@
 	unsigned long new;
 	new = ((smp_processor_id()+1) << 8) | reg;
 	for (;;) {
-		if (cmos_lock)
+		if (cmos_lock) {
+			cpu_relax();
 			continue;
+		}
 		if (__cmpxchg(&cmos_lock, 0, new, sizeof(cmos_lock)) == 0)
 			return;
 	}