write_unlock: replace clear_bit with byte store

write_unlock: replace clear_bit with byte store

[Christoph Lameter]

The rwlocks use IMHO too many semaphore operations.

write_lock uses a cmpxchg like the regular spin_lock but write_unlock uses
clear_bit which requires a load and then a loop over a cmpxchg. The
following patch makes write_unlock simply use a store to clear the highest
8 bits. We will then still have the lower 3 bytes (24 bits) left to count
the readers. I would expect the performance of write_lock and
write_unlock to be the same as regular spinlocks with this patch.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

Index: linux-2.6.11/include/asm-ia64/spinlock.h
=================================--- linux-2.6.11.orig/include/asm-ia64/spinlock.h	2005-03-01 23:37:48.000000000 -0800
+++ linux-2.6.11/include/asm-ia64/spinlock.h	2005-04-28 13:15:38.000000000 -0700
@@ -201,8 +201,9 @@ do {										\

 #define _raw_write_unlock(x)								\
 ({											\
-	smp_mb__before_clear_bit();	/* need barrier before releasing lock... */	\
-	clear_bit(31, (x));								\
+	u8 *y = (u8 *)x;								\
+	smp_wmb();			/* need barrier before releasing lock... */	\
+	y[3] = 0;									\
 })

 #endif /*  _ASM_IA64_SPINLOCK_H */

Re: write_unlock: replace clear_bit with byte store

[David Mosberger]

>>>>> On Thu, 28 Apr 2005 13:32:35 -0700 (PDT), Christoph Lameter <clameter@engr.sgi.com> said:

  Christoph> #define _raw_write_unlock(x)								\
  Christoph> ({											\
  Christoph> -	smp_mb__before_clear_bit();	/* need barrier before releasing lock... */	\
  Christoph> -	clear_bit(31, (x));								\
  Christoph> +	u8 *y = (u8 *)x;								\
  Christoph> +	smp_wmb();			/* need barrier before releasing lock... */	\
  Christoph> +	y[3] = 0;									\
  Christoph> })

I'm unsure if it's a good idea to reduce the counter size.
Independent of that: using a name like "y" in a macro is bound to
cause name collisions so it's a big no-no.  Then why not declare
"y" as a "volatile u8 *" so you can avoid the explicit smp_wmb()?
This is ia64-specific code after all.

	--david

Re: write_unlock: replace clear_bit with byte store

[Christoph Lameter]

On Thu, 28 Apr 2005, David Mosberger wrote:

> I'm unsure if it's a good idea to reduce the counter size.

We can still accomodate 2^24 = 16 million concurrent readers.

> Independent of that: using a name like "y" in a macro is bound to
> cause name collisions so it's a big no-no.  Then why not declare
> "y" as a "volatile u8 *" so you can avoid the explicit smp_wmb()?
> This is ia64-specific code after all.

I thought that a memory barrier was needed to insure that the clearing of
the lock becomes visible after changes done in the critical section?

y is defined in its own block.

May be better do inline asm with an .nta store?

Re: write_unlock: replace clear_bit with byte store

[Christoph Lameter]

This version uses an inline function and a nontemporal store to unlock
the rwlock.

write_lock uses a cmpxchg like the regular spin_lock but write_unlock uses
clear_bit which requires a load and then a loop over a cmpxchg. The
following patch makes write_unlock simply use a nontemporal store to clear
the highest 8 bits. We will then still have the lower 3 bytes (24 bits)
left to count the readers.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

Index: linux-2.6.11/include/asm-ia64/spinlock.h
=================================--- linux-2.6.11.orig/include/asm-ia64/spinlock.h	2005-03-01 23:37:48.000000000 -0800
+++ linux-2.6.11/include/asm-ia64/spinlock.h	2005-04-28 15:02:28.000000000 -0700
@@ -199,10 +199,10 @@ do {										\

 #define _raw_read_trylock(lock) generic_raw_read_trylock(lock)

-#define _raw_write_unlock(x)								\
-({											\
-	smp_mb__before_clear_bit();	/* need barrier before releasing lock... */	\
-	clear_bit(31, (x));								\
-})
+static inline void _raw_write_unlock(spinlock_t *x) {
+	u8 *y = (u8 *)x;
+	barrier();
+	asm volatile ("st1.rel.nta [%0] = r0\n\t" :: "r"(y+3) : "memory" );
+}

 #endif /*  _ASM_IA64_SPINLOCK_H */

RE: write_unlock: replace clear_bit with byte store

[Zou, Nanhai]

> -----Original Message-----
> From: linux-ia64-owner@vger.kernel.org
> [mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of Christoph
Lameter
> Sent: Friday, April 29, 2005 6:05 AM
> To: davidm@hpl.hp.com
> Cc: linux-ia64@vger.kernel.org
> Subject: Re: write_unlock: replace clear_bit with byte store
> 
> This version uses an inline function and a nontemporal store to unlock
> the rwlock.
> 
> write_lock uses a cmpxchg like the regular spin_lock but write_unlock
uses
> clear_bit which requires a load and then a loop over a cmpxchg. The
> following patch makes write_unlock simply use a nontemporal store to
clear
> the highest 8 bits. We will then still have the lower 3 bytes (24
bits)
> left to count the readers.
> 
> Signed-off-by: Christoph Lameter <clameter@sgi.com>
> 
> Index: linux-2.6.11/include/asm-ia64/spinlock.h
> =================================> --- linux-2.6.11.orig/include/asm-ia64/spinlock.h	2005-03-01
> 23:37:48.000000000 -0800
> +++ linux-2.6.11/include/asm-ia64/spinlock.h	2005-04-28
> 15:02:28.000000000 -0700
> @@ -199,10 +199,10 @@ do {
\
> 
>  #define _raw_read_trylock(lock) generic_raw_read_trylock(lock)
> 
> -#define _raw_write_unlock(x)
\
> -({
\
> -	smp_mb__before_clear_bit();	/* need barrier before releasing
lock... */
> 	\
> -	clear_bit(31, (x));
\
> -})
> +static inline void _raw_write_unlock(spinlock_t *x) {

Shouldn't that an rwlock_t?

> +	u8 *y = (u8 *)x;
> +	barrier();
> +	asm volatile ("st1.rel.nta [%0] = r0\n\t" :: "r"(y+3) : "memory"
);
> +}
> 
>  #endif /*  _ASM_IA64_SPINLOCK_H */
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64"
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: write_unlock: replace clear_bit with byte store

[Christoph Lameter]

On Fri, 29 Apr 2005, Zou, Nanhai wrote:

> > +static inline void _raw_write_unlock(spinlock_t *x) {
>
> Shouldn't that an rwlock_t?

Correct. CC does not check types when doing inline functions? If I mispell
the type name then the compilation fails. I can put u64 * in there or
anything else and CC  happily allows the passing of a rwlock_t *? Weird...
Maybe because of the type cast?

----------------------------------------------------------
write_lock uses a cmpxchg like the regular spin_lock but write_unlock uses
clear_bit which requires a load and then a loop over a cmpxchg. The
following patch makes write_unlock simply use a nontemporal store to clear
the highest 8 bits. We will then still have the lower 3 bytes (24 bits)
left to count the readers.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

Index: linux-2.6.11/include/asm-ia64/spinlock.h
=================================--- linux-2.6.11.orig/include/asm-ia64/spinlock.h	2005-03-01 23:37:48.000000000 -0800
+++ linux-2.6.11/include/asm-ia64/spinlock.h	2005-04-28 15:44:30.000000000 -0700
@@ -199,10 +199,10 @@ do {										\

 #define _raw_read_trylock(lock) generic_raw_read_trylock(lock)

-#define _raw_write_unlock(x)								\
-({											\
-	smp_mb__before_clear_bit();	/* need barrier before releasing lock... */	\
-	clear_bit(31, (x));								\
-})
+static inline void _raw_write_unlock(rwlock_t *x) {
+	u8 *y = (u8 *)x;
+	barrier();
+	asm volatile ("st1.rel.nta [%0] = r0\n\t" :: "r"(y+3) : "memory" );
+}

 #endif /*  _ASM_IA64_SPINLOCK_H */

RE: write_unlock: replace clear_bit with byte store

[Chen, Kenneth W]

Christoph Lameter wrote on Thursday, April 28, 2005 3:52 PM
> +static inline void _raw_write_unlock(rwlock_t *x) {
> +	u8 *y = (u8 *)x;
> +	barrier();
> +	asm volatile ("st1.rel.nta [%0] = r0\n\t" :: "r"(y+3) : "memory" ); }

Perhaps you should make this code icc friendly by not using asm directly.
Intel compiler doesn't support asm statement.

RE: write_unlock: replace clear_bit with byte store

[Christoph Lameter]

On Thu, 28 Apr 2005, Chen, Kenneth W wrote:

> Christoph Lameter wrote on Thursday, April 28, 2005 3:52 PM
> > +static inline void _raw_write_unlock(rwlock_t *x) {
> > +	u8 *y = (u8 *)x;
> > +	barrier();
> > +	asm volatile ("st1.rel.nta [%0] = r0\n\t" :: "r"(y+3) : "memory" ); }
>
> Perhaps you should make this code icc friendly by not using asm directly.
> Intel compiler doesn't support asm statement.

How do I do a store with release and nontemporal semantics without asm?

RE: write_unlock: replace clear_bit with byte store

[Chen, Kenneth W]

Christoph Lameter wrote on Thursday, April 28, 2005 5:22 PM
> How do I do a store with release and nontemporal semantics without asm?

Use ia64_st1_rel(), and add a wrapper in gcc_intrin.h.  Though that only
takes care of store with release semantics.

RE: write_unlock: replace clear_bit with byte store

[Christoph Lameter]

On Thu, 28 Apr 2005, Chen, Kenneth W wrote:

> Christoph Lameter wrote on Thursday, April 28, 2005 5:22 PM
> > How do I do a store with release and nontemporal semantics without asm?
>
> Use ia64_st1_rel(), and add a wrapper in gcc_intrin.h.  Though that only
> takes care of store with release semantics.

Hmm... How about this one. Its still better than the cmpxchg for C and you
can do all the tricks you want with the C code later. I hope the bitfields
allocate the lowest bits first?

---

write_lock uses a cmpxchg like the regular spin_lock but write_unlock uses
clear_bit which requires a load and then a loop over a cmpxchg. The
following patch makes write_unlock simply use a nontemporal store to clear
the highest 8 bits. We will then still have the lower 3 bytes (24 bits)
left to count the readers.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

Index: linux-2.6.11/include/asm-ia64/spinlock.h
=================================--- linux-2.6.11.orig/include/asm-ia64/spinlock.h	2005-03-01 23:37:48.000000000 -0800
+++ linux-2.6.11/include/asm-ia64/spinlock.h	2005-04-28 17:48:11.000000000 -0700
@@ -117,8 +117,8 @@ do {											\
 #define spin_unlock_wait(x)	do { barrier(); } while ((x)->lock)

 typedef struct {
-	volatile unsigned int read_counter	: 31;
-	volatile unsigned int write_lock	:  1;
+	volatile unsigned int read_counter	: 24;
+	volatile unsigned int write_lock	:  8;
 #ifdef CONFIG_PREEMPT
 	unsigned int break_lock;
 #endif
@@ -174,6 +174,13 @@ do {										\
 	(result = 0);								\
 })

+static inline void _raw_write_unlock(rwlock_t *x)
+{
+	u8 *y = (u8 *)x;
+	barrier();
+	asm volatile ("st1.rel.nta [%0] = r0\n\t" :: "r"(y+3) : "memory" );
+}
+
 #else /* !ASM_SUPPORTED */

 #define _raw_write_lock(l)								\
@@ -195,14 +202,14 @@ do {										\
 	(ia64_val = 0);						\
 })

+static inline void _raw_write_unlock(rwlock_t *x)
+{
+	barrier();
+	x->write_lock = 0;
+}
+
 #endif /* !ASM_SUPPORTED */

 #define _raw_read_trylock(lock) generic_raw_read_trylock(lock)

-#define _raw_write_unlock(x)								\
-({											\
-	smp_mb__before_clear_bit();	/* need barrier before releasing lock... */	\
-	clear_bit(31, (x));								\
-})
-
 #endif /*  _ASM_IA64_SPINLOCK_H */

RE: write_unlock: replace clear_bit with byte store

[Chen, Kenneth W]

Christoph Lameter wrote on Thursday, April 28, 2005 5:51 PM

> +static inline void _raw_write_unlock(rwlock_t *x) {
> +	barrier();
> +	x->write_lock = 0;
> +}
> +
>  #endif /* !ASM_SUPPORTED */

This portion is broken, where is the release semantics for updating
the lock variable?  Or where is the smp_mb() if you decide to use an
unordered store for the lock variable?

RE: write_unlock: replace clear_bit with byte store

[Chen, Kenneth W]

Chen, Kenneth W wrote on Thursday, April 28, 2005 6:04 PM
> Christoph Lameter wrote on Thursday, April 28, 2005 5:51 PM
> 
> > +static inline void _raw_write_unlock(rwlock_t *x) {
> > +	barrier();
> > +	x->write_lock = 0;
> > +}
> > +
> >  #endif /* !ASM_SUPPORTED */
> 
> This portion is broken, where is the release semantics for updating
> the lock variable?  Or where is the smp_mb() if you decide to use an
> unordered store for the lock variable?

Oops, never mind.  Missed the portion that write_lock is declared as
volatile.  My bad.  This code is OK.

Re: write_unlock: replace clear_bit with byte store

[David Mosberger]

>>>>> On Thu, 28 Apr 2005 15:05:16 -0700 (PDT), Christoph Lameter <clameter@engr.sgi.com> said:

  Christoph> +	u8 *y = (u8 *)x;

Same comments as before...

  Christoph> +	asm volatile ("st1.rel.nta [%0] = r0\n\t" :: "r"(y+3) : "memory" );

Let's not reintroduce in-line asm, please?

	--david

Re: write_unlock: replace clear_bit with byte store

[David Mosberger]

>>>>> On Thu, 28 Apr 2005 13:53:11 -0700 (PDT), Christoph Lameter <clameter@engr.sgi.com> said:

  Christoph> I thought that a memory barrier was needed to insure that
  Christoph> the clearing of the lock becomes visible after changes
  Christoph> done in the critical section?

With "volatile", the compiler will generate stX.rel...

  Christoph> y is defined in its own block.

Ummh, macros 101: think about what happens for _raw_write_unlock(y)...

	--david

Re: write_unlock: replace clear_bit with byte store

[Christoph Lameter]

On Fri, 29 Apr 2005, David Mosberger wrote:

> Let's not reintroduce in-line asm, please?

How do I do a nta store without inline asm? There is already lots of
inline asm in spinlock.h.

Re: write_unlock: replace clear_bit with byte store

[Christoph Lameter]

On Fri, 29 Apr 2005, David Mosberger wrote:

>   Christoph> y is defined in its own block.
>
> Ummh, macros 101: think about what happens for _raw_write_unlock(y)...

Not if its defined in an inline function.

Re: write_unlock: replace clear_bit with byte store

[David Mosberger]

>>>>> On Fri, 29 Apr 2005 08:20:25 -0700 (PDT), Christoph Lameter <clameter@engr.sgi.com> said:

  Christoph> On Fri, 29 Apr 2005, David Mosberger wrote:
  Christoph> y is defined in its own block.

  >> Ummh, macros 101: think about what happens for _raw_write_unlock(y)...

  Christoph> Not if its defined in an inline function.

I thought it was declared inside a macro.  Never mind.

	--david

Re: write_unlock: replace clear_bit with byte store

[David Mosberger]

>>>>> On Fri, 29 Apr 2005 08:19:20 -0700 (PDT), Christoph Lameter <clameter@engr.sgi.com> said:

  Christoph> On Fri, 29 Apr 2005, David Mosberger wrote:
  >> Let's not reintroduce in-line asm, please?

  Christoph> How do I do a nta store without inline asm?

You add a new intrinsic?

  Christoph> There is already lots of inline asm in spinlock.h.

spinlock.h is special because it performs non-standard calls to the
out-of-line spinlock contention-handler, but those places are enclosed
inside ASM_SUPPORTED.

	--david

Re: write_unlock: replace clear_bit with byte store

[Christoph Lameter]

On Fri, 29 Apr 2005, David Mosberger wrote:

> >>>>> On Fri, 29 Apr 2005 08:19:20 -0700 (PDT), Christoph Lameter <clameter@engr.sgi.com> said:
>
>   Christoph> On Fri, 29 Apr 2005, David Mosberger wrote:
>   >> Let's not reintroduce in-line asm, please?
>
>   Christoph> How do I do a nta store without inline asm?
>
> You add a new intrinsic?
>
>   Christoph> There is already lots of inline asm in spinlock.h.
>
> spinlock.h is special because it performs non-standard calls to the
> out-of-line spinlock contention-handler, but those places are enclosed
> inside ASM_SUPPORTED.

So is my version of inline asm int the latest version from yesterday.
There is an alternate version included for !ASM_SUPPORTED in C which does
not use an nta store.

Re: write_unlock: replace clear_bit with byte store

[David Mosberger]

>>>>> On Fri, 29 Apr 2005 08:30:27 -0700 (PDT), Christoph Lameter <clameter@engr.sgi.com> said:

  Christoph> So is my version of inline asm int the latest version
  Christoph> from yesterday.  There is an alternate version included
  Christoph> for !ASM_SUPPORTED in C which does not use an nta store.

Yes, but _if_ it's a good idea to use .nta with GCC, there is no
reason not to do the same with ICC.  Don't introduce unnecessary
divergence.

	--david

Re: write_unlock: replace clear_bit with byte store

[Christoph Lameter]

On Fri, 29 Apr 2005, David Mosberger wrote:

> >>>>> On Fri, 29 Apr 2005 08:30:27 -0700 (PDT), Christoph Lameter <clameter@engr.sgi.com> said:
>
>   Christoph> So is my version of inline asm int the latest version
>   Christoph> from yesterday.  There is an alternate version included
>   Christoph> for !ASM_SUPPORTED in C which does not use an nta store.
>
> Yes, but _if_ it's a good idea to use .nta with GCC, there is no
> reason not to do the same with ICC.  Don't introduce unnecessary
> divergence.

The same situation of .nta only for GCC already exists for regular
spinlocks as a result of my nta unlock patch that I posted a week or so
ago.

I do not know too much about ICC...

Re: write_unlock: replace clear_bit with byte store

[David Mosberger]

>>>>> On Fri, 29 Apr 2005 08:46:14 -0700 (PDT), Christoph Lameter <clameter@engr.sgi.com> said:

  Christoph> On Fri, 29 Apr 2005, David Mosberger wrote:
  >> >>>>> On Fri, 29 Apr 2005 08:30:27 -0700 (PDT), Christoph Lameter <clameter@engr.sgi.com> said:

  Christoph> So is my version of inline asm int the latest version
  Christoph> from yesterday.  There is an alternate version included
  Christoph> for !ASM_SUPPORTED in C which does not use an nta store.

  >> Yes, but _if_ it's a good idea to use .nta with GCC, there is no
  >> reason not to do the same with ICC.  Don't introduce unnecessary
  >> divergence.

  Christoph> The same situation of .nta only for GCC already exists
  Christoph> for regular spinlocks as a result of my nta unlock patch
  Christoph> that I posted a week or so ago.

And that's an argument to make the situation worse?  How about
cleaning up the previous patch instead?

	--david

Re: write_unlock: replace clear_bit with byte store

[Christoph Hellwig]

On Fri, Apr 29, 2005 at 08:48:43AM -0700, David Mosberger wrote:
>   >> Yes, but _if_ it's a good idea to use .nta with GCC, there is no
>   >> reason not to do the same with ICC.  Don't introduce unnecessary
>   >> divergence.
> 
>   Christoph> The same situation of .nta only for GCC already exists
>   Christoph> for regular spinlocks as a result of my nta unlock patch
>   Christoph> that I posted a week or so ago.
> 
> And that's an argument to make the situation worse?  How about
> cleaning up the previous patch instead?

I don't think it's fair to expect contributors to fix up ifdef'ed bits for
a propritary compiler.  If HP and Intel care about it they can add the
features for icc later.

Re: write_unlock: replace clear_bit with byte store

[Christoph Lameter]

On Fri, 29 Apr 2005, David Mosberger wrote:

> And that's an argument to make the situation worse?  How about
> cleaning up the previous patch instead?

I was not aware of the situation with ICC. Sorry. Maybe I can get
to solving the ICC issue later this fall when I finally get a week or
so to get familiar with the compiler...

Re: write_unlock: replace clear_bit with byte store

[David Mosberger]

>>>>> On Fri, 29 Apr 2005 16:51:53 +0100, Christoph Hellwig <hch@infradead.org> said:

  Christoph> On Fri, Apr 29, 2005 at 08:48:43AM -0700, David Mosberger wrote:
  >> >> Yes, but _if_ it's a good idea to use .nta with GCC, there is no
  >> >> reason not to do the same with ICC.  Don't introduce unnecessary
  >> >> divergence.

  Christoph> The same situation of .nta only for GCC already exists
  Christoph> for regular spinlocks as a result of my nta unlock patch
  Christoph> that I posted a week or so ago.

  >> And that's an argument to make the situation worse?  How about
  >> cleaning up the previous patch instead?

  Christoph> I don't think it's fair to expect contributors to fix up
  Christoph> ifdef'ed bits for a propritary compiler.  If HP and Intel
  Christoph> care about it they can add the features for icc later.

I don't think this is about proprietary vs non-proprietary.  It's
about whether the code is clean.  I'd _love_ to get rid of inline asm
in the future when GCC supports intrinsics since that could lead to
significantly improved code.

(And no, I don't expect Chris to necessarily fix up the ICC bits, though
 a reasonable best-effort wouldn't take much time.)

	--david