Re: [patch 1/2] read_barrier_depends fixlets

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Mon, May 05, 2008 at 01:20:21PM +0200, Nick Piggin wrote:
> While considering the impact of read_barrier_depends, it occurred to
> me that it should really be really a noop for the compiler. At least, it is
> better to have every arch the same than to have a few that are slightly
> different. (Does this mean SMP Alpha's read_barrier_depends could drop the
> "memory" clobber too?)

SMP Alpha's read_barrier_depends() needs the "memory" clobber
because the compiler is otherwise free to move code across the
smp_read_barrier_depends(), which would defeat its purpose.

> --
> It would be a highly unusual compiler that might try to issue a load of
> data1 before it loads a data2 which is data-dependant on data1.

A bit unusual, perhaps, but not unprecedented.  Value speculating
compilers, for example.

> There is the problem of the compiler trying to reload data1 _after_
> loading data2, and thus having a newer data1 than data2. However if the
> compiler is so inclined, then it could perform such a load at any point
> after the barrier, so the barrier itself will not guarantee correctness.
> 
> I think we've mostly hoped the compiler would not to do that.

Well, this does point me at one thing I missed with preemptable RCU,
namely all the open-coded sequences using smp_read_barrier_depends().
Quite embarrassing!!!  But a lot easier having you point me at it than
however long it would have taken me to figure it out on my own, so thank
you very much!!!

> This brings alpha and frv into line with all other architectures.

Assuming that we apply ACCESS_ONCE() as needed to the uses of
smp_read_barrier_depends():

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> Signed-off-by: Nick Piggin <npiggin@suse.de>
> 
> Index: linux-2.6/include/asm-alpha/barrier.h
> ===================================================================
> --- linux-2.6.orig/include/asm-alpha/barrier.h
> +++ linux-2.6/include/asm-alpha/barrier.h
> @@ -24,7 +24,7 @@ __asm__ __volatile__("mb": : :"memory")
>  #define smp_mb()	barrier()
>  #define smp_rmb()	barrier()
>  #define smp_wmb()	barrier()
> -#define smp_read_barrier_depends()	barrier()
> +#define smp_read_barrier_depends()	do { } while (0)
>  #endif
> 
>  #define set_mb(var, value) \
> Index: linux-2.6/include/asm-frv/system.h
> ===================================================================
> --- linux-2.6.orig/include/asm-frv/system.h
> +++ linux-2.6/include/asm-frv/system.h
> @@ -179,7 +179,7 @@ do {							\
>  #define mb()			asm volatile ("membar" : : :"memory")
>  #define rmb()			asm volatile ("membar" : : :"memory")
>  #define wmb()			asm volatile ("membar" : : :"memory")
> -#define read_barrier_depends()	barrier()
> +#define read_barrier_depends()	do { } while (0)
> 
>  #ifdef CONFIG_SMP
>  #define smp_mb()			mb()