[IA64] Effective __clear_bit_unlock V2

[IA64] Effective __clear_bit_unlock V2

[Christoph Lameter]

Avoid the use of cmpxchg in __clear_bit_unlock by calling __clear_bit.
__clear_bit uses a volatile pointer and will generate a store with
release semantics as shown by Zoltan. Add a barrier to make sure that
the compiler does not reorder instructions around it.

Signed-off-by: Christoph Lameter <clameter@sgi.com>
Acked-by: Nick Piggin <npiggin@suse.de>

---
 include/asm-ia64/bitops.h |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Index: linux-2.6/include/asm-ia64/bitops.h
=================================--- linux-2.6.orig/include/asm-ia64/bitops.h	2007-10-19 04:05:38.000000000 -0700
+++ linux-2.6/include/asm-ia64/bitops.h	2007-10-19 10:50:34.000000000 -0700
@@ -118,14 +118,6 @@ clear_bit_unlock (int nr, volatile void 
 }
 
 /**
- * __clear_bit_unlock - Non-atomically clear a bit with release
- *
- * This is like clear_bit_unlock, but the implementation may use a non-atomic
- * store (this one uses an atomic, however).
- */
-#define __clear_bit_unlock clear_bit_unlock
-
-/**
  * __clear_bit - Clears a bit in memory (non-atomic version)
  */
 static __inline__ void
@@ -137,6 +129,16 @@ __clear_bit (int nr, volatile void *addr
 }
 
 /**
+ * __clear_bit_unlock - Non-atomically clear a bit with release
+ */
+static __inline__ void
+__clear_bit_unlock (int nr, volatile void *addr)
+{
+	barrier();
+	__clear_bit(nr, addr);
+}
+
+/**
  * change_bit - Toggle a bit in memory
  * @nr: Bit to clear
  * @addr: Address to start counting from

Re: [IA64] Effective __clear_bit_unlock V2

[Zoltan Menyhart]

Christoph Lameter wrote:

> --- linux-2.6.orig/include/asm-ia64/bitops.h	2007-10-19 04:05:38.000000000 -0700
> +++ linux-2.6/include/asm-ia64/bitops.h	2007-10-19 10:50:34.000000000 -0700

...

>  /**
> + * __clear_bit_unlock - Non-atomically clear a bit with release
> + */
> +static __inline__ void
> +__clear_bit_unlock (int nr, volatile void *addr)
> +{
> +	barrier();
> +	__clear_bit(nr, addr);
> +}
> +
> +/**

Well, the "old" __clear_bit() went like this:

__clear_bit (int nr, volatile void *addr)
{
	*((__u32 *) addr + (nr >> 5)) &= ~(1 << (nr & 31));
}

(I really do not see why "addr" is a pointer to a volatile stuff.
If it really can change independently from the current thread of
control, then a non atomic operation is not much useful.
Luckily :-) "addr" is casted into a pointer to a non volatile stuff.)

It has been changed into:

__clear_bit (int nr, volatile void *addr)
{
        volatile __u32 *p = (__u32 *) addr + (nr >> 5);
        __u32 m = 1 << (nr & 31);
        *p &= ~m;
}

I guess because of __clear_bit_unlock() needs only.
__set_bit() still keeps its ".rel" / ".acq" free store / load instruction.

__set_bit (int nr, volatile void *addr)
{
        *((__u32 *) addr + (nr >> 5)) |= (1 << (nr & 31));
}

Note that most of the __clear_bit() / __set_bit() usage are some bit map
or indicator manipulations. They do not need any ".rel" / ".acq"
semantics.

Let's put back the old ".rel" / ".acq" free __clear_bit() and
let's use specific variant of ...clear_bit() where ".rel" semantics
is requred.
Let's avoid adding ".acq" semantics to ...clear_bit() where is is not
required.


Zoltan

RE: [IA64] Effective __clear_bit_unlock V2

[Luck, Tony]

Is this the final take on this ...  or was there a newer better faster cleaner
version?

-Tony 

-----Original Message-----
From: Christoph Lameter [mailto:clameter@sgi.com] 
Sent: Friday, October 19, 2007 10:54 AM
To: Luck, Tony
Cc: linux-ia64@vger.kernel.org; Zoltan Menyhart
Subject: [IA64] Effective __clear_bit_unlock V2

Avoid the use of cmpxchg in __clear_bit_unlock by calling __clear_bit.
__clear_bit uses a volatile pointer and will generate a store with
release semantics as shown by Zoltan. Add a barrier to make sure that
the compiler does not reorder instructions around it.

Signed-off-by: Christoph Lameter <clameter@sgi.com>
Acked-by: Nick Piggin <npiggin@suse.de>

---
 include/asm-ia64/bitops.h |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Index: linux-2.6/include/asm-ia64/bitops.h
=================================--- linux-2.6.orig/include/asm-ia64/bitops.h	2007-10-19 04:05:38.000000000 -0700
+++ linux-2.6/include/asm-ia64/bitops.h	2007-10-19 10:50:34.000000000 -0700
@@ -118,14 +118,6 @@ clear_bit_unlock (int nr, volatile void 
 }
 
 /**
- * __clear_bit_unlock - Non-atomically clear a bit with release
- *
- * This is like clear_bit_unlock, but the implementation may use a non-atomic
- * store (this one uses an atomic, however).
- */
-#define __clear_bit_unlock clear_bit_unlock
-
-/**
  * __clear_bit - Clears a bit in memory (non-atomic version)
  */
 static __inline__ void
@@ -137,6 +129,16 @@ __clear_bit (int nr, volatile void *addr
 }
 
 /**
+ * __clear_bit_unlock - Non-atomically clear a bit with release
+ */
+static __inline__ void
+__clear_bit_unlock (int nr, volatile void *addr)
+{
+	barrier();
+	__clear_bit(nr, addr);
+}
+
+/**
  * change_bit - Toggle a bit in memory
  * @nr: Bit to clear
  * @addr: Address to start counting from

RE: [IA64] Effective __clear_bit_unlock V2

[Christoph Lameter]

On Mon, 29 Oct 2007, Luck, Tony wrote:

> Is this the final take on this ...  or was there a newer better faster cleaner
> version?

AFAICT this is final.

Re: [IA64] Effective __clear_bit_unlock V2

[Zoltan Menyhart]

Christoph Lameter wrote:
> On Mon, 29 Oct 2007, Luck, Tony wrote:
> 
> 
>>Is this the final take on this ...  or was there a newer better faster cleaner
>>version?
> 
> 
> AFAICT this is final.
> 

Can you please explain:
- why should we have both acquisition and release semantics for
  __clear_bit(), while none of them is present for __set_bit() ?

- why should we have both acquisition and release semantics for
  __clear_bit() used for purposes other than clear_bit_unlock() ?

Thanks,

Zoltan