[Qemu-devel] Re: [PATCH-RFC 0/3] qemu: memory barriers in virtio

[Michael S. Tsirkin]

On Wed, Dec 23, 2009 at 12:25:46PM +0000, Paul Brook wrote:
> > > > So possibly this means that we
> > > > could optimize the barrier away, but I don't think this amounts to a
> > > > serious issue, I guess portability/readability is more important.
> > >
> > > The more important issue is that regular devices which to not require
> > > coherency or ordering can omit this lock.
> > >
> > So let them. What's the issue?
> 
> The issue is that at you're only fixing half the problem. Barriers aren't 
> sufficient to get the semantics you need. You also need atomicity.
> 
> Given we need both, why not actually defined an API that gives you this?

Because, I do not want to define APIs, I want to reuse an existing one.
E.g. what is stw_barrier?  atomic read followed by a barrier?  read
preceded by a barrier? and what kind of barrier? IMO
load
rmb()
is clearer than load_barrier()
as it makes it obvious *where* the barrier is.

> As shown with the stw issue, following the linux API is liable to introduce 
> subtle bugs that don't exist in a native kernel environment.

I don't think this shows this.  Linux does ring->idx++, qemu did memcpy
instead. We should just switch to doing what linux does, and keeping the
code similar will make it easier to reason about.

So, we should probably replace memcpy in stw with *p = v,
just in case someone wants to build qemu without -O.
But it won't be an issue for any real user.

> By far the simplest solution is to provide atomic, strictly ordered access to 
> guest memory.

virtio is very simple. It is  mostly doing things like:

        read index
        if index changed
        do all kind of reads

and the only point where we need a barrier
is after we check the index.

With stores:

        do all kind of writes
        increment index

so we only need a write barrier before index increment.

Anything else is overkill.

> Anything else adds significant complication[1],

Is this [1] a reference?

For reference, my patch adds 2 missing barriers in virtio,
it is a whole of:

 virtio.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

this is with comments.

> and is purely a performance optimization.  I'm not convinced that
> partial barriers are worth the effort.
> 
> Paul

Who's effort are you sparing?  For me it's much less effort to pair each
barrier in guest with the symmetrical barrier in host, and follow
existing documentation, than define our own APIs and track down subtle
use issues.

With KVM, even these partial barriers add small but measureable overhead
(about 2%). Modern CPUs just don't work very well without reordering
memory accesses.

-- 
MST