Re: [PATCH v2 15/32] powerpc: define __smp_xxx
On Tue, Jan 05, 2016 at 10:51:17AM +0200, Michael S. Tsirkin wrote: > On Tue, Jan 05, 2016 at 09:36:55AM +0800, Boqun Feng wrote: > > Hi Michael, > > > > On Thu, Dec 31, 2015 at 09:07:42PM +0200, Michael S. Tsirkin wrote: > > > This defines __smp_xxx barriers for powerpc > > > for use by virtualization. > > > > > > smp_xxx barriers are removed as they are > > > defined correctly by asm-generic/barriers.h > > I think this is the part that was missed in review. > Yes, I realized my mistake after reread the series. But smp_lwsync() is not defined in asm-generic/barriers.h, right? > > > This reduces the amount of arch-specific boiler-plate code. > > > > > > Signed-off-by: Michael S. Tsirkin> > > Acked-by: Arnd Bergmann > > > --- > > > arch/powerpc/include/asm/barrier.h | 24 > > > 1 file changed, 8 insertions(+), 16 deletions(-) > > > > > > diff --git a/arch/powerpc/include/asm/barrier.h > > > b/arch/powerpc/include/asm/barrier.h > > > index 980ad0c..c0deafc 100644 > > > --- a/arch/powerpc/include/asm/barrier.h > > > +++ b/arch/powerpc/include/asm/barrier.h > > > @@ -44,19 +44,11 @@ > > > #define dma_rmb()__lwsync() > > > #define dma_wmb()__asm__ __volatile__ (stringify_in_c(SMPWMB) : > > > : :"memory") > > > > > > -#ifdef CONFIG_SMP > > > -#define smp_lwsync() __lwsync() > > > +#define __smp_lwsync() __lwsync() > > > > > > > so __smp_lwsync() is always mapped to lwsync, right? > > Yes. > > > > -#define smp_mb() mb() > > > -#define smp_rmb()__lwsync() > > > -#define smp_wmb()__asm__ __volatile__ (stringify_in_c(SMPWMB) : > > > : :"memory") > > > -#else > > > -#define smp_lwsync() barrier() > > > - > > > -#define smp_mb() barrier() > > > -#define smp_rmb()barrier() > > > -#define smp_wmb()barrier() > > > -#endif /* CONFIG_SMP */ > > > +#define __smp_mb() mb() > > > +#define __smp_rmb() __lwsync() > > > +#define __smp_wmb() __asm__ __volatile__ (stringify_in_c(SMPWMB) : > > > : :"memory") > > > > > > /* > > > * This is a barrier which prevents following instructions from being > > > @@ -67,18 +59,18 @@ > > > #define data_barrier(x) \ > > > asm volatile("twi 0,%0,0; isync" : : "r" (x) : "memory"); > > > > > > -#define smp_store_release(p, v) > > > \ > > > +#define __smp_store_release(p, v) > > > \ > > > do { > > > \ > > > compiletime_assert_atomic_type(*p); \ > > > - smp_lwsync(); \ > > > + __smp_lwsync(); \ > > > > , therefore this will emit an lwsync no matter SMP or UP. > > Absolutely. But smp_store_release (without __) will not. > > Please note I did test this: for ppc code before and after > this patch generates exactly the same binary on SMP and UP. > Yes, you're right, sorry for my mistake... > > > Another thing is that smp_lwsync() may have a third user(other than > > smp_load_acquire() and smp_store_release()): > > > > http://article.gmane.org/gmane.linux.ports.ppc.embedded/89877 > > > > I'm OK to change my patch accordingly, but do we really want > > smp_lwsync() get involved in this cleanup? If I understand you > > correctly, this cleanup focuses on external API like smp_{r,w,}mb(), > > while smp_lwsync() is internal to PPC. > > > > Regards, > > Boqun > > I think you missed the leading ___ :) > What I mean here was smp_lwsync() was originally internal to PPC, but never mind ;-) > smp_store_release is external and it needs __smp_lwsync as > defined here. > > I can duplicate some code and have smp_lwsync *not* call __smp_lwsync You mean bringing smp_lwsync() back? because I haven't seen you defining in asm-generic/barriers.h in previous patches and you just delete it in this patch. > but why do this? Still, if you prefer it this way, > please let me know. > I think deleting smp_lwsync() is fine, though I need to change atomic variants patches on PPC because of it ;-/ Regards, Boqun > > > WRITE_ONCE(*p, v); \ > > > } while (0) > > > > > > -#define smp_load_acquire(p) > > > \ > > > +#define __smp_load_acquire(p) > > > \ > > > ({ > > > \ > > > typeof(*p) ___p1 = READ_ONCE(*p); \ > > > compiletime_assert_atomic_type(*p); \ > > > - smp_lwsync(); \ > > > + __smp_lwsync(); \ > > > ___p1; \ > > > }) > > > > > > -- > >
Re: [PATCH v2 22/32] s390: define __smp_xxx
On Tue, Jan 05, 2016 at 04:39:37PM +0100, Christian Borntraeger wrote: > On 01/05/2016 10:30 AM, Michael S. Tsirkin wrote: > > > > > arch/s390/kernel/vdso.c:smp_mb(); > > > > Looking at > > Author: Christian Borntraeger> > Date: Fri Sep 11 16:23:06 2015 +0200 > > > > s390/vdso: use correct memory barrier > > > > By definition smp_wmb only orders writes against writes. (Finish all > > previous writes, and do not start any future write). To protect the > > vdso init code against early reads on other CPUs, let's use a full > > smp_mb at the end of vdso init. As right now smp_wmb is implemented > > as full serialization, this needs no stable backport, but this > > change > > will be necessary if we reimplement smp_wmb. > > > > ok from hypervisor point of view, but it's also strange: > > 1. why isn't this paired with another mb somewhere? > >this seems to violate barrier pairing rules. > > 2. how does smp_mb protect against early reads on other CPUs? > >It normally does not: it orders reads from this CPU versus writes > >from same CPU. But init code does not appear to read anything. > >Maybe this is some s390 specific trick? > > > > I could not figure out the above commit. > > It was probably me misreading the code. I change a wmb into a full mb here > since I was changing the defintion of wmb to a compiler barrier. I tried to > fixup all users of wmb that really pair with other code. I assumed that there > must be some reader (as there was a wmb before) but I could not figure out > which. So I just played safe here. > > But it probably can be removed. > > > arch/s390/kvm/kvm-s390.c: smp_mb(); > > This can go. If you have a patch, I can carry that via the kvms390 tree, > or I will spin a new patch with you as suggested-by. > > Christian I have both, will post shortly. -- MST -- To unsubscribe from this list: send the line "unsubscribe linux-metag" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 12/32] x86/um: reuse asm-generic/barrier.h
Am 31.12.2015 um 20:07 schrieb Michael S. Tsirkin: > On x86/um CONFIG_SMP is never defined. As a result, several macros > match the asm-generic variant exactly. Drop the local definitions and > pull in asm-generic/barrier.h instead. > > This is in preparation to refactoring this code area. > > Signed-off-by: Michael S. Tsirkin> Acked-by: Arnd Bergmann Acked-by: Richard Weinberger Thanks, //richard -- To unsubscribe from this list: send the line "unsubscribe linux-metag" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 22/32] s390: define __smp_xxx
On Tue, 5 Jan 2016 11:30:19 +0200 "Michael S. Tsirkin"wrote: > On Tue, Jan 05, 2016 at 09:13:19AM +0100, Martin Schwidefsky wrote: > > On Mon, 4 Jan 2016 22:18:58 +0200 > > "Michael S. Tsirkin" wrote: > > > > > On Mon, Jan 04, 2016 at 02:45:25PM +0100, Peter Zijlstra wrote: > > > > On Thu, Dec 31, 2015 at 09:08:38PM +0200, Michael S. Tsirkin wrote: > > > > > This defines __smp_xxx barriers for s390, > > > > > for use by virtualization. > > > > > > > > > > Some smp_xxx barriers are removed as they are > > > > > defined correctly by asm-generic/barriers.h > > > > > > > > > > Note: smp_mb, smp_rmb and smp_wmb are defined as full barriers > > > > > unconditionally on this architecture. > > > > > > > > > > Signed-off-by: Michael S. Tsirkin > > > > > Acked-by: Arnd Bergmann > > > > > --- > > > > > arch/s390/include/asm/barrier.h | 15 +-- > > > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/arch/s390/include/asm/barrier.h > > > > > b/arch/s390/include/asm/barrier.h > > > > > index c358c31..fbd25b2 100644 > > > > > --- a/arch/s390/include/asm/barrier.h > > > > > +++ b/arch/s390/include/asm/barrier.h > > > > > @@ -26,18 +26,21 @@ > > > > > #define wmb()barrier() > > > > > #define dma_rmb()mb() > > > > > #define dma_wmb()mb() > > > > > -#define smp_mb() mb() > > > > > -#define smp_rmb()rmb() > > > > > -#define smp_wmb()wmb() > > > > > - > > > > > -#define smp_store_release(p, v) > > > > > \ > > > > > +#define __smp_mb() mb() > > > > > +#define __smp_rmb() rmb() > > > > > +#define __smp_wmb() wmb() > > > > > +#define smp_mb() __smp_mb() > > > > > +#define smp_rmb()__smp_rmb() > > > > > +#define smp_wmb()__smp_wmb() > > > > > > > > Why define the smp_*mb() primitives here? Would not the inclusion of > > > > asm-generic/barrier.h do this? > > > > > > No because the generic one is a nop on !SMP, this one isn't. > > > > > > Pls note this patch is just reordering code without making > > > functional changes. > > > And at the moment, on s390 smp_xxx barriers are always non empty. > > > > The s390 kernel is SMP to 99.99%, we just didn't bother with a > > non-smp variant for the memory-barriers. If the generic header > > is used we'd get the non-smp version for free. It will save a > > small amount of text space for CONFIG_SMP=n. > > OK, so I'll queue a patch to do this then? Yes please. > Just to make sure: the question would be, are smp_xxx barriers ever used > in s390 arch specific code to flush in/out memory accesses for > synchronization with the hypervisor? > > I went over s390 arch code and it seems to me the answer is no > (except of course for virtio). Correct. Guest to host communication either uses instructions which imply a memory barrier or QDIO which uses atomics. > But I also see a lot of weirdness on this architecture. Mostly historical, s390 actually is one of the easiest architectures in regard to memory barriers. > I found these calls: > > arch/s390/include/asm/bitops.h: smp_mb__before_atomic(); > arch/s390/include/asm/bitops.h: smp_mb(); > > Not used in arch specific code so this is likely OK. This has been introduced with git commit 5402ea6af11dc5a9, the smp_mb and smp_mb__before_atomic are used in clear_bit_unlock and __clear_bit_unlock which are 1:1 copies from the code in include/asm-generic/bitops/lock.h. Only test_and_set_bit_lock differs from the generic implementation. > arch/s390/kernel/vdso.c:smp_mb(); > > Looking at > Author: Christian Borntraeger > Date: Fri Sep 11 16:23:06 2015 +0200 > > s390/vdso: use correct memory barrier > > By definition smp_wmb only orders writes against writes. (Finish all > previous writes, and do not start any future write). To protect the > vdso init code against early reads on other CPUs, let's use a full > smp_mb at the end of vdso init. As right now smp_wmb is implemented > as full serialization, this needs no stable backport, but this > change > will be necessary if we reimplement smp_wmb. > > ok from hypervisor point of view, but it's also strange: > 1. why isn't this paired with another mb somewhere? >this seems to violate barrier pairing rules. > 2. how does smp_mb protect against early reads on other CPUs? >It normally does not: it orders reads from this CPU versus writes >from same CPU. But init code does not appear to read anything. >Maybe this is some s390 specific trick? > > I could not figure out the above commit. That smp_mb can be removed. The initial s390 vdso code is heavily
Re: [PATCH v2 15/32] powerpc: define __smp_xxx
On Tue, Jan 05, 2016 at 06:16:48PM +0200, Michael S. Tsirkin wrote: [snip] > > > > Another thing is that smp_lwsync() may have a third user(other than > > > > smp_load_acquire() and smp_store_release()): > > > > > > > > http://article.gmane.org/gmane.linux.ports.ppc.embedded/89877 > > > > > > > > I'm OK to change my patch accordingly, but do we really want > > > > smp_lwsync() get involved in this cleanup? If I understand you > > > > correctly, this cleanup focuses on external API like smp_{r,w,}mb(), > > > > while smp_lwsync() is internal to PPC. > > > > > > > > Regards, > > > > Boqun > > > > > > I think you missed the leading ___ :) > > > > > > > What I mean here was smp_lwsync() was originally internal to PPC, but > > never mind ;-) > > > > > smp_store_release is external and it needs __smp_lwsync as > > > defined here. > > > > > > I can duplicate some code and have smp_lwsync *not* call __smp_lwsync > > > > You mean bringing smp_lwsync() back? because I haven't seen you defining > > in asm-generic/barriers.h in previous patches and you just delete it in > > this patch. > > > > > but why do this? Still, if you prefer it this way, > > > please let me know. > > > > > > > I think deleting smp_lwsync() is fine, though I need to change atomic > > variants patches on PPC because of it ;-/ > > > > Regards, > > Boqun > > Sorry, I don't understand - why do you have to do anything? > I changed all users of smp_lwsync so they > use __smp_lwsync on SMP and barrier() on !SMP. > > This is exactly the current behaviour, I also tested that > generated code does not change at all. > > Is there a patch in your tree that conflicts with this? > Because in a patchset which implements atomic relaxed/acquire/release variants on PPC I use smp_lwsync(), this makes it have another user, please see this mail: http://article.gmane.org/gmane.linux.ports.ppc.embedded/89877 in definition of PPC's __atomic_op_release(). But I think removing smp_lwsync() is a good idea and actually I think we can go further to remove __smp_lwsync() and let __smp_load_acquire and __smp_store_release call __lwsync() directly, but that is another thing. Anyway, I will modify my patch. Regards, Boqun > > > > > > WRITE_ONCE(*p, v); > > > > > \ > > > > > } while (0) > > > > > > > > > > -#define smp_load_acquire(p) > > > > > \ > > > > > +#define __smp_load_acquire(p) > > > > > \ > > > > > ({ > > > > > \ > > > > > typeof(*p) ___p1 = READ_ONCE(*p); > > > > > \ > > > > > compiletime_assert_atomic_type(*p); > > > > > \ > > > > > - smp_lwsync(); > > > > > \ > > > > > + __smp_lwsync(); > > > > > \ > > > > > ___p1; > > > > > \ > > > > > }) > > > > > > > > > > -- > > > > > MST > > > > > > > > > > -- > > > > > To unsubscribe from this list: send the line "unsubscribe > > > > > linux-kernel" in > > > > > the body of a message to majord...@vger.kernel.org > > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-metag" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html