Re: [patch 1/2] read_barrier_depends fixlets

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

On Tue, May 06, 2008 at 11:01:56AM +0200, Nick Piggin wrote:
> On Mon, May 05, 2008 at 07:27:46AM -0700, Paul E. McKenney wrote:
> > 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.
> 
> Oh that's what does it. I was thinking of volatile, but I guess that is
> to prevent the statement from being eliminated.

Yep!!!

> > > 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.
> 
> Yes very true. Actually I guess it may even not be far off if we ever
> used gcc's builtin_expect for predicting data rather than control values.
> OTOH, would it help significantly over simply prefetching and then having
> the compiler issue the (non speculative) loads in the correct order? You
> would avoid speculation and fixup code in the generated code that way.

You would still need to control the ordering in the case of failed
speculation -- so there would need to be some additional built-ins to
handle this.  Might take some time...

> > > 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!!!
> 
> Heh, glad to be of help ;)

And the first couple I have looked at seem to need some help...

> > > 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():
> 
> Hmm, more on this in the next mail... (but I think it is important to
> bring other archs into line with the common case, even if the common
> case may have some issues that need sorting out).

I am not hung up on the order that the patches happen, as long as they
all happen.  ;-)

						Thanx, Paul

> > 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()

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>