asm-generic: Disallow no-op mb() for SMP systems

asm-generic: Disallow no-op mb() for SMP systems

[Peter Zijlstra]

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) <peterz@infradead.org>
---
 include/asm-generic/barrier.h | 6 ++++++
 1 file changed, 6 insertions(+)

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.
+ */
 #ifndef mb
 #define mb()	barrier()
 #endif
+#endif
 
 #ifndef rmb
 #define rmb()	mb()

Re: asm-generic: Disallow no-op mb() for SMP systems

[Will Deacon]

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) <peterz@infradead.org>
> ---
>  include/asm-generic/barrier.h | 6 ++++++
>  1 file changed, 6 insertions(+)

Acked-by: Will Deacon <will.deacon@arm.com>

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

[Peter Zijlstra]

On Wed, Jan 31, 2018 at 01:17:37PM +0000, 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) <peterz@infradead.org>
> > ---
> >  include/asm-generic/barrier.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> 
> Acked-by: Will Deacon <will.deacon@arm.com>
> 
> 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

[Stafford Horne]

On Wed, Jan 31, 2018 at 02:26:10PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 31, 2018 at 01:17:37PM +0000, 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) <peterz@infradead.org>
> > > ---
> > >  include/asm-generic/barrier.h | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > 
> > Acked-by: Will Deacon <will.deacon@arm.com>
> > 
> > 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

[Peter Zijlstra]

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

[Will Deacon]

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

[Peter Zijlstra]

On Thu, Feb 01, 2018 at 01:32:30PM +0000, 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

[Will Deacon]

On Thu, Feb 01, 2018 at 02:53:29PM +0100, Peter Zijlstra wrote:
> On Thu, Feb 01, 2018 at 01:32:30PM +0000, 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

[Peter Zijlstra]

On Thu, Feb 01, 2018 at 03:39:51PM +0000, 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

[Peter Zijlstra]

On Thu, Feb 01, 2018 at 04:50:07PM +0100, Peter Zijlstra wrote:
> On Thu, Feb 01, 2018 at 03:39:51PM +0000, 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

[Stafford Horne]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3964 bytes --]

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 <mst@redhat.com>
:::::: CC: Michael S. Tsirkin <mst@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 9403 bytes --]

Re: asm-generic: Disallow no-op mb() for SMP systems

[Peter Zijlstra]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3211 bytes --]

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 <mst@redhat.com>
:::::: CC: Michael S. Tsirkin <mst@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 8975 bytes --]

Re: asm-generic: Disallow no-op mb() for SMP systems

[Peter Zijlstra]

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

[Alan Cox]

On Fri, 2 Feb 2018 21:25:49 +0100
Peter Zijlstra <peterz@infradead.org> 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

[Peter Zijlstra]

On Fri, Feb 02, 2018 at 09:08:20PM +0000, Alan Cox wrote:
> On Fri, 2 Feb 2018 21:25:49 +0100
> Peter Zijlstra <peterz@infradead.org> 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 :-)