Re: [patch] i386: make bitops safe

Re: [patch] i386: make bitops safe

[Chuck Ebbert]

In-Reply-To: <20060228005436.GA24895@redhat.com>

On Mon, 27 Feb 2006 at 16:54:36 -0800, Richard Henderson wrote:

> On Tue, Feb 28, 2006 at 12:47:22AM +0100, Andi Kleen wrote:
> > I remember asking rth about this at some point and IIRC
> > he expressed doubts if it would actually do what expected. Richard?
> 
> It's a bit dicey to be sure.  GCC may or may not be able to look
> through the size of the array and not kill things beyond it.  If
> one could be *sure* of some actual maximum index, this would be
> fine, but I don't think you can.
> 

In theory the bit offset could be from -2**31 to 2**31 - 1

> One could reasonably argue that if you used a structure with a
> flexible array member, that GCC could not look through that.  But
> again I'm not 100% positive this is handled properly.

This seems to work but causes more problems than it solves:

#define vaddr ((volatile long *) addr)
static inline void set_bit(int nr, volatile unsigned long * addr)
{
        __asm__ __volatile__( "lock ; "
                "btsl %2,%1"
                :"+m" (*(vaddr + (nr>>5)))
                :"m" (*vaddr),"Ir" (nr)
                );
}

First, it generates the byte offset nr>>5 and puts it in a register
even though it will never be used in the asm.  I can't find a constraint
that says "I'll be accessing this address but I don't need you to generate
it for me."  Second, the compiler thinks *vaddr will be read when it
really won't (unless nr>>5 == 0 in which case constraint 0 takes care
of it.)

Generated code when nr is a variable:

        movl nr,%edx
        movl %edx,%eax
        sarl $5,%eax
        sall $2,%eax
        lock ; btsl %edx,addr

This causes a register reload afterward (assuming all regs are busy) and
can cause a function to use more stack space.  That plus the three extra
instructions made me go with the full memory clobber instead.

-- 
Chuck
"Equations are the Devil's sentences."  --Stephen Colbert

Re: [patch] i386: make bitops safe

[Richard Henderson]

On Tue, Feb 28, 2006 at 01:04:01AM -0500, Chuck Ebbert wrote:
> > One could reasonably argue that if you used a structure with a
> > flexible array member, that GCC could not look through that.  But
> > again I'm not 100% positive this is handled properly.
> 
> This seems to work but causes more problems than it solves:
> 
> #define vaddr ((volatile long *) addr)

This isn't a structure witha flexible array member.


r~

[patch] i386: make bitops safe

[Chuck Ebbert]

Make i386 bitops safe.  Currently they can be fooled, even on
uniprocessor, by code that uses regions of the bitmap before
invoking the bitop.  The least costly way to make them safe
is to add a memory clobber and tag all of them as volatile.

Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>

---

 include/asm-i386/bitops.h |   56 ++++++++++++++++++++++++++++++----------------
 1 files changed, 37 insertions(+), 19 deletions(-)

--- 2.6.16-rc4-32b.orig/include/asm-i386/bitops.h
+++ 2.6.16-rc4-32b/include/asm-i386/bitops.h
@@ -44,7 +44,8 @@ static inline void set_bit(int nr, volat
 	__asm__ __volatile__( LOCK_PREFIX
 		"btsl %1,%0"
 		:"+m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr)
+		:"memory");
 }
 
 /**
@@ -58,10 +59,11 @@ static inline void set_bit(int nr, volat
  */
 static inline void __set_bit(int nr, volatile unsigned long * addr)
 {
-	__asm__(
+	__asm__ __volatile__(
 		"btsl %1,%0"
 		:"+m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr)
+		:"memory");
 }
 
 /**
@@ -79,7 +81,8 @@ static inline void clear_bit(int nr, vol
 	__asm__ __volatile__( LOCK_PREFIX
 		"btrl %1,%0"
 		:"+m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr)
+		:"memory");
 }
 
 static inline void __clear_bit(int nr, volatile unsigned long * addr)
@@ -87,7 +90,8 @@ static inline void __clear_bit(int nr, v
 	__asm__ __volatile__(
 		"btrl %1,%0"
 		:"+m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr)
+		:"memory");
 }
 #define smp_mb__before_clear_bit()	barrier()
 #define smp_mb__after_clear_bit()	barrier()
@@ -106,7 +110,8 @@ static inline void __change_bit(int nr, 
 	__asm__ __volatile__(
 		"btcl %1,%0"
 		:"+m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr)
+		:"memory");
 }
 
 /**
@@ -124,7 +129,8 @@ static inline void change_bit(int nr, vo
 	__asm__ __volatile__( LOCK_PREFIX
 		"btcl %1,%0"
 		:"+m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr)
+		:"memory");
 }
 
 /**
@@ -143,7 +149,8 @@ static inline int test_and_set_bit(int n
 	__asm__ __volatile__( LOCK_PREFIX
 		"btsl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"+m" (ADDR)
-		:"Ir" (nr) : "memory");
+		:"Ir" (nr)
+		:"memory");
 	return oldbit;
 }
 
@@ -160,10 +167,11 @@ static inline int __test_and_set_bit(int
 {
 	int oldbit;
 
-	__asm__(
+	__asm__ __volatile__(
 		"btsl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"+m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr)
+		:"memory");
 	return oldbit;
 }
 
@@ -183,7 +191,8 @@ static inline int test_and_clear_bit(int
 	__asm__ __volatile__( LOCK_PREFIX
 		"btrl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"+m" (ADDR)
-		:"Ir" (nr) : "memory");
+		:"Ir" (nr)
+		:"memory");
 	return oldbit;
 }
 
@@ -200,10 +209,11 @@ static inline int __test_and_clear_bit(i
 {
 	int oldbit;
 
-	__asm__(
+	__asm__ __volatile__(
 		"btrl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"+m" (ADDR)
-		:"Ir" (nr));
+		:"Ir" (nr)
+		:"memory");
 	return oldbit;
 }
 
@@ -215,7 +225,8 @@ static inline int __test_and_change_bit(
 	__asm__ __volatile__(
 		"btcl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"+m" (ADDR)
-		:"Ir" (nr) : "memory");
+		:"Ir" (nr)
+		:"memory");
 	return oldbit;
 }
 
@@ -234,7 +245,8 @@ static inline int test_and_change_bit(in
 	__asm__ __volatile__( LOCK_PREFIX
 		"btcl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit),"+m" (ADDR)
-		:"Ir" (nr) : "memory");
+		:"Ir" (nr)
+		:"memory");
 	return oldbit;
 }
 
@@ -259,7 +271,8 @@ static inline int variable_test_bit(int 
 	__asm__ __volatile__(
 		"btl %2,%1\n\tsbbl %0,%0"
 		:"=r" (oldbit)
-		:"m" (ADDR),"Ir" (nr));
+		:"m" (ADDR),"Ir" (nr)
+		:"memory");
 	return oldbit;
 }
 
@@ -298,7 +311,8 @@ static inline int find_first_zero_bit(co
 		"shll $3,%%edi\n\t"
 		"addl %%edi,%%edx"
 		:"=d" (res), "=&c" (d0), "=&D" (d1), "=&a" (d2)
-		:"1" ((size + 31) >> 5), "2" (addr), "b" (addr) : "memory");
+		:"1" ((size + 31) >> 5), "2" (addr), "b" (addr)
+		:"memory");
 	return res;
 }
 
@@ -405,7 +419,9 @@ static inline int ffs(int x)
 	__asm__("bsfl %1,%0\n\t"
 		"jnz 1f\n\t"
 		"movl $-1,%0\n"
-		"1:" : "=r" (r) : "rm" (x));
+		"1:"
+		: "=r" (r)
+		: "rm" (x));
 	return r+1;
 }
 
@@ -422,7 +438,9 @@ static inline int fls(int x)
 	__asm__("bsrl %1,%0\n\t"
 		"jnz 1f\n\t"
 		"movl $-1,%0\n"
-		"1:" : "=r" (r) : "rm" (x));
+		"1:"
+		: "=r" (r)
+		: "rm" (x));
 	return r+1;
 }
 
-- 
Chuck
"Equations are the Devil's sentences."  --Stephen Colbert

Re: [patch] i386: make bitops safe

[Linus Torvalds]

On Mon, 27 Feb 2006, Chuck Ebbert wrote:
>
> Make i386 bitops safe.  Currently they can be fooled, even on
> uniprocessor, by code that uses regions of the bitmap before
> invoking the bitop.  The least costly way to make them safe
> is to add a memory clobber and tag all of them as volatile.

Actually, the least costly way should be to make the "ADDR" define work 
right again.

It used to do something magic like

	struct fake_area {
		unsigned long members[1000];
	};

	#define ADDR (*(volatile struct fake_area *)addr)

which was correct. I forget why it got broken into using just a "long *" 
(it happened a long long time ago).

		Linus

Re: [patch] i386: make bitops safe

[Andi Kleen]

On Tuesday 28 February 2006 00:06, Linus Torvalds wrote:
> On Mon, 27 Feb 2006, Chuck Ebbert wrote:
> > Make i386 bitops safe.  Currently they can be fooled, even on
> > uniprocessor, by code that uses regions of the bitmap before
> > invoking the bitop.  The least costly way to make them safe
> > is to add a memory clobber and tag all of them as volatile.
>
> Actually, the least costly way should be to make the "ADDR" define work
> right again.
>
> It used to do something magic like

I remember asking rth about this at some point and IIRC
he expressed doubts if it would actually do what expected. Richard?

-Andi

>
> 	struct fake_area {
> 		unsigned long members[1000];
> 	};
>
> 	#define ADDR (*(volatile struct fake_area *)addr)
>
> which was correct. I forget why it got broken into using just a "long *"
> (it happened a long long time ago).
>
> 		Linus

Re: [patch] i386: make bitops safe

[Richard Henderson]

On Tue, Feb 28, 2006 at 12:47:22AM +0100, Andi Kleen wrote:
> I remember asking rth about this at some point and IIRC
> he expressed doubts if it would actually do what expected. Richard?

It's a bit dicey to be sure.  GCC may or may not be able to look
through the size of the array and not kill things beyond it.  If
one could be *sure* of some actual maximum index, this would be
fine, but I don't think you can.

One could reasonably argue that if you used a structure with a
flexible array member, that GCC could not look through that.  But
again I'm not 100% positive this is handled properly.

I think the best argument for simply leaving things with a memory
clobber is that these are atomic operations, and are on occasion
used as locks, or parts of locks.


r~

Re: [patch] i386: make bitops safe

[Andi Kleen]

On Tuesday 28 February 2006 01:54, Richard Henderson wrote:

> I think the best argument for simply leaving things with a memory
> clobber is that these are atomic operations, and are on occasion
> used as locks, or parts of locks.

How about __set_bit? It is supposed to be not atomic. What would
be best there?

-Andi