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

[Michael S. Tsirkin]

On Wed, Dec 23, 2009 at 11:36:34AM +0000, Paul Brook wrote:
> > > I mean have a single function that does both the atomic load/store and
> > > the memory barrier. Instead of:
> > >
> > >   stw_phys(addr, val)
> > >   barrier();
> > >
> > > We do:
> > >
> > >   stw_phys_barrier(addr, val).
> > 
> > Well, I think it's a good idea to use Linux APIs instead of
> > inventing our own. A lot of people are familiar with them,
> > and there is decent documentation written.
> 
> Where is this documentation?

For barriers? In linux. E.g. look here:
http://www.mjmwired.net/kernel/Documentation/memory-barriers.txt


> Where does it say that stw_phys is atomic?
> 
> By my reading stw_phys is implemented using memcpy. This means that it is 
> almost certainly not atomic.  My guess is that this works entirely by chance, 
> because the window for observing that race condition on cached SMP systems is 
> exceedingly small.

Good point. Also, it is very rare in virtio to change the high byte,
which makes the window even smaller.  So yes, I suspect it's another
bug in virtio, unrelated to the barrier issue. Thanks for pointing it
out!

> > In the example above, the name does not make it clear
> > whether the barrier is before or after the store?
> 
> As a first implementation, both. I doubt we're really that sensitive to 
> performance. If we *are* that performance sensitive then I'm pretty sure some 
> architectures can do ordering of individual loads/stores, which are more 
> efficient than a global barrier.
> 
> > I think this demonstrates why it's a good idea to stick to Linux
> > standard.
> 
> qemu has restrictions and requirements that don't exist inside a linux kernel.

Well, this patch just adds a library to solve memory ordering problem.
Since memory ordering is a tricky field, I think using a well understood
and documented API as a base is very important, and at least for virtio,
it seems to fit the bill.  I agree we can layer qemu code on top of this
library.  Before we do, let's see more than just virtio use it so we
know which kind of abstractions make sense.  Makes sense?

> > > This avoids issues in the future (multithreaded TCG) where atomic memory
> > > accesses may be nontrivial.
> > 
> > Unfortunately I have no real idea how this will work and what the issues
> > are. I speculate stw_phys, on host platforms that can not write 2 bytes
> > atomically, will need to take some lock? 
> 
> Yes. As mentioned above, I don't believe stw_phys is atomic on any platform.

That's another bug in virtio then :)

> > 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.
> 
> Paul

So let them. What's the issue?

-- 
MST