Re: [PATCH v2 15/32] powerpc: define __smp_xxx

["Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Wed, Jan 06, 2016 at 09:51:52AM +0800, Boqun Feng wrote:
> On Tue, Jan 05, 2016 at 06:16:48PM +0200, Michael S. Tsirkin wrote:
> [snip]
> > > > > Another thing is that smp_lwsync() may have a third user(other than
> > > > > smp_load_acquire() and smp_store_release()):
> > > > > 
> > > > > http://article.gmane.org/gmane.linux.ports.ppc.embedded/89877
> > > > > 
> > > > > I'm OK to change my patch accordingly, but do we really want
> > > > > smp_lwsync() get involved in this cleanup? If I understand you
> > > > > correctly, this cleanup focuses on external API like smp_{r,w,}mb(),
> > > > > while smp_lwsync() is internal to PPC.
> > > > > 
> > > > > Regards,
> > > > > Boqun
> > > > 
> > > > I think you missed the leading ___ :)
> > > > 
> > > 
> > > What I mean here was smp_lwsync() was originally internal to PPC, but
> > > never mind ;-)
> > > 
> > > > smp_store_release is external and it needs __smp_lwsync as
> > > > defined here.
> > > > 
> > > > I can duplicate some code and have smp_lwsync *not* call __smp_lwsync
> > > 
> > > You mean bringing smp_lwsync() back? because I haven't seen you defining
> > > in asm-generic/barriers.h in previous patches and you just delete it in
> > > this patch.
> > > 
> > > > but why do this? Still, if you prefer it this way,
> > > > please let me know.
> > > > 
> > > 
> > > I think deleting smp_lwsync() is fine, though I need to change atomic
> > > variants patches on PPC because of it ;-/
> > > 
> > > Regards,
> > > Boqun
> > 
> > Sorry, I don't understand - why do you have to do anything?
> > I changed all users of smp_lwsync so they
> > use __smp_lwsync on SMP and barrier() on !SMP.
> > 
> > This is exactly the current behaviour, I also tested that
> > generated code does not change at all.
> > 
> > Is there a patch in your tree that conflicts with this?
> > 
> 
> Because in a patchset which implements atomic relaxed/acquire/release
> variants on PPC I use smp_lwsync(), this makes it have another user,
> please see this mail:
> 
> http://article.gmane.org/gmane.linux.ports.ppc.embedded/89877
> 
> in definition of PPC's __atomic_op_release().
> 
> 
> But I think removing smp_lwsync() is a good idea and actually I think we
> can go further to remove __smp_lwsync() and let __smp_load_acquire and
> __smp_store_release call __lwsync() directly, but that is another thing.
> 
> Anyway, I will modify my patch.
> 
> Regards,
> Boqun


Thanks!
Could you send an ack then please?

> > 
> > > > > >  	WRITE_ONCE(*p, v);						\
> > > > > >  } while (0)
> > > > > >  
> > > > > > -#define smp_load_acquire(p)						\
> > > > > > +#define __smp_load_acquire(p)						\
> > > > > >  ({									\
> > > > > >  	typeof(*p) ___p1 = READ_ONCE(*p);				\
> > > > > >  	compiletime_assert_atomic_type(*p);				\
> > > > > > -	smp_lwsync();							\
> > > > > > +	__smp_lwsync();							\
> > > > > >  	___p1;								\
> > > > > >  })
> > > > > >  
> > > > > > -- 
> > > > > > MST
> > > > > > 
> > > > > > --
> > > > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > > > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > > > Please read the FAQ at  http://www.tux.org/lkml/