On Wed, Feb 21, 2018 at 11:21:38AM +0000, Will Deacon wrote: > Hi Andrea, > > On Tue, Feb 20, 2018 at 07:45:56PM +0100, Andrea Parri wrote: > > Continuing along with the fight against smp_read_barrier_depends() [1] > > (or rather, against its improper use), add an unconditional barrier to > > cmpxchg. This guarantees that dependency ordering is preserved when a > > dependency is headed by an unsuccessful cmpxchg. As it turns out, the > > change could enable further simplification of LKMM as proposed in [2]. > > > > [1] https://marc.info/?l=linux-kernel&m=150884953419377&w=2 > > https://marc.info/?l=linux-kernel&m=150884946319353&w=2 > > https://marc.info/?l=linux-kernel&m=151215810824468&w=2 > > https://marc.info/?l=linux-kernel&m=151215816324484&w=2 > > > > [2] https://marc.info/?l=linux-kernel&m=151881978314872&w=2 > > > > Signed-off-by: Andrea Parri <parri.andrea@xxxxxxxxx> > > Acked-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx> > > Cc: Will Deacon <will.deacon@xxxxxxx> > > Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx> > > Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> > > Cc: Richard Henderson <rth@xxxxxxxxxxx> > > Cc: Ivan Kokshaysky <ink@xxxxxxxxxxxxxxxxxxxx> > > Cc: Matt Turner <mattst88@xxxxxxxxx> > > Cc: linux-alpha@xxxxxxxxxxxxxxx > > Cc: linux-kernel@xxxxxxxxxxxxxxx > > --- > > arch/alpha/include/asm/xchg.h | 15 +++++++-------- > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/arch/alpha/include/asm/xchg.h b/arch/alpha/include/asm/xchg.h > > index 68dfb3cb71454..e2660866ce972 100644 > > --- a/arch/alpha/include/asm/xchg.h > > +++ b/arch/alpha/include/asm/xchg.h > > @@ -128,10 +128,9 @@ ____xchg(, volatile void *ptr, unsigned long x, int size) > > * store NEW in MEM. Return the initial value in MEM. Success is > > * indicated by comparing RETURN with OLD. > > * > > - * The memory barrier should be placed in SMP only when we actually > > - * make the change. If we don't change anything (so if the returned > > - * prev is equal to old) then we aren't acquiring anything new and > > - * we don't need any memory barrier as far I can tell. > > + * The memory barrier is placed in SMP unconditionally, in order to > > + * guarantee that dependency ordering is preserved when a dependency > > + * is headed by an unsuccessful operation. > > */ > > > > static inline unsigned long > > @@ -150,8 +149,8 @@ ____cmpxchg(_u8, volatile char *m, unsigned char old, unsigned char new) > > " or %1,%2,%2\n" > > " stq_c %2,0(%4)\n" > > " beq %2,3f\n" > > - __ASM__MB > > "2:\n" > > + __ASM__MB > > ".subsection 2\n" > > "3: br 1b\n" > > ".previous" > > It might be better just to add smp_read_barrier_depends() into the cmpxchg > macro, then remove all of the __ASM__MB stuff. Mmh, it might be better to add smp_mb() into the cmpxchg macro (after the operation), then remove all the __ASM__MB stuff. > > That said, I don't actually understand how the Alpha cmpxchg or xchg > implementations satisfy the memory model, since they only appear to have > a barrier after the operation. > > So MP using xchg: > > WRITE_ONCE(x, 1) > xchg(y, 1) > > smp_load_acquire(y) == 1 > READ_ONCE(x) == 0 > > would be allowed. What am I missing? Good question ;-) The absence of an smp_mb() (or of an __ASM__MB) before the operation did upset me. If this question remains pending, I'll send a patch to add these barriers. > > Since I'm in the mood for dumb questions, do we need to care about > this_cpu_cmpxchg? I'm sure I've seen code that allows concurrent access to > per-cpu variables, but the asm-generic implementation of this_cpu_cmpxchg > doesn't use READ_ONCE. Frankly, I'm not sure if this's an issue in the generic implementation of this_cpu_* or, rather, in that code. let me dig a bit more into this ... Andrea > > Will -- To unsubscribe from this list: send the line "unsubscribe linux-alpha" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
