From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Aneesh Kumar K. V" Subject: Re: [PATCH 2/2] [net/9p] Add waitq to VirtIO transport. Date: Wed, 06 Oct 2010 11:51:34 +0530 Message-ID: References: <1285977377-28736-1-git-send-email-jvrao@linux.vnet.ibm.com> <1285977377-28736-2-git-send-email-jvrao@linux.vnet.ibm.com> <4CABB438.4090109@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain Cc: v9fs-developer@lists.sourceforge.net, linux-fsdevel@vger.kernel.org To: "Venkateswararao Jujjuri \(JV\)" Return-path: Received: from e23smtp01.au.ibm.com ([202.81.31.143]:50173 "EHLO e23smtp01.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757050Ab0JFGVn (ORCPT ); Wed, 6 Oct 2010 02:21:43 -0400 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [202.81.31.247]) by e23smtp01.au.ibm.com (8.14.4/8.13.1) with ESMTP id o966Ic3Q022071 for ; Wed, 6 Oct 2010 17:18:38 +1100 Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o966LfXP1945658 for ; Wed, 6 Oct 2010 17:21:41 +1100 Received: from d23av01.au.ibm.com (loopback [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id o966Le0F031373 for ; Wed, 6 Oct 2010 17:21:41 +1100 In-Reply-To: <4CABB438.4090109@linux.vnet.ibm.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, 05 Oct 2010 16:26:48 -0700, "Venkateswararao Jujjuri (JV)" 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)" 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 > >> --- > >> 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