Re: Merge of "virtio_net: fix race in RX VQ processing" for linux-3.2.48

["Michael S. Tsirkin" <mst@redhat.com>]

On Sat, Jul 27, 2013 at 03:42:53PM +0100, Ben Hutchings wrote:
> On Fri, 2013-07-12 at 23:13 +0200, Wolfram Gloger wrote:
> > Hi,
> > 
> > Today I merged M. Tsirkin's patch v2 "virtio_net: fix race in RX VQ
> > processing":
> > 
> > http://lkml.indiana.edu/hypermail/linux/kernel/1307.1/00503.html
> > 
> > into 3.2.48 and lightly tested the result (vhost-net, virtio-disk
> > and 9pfs using virtio).
> 
> This sounds like it could be suitable for stable, but that doesn't seem
> to have been requested by the author.

It was in the cover letter:
"Please review, and consider for 3.11 and stable."

> I'm cc'ing those involved so they
> can make a decision whether this should be included in 3.2.y or other
> stable branches.
> > Merge is not completely trivial, so might save you some time, if only
> > for a comparison that you arrive at the same result.
> 
> Thanks, but these are not in quite the right patch format.  The
> filenames should always have a leading directory name which will be
> stripped (patch -p1).  The original patch header must be preserved and a
> reference to the upstream commit inserted e.g.
> 'commit 0123456789abcdef0123456789abcdef012345678 upstream.'
> 
> (Attachments copied for reference.)
> 
> Ben.
> 
> -- 
> Ben Hutchings
> Once a job is fouled up, anything done to improve it makes it worse.
> 

> --- drivers/virtio/virtio_ring.c.orig	2013-06-30 11:35:44.000000000 +0200
> +++ drivers/virtio/virtio_ring.c	2013-07-12 15:18:48.000000000 +0200
> @@ -360,9 +360,22 @@ void virtqueue_disable_cb(struct virtque
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_disable_cb);
>  
> -bool virtqueue_enable_cb(struct virtqueue *_vq)
> +/**
> + * virtqueue_enable_cb_prepare - restart callbacks after disable_cb
> + * @vq: the struct virtqueue we're talking about.
> + *
> + * This re-enables callbacks; it returns current queue state
> + * in an opaque unsigned value. This value should be later tested by
> + * virtqueue_poll, to detect a possible race between the driver checking for
> + * more work, and enabling callbacks.
> + *
> + * Caller must ensure we don't call this with other virtqueue
> + * operations at the same time (except where noted).
> + */
> +unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
>  {
>  	struct vring_virtqueue *vq = to_vvq(_vq);
> +	u16 last_used_idx;
>  
>  	START_USE(vq);
>  
> @@ -372,15 +385,45 @@ bool virtqueue_enable_cb(struct virtqueu
>  	 * either clear the flags bit or point the event index at the next
>  	 * 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;
> +	vring_used_event(&vq->vring) = last_used_idx = vq->last_used_idx;
> +	END_USE(vq);
> +	return last_used_idx;
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_enable_cb_prepare);
> +
> +/**
> + * virtqueue_poll - query pending used buffers
> + * @vq: the struct virtqueue we're talking about.
> + * @last_used_idx: virtqueue state (from call to virtqueue_enable_cb_prepare).
> + *
> + * Returns "true" if there are pending used buffers in the queue.
> + *
> + * This does not need to be serialized.
> + */
> +bool virtqueue_poll(struct virtqueue *_vq, unsigned last_used_idx)
> +{
> +	struct vring_virtqueue *vq = to_vvq(_vq);
> +
>  	virtio_mb();
> -	if (unlikely(more_used(vq))) {
> -		END_USE(vq);
> -		return false;
> -	}
> +	return (u16)last_used_idx != vq->vring.used->idx;
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_poll);
>  
> -	END_USE(vq);
> -	return true;
> +/**
> + * virtqueue_enable_cb - restart callbacks after disable_cb.
> + * @vq: the struct virtqueue we're talking about.
> + *
> + * This re-enables callbacks; it returns "false" if there are pending
> + * buffers in the queue, to detect a possible race between the driver
> + * checking for more work, and enabling callbacks.
> + *
> + * Caller must ensure we don't call this with other virtqueue
> + * operations at the same time (except where noted).
> + */
> +bool virtqueue_enable_cb(struct virtqueue *_vq)
> +{
> +	unsigned last_used_idx = virtqueue_enable_cb_prepare(_vq);
> +	return !virtqueue_poll(_vq, last_used_idx);
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_enable_cb);
>  
> --- include/linux/virtio.h.orig	2012-01-05 00:55:44.000000000 +0100
> +++ include/linux/virtio.h	2013-07-12 14:42:26.000000000 +0200
> @@ -96,6 +96,10 @@ void virtqueue_disable_cb(struct virtque
>  
>  bool virtqueue_enable_cb(struct virtqueue *vq);
>  
> +unsigned virtqueue_enable_cb_prepare(struct virtqueue *vq);
> +
> +bool virtqueue_poll(struct virtqueue *vq, unsigned);
> +
>  bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
>  
>  void *virtqueue_detach_unused_buf(struct virtqueue *vq);

> --- drivers/net/virtio_net.c.orig	2012-01-05 00:55:44.000000000 +0100
> +++ drivers/net/virtio_net.c	2013-07-12 15:39:06.000000000 +0200
> @@ -508,7 +508,7 @@ static int virtnet_poll(struct napi_stru
>  {
>  	struct virtnet_info *vi = container_of(napi, struct virtnet_info, napi);
>  	void *buf;
> -	unsigned int len, received = 0;
> +	unsigned int r, len, received = 0;
>  
>  again:
>  	while (received < budget &&
> @@ -525,8 +525,9 @@ again:
>  
>  	/* Out of packets? */
>  	if (received < budget) {
> +		r = virtqueue_enable_cb_prepare(vi->rvq);
>  		napi_complete(napi);
> -		if (unlikely(!virtqueue_enable_cb(vi->rvq)) &&
> +		if (unlikely(virtqueue_poll(vi->rvq, r)) &&
>  		    napi_schedule_prep(napi)) {
>  			virtqueue_disable_cb(vi->rvq);
>  			__napi_schedule(napi);