Re: [RFC] virtio: use mandatory barriers for remote processor vdevs

[Amos Kong]

On 12/12/2011 01:12 PM, Rusty Russell wrote:
> On Mon, 12 Dec 2011 11:06:53 +0800, Amos Kong <akong@xxxxxxxxxx> wrote:
>> On 12/12/11 06:27, Benjamin Herrenschmidt wrote:
>>> On Sun, 2011-12-11 at 14:25 +0200, Michael S. Tsirkin wrote:
>>>
>>>> Forwarding some results by Amos, who run multiple netperf streams in
>>>> parallel, from an external box to the guest.  TCP_STREAM results were
>>>> noisy.  This could be due to buffering done by TCP, where packet size
>>>> varies even as message size is constant.
>>>>
>>>> TCP_RR results were consistent. In this benchmark, after switching
>>>> to mandatory barriers, CPU utilization increased by up to 35% while
>>>> throughput went down by up to 14%. the normalized throughput/cpu
>>>> regressed consistently, between 7 and 35%
>>>>
>>>> The "fix" applied was simply this:
>>>
>>> What machine&  processor was this  ?
>>
>> pined guest memory to numa node 1
> 
> Please try this patch.  How much does the branch cost us?

Ok, I will provide the result later.

Thanks,
Amos

> (Compiles, untested).
> 
> Thanks,
> Rusty.
> 
> From: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> Subject: virtio: harsher barriers for virtio-mmio.
> 
> We were cheating with our barriers; using the smp ones rather than the
> real device ones.  That was fine, until virtio-mmio came along, which
> could be talking to a real device (a non-SMP CPU).
> 
> Unfortunately, just putting back the real barriers (reverting
> d57ed95d) causes a performance regression on virtio-pci.  In
> particular, Amos reports netbench's TCP_RR over virtio_net CPU
> utilization increased up to 35% while throughput went down by up to
> 14%.
> 
> By comparison, this branch costs us???
> 
> Reference: https://lkml.org/lkml/2011/12/11/22
> 
> Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> ---
>  drivers/lguest/lguest_device.c |   10 ++++++----
>  drivers/s390/kvm/kvm_virtio.c  |    2 +-
>  drivers/virtio/virtio_mmio.c   |    7 ++++---
>  drivers/virtio/virtio_pci.c    |    4 ++--
>  drivers/virtio/virtio_ring.c   |   34 +++++++++++++++++++++-------------
>  include/linux/virtio_ring.h    |    1 +
>  tools/virtio/linux/virtio.h    |    1 +
>  tools/virtio/virtio_test.c     |    3 ++-
>  8 files changed, 38 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
> --- a/drivers/lguest/lguest_device.c
> +++ b/drivers/lguest/lguest_device.c
> @@ -291,11 +291,13 @@ static struct virtqueue *lg_find_vq(stru
>  	}
>  
>  	/*
> -	 * OK, tell virtio_ring.c to set up a virtqueue now we know its size
> -	 * and we've got a pointer to its pages.
> +	 * OK, tell virtio_ring.c to set up a virtqueue now we know its size
> +	 * and we've got a pointer to its pages.  Note that we set weak_barriers
> +	 * to 'true': the host just a(nother) SMP CPU, so we only need inter-cpu
> +	 * barriers.
>  	 */
> -	vq = vring_new_virtqueue(lvq->config.num, LGUEST_VRING_ALIGN,
> -				 vdev, lvq->pages, lg_notify, callback, name);
> +	vq = vring_new_virtqueue(lvq->config.num, LGUEST_VRING_ALIGN, vdev,
> +				 true, lvq->pages, lg_notify, callback, name);
>  	if (!vq) {
>  		err = -ENOMEM;
>  		goto unmap;
> diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
> --- a/drivers/s390/kvm/kvm_virtio.c
> +++ b/drivers/s390/kvm/kvm_virtio.c
> @@ -198,7 +198,7 @@ static struct virtqueue *kvm_find_vq(str
>  		goto out;
>  
>  	vq = vring_new_virtqueue(config->num, KVM_S390_VIRTIO_RING_ALIGN,
> -				 vdev, (void *) config->address,
> +				 vdev, true, (void *) config->address,
>  				 kvm_notify, callback, name);
>  	if (!vq) {
>  		err = -ENOMEM;
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -309,9 +309,10 @@ static struct virtqueue *vm_setup_vq(str
>  	writel(virt_to_phys(info->queue) >> PAGE_SHIFT,
>  			vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
>  
> -	/* Create the vring */
> -	vq = vring_new_virtqueue(info->num, VIRTIO_MMIO_VRING_ALIGN,
> -				 vdev, info->queue, vm_notify, callback, name);
> +	/* Create the vring: no weak barriers, the other side is could
> +	 * be an independent "device". */
> +	vq = vring_new_virtqueue(info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,
> +				 false, info->queue, vm_notify, callback, name);
>  	if (!vq) {
>  		err = -ENOMEM;
>  		goto error_new_virtqueue;
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -414,8 +414,8 @@ static struct virtqueue *setup_vq(struct
>  		  vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
>  
>  	/* create the vring */
> -	vq = vring_new_virtqueue(info->num, VIRTIO_PCI_VRING_ALIGN,
> -				 vdev, info->queue, vp_notify, callback, name);
> +	vq = vring_new_virtqueue(info->num, VIRTIO_PCI_VRING_ALIGN, vdev,
> +				 true, info->queue, vp_notify, callback, name);
>  	if (!vq) {
>  		err = -ENOMEM;
>  		goto out_activate_queue;
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -28,17 +28,20 @@
>  #ifdef CONFIG_SMP
>  /* Where possible, use SMP barriers which are more lightweight than mandatory
>   * barriers, because mandatory barriers control MMIO effects on accesses
> - * through relaxed memory I/O windows (which virtio does not use). */
> -#define virtio_mb() smp_mb()
> -#define virtio_rmb() smp_rmb()
> -#define virtio_wmb() smp_wmb()
> + * through relaxed memory I/O windows (which virtio-pci does not use). */
> +#define virtio_mb(vq) \
> +	do { if ((vq)->weak_barriers) smp_mb(); else mb(); } while(0)
> +#define virtio_rmb(vq) \
> +	do { if ((vq)->weak_barriers) smp_rmb(); else rmb(); } while(0)
> +#define virtio_wmb(vq) \
> +	do { if ((vq)->weak_barriers) smp_rmb(); else rmb(); } while(0)
>  #else
>  /* We must force memory ordering even if guest is UP since host could be
>   * running on another CPU, but SMP barriers are defined to barrier() in that
>   * configuration. So fall back to mandatory barriers instead. */
> -#define virtio_mb() mb()
> -#define virtio_rmb() rmb()
> -#define virtio_wmb() wmb()
> +#define virtio_mb(vq) mb()
> +#define virtio_rmb(vq) rmb()
> +#define virtio_wmb(vq) wmb()
>  #endif
>  
>  #ifdef DEBUG
> @@ -77,6 +80,9 @@ struct vring_virtqueue
>  	/* Actual memory layout for this queue */
>  	struct vring vring;
>  
> +	/* Can we use weak barriers? */
> +	bool weak_barriers;
> +
>  	/* Other side has made a mess, don't try any more. */
>  	bool broken;
>  
> @@ -245,14 +251,14 @@ void virtqueue_kick(struct virtqueue *_v
>  	START_USE(vq);
>  	/* Descriptors and available array need to be set before we expose the
>  	 * new available array entries. */
> -	virtio_wmb();
> +	virtio_wmb(vq);
>  
>  	old = vq->vring.avail->idx;
>  	new = vq->vring.avail->idx = old + vq->num_added;
>  	vq->num_added = 0;
>  
>  	/* Need to update avail index before checking if we should notify */
> -	virtio_mb();
> +	virtio_mb(vq);
>  
>  	if (vq->event ?
>  	    vring_need_event(vring_avail_event(&vq->vring), new, old) :
> @@ -314,7 +320,7 @@ void *virtqueue_get_buf(struct virtqueue
>  	}
>  
>  	/* Only get used array entries after they have been exposed by host. */
> -	virtio_rmb();
> +	virtio_rmb(vq);
>  
>  	i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
>  	*len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
> @@ -337,7 +343,7 @@ void *virtqueue_get_buf(struct virtqueue
>  	 * the read in the next get_buf call. */
>  	if (!(vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)) {
>  		vring_used_event(&vq->vring) = vq->last_used_idx;
> -		virtio_mb();
> +		virtio_mb(vq);
>  	}
>  
>  	END_USE(vq);
> @@ -366,7 +372,7 @@ bool virtqueue_enable_cb(struct virtqueu
>  	 * entry. Always do both to keep code simple. */
>  	vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
>  	vring_used_event(&vq->vring) = vq->last_used_idx;
> -	virtio_mb();
> +	virtio_mb(vq);
>  	if (unlikely(more_used(vq))) {
>  		END_USE(vq);
>  		return false;
> @@ -393,7 +399,7 @@ bool virtqueue_enable_cb_delayed(struct 
>  	/* TODO: tune this threshold */
>  	bufs = (u16)(vq->vring.avail->idx - vq->last_used_idx) * 3 / 4;
>  	vring_used_event(&vq->vring) = vq->last_used_idx + bufs;
> -	virtio_mb();
> +	virtio_mb(vq);
>  	if (unlikely((u16)(vq->vring.used->idx - vq->last_used_idx) > bufs)) {
>  		END_USE(vq);
>  		return false;
> @@ -453,6 +459,7 @@ EXPORT_SYMBOL_GPL(vring_interrupt);
>  struct virtqueue *vring_new_virtqueue(unsigned int num,
>  				      unsigned int vring_align,
>  				      struct virtio_device *vdev,
> +				      bool weak_barriers,
>  				      void *pages,
>  				      void (*notify)(struct virtqueue *),
>  				      void (*callback)(struct virtqueue *),
> @@ -476,6 +483,7 @@ struct virtqueue *vring_new_virtqueue(un
>  	vq->vq.vdev = vdev;
>  	vq->vq.name = name;
>  	vq->notify = notify;
> +	vq->weak_barriers = weak_barriers;
>  	vq->broken = false;
>  	vq->last_used_idx = 0;
>  	vq->num_added = 0;
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -168,6 +168,7 @@ struct virtqueue;
>  struct virtqueue *vring_new_virtqueue(unsigned int num,
>  				      unsigned int vring_align,
>  				      struct virtio_device *vdev,
> +				      bool weak_barriers,
>  				      void *pages,
>  				      void (*notify)(struct virtqueue *vq),
>  				      void (*callback)(struct virtqueue *vq),
> diff --git a/tools/virtio/linux/virtio.h b/tools/virtio/linux/virtio.h
> --- a/tools/virtio/linux/virtio.h
> +++ b/tools/virtio/linux/virtio.h
> @@ -214,6 +214,7 @@ void *virtqueue_detach_unused_buf(struct
>  struct virtqueue *vring_new_virtqueue(unsigned int num,
>  				      unsigned int vring_align,
>  				      struct virtio_device *vdev,
> +				      bool weak_barriers,
>  				      void *pages,
>  				      void (*notify)(struct virtqueue *vq),
>  				      void (*callback)(struct virtqueue *vq),
> diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c
> --- a/tools/virtio/virtio_test.c
> +++ b/tools/virtio/virtio_test.c
> @@ -92,7 +92,8 @@ static void vq_info_add(struct vdev_info
>  	assert(r >= 0);
>  	memset(info->ring, 0, vring_size(num, 4096));
>  	vring_init(&info->vring, num, info->ring, 4096);
> -	info->vq = vring_new_virtqueue(info->vring.num, 4096, &dev->vdev, info->ring,
> +	info->vq = vring_new_virtqueue(info->vring.num, 4096, &dev->vdev,
> +				       true, info->ring,
>  				       vq_notify, vq_callback, "test");
>  	assert(info->vq);
>  	info->vq->priv = info;
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/