[RFC,0/4] barriers using data dependency
mbox series

Message ID 20190102205715.14054-1-mst@redhat.com
Headers show
Series
  • barriers using data dependency
Related show

Message

Michael S. Tsirkin Jan. 2, 2019, 8:57 p.m. UTC
So as explained in Documentation/memory-barriers.txt e.g.
a load followed by a store require a full memory barrier,
to avoid store being ordered before the load.
Similarly load-load requires a read memory barrier.

Thinking about it, we can actually create a data dependency
by mixing the first loaded value into the pointer being
accessed.

This adds an API for this and uses it in virtio.

Written over the holiday and build tested only so far.

This patchset is also suboptimal on e.g. x86 where e.g. smp_rmb is a nop.

Sending out for early feedback/flames.

Michael S. Tsirkin (4):
  include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR
  include/linux/compiler.h: allow memory operands
  barriers: convert a control to a data dependency
  virtio: use dependent_ptr_mb

 Documentation/memory-barriers.txt | 20 ++++++++++++++++++++
 arch/alpha/include/asm/barrier.h  |  1 +
 drivers/virtio/virtio_ring.c      |  6 ++++--
 include/asm-generic/barrier.h     | 18 ++++++++++++++++++
 include/linux/compiler-clang.h    |  5 ++---
 include/linux/compiler-gcc.h      |  4 ----
 include/linux/compiler-intel.h    |  4 +---
 include/linux/compiler.h          |  8 +++++++-
 8 files changed, 53 insertions(+), 13 deletions(-)

Comments

Alan Stern Jan. 2, 2019, 9:36 p.m. UTC | #1
On Wed, 2 Jan 2019, Michael S. Tsirkin wrote:

> So as explained in Documentation/memory-barriers.txt e.g.
> a load followed by a store require a full memory barrier,
> to avoid store being ordered before the load.
> Similarly load-load requires a read memory barrier.
> 
> Thinking about it, we can actually create a data dependency
> by mixing the first loaded value into the pointer being
> accessed.
> 
> This adds an API for this and uses it in virtio.
> 
> Written over the holiday and build tested only so far.

You are using the terminology from memory-barriers.txt, referring to
the new dependency you create as a data dependency.  However,
tools/memory-model/* uses a more precise name, calling it an address
dependency.  Could you change the comments in the patches to use this
name instead?

> This patchset is also suboptimal on e.g. x86 where e.g. smp_rmb is a nop.

This should be easy to fix with an architecture-specific override.

Alan Stern

> Sending out for early feedback/flames.
> 
> Michael S. Tsirkin (4):
>   include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR
>   include/linux/compiler.h: allow memory operands
>   barriers: convert a control to a data dependency
>   virtio: use dependent_ptr_mb
> 
>  Documentation/memory-barriers.txt | 20 ++++++++++++++++++++
>  arch/alpha/include/asm/barrier.h  |  1 +
>  drivers/virtio/virtio_ring.c      |  6 ++++--
>  include/asm-generic/barrier.h     | 18 ++++++++++++++++++
>  include/linux/compiler-clang.h    |  5 ++---
>  include/linux/compiler-gcc.h      |  4 ----
>  include/linux/compiler-intel.h    |  4 +---
>  include/linux/compiler.h          |  8 +++++++-
>  8 files changed, 53 insertions(+), 13 deletions(-)
Michael S. Tsirkin Jan. 2, 2019, 11:04 p.m. UTC | #2
On Wed, Jan 02, 2019 at 04:36:40PM -0500, Alan Stern wrote:
> On Wed, 2 Jan 2019, Michael S. Tsirkin wrote:
> 
> > So as explained in Documentation/memory-barriers.txt e.g.
> > a load followed by a store require a full memory barrier,
> > to avoid store being ordered before the load.
> > Similarly load-load requires a read memory barrier.
> > 
> > Thinking about it, we can actually create a data dependency
> > by mixing the first loaded value into the pointer being
> > accessed.
> > 
> > This adds an API for this and uses it in virtio.
> > 
> > Written over the holiday and build tested only so far.
> 
> You are using the terminology from memory-barriers.txt, referring to
> the new dependency you create as a data dependency.  However,
> tools/memory-model/* uses a more precise name, calling it an address
> dependency.  Could you change the comments in the patches to use this
> name instead?

Sure, sounds good. While I'm at it, should memory-barriers.txt be
switched over too?

> > This patchset is also suboptimal on e.g. x86 where e.g. smp_rmb is a nop.
> 
> This should be easy to fix with an architecture-specific override.
> 
> Alan Stern

Absolutely. It does however mean that we'll need several
variants: mb/rmb, smp/dma/virt/mandatory.

I am still trying to decide whether it's good since it documents the
kind of barrier that we are trying to use - or bad since it's more
verbose and makes you choose one where they are all pretty cheap.

> > Sending out for early feedback/flames.
> > 
> > Michael S. Tsirkin (4):
> >   include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR
> >   include/linux/compiler.h: allow memory operands
> >   barriers: convert a control to a data dependency
> >   virtio: use dependent_ptr_mb
> > 
> >  Documentation/memory-barriers.txt | 20 ++++++++++++++++++++
> >  arch/alpha/include/asm/barrier.h  |  1 +
> >  drivers/virtio/virtio_ring.c      |  6 ++++--
> >  include/asm-generic/barrier.h     | 18 ++++++++++++++++++
> >  include/linux/compiler-clang.h    |  5 ++---
> >  include/linux/compiler-gcc.h      |  4 ----
> >  include/linux/compiler-intel.h    |  4 +---
> >  include/linux/compiler.h          |  8 +++++++-
> >  8 files changed, 53 insertions(+), 13 deletions(-)
Alan Stern Jan. 3, 2019, 3:11 p.m. UTC | #3
On Wed, 2 Jan 2019, Michael S. Tsirkin wrote:

> On Wed, Jan 02, 2019 at 04:36:40PM -0500, Alan Stern wrote:
> > On Wed, 2 Jan 2019, Michael S. Tsirkin wrote:
> > 
> > > So as explained in Documentation/memory-barriers.txt e.g.
> > > a load followed by a store require a full memory barrier,
> > > to avoid store being ordered before the load.
> > > Similarly load-load requires a read memory barrier.
> > > 
> > > Thinking about it, we can actually create a data dependency
> > > by mixing the first loaded value into the pointer being
> > > accessed.
> > > 
> > > This adds an API for this and uses it in virtio.
> > > 
> > > Written over the holiday and build tested only so far.
> > 
> > You are using the terminology from memory-barriers.txt, referring to
> > the new dependency you create as a data dependency.  However,
> > tools/memory-model/* uses a more precise name, calling it an address
> > dependency.  Could you change the comments in the patches to use this
> > name instead?
> 
> Sure, sounds good. While I'm at it, should memory-barriers.txt be
> switched over too?

If you want to take care of that, great!  I never seem to get around to 
doing it.

> > > This patchset is also suboptimal on e.g. x86 where e.g. smp_rmb is a nop.
> > 
> > This should be easy to fix with an architecture-specific override.
> > 
> > Alan Stern
> 
> Absolutely. It does however mean that we'll need several
> variants: mb/rmb, smp/dma/virt/mandatory.
> 
> I am still trying to decide whether it's good since it documents the
> kind of barrier that we are trying to use - or bad since it's more
> verbose and makes you choose one where they are all pretty cheap.

How many places can these things be used?  My guess is not very many,
or at least, there aren't very many different _types_ of usage.  So
start only with variants you know will be used.  More can be added
later if we want.

Alan Stern