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

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

On Tue, 05 Oct 2010 16:26:48 -0700, "Venkateswararao Jujjuri (JV)" <jvrao@linux.vnet.ibm.com> wrote:
> On 10/5/2010 7:45 AM, Aneesh Kumar K. V wrote:
> > 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 ?
> 
> We are waiting for some other request to complete and the wait is outside the lock.
> Hence it is possible miss the wakwup...hence timed wait.

I still don't see how we could miss the wakeup. 

> 
> No We loop here for ever as long as we get -ENOSPC is returned by virtqueue_add_buf.
> 

I would consider that to be wrong. User should be able interrupt the
wait by signal. 

-aneesh