Re: [RFC PATCH] Subject: virtio: Add unused buffers detach from vring
On Tue, Dec 15, 2009 at 11:14:07AM -0800, Shirley Ma wrote: > Hello Michael, > > On Tue, 2009-12-15 at 20:47 +0200, Michael S. Tsirkin wrote: > > > + detach_buf(vq, i); > > > + END_USE(vq); > > > + return vq->data[i]; > > > > In fact, this will return NULL always, won't it? > > Nope, I changed the destroy to detach and return the buffers without > destroying them within the call. I thought it might be useful in some > other case. > > Maybe I should put destroy call back? > > Thanks > Shirley No I think it's good as is, we do not need a callback. I was simply saying that detach_buf sets data to NULL, so return vq->data[i] after detach does not make sense. You need to save data as comment below says.c -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] Subject: virtio: Add unused buffers detach from vring
Hello Michael, On Tue, 2009-12-15 at 20:47 +0200, Michael S. Tsirkin wrote: > > + detach_buf(vq, i); > > + END_USE(vq); > > + return vq->data[i]; > > In fact, this will return NULL always, won't it? Nope, I changed the destroy to detach and return the buffers without destroying them within the call. I thought it might be useful in some other case. Maybe I should put destroy call back? Thanks Shirley -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] Subject: virtio: Add unused buffers detach from vring
Thanks Michael, will fix them all. Shirley -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] Subject: virtio: Add unused buffers detach from vring
On Tue, Dec 15, 2009 at 10:42:53AM -0800, Shirley Ma wrote:
> I submit one split patch for review to make sure that's the right format.
> I copied Rusty's comment for the commit message, and change destroy
> to detach since we destroy the buffers in caller. This patch is built
> against Dave's net-next tree.
Almost :) text not intended for git commit logs like the above
should go after ---, this way git am knows to skip it.
> There's currently no way for a virtio driver to ask for unused
> buffers, so it has to keep a list itself to reclaim them at shutdown.
> This is redundant, since virtio_ring stores that information. So
> add a new hook to do this: virtio_net will be the first user.
>
> Signed-off-by: Shirley Ma
> ---
> drivers/virtio/virtio_ring.c | 24
> include/linux/virtio.h |1 +
> 2 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index fbd2ecd..f847bc3 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -334,6 +334,29 @@ static bool vring_enable_cb(struct virtqueue *_vq)
> return true;
> }
>
> +/* This function is used to return vring unused buffers to caller for free */
> +static void *vring_detach_bufs(struct virtqueue *_vq)
> +{
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + unsigned int i;
> +
> + START_USE(vq);
> +
> + for (i = 0; i < vq->vring.num; ++i) {
This is a single statement loop, so you do not need {}
around it. Or even better:
for (i = 0; i < vq->vring.num; ++i) {
if (!vq->data[i])
continue;
...
}
which has less nesting.
> + if (vq->data[i]) {
> + /* detach_buf clears data, so grab it now. */
You wrote that comment, but did you read it :)
> + detach_buf(vq, i);
> + END_USE(vq);
> + return vq->data[i];
In fact, this will return NULL always, won't it?
> + }
> + }
> + /* That should have freed everything. */
> + BUG_ON(vq->num_free != vq->vring.num);
> +
> + END_USE(vq);
> + return NULL;
> +}
> +
> irqreturn_t vring_interrupt(int irq, void *_vq)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> @@ -360,6 +383,7 @@ static struct virtqueue_ops vring_vq_ops = {
> .kick = vring_kick,
> .disable_cb = vring_disable_cb,
> .enable_cb = vring_enable_cb,
> + .detach_bufs = vring_detach_bufs,
> };
>
> struct virtqueue *vring_new_virtqueue(unsigned int num,
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 057a2e0..d7da456 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -71,6 +71,7 @@ struct virtqueue_ops {
>
> void (*disable_cb)(struct virtqueue *vq);
> bool (*enable_cb)(struct virtqueue *vq);
> + void *(*detach_bufs)(struct virtqueue *vq);
> };
Please add documentation in virtio.h
Thanks!
>
> /**
>
> Thanks
> Shirley
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH] Subject: virtio: Add unused buffers detach from vring
I submit one split patch for review to make sure that's the right format.
I copied Rusty's comment for the commit message, and change destroy
to detach since we destroy the buffers in caller. This patch is built
against Dave's net-next tree.
There's currently no way for a virtio driver to ask for unused
buffers, so it has to keep a list itself to reclaim them at shutdown.
This is redundant, since virtio_ring stores that information. So
add a new hook to do this: virtio_net will be the first user.
Signed-off-by: Shirley Ma
---
drivers/virtio/virtio_ring.c | 24
include/linux/virtio.h |1 +
2 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index fbd2ecd..f847bc3 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -334,6 +334,29 @@ static bool vring_enable_cb(struct virtqueue *_vq)
return true;
}
+/* This function is used to return vring unused buffers to caller for free */
+static void *vring_detach_bufs(struct virtqueue *_vq)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ unsigned int i;
+
+ START_USE(vq);
+
+ for (i = 0; i < vq->vring.num; ++i) {
+ if (vq->data[i]) {
+ /* detach_buf clears data, so grab it now. */
+ detach_buf(vq, i);
+ END_USE(vq);
+ return vq->data[i];
+ }
+ }
+ /* That should have freed everything. */
+ BUG_ON(vq->num_free != vq->vring.num);
+
+ END_USE(vq);
+ return NULL;
+}
+
irqreturn_t vring_interrupt(int irq, void *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
@@ -360,6 +383,7 @@ static struct virtqueue_ops vring_vq_ops = {
.kick = vring_kick,
.disable_cb = vring_disable_cb,
.enable_cb = vring_enable_cb,
+ .detach_bufs = vring_detach_bufs,
};
struct virtqueue *vring_new_virtqueue(unsigned int num,
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 057a2e0..d7da456 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -71,6 +71,7 @@ struct virtqueue_ops {
void (*disable_cb)(struct virtqueue *vq);
bool (*enable_cb)(struct virtqueue *vq);
+ void *(*detach_bufs)(struct virtqueue *vq);
};
/**
Thanks
Shirley
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
