Re: asm-generic: Disallow no-op mb() for SMP systems
On Sat, Feb 03, 2018 at 03:14:01AM +0800, kbuild test robot wrote: > Hi Peter, > > I love your patch! Yet something to improve: > > [auto build test ERROR on asm-generic/master] > [also build test ERROR on v4.15 next-20180202] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Peter-Zijlstra/asm-generic-Disallow-no-op-mb-for-SMP-systems/20180203-000108 > base: https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git > master > config: mn10300-asb2364_defconfig (attached as .config) > compiler: am33_2.0-linux-gcc (GCC) 7.2.0 David, this one is yours :-) Does that mn10300 thing really have sequentially consistent SMP (no store buffers or anything?)
Re: asm-generic: Disallow no-op mb() for SMP systems
On Sat, Feb 03, 2018 at 03:14:01AM +0800, kbuild test robot wrote: > Hi Peter, > > I love your patch! Yet something to improve: > > [auto build test ERROR on asm-generic/master] > [also build test ERROR on v4.15 next-20180202] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Peter-Zijlstra/asm-generic-Disallow-no-op-mb-for-SMP-systems/20180203-000108 > base: https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git > master > config: mn10300-asb2364_defconfig (attached as .config) > compiler: am33_2.0-linux-gcc (GCC) 7.2.0 David, this one is yours :-) Does that mn10300 thing really have sequentially consistent SMP (no store buffers or anything?)
Re: asm-generic: Disallow no-op mb() for SMP systems
On Fri, Feb 02, 2018 at 09:08:20PM +, Alan Cox wrote: > On Fri, 2 Feb 2018 21:25:49 +0100 > Peter Zijlstrawrote: > > > On Sat, Feb 03, 2018 at 04:00:35AM +0800, kbuild test robot wrote: > > > Hi Peter, > > > > > > I love your patch! Yet something to improve: > > > > Seriously? Bots have feelings? > > > > > [auto build test ERROR on asm-generic/master] > > > [also build test ERROR on v4.15 next-20180202] > > > [if your patch is applied to the wrong git tree, please drop us a note to > > > help improve the system] > > > > > > url: > > > https://github.com/0day-ci/linux/commits/Peter-Zijlstra/asm-generic-Disallow-no-op-mb-for-SMP-systems/20180203-000108 > > > base: > > > https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git > > > master > > > config: m32r-usrv_defconfig (attached as .config) > > > compiler: m32r-linux-gcc (GCC) 7.2.0 > > > > Awesome, another broken architecture.. and this one is Orphaned :-( > > No maintainer to bug.. > > Renesas claim to still support the processor family so perhaps they can > provide a maintainer if you instead submit a patch to remove it from the > tree ;-) There's an idea :-) > From the documentation however the processor only does explicit dual > issue instructions, and the SMP is cache coherent so I'm not convinced > there is a problem with it. Thanks for looking at that, got a link handy to said documentation? Does it really not have a store-buffer? Getting such details sorted was exactly the purpose of the patch, so in that regards its a success :-)
Re: asm-generic: Disallow no-op mb() for SMP systems
On Fri, Feb 02, 2018 at 09:08:20PM +, Alan Cox wrote: > On Fri, 2 Feb 2018 21:25:49 +0100 > Peter Zijlstra wrote: > > > On Sat, Feb 03, 2018 at 04:00:35AM +0800, kbuild test robot wrote: > > > Hi Peter, > > > > > > I love your patch! Yet something to improve: > > > > Seriously? Bots have feelings? > > > > > [auto build test ERROR on asm-generic/master] > > > [also build test ERROR on v4.15 next-20180202] > > > [if your patch is applied to the wrong git tree, please drop us a note to > > > help improve the system] > > > > > > url: > > > https://github.com/0day-ci/linux/commits/Peter-Zijlstra/asm-generic-Disallow-no-op-mb-for-SMP-systems/20180203-000108 > > > base: > > > https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git > > > master > > > config: m32r-usrv_defconfig (attached as .config) > > > compiler: m32r-linux-gcc (GCC) 7.2.0 > > > > Awesome, another broken architecture.. and this one is Orphaned :-( > > No maintainer to bug.. > > Renesas claim to still support the processor family so perhaps they can > provide a maintainer if you instead submit a patch to remove it from the > tree ;-) There's an idea :-) > From the documentation however the processor only does explicit dual > issue instructions, and the SMP is cache coherent so I'm not convinced > there is a problem with it. Thanks for looking at that, got a link handy to said documentation? Does it really not have a store-buffer? Getting such details sorted was exactly the purpose of the patch, so in that regards its a success :-)
Re: asm-generic: Disallow no-op mb() for SMP systems
On Fri, 2 Feb 2018 21:25:49 +0100 Peter Zijlstrawrote: > On Sat, Feb 03, 2018 at 04:00:35AM +0800, kbuild test robot wrote: > > Hi Peter, > > > > I love your patch! Yet something to improve: > > Seriously? Bots have feelings? > > > [auto build test ERROR on asm-generic/master] > > [also build test ERROR on v4.15 next-20180202] > > [if your patch is applied to the wrong git tree, please drop us a note to > > help improve the system] > > > > url: > > https://github.com/0day-ci/linux/commits/Peter-Zijlstra/asm-generic-Disallow-no-op-mb-for-SMP-systems/20180203-000108 > > base: > > https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git master > > config: m32r-usrv_defconfig (attached as .config) > > compiler: m32r-linux-gcc (GCC) 7.2.0 > > Awesome, another broken architecture.. and this one is Orphaned :-( > No maintainer to bug.. Renesas claim to still support the processor family so perhaps they can provide a maintainer if you instead submit a patch to remove it from the tree ;-) >From the documentation however the processor only does explicit dual issue instructions, and the SMP is cache coherent so I'm not convinced there is a problem with it. Alan
Re: asm-generic: Disallow no-op mb() for SMP systems
On Fri, 2 Feb 2018 21:25:49 +0100 Peter Zijlstra wrote: > On Sat, Feb 03, 2018 at 04:00:35AM +0800, kbuild test robot wrote: > > Hi Peter, > > > > I love your patch! Yet something to improve: > > Seriously? Bots have feelings? > > > [auto build test ERROR on asm-generic/master] > > [also build test ERROR on v4.15 next-20180202] > > [if your patch is applied to the wrong git tree, please drop us a note to > > help improve the system] > > > > url: > > https://github.com/0day-ci/linux/commits/Peter-Zijlstra/asm-generic-Disallow-no-op-mb-for-SMP-systems/20180203-000108 > > base: > > https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git master > > config: m32r-usrv_defconfig (attached as .config) > > compiler: m32r-linux-gcc (GCC) 7.2.0 > > Awesome, another broken architecture.. and this one is Orphaned :-( > No maintainer to bug.. Renesas claim to still support the processor family so perhaps they can provide a maintainer if you instead submit a patch to remove it from the tree ;-) >From the documentation however the processor only does explicit dual issue instructions, and the SMP is cache coherent so I'm not convinced there is a problem with it. Alan
Re: asm-generic: Disallow no-op mb() for SMP systems
On Sat, Feb 03, 2018 at 04:00:35AM +0800, kbuild test robot wrote: > Hi Peter, > > I love your patch! Yet something to improve: Seriously? Bots have feelings? > [auto build test ERROR on asm-generic/master] > [also build test ERROR on v4.15 next-20180202] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Peter-Zijlstra/asm-generic-Disallow-no-op-mb-for-SMP-systems/20180203-000108 > base: https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git > master > config: m32r-usrv_defconfig (attached as .config) > compiler: m32r-linux-gcc (GCC) 7.2.0 Awesome, another broken architecture.. and this one is Orphaned :-( No maintainer to bug..
Re: asm-generic: Disallow no-op mb() for SMP systems
On Sat, Feb 03, 2018 at 04:00:35AM +0800, kbuild test robot wrote: > Hi Peter, > > I love your patch! Yet something to improve: Seriously? Bots have feelings? > [auto build test ERROR on asm-generic/master] > [also build test ERROR on v4.15 next-20180202] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Peter-Zijlstra/asm-generic-Disallow-no-op-mb-for-SMP-systems/20180203-000108 > base: https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git > master > config: m32r-usrv_defconfig (attached as .config) > compiler: m32r-linux-gcc (GCC) 7.2.0 Awesome, another broken architecture.. and this one is Orphaned :-( No maintainer to bug..
Re: asm-generic: Disallow no-op mb() for SMP systems
Hi Peter, I love your patch! Yet something to improve: [auto build test ERROR on asm-generic/master] [also build test ERROR on v4.15 next-20180202] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Peter-Zijlstra/asm-generic-Disallow-no-op-mb-for-SMP-systems/20180203-000108 base: https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git master config: m32r-usrv_defconfig (attached as .config) compiler: m32r-linux-gcc (GCC) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=m32r All errors (new ones prefixed by >>): In file included from arch/m32r/include/asm/barrier.h:14:0, from arch/m32r/include/asm/atomic.h:16, from include/linux/atomic.h:4, from include/linux/filter.h:9, from include/net/sock_reuseport.h:4, from net/core/sock_reuseport.c:8: include/asm-generic/atomic-long.h: In function 'atomic_long_read_acquire': >> include/asm-generic/barrier.h:64:20: error: implicit declaration of function >> 'mb'; did you mean 'wmb'? [-Werror=implicit-function-declaration] #define __smp_mb() mb() ^ include/asm-generic/barrier.h:143:2: note: in expansion of macro '__smp_mb' __smp_mb(); \ ^~~~ include/asm-generic/barrier.h:167:29: note: in expansion of macro '__smp_load_acquire' #define smp_load_acquire(p) __smp_load_acquire(p) ^~ include/linux/atomic.h:26:34: note: in expansion of macro 'smp_load_acquire' #define atomic_read_acquire(v) smp_load_acquire(&(v)->counter) ^~~~ include/asm-generic/atomic-long.h:33:28: note: in expansion of macro 'atomic_read_acquire' #define ATOMIC_LONG_PFX(x) atomic ## x ^~ include/asm-generic/atomic-long.h:42:15: note: in expansion of macro 'ATOMIC_LONG_PFX' return (long)ATOMIC_LONG_PFX(_read##mo)(v); \ ^~~ include/asm-generic/atomic-long.h:45:1: note: in expansion of macro 'ATOMIC_LONG_READ_OP' ATOMIC_LONG_READ_OP(_acquire) ^~~ cc1: some warnings being treated as errors vim +64 include/asm-generic/barrier.h 885df91c David Howells 2012-03-28 62 a9e4252a Michael S. Tsirkin 2015-12-27 63 #ifndef __smp_mb a9e4252a Michael S. Tsirkin 2015-12-27 @64 #define __smp_mb() mb() a9e4252a Michael S. Tsirkin 2015-12-27 65 #endif a9e4252a Michael S. Tsirkin 2015-12-27 66 :: The code at line 64 was first introduced by commit :: a9e4252a9b147043142282ebb65da94dcb951e2a asm-generic: add __smp_xxx wrappers :: TO: Michael S. Tsirkin:: CC: Michael S. Tsirkin --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: asm-generic: Disallow no-op mb() for SMP systems
Hi Peter, I love your patch! Yet something to improve: [auto build test ERROR on asm-generic/master] [also build test ERROR on v4.15 next-20180202] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Peter-Zijlstra/asm-generic-Disallow-no-op-mb-for-SMP-systems/20180203-000108 base: https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git master config: m32r-usrv_defconfig (attached as .config) compiler: m32r-linux-gcc (GCC) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=m32r All errors (new ones prefixed by >>): In file included from arch/m32r/include/asm/barrier.h:14:0, from arch/m32r/include/asm/atomic.h:16, from include/linux/atomic.h:4, from include/linux/filter.h:9, from include/net/sock_reuseport.h:4, from net/core/sock_reuseport.c:8: include/asm-generic/atomic-long.h: In function 'atomic_long_read_acquire': >> include/asm-generic/barrier.h:64:20: error: implicit declaration of function >> 'mb'; did you mean 'wmb'? [-Werror=implicit-function-declaration] #define __smp_mb() mb() ^ include/asm-generic/barrier.h:143:2: note: in expansion of macro '__smp_mb' __smp_mb(); \ ^~~~ include/asm-generic/barrier.h:167:29: note: in expansion of macro '__smp_load_acquire' #define smp_load_acquire(p) __smp_load_acquire(p) ^~ include/linux/atomic.h:26:34: note: in expansion of macro 'smp_load_acquire' #define atomic_read_acquire(v) smp_load_acquire(&(v)->counter) ^~~~ include/asm-generic/atomic-long.h:33:28: note: in expansion of macro 'atomic_read_acquire' #define ATOMIC_LONG_PFX(x) atomic ## x ^~ include/asm-generic/atomic-long.h:42:15: note: in expansion of macro 'ATOMIC_LONG_PFX' return (long)ATOMIC_LONG_PFX(_read##mo)(v); \ ^~~ include/asm-generic/atomic-long.h:45:1: note: in expansion of macro 'ATOMIC_LONG_READ_OP' ATOMIC_LONG_READ_OP(_acquire) ^~~ cc1: some warnings being treated as errors vim +64 include/asm-generic/barrier.h 885df91c David Howells 2012-03-28 62 a9e4252a Michael S. Tsirkin 2015-12-27 63 #ifndef __smp_mb a9e4252a Michael S. Tsirkin 2015-12-27 @64 #define __smp_mb() mb() a9e4252a Michael S. Tsirkin 2015-12-27 65 #endif a9e4252a Michael S. Tsirkin 2015-12-27 66 :: The code at line 64 was first introduced by commit :: a9e4252a9b147043142282ebb65da94dcb951e2a asm-generic: add __smp_xxx wrappers :: TO: Michael S. Tsirkin :: CC: Michael S. Tsirkin --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: asm-generic: Disallow no-op mb() for SMP systems
Hi Peter, I love your patch! Yet something to improve: [auto build test ERROR on asm-generic/master] [also build test ERROR on v4.15 next-20180202] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Peter-Zijlstra/asm-generic-Disallow-no-op-mb-for-SMP-systems/20180203-000108 base: https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git master config: mn10300-asb2364_defconfig (attached as .config) compiler: am33_2.0-linux-gcc (GCC) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=mn10300 All errors (new ones prefixed by >>): warning: (MN10300) selects HAVE_NMI_WATCHDOG which has unmet direct dependencies (HAVE_NMI) warning: (MN10300) selects HAVE_NMI_WATCHDOG which has unmet direct dependencies (HAVE_NMI) In file included from ./arch/mn10300/include/generated/asm/barrier.h:1:0, from arch/mn10300/include/asm/bitops.h:21, from include/linux/bitops.h:36, from include/linux/kernel.h:10, from include/asm-generic/bug.h:13, from arch/mn10300/include/asm/bug.h:35, from include/linux/bug.h:4, from include/linux/thread_info.h:11, from arch/mn10300/include/asm/current.h:14, from include/linux/sched.h:11, from arch/mn10300/kernel/asm-offsets.c:7: include/asm-generic/atomic-long.h: In function 'atomic_long_read_acquire': >> include/asm-generic/barrier.h:64:20: error: implicit declaration of function >> 'mb'; did you mean 'rmb'? [-Werror=implicit-function-declaration] #define __smp_mb() mb() ^ include/asm-generic/barrier.h:143:2: note: in expansion of macro '__smp_mb' __smp_mb(); \ ^~~~ include/asm-generic/barrier.h:167:29: note: in expansion of macro '__smp_load_acquire' #define smp_load_acquire(p) __smp_load_acquire(p) ^~ include/linux/atomic.h:26:34: note: in expansion of macro 'smp_load_acquire' #define atomic_read_acquire(v) smp_load_acquire(&(v)->counter) ^~~~ include/asm-generic/atomic-long.h:33:28: note: in expansion of macro 'atomic_read_acquire' #define ATOMIC_LONG_PFX(x) atomic ## x ^~ include/asm-generic/atomic-long.h:42:15: note: in expansion of macro 'ATOMIC_LONG_PFX' return (long)ATOMIC_LONG_PFX(_read##mo)(v); \ ^~~ include/asm-generic/atomic-long.h:45:1: note: in expansion of macro 'ATOMIC_LONG_READ_OP' ATOMIC_LONG_READ_OP(_acquire) ^~~ cc1: some warnings being treated as errors make[2]: *** [arch/mn10300/kernel/asm-offsets.s] Error 1 make[2]: Target '__build' not remade because of errors. make[1]: *** [prepare0] Error 2 make[1]: Target 'prepare' not remade because of errors. make: *** [sub-make] Error 2 vim +64 include/asm-generic/barrier.h 885df91c David Howells 2012-03-28 62 a9e4252a Michael S. Tsirkin 2015-12-27 63 #ifndef __smp_mb a9e4252a Michael S. Tsirkin 2015-12-27 @64 #define __smp_mb() mb() a9e4252a Michael S. Tsirkin 2015-12-27 65 #endif a9e4252a Michael S. Tsirkin 2015-12-27 66 :: The code at line 64 was first introduced by commit :: a9e4252a9b147043142282ebb65da94dcb951e2a asm-generic: add __smp_xxx wrappers :: TO: Michael S. Tsirkin:: CC: Michael S. Tsirkin --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: asm-generic: Disallow no-op mb() for SMP systems
Hi Peter, I love your patch! Yet something to improve: [auto build test ERROR on asm-generic/master] [also build test ERROR on v4.15 next-20180202] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Peter-Zijlstra/asm-generic-Disallow-no-op-mb-for-SMP-systems/20180203-000108 base: https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git master config: mn10300-asb2364_defconfig (attached as .config) compiler: am33_2.0-linux-gcc (GCC) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=mn10300 All errors (new ones prefixed by >>): warning: (MN10300) selects HAVE_NMI_WATCHDOG which has unmet direct dependencies (HAVE_NMI) warning: (MN10300) selects HAVE_NMI_WATCHDOG which has unmet direct dependencies (HAVE_NMI) In file included from ./arch/mn10300/include/generated/asm/barrier.h:1:0, from arch/mn10300/include/asm/bitops.h:21, from include/linux/bitops.h:36, from include/linux/kernel.h:10, from include/asm-generic/bug.h:13, from arch/mn10300/include/asm/bug.h:35, from include/linux/bug.h:4, from include/linux/thread_info.h:11, from arch/mn10300/include/asm/current.h:14, from include/linux/sched.h:11, from arch/mn10300/kernel/asm-offsets.c:7: include/asm-generic/atomic-long.h: In function 'atomic_long_read_acquire': >> include/asm-generic/barrier.h:64:20: error: implicit declaration of function >> 'mb'; did you mean 'rmb'? [-Werror=implicit-function-declaration] #define __smp_mb() mb() ^ include/asm-generic/barrier.h:143:2: note: in expansion of macro '__smp_mb' __smp_mb(); \ ^~~~ include/asm-generic/barrier.h:167:29: note: in expansion of macro '__smp_load_acquire' #define smp_load_acquire(p) __smp_load_acquire(p) ^~ include/linux/atomic.h:26:34: note: in expansion of macro 'smp_load_acquire' #define atomic_read_acquire(v) smp_load_acquire(&(v)->counter) ^~~~ include/asm-generic/atomic-long.h:33:28: note: in expansion of macro 'atomic_read_acquire' #define ATOMIC_LONG_PFX(x) atomic ## x ^~ include/asm-generic/atomic-long.h:42:15: note: in expansion of macro 'ATOMIC_LONG_PFX' return (long)ATOMIC_LONG_PFX(_read##mo)(v); \ ^~~ include/asm-generic/atomic-long.h:45:1: note: in expansion of macro 'ATOMIC_LONG_READ_OP' ATOMIC_LONG_READ_OP(_acquire) ^~~ cc1: some warnings being treated as errors make[2]: *** [arch/mn10300/kernel/asm-offsets.s] Error 1 make[2]: Target '__build' not remade because of errors. make[1]: *** [prepare0] Error 2 make[1]: Target 'prepare' not remade because of errors. make: *** [sub-make] Error 2 vim +64 include/asm-generic/barrier.h 885df91c David Howells 2012-03-28 62 a9e4252a Michael S. Tsirkin 2015-12-27 63 #ifndef __smp_mb a9e4252a Michael S. Tsirkin 2015-12-27 @64 #define __smp_mb() mb() a9e4252a Michael S. Tsirkin 2015-12-27 65 #endif a9e4252a Michael S. Tsirkin 2015-12-27 66 :: The code at line 64 was first introduced by commit :: a9e4252a9b147043142282ebb65da94dcb951e2a asm-generic: add __smp_xxx wrappers :: TO: Michael S. Tsirkin :: CC: Michael S. Tsirkin --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: asm-generic: Disallow no-op mb() for SMP systems
On Thu, Feb 01, 2018 at 02:29:09PM +0100, Peter Zijlstra wrote: > On Thu, Feb 01, 2018 at 09:27:50PM +0900, Stafford Horne wrote: > > I tried to clarify some of this in the spec v1.2 [0] which help formalize > > some of > > the techniques we used for the SMP implementation. Its probably not > > perfect, > > but I added a section "10. Multicore support" and tried to clarify some > > things > > in section 7 on Atomicity. But it seems I dont cover exactly what are are > > mentioning here. In general: > > > > 1 Secondary cores have memory snooping enabled meaning that any write to a > > cached address will cause the cache line to be invalidated. > > 2 l.swa (store atomic word) implies a store buffer flush. > > What about l.lwa? Can that observe 'old' values, or rather, miss values > stuck in a remote store buffer? > > This will then cause the first l.swa to fail, which, per the above, > would then sync things up? Which means you get that one extra > merry-go-round. Sorry, I remembered incorrectly, l.lwa also implies a (l.msync) store buffer flush for the local cpu. However, in order to see something stuch in the remote store buffer a flush would need to be inititiated on the remote core. I think that is what we would expect though right? > > 3 l.msync is used to flush the store buffer > > > > Also, during the IPI controller review [1] Marc Z asked many similar > > questions. > > I believe he was ok in the end. > > > > Anyway, > > Thanks for thanks for spotting the issue here. For some reason I remember > > we > > did have an l.msync for our mb(). Let me think about and test out this > > patch > > (and the fix to actually define mb) to see if anything comes up. > > > > Also, I haven't seen any implementations that use WOM. Stefan might know > > better. > > So if the strong model has a store buffer, as I think the above says, > then it is _NOT_ correct for l.msync to be treated as a NOP, it _must_ > flush the store buffer. > > At which point I think your 'strong' model is basically TSO. So it would > be very good to get that spelled out somewhere. Yes, I think the original author did not think of PSO/TSO and store buffers. Its not clear of the authors intention. It should be cleared up. I would say: 1 Weak order model with store buffers is PSO (must implement l.msync) 2 Strong model with store buffers is TSO (must implement l.msync) 3 Implementations without store buffers could be weak or strong? a weak meaning cpu could schedule loads stores out of order l.msync would cause all pending load/store instructions to be retired. b strong meaning loads/stores would happen in instruction order, in this case l.msync could be a no-op as there is no buffering of stores or loads. 1 doesnt exist as far as I know. So its probably better to remove. 2 is what we have now in mor1kx. 3.b it possible, but we always have a l.msync implementation. But maybe it doesnt make sense when there is no store buffer. -Stafford
Re: asm-generic: Disallow no-op mb() for SMP systems
On Thu, Feb 01, 2018 at 02:29:09PM +0100, Peter Zijlstra wrote: > On Thu, Feb 01, 2018 at 09:27:50PM +0900, Stafford Horne wrote: > > I tried to clarify some of this in the spec v1.2 [0] which help formalize > > some of > > the techniques we used for the SMP implementation. Its probably not > > perfect, > > but I added a section "10. Multicore support" and tried to clarify some > > things > > in section 7 on Atomicity. But it seems I dont cover exactly what are are > > mentioning here. In general: > > > > 1 Secondary cores have memory snooping enabled meaning that any write to a > > cached address will cause the cache line to be invalidated. > > 2 l.swa (store atomic word) implies a store buffer flush. > > What about l.lwa? Can that observe 'old' values, or rather, miss values > stuck in a remote store buffer? > > This will then cause the first l.swa to fail, which, per the above, > would then sync things up? Which means you get that one extra > merry-go-round. Sorry, I remembered incorrectly, l.lwa also implies a (l.msync) store buffer flush for the local cpu. However, in order to see something stuch in the remote store buffer a flush would need to be inititiated on the remote core. I think that is what we would expect though right? > > 3 l.msync is used to flush the store buffer > > > > Also, during the IPI controller review [1] Marc Z asked many similar > > questions. > > I believe he was ok in the end. > > > > Anyway, > > Thanks for thanks for spotting the issue here. For some reason I remember > > we > > did have an l.msync for our mb(). Let me think about and test out this > > patch > > (and the fix to actually define mb) to see if anything comes up. > > > > Also, I haven't seen any implementations that use WOM. Stefan might know > > better. > > So if the strong model has a store buffer, as I think the above says, > then it is _NOT_ correct for l.msync to be treated as a NOP, it _must_ > flush the store buffer. > > At which point I think your 'strong' model is basically TSO. So it would > be very good to get that spelled out somewhere. Yes, I think the original author did not think of PSO/TSO and store buffers. Its not clear of the authors intention. It should be cleared up. I would say: 1 Weak order model with store buffers is PSO (must implement l.msync) 2 Strong model with store buffers is TSO (must implement l.msync) 3 Implementations without store buffers could be weak or strong? a weak meaning cpu could schedule loads stores out of order l.msync would cause all pending load/store instructions to be retired. b strong meaning loads/stores would happen in instruction order, in this case l.msync could be a no-op as there is no buffering of stores or loads. 1 doesnt exist as far as I know. So its probably better to remove. 2 is what we have now in mor1kx. 3.b it possible, but we always have a l.msync implementation. But maybe it doesnt make sense when there is no store buffer. -Stafford
Re: asm-generic: Disallow no-op mb() for SMP systems
On Thu, Feb 01, 2018 at 04:50:07PM +0100, Peter Zijlstra wrote: > On Thu, Feb 01, 2018 at 03:39:51PM +, Will Deacon wrote: > > > I could've gotten my brain in a twist or course, which isn't _that_ > > > unusual. I never seem to be able to quite remember the holes you have > > > with ll/sc on arm64 :-) > > > > Is smp_mb__before_atomic supposed to provide ordering guarantees if it's > > used before a failed cmpxchg? If so, I think it's needed here because the > > l.swa might not even execute. Or did I just invent another problem? > > I think it should do so indeed (and afaik all our current archs are good > that way). See commit: 34d54f3d6917 ("locking/pvqspinlock: Relax cmpxchg's to improve performance on some architectures")
Re: asm-generic: Disallow no-op mb() for SMP systems
On Thu, Feb 01, 2018 at 04:50:07PM +0100, Peter Zijlstra wrote: > On Thu, Feb 01, 2018 at 03:39:51PM +, Will Deacon wrote: > > > I could've gotten my brain in a twist or course, which isn't _that_ > > > unusual. I never seem to be able to quite remember the holes you have > > > with ll/sc on arm64 :-) > > > > Is smp_mb__before_atomic supposed to provide ordering guarantees if it's > > used before a failed cmpxchg? If so, I think it's needed here because the > > l.swa might not even execute. Or did I just invent another problem? > > I think it should do so indeed (and afaik all our current archs are good > that way). See commit: 34d54f3d6917 ("locking/pvqspinlock: Relax cmpxchg's to improve performance on some architectures")
Re: asm-generic: Disallow no-op mb() for SMP systems
On Thu, Feb 01, 2018 at 03:39:51PM +, Will Deacon wrote: > > I could've gotten my brain in a twist or course, which isn't _that_ > > unusual. I never seem to be able to quite remember the holes you have > > with ll/sc on arm64 :-) > > Is smp_mb__before_atomic supposed to provide ordering guarantees if it's > used before a failed cmpxchg? If so, I think it's needed here because the > l.swa might not even execute. Or did I just invent another problem? I think it should do so indeed (and afaik all our current archs are good that way).
Re: asm-generic: Disallow no-op mb() for SMP systems
On Thu, Feb 01, 2018 at 03:39:51PM +, Will Deacon wrote: > > I could've gotten my brain in a twist or course, which isn't _that_ > > unusual. I never seem to be able to quite remember the holes you have > > with ll/sc on arm64 :-) > > Is smp_mb__before_atomic supposed to provide ordering guarantees if it's > used before a failed cmpxchg? If so, I think it's needed here because the > l.swa might not even execute. Or did I just invent another problem? I think it should do so indeed (and afaik all our current archs are good that way).
Re: asm-generic: Disallow no-op mb() for SMP systems
On Thu, Feb 01, 2018 at 02:53:29PM +0100, Peter Zijlstra wrote: > On Thu, Feb 01, 2018 at 01:32:30PM +, Will Deacon wrote: > > On Thu, Feb 01, 2018 at 02:29:09PM +0100, Peter Zijlstra wrote: > > > On Thu, Feb 01, 2018 at 09:27:50PM +0900, Stafford Horne wrote: > > > > I tried to clarify some of this in the spec v1.2 [0] which help > > > > formalize some of > > > > the techniques we used for the SMP implementation. Its probably not > > > > perfect, > > > > but I added a section "10. Multicore support" and tried to clarify some > > > > things > > > > in section 7 on Atomicity. But it seems I dont cover exactly what are > > > > are > > > > mentioning here. In general: > > > > > > > > 1 Secondary cores have memory snooping enabled meaning that any write > > > > to a > > > > cached address will cause the cache line to be invalidated. > > > > 2 l.swa (store atomic word) implies a store buffer flush. > > > > > > What about l.lwa? Can that observe 'old' values, or rather, miss values > > > stuck in a remote store buffer? > > > > > > This will then cause the first l.swa to fail, which, per the above, > > > would then sync things up? Which means you get that one extra > > > merry-go-round. > > > > That's ok from a correctness perspective, though, as long as store buffers > > are guaranteed to drain. > > Depends a bit if you can build control dependencies off of l.swa > succeding or not I think :-) Otherwise you get into that dodgy state you > suffer from where bits can leak right through. > > That is, I was thinking what we need for smp_mb__before_atomic. > > I could've gotten my brain in a twist or course, which isn't _that_ > unusual. I never seem to be able to quite remember the holes you have > with ll/sc on arm64 :-) Is smp_mb__before_atomic supposed to provide ordering guarantees if it's used before a failed cmpxchg? If so, I think it's needed here because the l.swa might not even execute. Or did I just invent another problem? Will
Re: asm-generic: Disallow no-op mb() for SMP systems
On Thu, Feb 01, 2018 at 02:53:29PM +0100, Peter Zijlstra wrote: > On Thu, Feb 01, 2018 at 01:32:30PM +, Will Deacon wrote: > > On Thu, Feb 01, 2018 at 02:29:09PM +0100, Peter Zijlstra wrote: > > > On Thu, Feb 01, 2018 at 09:27:50PM +0900, Stafford Horne wrote: > > > > I tried to clarify some of this in the spec v1.2 [0] which help > > > > formalize some of > > > > the techniques we used for the SMP implementation. Its probably not > > > > perfect, > > > > but I added a section "10. Multicore support" and tried to clarify some > > > > things > > > > in section 7 on Atomicity. But it seems I dont cover exactly what are > > > > are > > > > mentioning here. In general: > > > > > > > > 1 Secondary cores have memory snooping enabled meaning that any write > > > > to a > > > > cached address will cause the cache line to be invalidated. > > > > 2 l.swa (store atomic word) implies a store buffer flush. > > > > > > What about l.lwa? Can that observe 'old' values, or rather, miss values > > > stuck in a remote store buffer? > > > > > > This will then cause the first l.swa to fail, which, per the above, > > > would then sync things up? Which means you get that one extra > > > merry-go-round. > > > > That's ok from a correctness perspective, though, as long as store buffers > > are guaranteed to drain. > > Depends a bit if you can build control dependencies off of l.swa > succeding or not I think :-) Otherwise you get into that dodgy state you > suffer from where bits can leak right through. > > That is, I was thinking what we need for smp_mb__before_atomic. > > I could've gotten my brain in a twist or course, which isn't _that_ > unusual. I never seem to be able to quite remember the holes you have > with ll/sc on arm64 :-) Is smp_mb__before_atomic supposed to provide ordering guarantees if it's used before a failed cmpxchg? If so, I think it's needed here because the l.swa might not even execute. Or did I just invent another problem? Will
Re: asm-generic: Disallow no-op mb() for SMP systems
On Thu, Feb 01, 2018 at 01:32:30PM +, Will Deacon wrote: > On Thu, Feb 01, 2018 at 02:29:09PM +0100, Peter Zijlstra wrote: > > On Thu, Feb 01, 2018 at 09:27:50PM +0900, Stafford Horne wrote: > > > I tried to clarify some of this in the spec v1.2 [0] which help formalize > > > some of > > > the techniques we used for the SMP implementation. Its probably not > > > perfect, > > > but I added a section "10. Multicore support" and tried to clarify some > > > things > > > in section 7 on Atomicity. But it seems I dont cover exactly what are are > > > mentioning here. In general: > > > > > > 1 Secondary cores have memory snooping enabled meaning that any write > > > to a > > > cached address will cause the cache line to be invalidated. > > > 2 l.swa (store atomic word) implies a store buffer flush. > > > > What about l.lwa? Can that observe 'old' values, or rather, miss values > > stuck in a remote store buffer? > > > > This will then cause the first l.swa to fail, which, per the above, > > would then sync things up? Which means you get that one extra > > merry-go-round. > > That's ok from a correctness perspective, though, as long as store buffers > are guaranteed to drain. Depends a bit if you can build control dependencies off of l.swa succeding or not I think :-) Otherwise you get into that dodgy state you suffer from where bits can leak right through. That is, I was thinking what we need for smp_mb__before_atomic. I could've gotten my brain in a twist or course, which isn't _that_ unusual. I never seem to be able to quite remember the holes you have with ll/sc on arm64 :-)
Re: asm-generic: Disallow no-op mb() for SMP systems
On Thu, Feb 01, 2018 at 01:32:30PM +, Will Deacon wrote: > On Thu, Feb 01, 2018 at 02:29:09PM +0100, Peter Zijlstra wrote: > > On Thu, Feb 01, 2018 at 09:27:50PM +0900, Stafford Horne wrote: > > > I tried to clarify some of this in the spec v1.2 [0] which help formalize > > > some of > > > the techniques we used for the SMP implementation. Its probably not > > > perfect, > > > but I added a section "10. Multicore support" and tried to clarify some > > > things > > > in section 7 on Atomicity. But it seems I dont cover exactly what are are > > > mentioning here. In general: > > > > > > 1 Secondary cores have memory snooping enabled meaning that any write > > > to a > > > cached address will cause the cache line to be invalidated. > > > 2 l.swa (store atomic word) implies a store buffer flush. > > > > What about l.lwa? Can that observe 'old' values, or rather, miss values > > stuck in a remote store buffer? > > > > This will then cause the first l.swa to fail, which, per the above, > > would then sync things up? Which means you get that one extra > > merry-go-round. > > That's ok from a correctness perspective, though, as long as store buffers > are guaranteed to drain. Depends a bit if you can build control dependencies off of l.swa succeding or not I think :-) Otherwise you get into that dodgy state you suffer from where bits can leak right through. That is, I was thinking what we need for smp_mb__before_atomic. I could've gotten my brain in a twist or course, which isn't _that_ unusual. I never seem to be able to quite remember the holes you have with ll/sc on arm64 :-)
Re: asm-generic: Disallow no-op mb() for SMP systems
On Thu, Feb 01, 2018 at 02:29:09PM +0100, Peter Zijlstra wrote: > On Thu, Feb 01, 2018 at 09:27:50PM +0900, Stafford Horne wrote: > > I tried to clarify some of this in the spec v1.2 [0] which help formalize > > some of > > the techniques we used for the SMP implementation. Its probably not > > perfect, > > but I added a section "10. Multicore support" and tried to clarify some > > things > > in section 7 on Atomicity. But it seems I dont cover exactly what are are > > mentioning here. In general: > > > > 1 Secondary cores have memory snooping enabled meaning that any write to a > > cached address will cause the cache line to be invalidated. > > 2 l.swa (store atomic word) implies a store buffer flush. > > What about l.lwa? Can that observe 'old' values, or rather, miss values > stuck in a remote store buffer? > > This will then cause the first l.swa to fail, which, per the above, > would then sync things up? Which means you get that one extra > merry-go-round. That's ok from a correctness perspective, though, as long as store buffers are guaranteed to drain. > > 3 l.msync is used to flush the store buffer > > > > Also, during the IPI controller review [1] Marc Z asked many similar > > questions. > > I believe he was ok in the end. > > > > Anyway, > > Thanks for thanks for spotting the issue here. For some reason I remember > > we > > did have an l.msync for our mb(). Let me think about and test out this > > patch > > (and the fix to actually define mb) to see if anything comes up. > > > > Also, I haven't seen any implementations that use WOM. Stefan might know > > better. > > So if the strong model has a store buffer, as I think the above says, > then it is _NOT_ correct for l.msync to be treated as a NOP, it _must_ > flush the store buffer. > > At which point I think your 'strong' model is basically TSO. So it would > be very good to get that spelled out somewhere. Agreed, especially as a software person reading the spec may end up thinking that they don't need to use l.msync if they never set WOM. At least, I read it this way despite knowing that it's probably not what you intended. Will
Re: asm-generic: Disallow no-op mb() for SMP systems
On Thu, Feb 01, 2018 at 02:29:09PM +0100, Peter Zijlstra wrote: > On Thu, Feb 01, 2018 at 09:27:50PM +0900, Stafford Horne wrote: > > I tried to clarify some of this in the spec v1.2 [0] which help formalize > > some of > > the techniques we used for the SMP implementation. Its probably not > > perfect, > > but I added a section "10. Multicore support" and tried to clarify some > > things > > in section 7 on Atomicity. But it seems I dont cover exactly what are are > > mentioning here. In general: > > > > 1 Secondary cores have memory snooping enabled meaning that any write to a > > cached address will cause the cache line to be invalidated. > > 2 l.swa (store atomic word) implies a store buffer flush. > > What about l.lwa? Can that observe 'old' values, or rather, miss values > stuck in a remote store buffer? > > This will then cause the first l.swa to fail, which, per the above, > would then sync things up? Which means you get that one extra > merry-go-round. That's ok from a correctness perspective, though, as long as store buffers are guaranteed to drain. > > 3 l.msync is used to flush the store buffer > > > > Also, during the IPI controller review [1] Marc Z asked many similar > > questions. > > I believe he was ok in the end. > > > > Anyway, > > Thanks for thanks for spotting the issue here. For some reason I remember > > we > > did have an l.msync for our mb(). Let me think about and test out this > > patch > > (and the fix to actually define mb) to see if anything comes up. > > > > Also, I haven't seen any implementations that use WOM. Stefan might know > > better. > > So if the strong model has a store buffer, as I think the above says, > then it is _NOT_ correct for l.msync to be treated as a NOP, it _must_ > flush the store buffer. > > At which point I think your 'strong' model is basically TSO. So it would > be very good to get that spelled out somewhere. Agreed, especially as a software person reading the spec may end up thinking that they don't need to use l.msync if they never set WOM. At least, I read it this way despite knowing that it's probably not what you intended. Will
Re: asm-generic: Disallow no-op mb() for SMP systems
On Thu, Feb 01, 2018 at 09:27:50PM +0900, Stafford Horne wrote: > I tried to clarify some of this in the spec v1.2 [0] which help formalize > some of > the techniques we used for the SMP implementation. Its probably not perfect, > but I added a section "10. Multicore support" and tried to clarify some things > in section 7 on Atomicity. But it seems I dont cover exactly what are are > mentioning here. In general: > > 1 Secondary cores have memory snooping enabled meaning that any write to a > cached address will cause the cache line to be invalidated. > 2 l.swa (store atomic word) implies a store buffer flush. What about l.lwa? Can that observe 'old' values, or rather, miss values stuck in a remote store buffer? This will then cause the first l.swa to fail, which, per the above, would then sync things up? Which means you get that one extra merry-go-round. > 3 l.msync is used to flush the store buffer > > Also, during the IPI controller review [1] Marc Z asked many similar > questions. > I believe he was ok in the end. > > Anyway, > Thanks for thanks for spotting the issue here. For some reason I remember we > did have an l.msync for our mb(). Let me think about and test out this patch > (and the fix to actually define mb) to see if anything comes up. > > Also, I haven't seen any implementations that use WOM. Stefan might know > better. So if the strong model has a store buffer, as I think the above says, then it is _NOT_ correct for l.msync to be treated as a NOP, it _must_ flush the store buffer. At which point I think your 'strong' model is basically TSO. So it would be very good to get that spelled out somewhere.
Re: asm-generic: Disallow no-op mb() for SMP systems
On Thu, Feb 01, 2018 at 09:27:50PM +0900, Stafford Horne wrote: > I tried to clarify some of this in the spec v1.2 [0] which help formalize > some of > the techniques we used for the SMP implementation. Its probably not perfect, > but I added a section "10. Multicore support" and tried to clarify some things > in section 7 on Atomicity. But it seems I dont cover exactly what are are > mentioning here. In general: > > 1 Secondary cores have memory snooping enabled meaning that any write to a > cached address will cause the cache line to be invalidated. > 2 l.swa (store atomic word) implies a store buffer flush. What about l.lwa? Can that observe 'old' values, or rather, miss values stuck in a remote store buffer? This will then cause the first l.swa to fail, which, per the above, would then sync things up? Which means you get that one extra merry-go-round. > 3 l.msync is used to flush the store buffer > > Also, during the IPI controller review [1] Marc Z asked many similar > questions. > I believe he was ok in the end. > > Anyway, > Thanks for thanks for spotting the issue here. For some reason I remember we > did have an l.msync for our mb(). Let me think about and test out this patch > (and the fix to actually define mb) to see if anything comes up. > > Also, I haven't seen any implementations that use WOM. Stefan might know > better. So if the strong model has a store buffer, as I think the above says, then it is _NOT_ correct for l.msync to be treated as a NOP, it _must_ flush the store buffer. At which point I think your 'strong' model is basically TSO. So it would be very good to get that spelled out somewhere.
Re: asm-generic: Disallow no-op mb() for SMP systems
On Wed, Jan 31, 2018 at 02:26:10PM +0100, Peter Zijlstra wrote: > On Wed, Jan 31, 2018 at 01:17:37PM +, Will Deacon wrote: > > On Wed, Jan 31, 2018 at 02:00:34PM +0100, Peter Zijlstra wrote: > > > > > > While looking through the qspinlock users, I stumbled upon openrisc and > > > being curious, I wanted to have a look at its memory model. > > > > > > To my surprise it doesn't have asm/barrier.h. It does however support > > > SMP. This lead me to wonder why it would compile, turns out we provide a > > > no-op mb() if the architecture does not provide one. > > > > > > With the obvious exception of Sequentially Consistent SMP systems, this > > > is terribly broken. And seeing how SC SMP is really rather unusual (and > > > unlikely), change the asm-generic/barrier.h to not provide this. > > > > > > This _will_ break openrisc builds, they had better clarify their > > > position by writing their own barrier.h with a comment in. > > > > > > Signed-off-by: Peter Zijlstra (Intel)> > > --- > > > include/asm-generic/barrier.h | 6 ++ > > > 1 file changed, 6 insertions(+) > > > > Acked-by: Will Deacon > > > > One comment below... > > > > > diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h > > > index fe297b599b0a..7a10748615ff 100644 > > > --- a/include/asm-generic/barrier.h > > > +++ b/include/asm-generic/barrier.h > > > @@ -30,9 +30,15 @@ > > > * Fall back to compiler barriers if nothing better is provided. > > > */ > > > > > > +#ifndef CONFIG_SMP > > > +/* > > > + * If your SMP architecture really is Sequentially Consistent, you get > > > + * barrier.h to write a nice big comment about it. > > > + */ > > > > I couldn't find any documentation about the openrisc memory model other > > than: > > > > https://openrisc.io/or1k.html#__RefHeading__504775_595890882 > > > > which talks about the WOM bit in the page table being used to select a weak > > memory model for the page being mapped as opposed to the strong memory > > model. Furthermore, the only fence instruction (l.msync) is permitted to > > be a NOP for the strong model, which implies that the strong model is SC > > and things like write buffers are forbidden. However, Google suggests that > > there are implementations of openrisc with write buffers so I'm not sure I > > understand what's going on here (maybe they force WOM and/or l.msync isn't > > actually a NOP?) > > Right, so by that document they need: > > #define mb() asm volatile ("l.msync":::"memory) > > but yes, that document raises more questions than it answers. It would > be very good to get clarification on how strong their strong model > really is. SMP without store buffers is, as I wrote above, 'unusual'. Hello, I tried to clarify some of this in the spec v1.2 [0] which help formalize some of the techniques we used for the SMP implementation. Its probably not perfect, but I added a section "10. Multicore support" and tried to clarify some things in section 7 on Atomicity. But it seems I dont cover exactly what are are mentioning here. In general: 1 Secondary cores have memory snooping enabled meaning that any write to a cached address will cause the cache line to be invalidated. 2 l.swa (store atomic word) implies a store buffer flush. 3 l.msync is used to flush the store buffer Also, during the IPI controller review [1] Marc Z asked many similar questions. I believe he was ok in the end. Anyway, Thanks for thanks for spotting the issue here. For some reason I remember we did have an l.msync for our mb(). Let me think about and test out this patch (and the fix to actually define mb) to see if anything comes up. Also, I haven't seen any implementations that use WOM. Stefan might know better. [0] https://raw.githubusercontent.com/openrisc/doc/master/openrisc-arch-1.2-rev0.pdf [1] https://lkml.org/lkml/2017/9/14/487 -Stafford
Re: asm-generic: Disallow no-op mb() for SMP systems
On Wed, Jan 31, 2018 at 02:26:10PM +0100, Peter Zijlstra wrote: > On Wed, Jan 31, 2018 at 01:17:37PM +, Will Deacon wrote: > > On Wed, Jan 31, 2018 at 02:00:34PM +0100, Peter Zijlstra wrote: > > > > > > While looking through the qspinlock users, I stumbled upon openrisc and > > > being curious, I wanted to have a look at its memory model. > > > > > > To my surprise it doesn't have asm/barrier.h. It does however support > > > SMP. This lead me to wonder why it would compile, turns out we provide a > > > no-op mb() if the architecture does not provide one. > > > > > > With the obvious exception of Sequentially Consistent SMP systems, this > > > is terribly broken. And seeing how SC SMP is really rather unusual (and > > > unlikely), change the asm-generic/barrier.h to not provide this. > > > > > > This _will_ break openrisc builds, they had better clarify their > > > position by writing their own barrier.h with a comment in. > > > > > > Signed-off-by: Peter Zijlstra (Intel) > > > --- > > > include/asm-generic/barrier.h | 6 ++ > > > 1 file changed, 6 insertions(+) > > > > Acked-by: Will Deacon > > > > One comment below... > > > > > diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h > > > index fe297b599b0a..7a10748615ff 100644 > > > --- a/include/asm-generic/barrier.h > > > +++ b/include/asm-generic/barrier.h > > > @@ -30,9 +30,15 @@ > > > * Fall back to compiler barriers if nothing better is provided. > > > */ > > > > > > +#ifndef CONFIG_SMP > > > +/* > > > + * If your SMP architecture really is Sequentially Consistent, you get > > > + * barrier.h to write a nice big comment about it. > > > + */ > > > > I couldn't find any documentation about the openrisc memory model other > > than: > > > > https://openrisc.io/or1k.html#__RefHeading__504775_595890882 > > > > which talks about the WOM bit in the page table being used to select a weak > > memory model for the page being mapped as opposed to the strong memory > > model. Furthermore, the only fence instruction (l.msync) is permitted to > > be a NOP for the strong model, which implies that the strong model is SC > > and things like write buffers are forbidden. However, Google suggests that > > there are implementations of openrisc with write buffers so I'm not sure I > > understand what's going on here (maybe they force WOM and/or l.msync isn't > > actually a NOP?) > > Right, so by that document they need: > > #define mb() asm volatile ("l.msync":::"memory) > > but yes, that document raises more questions than it answers. It would > be very good to get clarification on how strong their strong model > really is. SMP without store buffers is, as I wrote above, 'unusual'. Hello, I tried to clarify some of this in the spec v1.2 [0] which help formalize some of the techniques we used for the SMP implementation. Its probably not perfect, but I added a section "10. Multicore support" and tried to clarify some things in section 7 on Atomicity. But it seems I dont cover exactly what are are mentioning here. In general: 1 Secondary cores have memory snooping enabled meaning that any write to a cached address will cause the cache line to be invalidated. 2 l.swa (store atomic word) implies a store buffer flush. 3 l.msync is used to flush the store buffer Also, during the IPI controller review [1] Marc Z asked many similar questions. I believe he was ok in the end. Anyway, Thanks for thanks for spotting the issue here. For some reason I remember we did have an l.msync for our mb(). Let me think about and test out this patch (and the fix to actually define mb) to see if anything comes up. Also, I haven't seen any implementations that use WOM. Stefan might know better. [0] https://raw.githubusercontent.com/openrisc/doc/master/openrisc-arch-1.2-rev0.pdf [1] https://lkml.org/lkml/2017/9/14/487 -Stafford
Re: asm-generic: Disallow no-op mb() for SMP systems
On Wed, Jan 31, 2018 at 01:17:37PM +, Will Deacon wrote: > On Wed, Jan 31, 2018 at 02:00:34PM +0100, Peter Zijlstra wrote: > > > > While looking through the qspinlock users, I stumbled upon openrisc and > > being curious, I wanted to have a look at its memory model. > > > > To my surprise it doesn't have asm/barrier.h. It does however support > > SMP. This lead me to wonder why it would compile, turns out we provide a > > no-op mb() if the architecture does not provide one. > > > > With the obvious exception of Sequentially Consistent SMP systems, this > > is terribly broken. And seeing how SC SMP is really rather unusual (and > > unlikely), change the asm-generic/barrier.h to not provide this. > > > > This _will_ break openrisc builds, they had better clarify their > > position by writing their own barrier.h with a comment in. > > > > Signed-off-by: Peter Zijlstra (Intel)> > --- > > include/asm-generic/barrier.h | 6 ++ > > 1 file changed, 6 insertions(+) > > Acked-by: Will Deacon > > One comment below... > > > diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h > > index fe297b599b0a..7a10748615ff 100644 > > --- a/include/asm-generic/barrier.h > > +++ b/include/asm-generic/barrier.h > > @@ -30,9 +30,15 @@ > > * Fall back to compiler barriers if nothing better is provided. > > */ > > > > +#ifndef CONFIG_SMP > > +/* > > + * If your SMP architecture really is Sequentially Consistent, you get > > + * barrier.h to write a nice big comment about it. > > + */ > > I couldn't find any documentation about the openrisc memory model other > than: > > https://openrisc.io/or1k.html#__RefHeading__504775_595890882 > > which talks about the WOM bit in the page table being used to select a weak > memory model for the page being mapped as opposed to the strong memory > model. Furthermore, the only fence instruction (l.msync) is permitted to > be a NOP for the strong model, which implies that the strong model is SC > and things like write buffers are forbidden. However, Google suggests that > there are implementations of openrisc with write buffers so I'm not sure I > understand what's going on here (maybe they force WOM and/or l.msync isn't > actually a NOP?) Right, so by that document they need: #define mb() asm volatile ("l.msync":::"memory) but yes, that document raises more questions than it answers. It would be very good to get clarification on how strong their strong model really is. SMP without store buffers is, as I wrote above, 'unusual'.
Re: asm-generic: Disallow no-op mb() for SMP systems
On Wed, Jan 31, 2018 at 01:17:37PM +, Will Deacon wrote: > On Wed, Jan 31, 2018 at 02:00:34PM +0100, Peter Zijlstra wrote: > > > > While looking through the qspinlock users, I stumbled upon openrisc and > > being curious, I wanted to have a look at its memory model. > > > > To my surprise it doesn't have asm/barrier.h. It does however support > > SMP. This lead me to wonder why it would compile, turns out we provide a > > no-op mb() if the architecture does not provide one. > > > > With the obvious exception of Sequentially Consistent SMP systems, this > > is terribly broken. And seeing how SC SMP is really rather unusual (and > > unlikely), change the asm-generic/barrier.h to not provide this. > > > > This _will_ break openrisc builds, they had better clarify their > > position by writing their own barrier.h with a comment in. > > > > Signed-off-by: Peter Zijlstra (Intel) > > --- > > include/asm-generic/barrier.h | 6 ++ > > 1 file changed, 6 insertions(+) > > Acked-by: Will Deacon > > One comment below... > > > diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h > > index fe297b599b0a..7a10748615ff 100644 > > --- a/include/asm-generic/barrier.h > > +++ b/include/asm-generic/barrier.h > > @@ -30,9 +30,15 @@ > > * Fall back to compiler barriers if nothing better is provided. > > */ > > > > +#ifndef CONFIG_SMP > > +/* > > + * If your SMP architecture really is Sequentially Consistent, you get > > + * barrier.h to write a nice big comment about it. > > + */ > > I couldn't find any documentation about the openrisc memory model other > than: > > https://openrisc.io/or1k.html#__RefHeading__504775_595890882 > > which talks about the WOM bit in the page table being used to select a weak > memory model for the page being mapped as opposed to the strong memory > model. Furthermore, the only fence instruction (l.msync) is permitted to > be a NOP for the strong model, which implies that the strong model is SC > and things like write buffers are forbidden. However, Google suggests that > there are implementations of openrisc with write buffers so I'm not sure I > understand what's going on here (maybe they force WOM and/or l.msync isn't > actually a NOP?) Right, so by that document they need: #define mb() asm volatile ("l.msync":::"memory) but yes, that document raises more questions than it answers. It would be very good to get clarification on how strong their strong model really is. SMP without store buffers is, as I wrote above, 'unusual'.
Re: asm-generic: Disallow no-op mb() for SMP systems
On Wed, Jan 31, 2018 at 02:00:34PM +0100, Peter Zijlstra wrote: > > While looking through the qspinlock users, I stumbled upon openrisc and > being curious, I wanted to have a look at its memory model. > > To my surprise it doesn't have asm/barrier.h. It does however support > SMP. This lead me to wonder why it would compile, turns out we provide a > no-op mb() if the architecture does not provide one. > > With the obvious exception of Sequentially Consistent SMP systems, this > is terribly broken. And seeing how SC SMP is really rather unusual (and > unlikely), change the asm-generic/barrier.h to not provide this. > > This _will_ break openrisc builds, they had better clarify their > position by writing their own barrier.h with a comment in. > > Signed-off-by: Peter Zijlstra (Intel)> --- > include/asm-generic/barrier.h | 6 ++ > 1 file changed, 6 insertions(+) Acked-by: Will Deacon One comment below... > diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h > index fe297b599b0a..7a10748615ff 100644 > --- a/include/asm-generic/barrier.h > +++ b/include/asm-generic/barrier.h > @@ -30,9 +30,15 @@ > * Fall back to compiler barriers if nothing better is provided. > */ > > +#ifndef CONFIG_SMP > +/* > + * If your SMP architecture really is Sequentially Consistent, you get > + * barrier.h to write a nice big comment about it. > + */ I couldn't find any documentation about the openrisc memory model other than: https://openrisc.io/or1k.html#__RefHeading__504775_595890882 which talks about the WOM bit in the page table being used to select a weak memory model for the page being mapped as opposed to the strong memory model. Furthermore, the only fence instruction (l.msync) is permitted to be a NOP for the strong model, which implies that the strong model is SC and things like write buffers are forbidden. However, Google suggests that there are implementations of openrisc with write buffers so I'm not sure I understand what's going on here (maybe they force WOM and/or l.msync isn't actually a NOP?) Will
Re: asm-generic: Disallow no-op mb() for SMP systems
On Wed, Jan 31, 2018 at 02:00:34PM +0100, Peter Zijlstra wrote: > > While looking through the qspinlock users, I stumbled upon openrisc and > being curious, I wanted to have a look at its memory model. > > To my surprise it doesn't have asm/barrier.h. It does however support > SMP. This lead me to wonder why it would compile, turns out we provide a > no-op mb() if the architecture does not provide one. > > With the obvious exception of Sequentially Consistent SMP systems, this > is terribly broken. And seeing how SC SMP is really rather unusual (and > unlikely), change the asm-generic/barrier.h to not provide this. > > This _will_ break openrisc builds, they had better clarify their > position by writing their own barrier.h with a comment in. > > Signed-off-by: Peter Zijlstra (Intel) > --- > include/asm-generic/barrier.h | 6 ++ > 1 file changed, 6 insertions(+) Acked-by: Will Deacon One comment below... > diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h > index fe297b599b0a..7a10748615ff 100644 > --- a/include/asm-generic/barrier.h > +++ b/include/asm-generic/barrier.h > @@ -30,9 +30,15 @@ > * Fall back to compiler barriers if nothing better is provided. > */ > > +#ifndef CONFIG_SMP > +/* > + * If your SMP architecture really is Sequentially Consistent, you get > + * barrier.h to write a nice big comment about it. > + */ I couldn't find any documentation about the openrisc memory model other than: https://openrisc.io/or1k.html#__RefHeading__504775_595890882 which talks about the WOM bit in the page table being used to select a weak memory model for the page being mapped as opposed to the strong memory model. Furthermore, the only fence instruction (l.msync) is permitted to be a NOP for the strong model, which implies that the strong model is SC and things like write buffers are forbidden. However, Google suggests that there are implementations of openrisc with write buffers so I'm not sure I understand what's going on here (maybe they force WOM and/or l.msync isn't actually a NOP?) Will