Re: [PATCH RESEND v3 1/6] powerpc: atomic: Make *xchg and *cmpxchg a full barrier

[Boqun Feng]

On Wed, Oct 14, 2015 at 11:10:00AM +1100, Michael Ellerman wrote:
> On Mon, 2015-10-12 at 22:30 +0800, Boqun Feng wrote:
> > According to memory-barriers.txt, xchg, cmpxchg and their atomic{,64}_
> > versions all need to imply a full barrier, however they are now just
> > RELEASE+ACQUIRE, which is not a full barrier.
> > 
> > So replace PPC_RELEASE_BARRIER and PPC_ACQUIRE_BARRIER with
> > PPC_ATOMIC_ENTRY_BARRIER and PPC_ATOMIC_EXIT_BARRIER in
> > __{cmp,}xchg_{u32,u64} respectively to guarantee a full barrier
> > semantics of atomic{,64}_{cmp,}xchg() and {cmp,}xchg().
> > 
> > This patch is a complement of commit b97021f85517 ("powerpc: Fix
> > atomic_xxx_return barrier semantics").
> > 
> > Cc: <stable@xxxxxxxxxxxxxxx> # 3.4.y-
> > Signed-off-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> > ---
> >  arch/powerpc/include/asm/cmpxchg.h | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> Hi Boqun,
> 

Hello, Michael

> Thanks for fixing this. In future you should send a patch like this as a
> separate patch. I've not been paying attention to it because I assumed it was

Got it. However, here is the thing, in previous version, this fix
depends on some of other patches in this patchset. So to make this fix
applied cleanly, I reorder my patchset to put this patch first, and the
result is that some of other patches in this patchset depends on
this(they need to remove code modified by this patch).

So I guess I'd better to stop Cc stable for this one, and wait until
this patchset merged and send a separate patch for -stable tree. Does
that work for you? I think this is what Peter want to suggests me to do
when he asked me about this, right, Peter?

> part of your full series and was still under discussion like the other patches.
> 
> I don't think we've seen any crashes caused by this have we? So I guess I'll

No, we haven't seen any.

> put it in next to let it get some wider testing rather than sending it straight
> to Linus.
> 

Good idea, thank you ;-)

> To be clear you're doing:
> 
> > -	PPC_RELEASE_BARRIER
> > +	PPC_ATOMIC_ENTRY_BARRIER
> 
> Which is correct but doesn't actually change anything at the moment, because
> both macros turn into LWSYNC.
> 
> On the other hand:
> 
> > -	PPC_ACQUIRE_BARRIER
> > +	PPC_ATOMIC_EXIT_BARRIER
> 
> Is changing an isync (which is then patched to lwsync on some cpus), with a sync.
> 

These macros are introduced by commit b97021f85517 ("powerpc: Fix
atomic_xxx_return barrier semantics") to fix a similar problem, so I use
them to keep code similar.

> 
> Also I'm not clear what your stable line means:
> 
> > Cc: <stable@xxxxxxxxxxxxxxx> # 3.4.y-
> 
> Do you mean 3.4 and anything after? I usually write that as 3.4+, but I'm not
> sure if that's the correct syntax either.
> 

Quote from Documentation/stable_kernel_rules.txt:

"""
Also, some patches may have kernel version prerequisites.  This can be
specified in the following format in the sign-off area:

     Cc:  <stable@xxxxxxxxxxxxxxx> # 3.3.x-

   The tag has the meaning of:
     git cherry-pick <this commit>

   For each "-stable" tree starting with the specified version.
"""

But yes, I have seen several people use like "3.4+", I'm not sure either

Regards,
Boqun

Attachment: signature.asc
Description: PGP signature