Linux Virtualization - Dec 2015 - [PATCH] virtio_ring: use smp_store_mb

On Thu, Dec 17, 2015 at 02:57:26PM +0100, Peter Zijlstra
wrote:
> On Thu, Dec 17, 2015 at 03:16:20PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Dec 17, 2015 at 11:52:38AM +0100, Peter Zijlstra wrote:
> > > On Thu, Dec 17, 2015 at 12:32:53PM +0200, Michael S. Tsirkin
wrote:
> > > > +static inline void virtio_store_mb(bool weak_barriers,
> > > > +				   __virtio16 *p, __virtio16 v)
> > > > +{
> > > > +#ifdef CONFIG_SMP
> > > > +	if (weak_barriers)
> > > > +		smp_store_mb(*p, v);
> > > > +	else
> > > > +#endif
> > > > +	{
> > > > +		WRITE_ONCE(*p, v);
> > > > +		mb();
> > > > +	}
> > > > +}
> > > 
> > > This is a different barrier depending on SMP, that seems wrong.
> > 
> > Of course it's wrong in the sense that it's
> > suboptimal on UP. What we would really like is to
> > have, on UP, exactly the same barrier as on SMP.
> > This is because a UP guest can run on an SMP host.
> > 
> > But Linux doesn't provide this ability: if CONFIG_SMP is
> > not defined is optimizes most barriers out to a
> > compiler barrier.
> > 
> > Consider for example x86: what we want is xchg (NOT
> > mfence - see below for why) but if built without CONFIG_SMP
> > smp_store_mb does not include this.
> 
> You could of course go fix that instead of mutilating things into
> sort-of functional state.
Yes, we'd just need to touch all architectures, all for
the sake of UP which almost no one uses.
Basically, we need APIs that explicitly are
for talking to another kernel on a different CPU on
the same SMP system, and implemented identically
between CONFIG_SMP and !CONFIG_SMP on all architectures.

Do you think this is something of general usefulness,
outside virtio?
> > 
> > 
> > > smp_mb(), as (should be) used by smp_store_mb() does not provide
a
> > > barrier against IO. mb() otoh does.
> > > 
> > > Since this is virtIO I would expect you always want mb().
> > 
> > No because it's VIRTio not real io :) It just switches to the
hyprevisor
> > mode - kind of like a function call really.
> > The weak_barriers flag is cleared for when it's used
> > with real devices with real IO.
> > 
> > 
> > All this is explained in some detail at the top of
> > include/linux/virtio.h
> 
> I did read that, it didn't make any sense wrt the code below it.
> 
> For instance it seems to imply weak_barriers is for smp like stuff while
> !weak_barriers is for actual devices.
> 
> But then you go use dma_*mb() ops, which are specifially for devices
> only for weak_barrier.
Yes the dma_*mb thing is a kind of an interface misuse.
It's an optimization for UP introduced here:

commit 9e1a27ea42691429e31f158cce6fc61bc79bb2e9
Author: Alexander Duyck <alexander.h.duyck at redhat.com>
Date:   Mon Apr 13 21:03:49 2015 +0930

    virtio_ring: Update weak barriers to use dma_wmb/rmb 
    This change makes it so that instead of using smp_wmb/rmb which varies
    depending on the kernel configuration we can can use dma_wmb/rmb which for
    most architectures should be equal to or slightly more strict than
    smp_wmb/rmb.
    
    The advantage to this is that these barriers are available to uniprocessor
    builds as well so the performance should improve under such a
    configuration.
    
    Signed-off-by: Alexander Duyck <alexander.h.duyck at redhat.com>
    Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>

but given the confusion this causes, I'm inclined to revert
this, and later switch to for cleaner API if that
appears. Cc Alexander (at the new address).


-- 
MST

On Thu, Dec 17, 2015 at 04:33:44PM +0200, Michael S. Tsirkin
wrote:
> On Thu, Dec 17, 2015 at 02:57:26PM +0100, Peter Zijlstra wrote:
> > 
> > You could of course go fix that instead of mutilating things into
> > sort-of functional state.
> 
> Yes, we'd just need to touch all architectures, all for
> the sake of UP which almost no one uses.
> Basically, we need APIs that explicitly are
> for talking to another kernel on a different CPU on
> the same SMP system, and implemented identically
> between CONFIG_SMP and !CONFIG_SMP on all architectures.
> 
> Do you think this is something of general usefulness,
> outside virtio?
I'm not aware of any other case, but if there are more parts of virt
that need this then I see no problem adding it.

That is, virt in general is the only use-case that I can think of,
because this really is an artifact of interfacing with an SMP host while
running an UP kernel.

But I'm really not familiar with virt, so I do not know if there's more
sites outside of virtio that could use this.

Touching all archs is a tad tedious, but its fairly straight forward.

On Thu, Dec 17, 2015 at 03:39:10PM +0100, Peter Zijlstra
wrote:
> On Thu, Dec 17, 2015 at 04:33:44PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Dec 17, 2015 at 02:57:26PM +0100, Peter Zijlstra wrote:
> > > 
> > > You could of course go fix that instead of mutilating things into
> > > sort-of functional state.
> > 
> > Yes, we'd just need to touch all architectures, all for
> > the sake of UP which almost no one uses.
> > Basically, we need APIs that explicitly are
> > for talking to another kernel on a different CPU on
> > the same SMP system, and implemented identically
> > between CONFIG_SMP and !CONFIG_SMP on all architectures.
> > 
> > Do you think this is something of general usefulness,
> > outside virtio?
> 
> I'm not aware of any other case, but if there are more parts of virt
> that need this then I see no problem adding it.
> 
> That is, virt in general is the only use-case that I can think of,
> because this really is an artifact of interfacing with an SMP host while
> running an UP kernel.
> 
> But I'm really not familiar with virt, so I do not know if there's
more
> sites outside of virtio that could use this.
> 
> Touching all archs is a tad tedious, but its fairly straight forward.
Thanks, will take a stub at this.

On Thu, Dec 17, 2015 at 03:39:10PM +0100, Peter Zijlstra
wrote:
> On Thu, Dec 17, 2015 at 04:33:44PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Dec 17, 2015 at 02:57:26PM +0100, Peter Zijlstra wrote:
> > > 
> > > You could of course go fix that instead of mutilating things into
> > > sort-of functional state.
> > 
> > Yes, we'd just need to touch all architectures, all for
> > the sake of UP which almost no one uses.
> > Basically, we need APIs that explicitly are
> > for talking to another kernel on a different CPU on
> > the same SMP system, and implemented identically
> > between CONFIG_SMP and !CONFIG_SMP on all architectures.
> > 
> > Do you think this is something of general usefulness,
> > outside virtio?
> 
> I'm not aware of any other case, but if there are more parts of virt
> that need this then I see no problem adding it.
When I wrote this, I assumed there are no other users, and I'm still not
sure there are other users at the moment. Do you see a problem then?
> That is, virt in general is the only use-case that I can think of,
> because this really is an artifact of interfacing with an SMP host while
> running an UP kernel.
Or another guest kernel on an SMP host.
> But I'm really not familiar with virt, so I do not know if there's
more
> sites outside of virtio that could use this.
> Touching all archs is a tad tedious, but its fairly straight forward.
So I looked and I was only able to find other another possible user in Xen.

Cc Xen folks.

I noticed that drivers/xen/xenbus/xenbus_comms.c uses
full memory barriers to communicate with the other side.
For example:

                /* Must write data /after/ reading the consumer index.  * */
                mb();

                memcpy(dst, data, avail);
                data += avail;
                len -= avail;
        
                /* Other side must not see new producer until data is * there.
*/
                wmb();
                intf->req_prod += avail;
                
                /* Implies mb(): other side will see the updated producer. */
                notify_remote_via_evtchn(xen_store_evtchn);

To me, it looks like for guests compiled with CONFIG_SMP, smp_wmb and smp_mb
would be sufficient, so mb() and wmb() here are only needed if
a non-SMP guest runs on an SMP host.

Is my analysis correct?

So what I'm suggesting is something like the below patch,
except instead of using virtio directly, a new set of barriers
that behaves identically for SMP and non-SMP guests will be introduced.

And of course the weak barriers flag is not needed for Xen -
that's a virtio only thing.

For example:

smp_pv_wmb()
smp_pv_rmb()
smp_pv_mb()

I'd like to get confirmation from Xen folks before posting
this patchset.

Comments/suggestions?

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>

--

compile-tested only.

diff --git a/drivers/xen/xenbus/xenbus_comms.c
b/drivers/xen/xenbus/xenbus_comms.c
index fdb0f33..a28f049 100644
--- a/drivers/xen/xenbus/xenbus_comms.c
+++ b/drivers/xen/xenbus/xenbus_comms.c
@@ -36,6 +36,7 @@
 #include <linux/interrupt.h>
 #include <linux/sched.h>
 #include <linux/err.h>
+#include <linux/virtio_ring.h>
 #include <xen/xenbus.h>
 #include <asm/xen/hypervisor.h>
 #include <xen/events.h>
@@ -123,14 +124,14 @@ int xb_write(const void *data, unsigned len)
 			avail = len;
 
 		/* Must write data /after/ reading the consumer index. */
-		mb();
+		virtio_mb(true);
 
 		memcpy(dst, data, avail);
 		data += avail;
 		len -= avail;
 
 		/* Other side must not see new producer until data is there. */
-		wmb();
+		virtio_wmb(true);
 		intf->req_prod += avail;
 
 		/* Implies mb(): other side will see the updated producer. */
@@ -180,14 +181,14 @@ int xb_read(void *data, unsigned len)
 			avail = len;
 
 		/* Must read data /after/ reading the producer index. */
-		rmb();
+		virtio_rmb(true);
 
 		memcpy(data, src, avail);
 		data += avail;
 		len -= avail;
 
 		/* Other side must not see free space until we've copied out */
-		mb();
+		virtio_mb(true);
 		intf->rsp_cons += avail;
 
 		pr_debug("Finished read of %i bytes (%i to go)\n", avail, len);

-- 
MST