Re: [Qemu-devel] [PATCH] Add a memory barrier to guest memory access functions

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Mon, 2012-05-21 at 11:31 +0300, Michael S. Tsirkin wrote:
> On Mon, May 21, 2012 at 10:11:06AM +0200, Paolo Bonzini wrote:
> > Il 21/05/2012 03:56, Benjamin Herrenschmidt ha scritto:
> > > I don't see an obvious need to provide a "relaxed" variant of the
> > > later at this stage, a quick grep doesn't seem to show that most cases
> > > where it's used are either not performance sensitive or the barrier
> > > makes sense, but feel free to prove me wrong :-)
> > 
> > The only problem here is that you have useless memory barriers when
> > calling cpu_physical_memory_map in a loop (see virtqueue_map_sg).
> > 
> > Paolo
> 
> More than that. smp_mb is pretty expensive. You
> often can do just smp_wmb and smp_rmb and that is
> very cheap.

Except that you mostly don't know at that level what you can or cannot
do, it depends on the caller. We should have the standard accessors do
it the "safe" way and have performance sensitive stuff do map/unmap, at
least that's the result of the discussions with Anthony.

If we can address the virtqueue_map_sg problem, I think we should be
good, I'll look at it tomorrow. Maybe the right way for now is to remove
the barrier I added to "map" and only leave the one in _rw

> Many operations run in the vcpu context
> or start when guest exits to host and work
> is bounced from there and thus no barrier is needed
> here.

But we don't always know it unless we start sprinkling the drivers.

> Example? start_xmit in e1000. Executed in vcpu context
> so no barrier is needed.

If it's performance sensitive, it can always use map/unmap and
hand-tuned barriers. I suspect the cost of the barrier is drowned in the
cost of the exit tho, isn't it ?

Also while it doesn't need barriers for the read it does of the
descriptor it still needs one barrier for the write back to it.

> virtio of course is another example since it does its own
> barriers. But even without that, virtio_blk_handle_output
> runs in vcpu context.
> 
> But more importantly, this hack just sweeps the
> dirt under the carpet. Understanding the interaction
> with guest drivers is important anyway. So
> I really don't see why don't we audit devices
> and add proper barriers.

Because my experience is that you'll never get them all right and will
keep getting more breakage as devices are added. This has always been
the case kernel wise and I don't think qemu will get it any better.

That's why Linus always insisted that our basic MMIO accessors provide
full ordering vs. each other and vs. accesses to main memory (DMA)
limiting the extent of the barriers needed for drivers for example.

It's a small price to pay considered the risk and how nasty those bugs
are to debug. And performance critical drivers are few and can be hand
tuned.

Cheers,
Ben.