[RFC] 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 <xma@us.ibm.com> 
---
 drivers/virtio/virtio_ring.c |   24 ++++++++++++++++++++++++
 include/linux/virtio.h       |    1 +
 2 files changed, 25 insertions(+), 0 deletions(-)


Thanks
Shirley

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <xma@us.ibm.com> 
> ---
>  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 netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks Michael, will fix them all.

Shirley

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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