2.4.x: i386/x86_64 bitops clobberings

2.4.x: i386/x86_64 bitops clobberings

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

Hi,

after going through debugging hell with some out-of-tree code, I
realised that this patch

http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=92934bcbf96bc9dc931c40ca5f1a57685b7b813b

makes a difference: current 2.6 works with the following code sequence
as expected (printk is executed), 2.4 fails.


#include <asm/bitops.h>
#include <linux/module.h>

unsigned long a = 1;

int module_init(void)
{
	unsigned long b = 0;
	int x;

	x = __test_and_set_bit(0, &b);
	if (__test_and_set_bit(0, &a))
		printk("x = %d\n", x);

	return -1;
}


There will likely be a way to work around my issue. Nevertheless, I
wondered if that patch was already considered for 2.4 inclusion. Or is
there no risk that in-tree code is affected?

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: 2.4.x: i386/x86_64 bitops clobberings

[Willy Tarreau]

Hi Jan,

On Sun, Oct 08, 2006 at 11:42:07PM +0200, Jan Kiszka wrote:
> Hi,
> 
> after going through debugging hell with some out-of-tree code, I
> realised that this patch
> 
> http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=92934bcbf96bc9dc931c40ca5f1a57685b7b813b
> 
> makes a difference: current 2.6 works with the following code sequence
> as expected (printk is executed), 2.4 fails.
> 
> 
> #include <asm/bitops.h>
> #include <linux/module.h>
> 
> unsigned long a = 1;
> 
> int module_init(void)
> {
> 	unsigned long b = 0;
> 	int x;
> 
> 	x = __test_and_set_bit(0, &b);
> 	if (__test_and_set_bit(0, &a))
> 		printk("x = %d\n", x);
> 
> 	return -1;
> }
> 
> 
> There will likely be a way to work around my issue. Nevertheless, I
> wondered if that patch was already considered for 2.4 inclusion. Or is
> there no risk that in-tree code is affected?

While I remember some discussion on the subject for 2.6, I don't
recall anything similiar in 2.4. Wouldn't you happen to build with
gcc-3.4 ? IIRC, the clobbering changed around this version. Could
you confirm that the patch you pointed above fixes the problem for
your case ?

Thanks in advance,
Willy

Re: 2.4.x: i386/x86_64 bitops clobberings

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]

Willy Tarreau wrote:
> Hi Jan,
> 
> On Sun, Oct 08, 2006 at 11:42:07PM +0200, Jan Kiszka wrote:
>> Hi,
>>
>> after going through debugging hell with some out-of-tree code, I
>> realised that this patch
>>
>> http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=92934bcbf96bc9dc931c40ca5f1a57685b7b813b
>>
>> makes a difference: current 2.6 works with the following code sequence
>> as expected (printk is executed), 2.4 fails.
>>
>>
>> #include <asm/bitops.h>
>> #include <linux/module.h>
>>
>> unsigned long a = 1;
>>
>> int module_init(void)
>> {
>> 	unsigned long b = 0;
>> 	int x;
>>
>> 	x = __test_and_set_bit(0, &b);
>> 	if (__test_and_set_bit(0, &a))
>> 		printk("x = %d\n", x);
>>
>> 	return -1;
>> }
>>
>>
>> There will likely be a way to work around my issue. Nevertheless, I
>> wondered if that patch was already considered for 2.4 inclusion. Or is
>> there no risk that in-tree code is affected?
> 
> While I remember some discussion on the subject for 2.6, I don't
> recall anything similiar in 2.4. Wouldn't you happen to build with
> gcc-3.4 ? IIRC, the clobbering changed around this version. Could

I used gcc 3.3.6-13 (Debian).

> you confirm that the patch you pointed above fixes the problem for
> your case ?

Yes. I ported the x86 part back to 2.4.33 and my problem disappeared.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 246 bytes --]

Re: 2.4.x: i386/x86_64 bitops clobberings

[Willy Tarreau]

On Mon, Oct 09, 2006 at 12:53:56AM +0200, Jan Kiszka wrote:
> Willy Tarreau wrote:
> > Hi Jan,
> > 
> > On Sun, Oct 08, 2006 at 11:42:07PM +0200, Jan Kiszka wrote:
> >> Hi,
> >>
> >> after going through debugging hell with some out-of-tree code, I
> >> realised that this patch
> >>
> >> http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=92934bcbf96bc9dc931c40ca5f1a57685b7b813b
> >>
> >> makes a difference: current 2.6 works with the following code sequence
> >> as expected (printk is executed), 2.4 fails.
> >>
> >>
> >> #include <asm/bitops.h>
> >> #include <linux/module.h>
> >>
> >> unsigned long a = 1;
> >>
> >> int module_init(void)
> >> {
> >> 	unsigned long b = 0;
> >> 	int x;
> >>
> >> 	x = __test_and_set_bit(0, &b);
> >> 	if (__test_and_set_bit(0, &a))
> >> 		printk("x = %d\n", x);
> >>
> >> 	return -1;
> >> }
> >>
> >>
> >> There will likely be a way to work around my issue. Nevertheless, I
> >> wondered if that patch was already considered for 2.4 inclusion. Or is
> >> there no risk that in-tree code is affected?
> > 
> > While I remember some discussion on the subject for 2.6, I don't
> > recall anything similiar in 2.4. Wouldn't you happen to build with
> > gcc-3.4 ? IIRC, the clobbering changed around this version. Could
> 
> I used gcc 3.3.6-13 (Debian).

OK, fine.

> > you confirm that the patch you pointed above fixes the problem for
> > your case ?
> 
> Yes. I ported the x86 part back to 2.4.33 and my problem disappeared.

I've done it too, but unfortunately, I discovered that it does not build
with gcc-2.95 anymore, while 3.3 is fine :

cyclades.c: In function `cyy_interrupt':
/usr/src/git/linux-2.4/include/asm/bitops.h:130: inconsistent operand constraints in an `asm'
cyclades.c: In function `cyz_handle_rx':
/usr/src/git/linux-2.4/include/asm/bitops.h:130: inconsistent operand constraints in an `asm'

127        __asm__ __volatile__( LOCK_PREFIX
128                "btsl %2,%1\n\tsbbl %0,%0"
129                :"=r" (oldbit),"+m" (ADDR)
130                :"Ir" (nr) : "memory");


I don't know what the right solution should be. I'm not fond of #ifdefs,
and I'm embarrassed to know that gcc can do all sorts of nasty things due
to a wrong clobbering :-/

If you have any idea on the subject, I'm willing to try.

Regards,
Willy

Re: 2.4.x: i386/x86_64 bitops clobberings

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]

Willy Tarreau wrote:
> I've done it too, but unfortunately, I discovered that it does not build
> with gcc-2.95 anymore, while 3.3 is fine :
> 
> cyclades.c: In function `cyy_interrupt':
> /usr/src/git/linux-2.4/include/asm/bitops.h:130: inconsistent operand constraints in an `asm'
> cyclades.c: In function `cyz_handle_rx':
> /usr/src/git/linux-2.4/include/asm/bitops.h:130: inconsistent operand constraints in an `asm'
> 
> 127        __asm__ __volatile__( LOCK_PREFIX
> 128                "btsl %2,%1\n\tsbbl %0,%0"
> 129                :"=r" (oldbit),"+m" (ADDR)
> 130                :"Ir" (nr) : "memory");
> 
> 
> I don't know what the right solution should be. I'm not fond of #ifdefs,
> and I'm embarrassed to know that gcc can do all sorts of nasty things due
> to a wrong clobbering :-/
> 
> If you have any idea on the subject, I'm willing to try.

What about

#if __GNUC__ < 3
#define ADDR_DEPS "=m"
#else
#define ADDR_DEPS "+m"
#endif

	__asm__ __volatile__( LOCK_PREFIX
		"btsl %2,%1\n\tsbbl %0,%0"
		:"=r" (oldbit),ADDR_DEPS (ADDR)
		:"Ir" (nr) : "memory");

or something similar? Would keep the number of #ifs low.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: 2.4.x: i386/x86_64 bitops clobberings

[Willy Tarreau]

Hi Jan,

On Mon, Oct 09, 2006 at 08:27:54AM +0200, Jan Kiszka wrote:
> Willy Tarreau wrote:
> > I've done it too, but unfortunately, I discovered that it does not build
> > with gcc-2.95 anymore, while 3.3 is fine :
> > 
> > cyclades.c: In function `cyy_interrupt':
> > /usr/src/git/linux-2.4/include/asm/bitops.h:130: inconsistent operand constraints in an `asm'
> > cyclades.c: In function `cyz_handle_rx':
> > /usr/src/git/linux-2.4/include/asm/bitops.h:130: inconsistent operand constraints in an `asm'
> > 
> > 127        __asm__ __volatile__( LOCK_PREFIX
> > 128                "btsl %2,%1\n\tsbbl %0,%0"
> > 129                :"=r" (oldbit),"+m" (ADDR)
> > 130                :"Ir" (nr) : "memory");
> > 
> > 
> > I don't know what the right solution should be. I'm not fond of #ifdefs,
> > and I'm embarrassed to know that gcc can do all sorts of nasty things due
> > to a wrong clobbering :-/
> > 
> > If you have any idea on the subject, I'm willing to try.
> 
> What about
> 
> #if __GNUC__ < 3
> #define ADDR_DEPS "=m"
> #else
> #define ADDR_DEPS "+m"
> #endif
> 
> 	__asm__ __volatile__( LOCK_PREFIX
> 		"btsl %2,%1\n\tsbbl %0,%0"
> 		:"=r" (oldbit),ADDR_DEPS (ADDR)
> 		:"Ir" (nr) : "memory");
> 
> or something similar? Would keep the number of #ifs low.

I don't like it for two reasons :

  - I don't know how gcc-3.0 to 3.2 behave on such code, so may be this
    would implicitly categorize them in the wrong group

  - it makes the asm code very hard to read. Operand dependencies are
    very sensible tricks which need to be extremely clear.

I have searched the archives for previous occurrences of this recurring
problem, and found another alternative which is said to work on all gcc
versions. So here's the patch for both x86 and x86_64. I checked that it
produces identical code as I obtain with the patch from 2.6. It also
fixes your testcase for gcc-2.95 to 4.1.1.

Could you please give it a try ?

Andi, Linus, I'd be very interested in your comments on this patch, since
you're more used to deal with this problem than me.

Thanks in advance,
Willy

>From 695f7e0b3806ff44def385856cda60d7f1c2df1c Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Sat, 14 Oct 2006 21:39:06 +0200
Subject: [PATCH] 2.4.x: i386/x86_64 bitops clobberings

Problem reported by Jan Kiszka <jan.kiszka@web.de> :
 "After going through debugging hell with some out-of-tree code,
  I realised that this patch :

  http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=92934bcbf96bc9dc931c40ca5f1a57685b7b813b

  makes a difference: current 2.6 works with the following code
  sequence as expected (printk is executed), 2.4 fails. I used
  gcc 3.3.6-13 (Debian).

  unsigned long a = 1;

  int module_init(void)
  {
        unsigned long b = 0;
        int x;

        x = __test_and_set_bit(0, &b);
        if (__test_and_set_bit(0, &a))
                printk("x = %d\n", x);

        return -1;
  }

  I ported the x86 part back to 2.4.33 and my problem disappeared. "

The problem with this patch is that the use of "+m" in asm statements
triggers bugs in gcc 2.95 which is supported by kernel 2.4. For
instance, cyclades.c does not build anymore :

$ gcc -O2 -pipe -c cyclades-c.c
cyclades.c: In function `cyy_interrupt':
/usr/src/git/linux-2.4/include/asm/bitops.h:134: inconsistent operand constraints in an `asm'
cyclades.c: In function `cyz_handle_rx':
/usr/src/git/linux-2.4/include/asm/bitops.h:134: inconsistent operand constraints in an `asm'

As explained by Richard Henderson here, the equivalent construct
"=m"(x) : "m"(x) works on any version of gcc :

  http://marc.theaimsgroup.com/?l=3Dlinux-kernel&m=3D107475162200773&w=3D2

I successfully verified on the example code above and on cyclades.c
that the exact same code is produced on x86 with this construct with
gcc 2.95.3, 3.3.6, 3.4.4, and 4.1.1, so it looks fine.

Just like in the former patch for 2.6, this one was applied to both
i386 and x86_64.

Signed-Off-By: Willy Tarreau <w@1wt.eu>
---
 include/asm-i386/bitops.h   |   22 +++++++++++-----------
 include/asm-x86_64/bitops.h |   22 +++++++++++-----------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/include/asm-i386/bitops.h b/include/asm-i386/bitops.h
index 9b6b55e..bb63c10 100644
--- a/include/asm-i386/bitops.h
+++ b/include/asm-i386/bitops.h
@@ -38,7 +38,7 @@ static __inline__ void set_bit(int nr, v
 	__asm__ __volatile__( LOCK_PREFIX
 		"btsl %1,%0"
 		:"=m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr), "m" (ADDR));
 }
 
 /**
@@ -55,7 +55,7 @@ static __inline__ void __set_bit(int nr,
 	__asm__(
 		"btsl %1,%0"
 		:"=m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr), "m" (ADDR));
 }
 
 /**
@@ -73,7 +73,7 @@ static __inline__ void clear_bit(int nr,
 	__asm__ __volatile__( LOCK_PREFIX
 		"btrl %1,%0"
 		:"=m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr), "m" (ADDR));
 }
 #define smp_mb__before_clear_bit()	barrier()
 #define smp_mb__after_clear_bit()	barrier()
@@ -92,7 +92,7 @@ static __inline__ void __change_bit(int 
 	__asm__ __volatile__(
 		"btcl %1,%0"
 		:"=m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr), "m" (ADDR));
 }
 
 /**
@@ -109,7 +109,7 @@ static __inline__ void change_bit(int nr
 	__asm__ __volatile__( LOCK_PREFIX
 		"btcl %1,%0"
 		:"=m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr), "m" (ADDR));
 }
 
 /**
@@ -127,7 +127,7 @@ static __inline__ int test_and_set_bit(i
 	__asm__ __volatile__( LOCK_PREFIX
 		"btsl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
-		:"Ir" (nr) : "memory");
+		:"Ir" (nr), "m" (ADDR) : "memory");
 	return oldbit;
 }
 
@@ -147,7 +147,7 @@ static __inline__ int __test_and_set_bit
 	__asm__(
 		"btsl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr), "m" (ADDR));
 	return oldbit;
 }
 
@@ -166,7 +166,7 @@ static __inline__ int test_and_clear_bit
 	__asm__ __volatile__( LOCK_PREFIX
 		"btrl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
-		:"Ir" (nr) : "memory");
+		:"Ir" (nr), "m" (ADDR) : "memory");
 	return oldbit;
 }
 
@@ -186,7 +186,7 @@ static __inline__ int __test_and_clear_b
 	__asm__(
 		"btrl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr), "m" (ADDR));
 	return oldbit;
 }
 
@@ -198,7 +198,7 @@ static __inline__ int __test_and_change_
 	__asm__ __volatile__(
 		"btcl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
-		:"Ir" (nr) : "memory");
+		:"Ir" (nr), "m" (ADDR) : "memory");
 	return oldbit;
 }
 
@@ -217,7 +217,7 @@ static __inline__ int test_and_change_bi
 	__asm__ __volatile__( LOCK_PREFIX
 		"btcl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
-		:"Ir" (nr) : "memory");
+		:"Ir" (nr), "m" (ADDR) : "memory");
 	return oldbit;
 }
 
diff --git a/include/asm-x86_64/bitops.h b/include/asm-x86_64/bitops.h
index c8a4749..f6a2a4f 100644
--- a/include/asm-x86_64/bitops.h
+++ b/include/asm-x86_64/bitops.h
@@ -38,7 +38,7 @@ static __inline__ void set_bit(long nr, 
 	__asm__ __volatile__( LOCK_PREFIX
 		"btsq %1,%0"
 		:"=m" (ADDR)
-		:"dIr" (nr));
+		:"dIr" (nr), "m" (ADDR));
 }
 
 /**
@@ -55,7 +55,7 @@ static __inline__ void __set_bit(long nr
 	__asm__(
 		"btsq %1,%0"
 		:"=m" (ADDR)
-		:"dIr" (nr));
+		:"dIr" (nr), "m" (ADDR));
 }
 
 /**
@@ -73,7 +73,7 @@ static __inline__ void clear_bit(long nr
 	__asm__ __volatile__( LOCK_PREFIX
 		"btrq %1,%0"
 		:"=m" (ADDR)
-		:"dIr" (nr));
+		:"dIr" (nr), "m" (ADDR));
 }
 #define smp_mb__before_clear_bit()	barrier()
 #define smp_mb__after_clear_bit()	barrier()
@@ -92,7 +92,7 @@ static __inline__ void __change_bit(long
 	__asm__ __volatile__(
 		"btcq %1,%0"
 		:"=m" (ADDR)
-		:"dIr" (nr));
+		:"dIr" (nr), "m" (ADDR));
 }
 
 /**
@@ -109,7 +109,7 @@ static __inline__ void change_bit(long n
 	__asm__ __volatile__( LOCK_PREFIX
 		"btcq %1,%0"
 		:"=m" (ADDR)
-		:"dIr" (nr));
+		:"dIr" (nr), "m" (ADDR));
 }
 
 /**
@@ -127,7 +127,7 @@ static __inline__ int test_and_set_bit(l
 	__asm__ __volatile__( LOCK_PREFIX
 		"btsq %2,%1\n\tsbbq %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
-		:"dIr" (nr) : "memory");
+		:"dIr" (nr), "m" (ADDR) : "memory");
 	return oldbit;
 }
 
@@ -147,7 +147,7 @@ static __inline__ int __test_and_set_bit
 	__asm__(
 		"btsq %2,%1\n\tsbbq %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
-		:"dIr" (nr));
+		:"dIr" (nr), "m" (ADDR));
 	return oldbit;
 }
 
@@ -166,7 +166,7 @@ static __inline__ int test_and_clear_bit
 	__asm__ __volatile__( LOCK_PREFIX
 		"btrq %2,%1\n\tsbbq %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
-		:"dIr" (nr) : "memory");
+		:"dIr" (nr), "m" (ADDR) : "memory");
 	return oldbit;
 }
 
@@ -186,7 +186,7 @@ static __inline__ int __test_and_clear_b
 	__asm__(
 		"btrq %2,%1\n\tsbbq %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
-		:"dIr" (nr));
+		:"dIr" (nr), "m" (ADDR));
 	return oldbit;
 }
 
@@ -198,7 +198,7 @@ static __inline__ int __test_and_change_
 	__asm__ __volatile__(
 		"btcq %2,%1\n\tsbbq %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
-		:"dIr" (nr) : "memory");
+		:"dIr" (nr), "m" (ADDR) : "memory");
 	return oldbit;
 }
 
@@ -217,7 +217,7 @@ static __inline__ int test_and_change_bi
 	__asm__ __volatile__( LOCK_PREFIX
 		"btcq %2,%1\n\tsbbq %0,%0"
 		:"=r" (oldbit),"=m" (ADDR)
-		:"dIr" (nr) : "memory");
+		:"dIr" (nr), "m" (ADDR) : "memory");
 	return oldbit;
 }
 
-- 
1.4.1

Re: 2.4.x: i386/x86_64 bitops clobberings

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]

Willy Tarreau wrote:
> ...
> I have searched the archives for previous occurrences of this recurring
> problem, and found another alternative which is said to work on all gcc
> versions. So here's the patch for both x86 and x86_64. I checked that it
> produces identical code as I obtain with the patch from 2.6. It also
> fixes your testcase for gcc-2.95 to 4.1.1.
> 
> Could you please give it a try ?

Works fine here as well. Merging it would be welcome.

Thanks a lot,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

Re: 2.4.x: i386/x86_64 bitops clobberings

[Willy Tarreau]

On Sun, Oct 15, 2006 at 11:56:55AM +0200, Jan Kiszka wrote:
> Willy Tarreau wrote:
> > ...
> > I have searched the archives for previous occurrences of this recurring
> > problem, and found another alternative which is said to work on all gcc
> > versions. So here's the patch for both x86 and x86_64. I checked that it
> > produces identical code as I obtain with the patch from 2.6. It also
> > fixes your testcase for gcc-2.95 to 4.1.1.
> > 
> > Could you please give it a try ?
> 
> Works fine here as well. Merging it would be welcome.
> 
> Thanks a lot,
> Jan

Thanks for your feedback Jan.
Unless someone quickly raises a problem or points a risk, I will merge it,
and will also put it in next -stable.

Regards,
Willy