Re: [PATCH v8 4/4] qrwlock: Use smp_store_release() in write_unlock()

[Daniel J Blueman]

On 14/01/2014 00:41, Waiman Long wrote:
> On 01/12/2014 09:47 PM, Daniel J Blueman wrote:
> > On Thursday, 9 January 2014 01:10:03 UTC+8, Waiman Long  wrote:
> > > This patch modifies the queue_write_unlock() function to use the
> > > new smp_store_release() function in another pending patch. It also
> > > removes the temporary implementation of smp_load_acquire() and
> > > smp_store_release() function in qrwlock.c.
> > >
> > > This patch should only be merged if PeterZ's linux-arch patch patch
> > > was merged.
> > >
> > > Signed-off-by: Waiman Long <Waiman.Long@xxxxxx>
> > > Reviewed-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >  include/asm-generic/qrwlock.h |    4 +---
> > >  kernel/locking/qrwlock.c      |   34
> > ----------------------------------
> > >  2 files changed, 1 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/include/asm-generic/qrwlock.h
> > b/include/asm-generic/qrwlock.h
> > > index 2b9a7b4..4d4bd04 100644
> > > --- a/include/asm-generic/qrwlock.h
> > > +++ b/include/asm-generic/qrwlock.h
> > > @@ -179,9 +179,7 @@ static inline void queue_write_unlock(struct
> > qrwlock *lock)
> > >      /*
> > >       * Make sure that none of the critical section will be leaked out.
> > >       */
> > > -    smp_mb__before_clear_bit();
> > > -    ACCESS_ONCE(lock->cnts.writer) = 0;
> > > -    smp_mb__after_clear_bit();
> > > +    smp_store_release(&lock->cnts.writer, 0)
> >
> > This will fail compilation, so probably needs further testing with
> > Peter's load_acquire/store_release barrier patches.
>
> Peter,
>
> I found out that the build failure was caused by the fact that the
> __native_word() macro (used internally by compiletime_assert_atomic())
> allows only a size of 4 or 8 for x86-64. The data type that I used is a
> byte. Is there a reason why byte and short are not considered native?

It seems likely it was implemented like that since there was no existing 
need; long can be relied on as the largest native type, so this should 
suffice and works here:

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index fe7a686..dac91d7 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -299,8 +299,8 @@ void ftrace_likely_update(struct ftrace_branch_data 
*f, int val, int expect);
 #endif

 /* Is this type a native word size -- useful for atomic operations */
-#ifndef __native_word
-# define __native_word(t) (sizeof(t) == sizeof(int) || sizeof(t) == 
sizeof(long))
+#ifndef __native_type
+# define __native_type(t) (sizeof(t) <= sizeof(long))
 #endif

 /* Compile time object size, -1 for unknown */
@@ -343,8 +343,8 @@ void ftrace_likely_update(struct ftrace_branch_data 
*f, int val, int expect);
 	_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)

 #define compiletime_assert_atomic_type(t)				\
-	compiletime_assert(__native_word(t),				\
-		"Need native word sized stores/loads for atomicity.")
+	compiletime_assert(__native_type(t),				\
+		"Need native sized stores/loads for atomicity.")

 /*
  * Prevent the compiler from merging or refetching accesses.  The compiler

Signed-off-by: Daniel J Blueman <daniel@xxxxxxxxxxxxx>
--
Daniel J Blueman
Principal Software Engineer, Numascale
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/