Re: [PATCH 2/2] [net/9p] Add waitq to VirtIO transport.

["Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com>]

On Fri,  1 Oct 2010 16:56:17 -0700, "Venkateswararao Jujjuri (JV)" <jvrao@linux.vnet.ibm.com> wrote:
> If there is not enough space for the PDU on the VirtIO ring, current
> code returns -EIO propagating the error to user.
> 
> This patch introduced a wqit_queue on the channel, and lets the process
> wait on this queue until VirtIO ring frees up.
> 
> Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>
> ---
>  net/9p/trans_virtio.c |   44 +++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 0df84bf..2de5144 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -75,6 +75,8 @@ struct virtio_chan {
>  	struct p9_client *client;
>  	struct virtio_device *vdev;
>  	struct virtqueue *vq;
> +	int ring_bufs_avail;
> +	wait_queue_head_t *vc_wq;
> 
>  	/* Scatterlist: can be too big for stack. */
>  	struct scatterlist sg[VIRTQUEUE_NUM];
> @@ -141,15 +143,21 @@ static void req_done(struct virtqueue *vq)
>  	do {
>  		spin_lock_irqsave(&chan->lock, flags);
>  		rc = virtqueue_get_buf(chan->vq, &len);
> -		spin_unlock_irqrestore(&chan->lock, flags);
> 
>  		if (rc != NULL) {
> +			if (!chan->ring_bufs_avail) {
> +				chan->ring_bufs_avail = 1;
> +				wake_up(chan->vc_wq);
> +			}
> +			spin_unlock_irqrestore(&chan->lock, flags);
>  			P9_DPRINTK(P9_DEBUG_TRANS, ": rc %p\n", rc);
>  			P9_DPRINTK(P9_DEBUG_TRANS, ": lookup tag %d\n",
>  					rc->tag);
>  			req = p9_tag_lookup(chan->client, rc->tag);
>  			req->status = REQ_STATUS_RCVD;
>  			p9_client_cb(chan->client, req);
> +		} else {
> +			spin_unlock_irqrestore(&chan->lock, flags);
>  		}
>  	} while (rc != NULL);
>  }
> @@ -212,6 +220,7 @@ p9_virtio_request(struct p9_client *client, struct p9_req_t *req)
> 
>  	P9_DPRINTK(P9_DEBUG_TRANS, "9p debug: virtio request\n");
> 
> +req_retry:
>  	req->status = REQ_STATUS_SENT;
> 
>  	spin_lock_irqsave(&chan->lock, flags);
> @@ -222,10 +231,21 @@ p9_virtio_request(struct p9_client *client, struct p9_req_t *req)
> 
>  	err = virtqueue_add_buf(chan->vq, chan->sg, out, in, req->tc);
>  	if (err < 0) {
> -		spin_unlock_irqrestore(&chan->lock, flags);
> -		P9_DPRINTK(P9_DEBUG_TRANS,
> -			"9p debug: virtio rpc add_buf returned failure");
> -		return -EIO;
> +		if (err == -ENOSPC) {
> +			chan->ring_bufs_avail = 0;
> +			spin_unlock_irqrestore(&chan->lock, flags);
> +			err = wait_event_interruptible_timeout(*chan->vc_wq,
> +							chan->ring_bufs_avail,
> +							HZ/4);

Why do we need this to be _timeout ? Also if interrupted by a signal we do
want to return to user space with -EIO right ? Or we loop here always ?

> +			P9_DPRINTK(P9_DEBUG_TRANS, "9p:Retry virtio request\n");
> +			goto req_retry;
> +		} else {
> +			spin_unlock_irqrestore(&chan->lock, flags);
> +			P9_DPRINTK(P9_DEBUG_TRANS,
> +					"9p debug: "
> +					"virtio rpc add_buf returned failure");
> +			return -EIO;
> +		}
>  	}

-aneesh