On Tue, 05 Oct 2010 16:26:48 -0700, "Venkateswararao Jujjuri (JV)" <jvrao@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> >> ---
> >> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
