* [PATCH 1/2] [net/9p]Serialize virtqueue operations to make VirtIO transport SMP safe.
@ 2010-10-01 23:56 Venkateswararao Jujjuri (JV)
2010-10-01 23:56 ` [PATCH 2/2] [net/9p] Add waitq to VirtIO transport Venkateswararao Jujjuri (JV)
0 siblings, 1 reply; 5+ messages in thread
From: Venkateswararao Jujjuri (JV) @ 2010-10-01 23:56 UTC (permalink / raw)
To: v9fs-developer; +Cc: linux-fsdevel, Venkateswararao Jujjuri (JV)
Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>
---
net/9p/trans_virtio.c | 34 ++++++++++++++++++++++++----------
1 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index dcfbe99..0df84bf 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -134,16 +134,24 @@ static void req_done(struct virtqueue *vq)
struct p9_fcall *rc;
unsigned int len;
struct p9_req_t *req;
+ unsigned long flags;
P9_DPRINTK(P9_DEBUG_TRANS, ": request done\n");
- while ((rc = virtqueue_get_buf(chan->vq, &len)) != NULL) {
- 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);
- }
+ do {
+ spin_lock_irqsave(&chan->lock, flags);
+ rc = virtqueue_get_buf(chan->vq, &len);
+ spin_unlock_irqrestore(&chan->lock, flags);
+
+ if (rc != NULL) {
+ 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);
+ }
+ } while (rc != NULL);
}
/**
@@ -199,23 +207,29 @@ p9_virtio_request(struct p9_client *client, struct p9_req_t *req)
int in, out;
struct virtio_chan *chan = client->trans;
char *rdata = (char *)req->rc+sizeof(struct p9_fcall);
+ unsigned long flags;
+ int err;
P9_DPRINTK(P9_DEBUG_TRANS, "9p debug: virtio request\n");
+ req->status = REQ_STATUS_SENT;
+
+ spin_lock_irqsave(&chan->lock, flags);
out = pack_sg_list(chan->sg, 0, VIRTQUEUE_NUM, req->tc->sdata,
req->tc->size);
in = pack_sg_list(chan->sg, out, VIRTQUEUE_NUM-out, rdata,
client->msize);
- req->status = REQ_STATUS_SENT;
-
- if (virtqueue_add_buf(chan->vq, chan->sg, out, in, req->tc) < 0) {
+ 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;
}
virtqueue_kick(chan->vq);
+ spin_unlock_irqrestore(&chan->lock, flags);
P9_DPRINTK(P9_DEBUG_TRANS, "9p debug: virtio request kicked\n");
return 0;
--
1.6.5.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/2] [net/9p] Add waitq to VirtIO transport.
2010-10-01 23:56 [PATCH 1/2] [net/9p]Serialize virtqueue operations to make VirtIO transport SMP safe Venkateswararao Jujjuri (JV)
@ 2010-10-01 23:56 ` Venkateswararao Jujjuri (JV)
2010-10-05 14:45 ` Aneesh Kumar K. V
0 siblings, 1 reply; 5+ messages in thread
From: Venkateswararao Jujjuri (JV) @ 2010-10-01 23:56 UTC (permalink / raw)
To: v9fs-developer; +Cc: linux-fsdevel, Venkateswararao Jujjuri (JV)
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);
+ 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;
+ }
}
virtqueue_kick(chan->vq);
@@ -304,14 +324,23 @@ static int p9_virtio_probe(struct virtio_device *vdev)
chan->tag_len = tag_len;
err = sysfs_create_file(&(vdev->dev.kobj), &dev_attr_mount_tag.attr);
if (err) {
- kfree(tag);
- goto out_free_vq;
+ goto out_free_tag;
}
+ chan->vc_wq = kmalloc(sizeof(wait_queue_head_t), GFP_KERNEL);
+ if (!chan->vc_wq) {
+ err = -ENOMEM;
+ goto out_free_tag;
+ }
+ init_waitqueue_head(chan->vc_wq);
+ chan->ring_bufs_avail = 1;
+
mutex_lock(&virtio_9p_lock);
list_add_tail(&chan->chan_list, &virtio_chan_list);
mutex_unlock(&virtio_9p_lock);
return 0;
+out_free_tag:
+ kfree(tag);
out_free_vq:
vdev->config->del_vqs(vdev);
kfree(chan);
@@ -384,6 +413,7 @@ static void p9_virtio_remove(struct virtio_device *vdev)
mutex_unlock(&virtio_9p_lock);
sysfs_remove_file(&(vdev->dev.kobj), &dev_attr_mount_tag.attr);
kfree(chan->tag);
+ kfree(chan->vc_wq);
kfree(chan);
}
--
1.6.5.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 2/2] [net/9p] Add waitq to VirtIO transport.
2010-10-01 23:56 ` [PATCH 2/2] [net/9p] Add waitq to VirtIO transport Venkateswararao Jujjuri (JV)
@ 2010-10-05 14:45 ` Aneesh Kumar K. V
2010-10-05 23:26 ` Venkateswararao Jujjuri (JV)
0 siblings, 1 reply; 5+ messages in thread
From: Aneesh Kumar K. V @ 2010-10-05 14:45 UTC (permalink / raw)
To: Venkateswararao Jujjuri (JV), v9fs-developer
Cc: linux-fsdevel, Venkateswararao Jujjuri (JV)
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 2/2] [net/9p] Add waitq to VirtIO transport.
2010-10-05 14:45 ` Aneesh Kumar K. V
@ 2010-10-05 23:26 ` Venkateswararao Jujjuri (JV)
2010-10-06 6:21 ` Aneesh Kumar K. V
0 siblings, 1 reply; 5+ messages in thread
From: Venkateswararao Jujjuri (JV) @ 2010-10-05 23:26 UTC (permalink / raw)
To: Aneesh Kumar K. V; +Cc: v9fs-developer, linux-fsdevel
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.
No We loop here for ever as long as we get -ENOSPC is returned by virtqueue_add_buf.
- JV
>
>> + 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH 2/2] [net/9p] Add waitq to VirtIO transport.
2010-10-05 23:26 ` Venkateswararao Jujjuri (JV)
@ 2010-10-06 6:21 ` Aneesh Kumar K. V
0 siblings, 0 replies; 5+ messages in thread
From: Aneesh Kumar K. V @ 2010-10-06 6:21 UTC (permalink / raw)
To: Venkateswararao Jujjuri (JV); +Cc: v9fs-developer, linux-fsdevel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-10-06 6:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-10-01 23:56 [PATCH 1/2] [net/9p]Serialize virtqueue operations to make VirtIO transport SMP safe Venkateswararao Jujjuri (JV)
2010-10-01 23:56 ` [PATCH 2/2] [net/9p] Add waitq to VirtIO transport Venkateswararao Jujjuri (JV)
2010-10-05 14:45 ` Aneesh Kumar K. V
2010-10-05 23:26 ` Venkateswararao Jujjuri (JV)
2010-10-06 6:21 ` Aneesh Kumar K. V
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).