| Message ID | 1444660220-25559-1-git-send-email-boqun.feng@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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@vger.kernel.org> # 3.4.y- > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > --- > arch/powerpc/include/asm/cmpxchg.h | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) Hi Boqun, 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 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 put it in next to let it get some wider testing rather than sending it straight to Linus. 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. Also I'm not clear what your stable line means: > Cc: <stable@vger.kernel.org> # 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. cheers
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@vger.kernel.org> # 3.4.y- > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > > --- > > 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@vger.kernel.org> # 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@vger.kernel.org> # 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
On Wed, Oct 14, 2015 at 08:51:34AM +0800, Boqun Feng wrote: > On Wed, Oct 14, 2015 at 11:10:00AM +1100, Michael Ellerman wrote: > > 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? I don't think I had explicit thoughts about any of that, just that it might make sense to have this patch not depend on the rest such that it could indeed be stuffed into stable. I'll leave the details up to Michael since he's PPC maintainer.
On Wed, Oct 14, 2015 at 10:06:13AM +0200, Peter Zijlstra wrote: > On Wed, Oct 14, 2015 at 08:51:34AM +0800, Boqun Feng wrote: > > On Wed, Oct 14, 2015 at 11:10:00AM +1100, Michael Ellerman wrote: > > > > 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? > > I don't think I had explicit thoughts about any of that, just that it > might make sense to have this patch not depend on the rest such that it > could indeed be stuffed into stable. > Got that. Sorry for misunderstanding you... > I'll leave the details up to Michael since he's PPC maintainer. Michael and Peter, rest of this patchset depends on commits which are currently in the locking/core branch of the tip, so I would like it as a whole queued there. Besides, I will keep this patch Cc'ed to stable in future versions, that works for you both? Regards, Boqun
On Wed, Oct 14, 2015 at 05:26:53PM +0800, Boqun Feng wrote: > Michael and Peter, rest of this patchset depends on commits which are > currently in the locking/core branch of the tip, so I would like it as a > whole queued there. Besides, I will keep this patch Cc'ed to stable in > future versions, that works for you both? From my POV having the Cc stable in there is fine if Michael actually wants them to go there. GregKH will vacuum them up once they hit Linus' tree and we don't need to think about it anymore. Alternatively, Michael could put the patch in a separate branch and we could both merge that. Or even, seeing how its a single patch and git mostly does the right thing, we could just merge it independently in both trees and let git sort it out at merge time.
On Wed, 2015-10-14 at 11:33 +0200, Peter Zijlstra wrote: > On Wed, Oct 14, 2015 at 05:26:53PM +0800, Boqun Feng wrote: > > Michael and Peter, rest of this patchset depends on commits which are > > currently in the locking/core branch of the tip, so I would like it as a > > whole queued there. Besides, I will keep this patch Cc'ed to stable in > > future versions, that works for you both? > > From my POV having the Cc stable in there is fine if Michael actually > wants them to go there. GregKH will vacuum them up once they hit Linus' > tree and we don't need to think about it anymore. Yeah that's fine by me. Here's an Ack if you want one: Acked-by: Michael Ellerman <mpe@ellerman.id.au> > Alternatively, Michael could put the patch in a separate branch and we > could both merge that. > > Or even, seeing how its a single patch and git mostly does the right > thing, we could just merge it independently in both trees and let git > sort it out at merge time. That probably would work, but I don't think it's necessary. My tree doesn't get much (or any) more testing than linux-next, so as long as locking/core is in linux-next then it will be tested just fine that way. cheers
diff --git a/arch/powerpc/include/asm/cmpxchg.h b/arch/powerpc/include/asm/cmpxchg.h index ad6263c..d1a8d93 100644 --- a/arch/powerpc/include/asm/cmpxchg.h +++ b/arch/powerpc/include/asm/cmpxchg.h @@ -18,12 +18,12 @@ __xchg_u32(volatile void *p, unsigned long val) unsigned long prev; __asm__ __volatile__( - PPC_RELEASE_BARRIER + PPC_ATOMIC_ENTRY_BARRIER "1: lwarx %0,0,%2 \n" PPC405_ERR77(0,%2) " stwcx. %3,0,%2 \n\ bne- 1b" - PPC_ACQUIRE_BARRIER + PPC_ATOMIC_EXIT_BARRIER : "=&r" (prev), "+m" (*(volatile unsigned int *)p) : "r" (p), "r" (val) : "cc", "memory"); @@ -61,12 +61,12 @@ __xchg_u64(volatile void *p, unsigned long val) unsigned long prev; __asm__ __volatile__( - PPC_RELEASE_BARRIER + PPC_ATOMIC_ENTRY_BARRIER "1: ldarx %0,0,%2 \n" PPC405_ERR77(0,%2) " stdcx. %3,0,%2 \n\ bne- 1b" - PPC_ACQUIRE_BARRIER + PPC_ATOMIC_EXIT_BARRIER : "=&r" (prev), "+m" (*(volatile unsigned long *)p) : "r" (p), "r" (val) : "cc", "memory"); @@ -151,14 +151,14 @@ __cmpxchg_u32(volatile unsigned int *p, unsigned long old, unsigned long new) unsigned int prev; __asm__ __volatile__ ( - PPC_RELEASE_BARRIER + PPC_ATOMIC_ENTRY_BARRIER "1: lwarx %0,0,%2 # __cmpxchg_u32\n\ cmpw 0,%0,%3\n\ bne- 2f\n" PPC405_ERR77(0,%2) " stwcx. %4,0,%2\n\ bne- 1b" - PPC_ACQUIRE_BARRIER + PPC_ATOMIC_EXIT_BARRIER "\n\ 2:" : "=&r" (prev), "+m" (*p) @@ -197,13 +197,13 @@ __cmpxchg_u64(volatile unsigned long *p, unsigned long old, unsigned long new) unsigned long prev; __asm__ __volatile__ ( - PPC_RELEASE_BARRIER + PPC_ATOMIC_ENTRY_BARRIER "1: ldarx %0,0,%2 # __cmpxchg_u64\n\ cmpd 0,%0,%3\n\ bne- 2f\n\ stdcx. %4,0,%2\n\ bne- 1b" - PPC_ACQUIRE_BARRIER + PPC_ATOMIC_EXIT_BARRIER "\n\ 2:" : "=&r" (prev), "+m" (*p)
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@vger.kernel.org> # 3.4.y- Signed-off-by: Boqun Feng <boqun.feng@gmail.com> --- arch/powerpc/include/asm/cmpxchg.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)