| Message ID | 1260534805.30371.10.camel@localhost.localdomain |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Dec 11, 2009 at 04:33:25AM -0800, Shirley Ma wrote: > Signed-off-by: <xma@us.ibm.com> > ------------- > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index c708ecc..bb5eb7b 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -107,6 +107,16 @@ static struct page *get_a_page(struct virtnet_info *vi, gfp_t gfp_mask) > return p; > } > > +static void virtio_net_free_pages(void *buf) > +{ > + struct page *page, *next; > + > + for (page = buf; page; page = next) { > + next = (struct page *)page->private; > + __free_pages(page, 0); > + } > +} > + > static void skb_xmit_done(struct virtqueue *svq) > { > struct virtnet_info *vi = svq->vdev->priv; This is not the best way to split the patch, as I commented separately: this adds static function but no way to see whether it does what it should do. If you think about it, free should always go into same patch that does alloc. > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index fbd2ecd..db83465 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; > } > > +static int vring_destroy_bufs(struct virtqueue *_vq, void (*destroy)(void *)) > +{ > + struct vring_virtqueue *vq = to_vvq(_vq); > + void *buf; > + unsigned int i; > + int freed = 0; > + > + START_USE(vq); > + > + for (i = 0; i < vq->vring.num; i++) { I prefer ++i in such code personally. > + if (vq->data[i]) { > + /* detach_buf clears data, so grab it now. */ > + buf = vq->data[i]; > + detach_buf(vq, i); Hmm, you are calling detach on a buffer which was not used. It's not safe to call this while host might use buffers, is it? Please document this and other assumptions you are making. > + destroy(buf); > + freed++; ++freed here as well. > + } > + } > + It's usually better not to put {} around single statement blocks. > + END_USE(vq); > + return freed; > +} > + > 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, > + .destroy_bufs = vring_destroy_bufs, > }; > > struct virtqueue *vring_new_virtqueue(unsigned int num, > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > index 057a2e0..e6d7d77 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); > + int (*destroy_bufs)(struct virtqueue *vq, void (*destory)(void *)); Typo above. Please document the new field. Also please document assumptions about usage. I think callback should get struct virtqueue *vq, and decrement num. And destroy_bufs should return void, which is much more natural for a destructor. That said - do we have to use a callback? I think destroy_buf which returns data pointer, and which we call repeatedly until we get NULL or error, would be an a better, more flexible API. This is not critical though. > }; > > /** > -- 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 Fri, 11 Dec 2009 11:03:25 pm Shirley Ma wrote: > Signed-off-by: <xma@us.ibm.com> Hi Shirley, These patches look quite close. More review to follow :) This title needs revision. It should start with virtio: (all the virtio patches do, for easy identification after merge), eg: Subject: virtio: Add destroy callback for unused buffers It also needs a commit message which explains the problem and the solution. Something like this: 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. > ------------- > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index c708ecc..bb5eb7b 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -107,6 +107,16 @@ static struct page *get_a_page(struct virtnet_info *vi, gfp_t gfp_mask) > return p; > } > > +static void virtio_net_free_pages(void *buf) > +{ > + struct page *page, *next; > + > + for (page = buf; page; page = next) { > + next = (struct page *)page->private; > + __free_pages(page, 0); > + } > +} > + > static void skb_xmit_done(struct virtqueue *svq) > { > struct virtnet_info *vi = svq->vdev->priv; This belongs in one of the future patches: it will cause an unused warning and is logically not part of this patch anyway. > +static int vring_destroy_bufs(struct virtqueue *_vq, void (*destroy)(void *)) > +{ > + struct vring_virtqueue *vq = to_vvq(_vq); > + void *buf; > + unsigned int i; > + int freed = 0; unsigned int for return and freed counter? Means changing prototype, but negative return isn't useful here. > + > + START_USE(vq); > + > + for (i = 0; i < vq->vring.num; i++) { > + if (vq->data[i]) { > + /* detach_buf clears data, so grab it now. */ > + buf = vq->data[i]; > + detach_buf(vq, i); > + destroy(buf); > + freed++; You could simplify this a bit, since the queue must be inactive: destroy(vq->data[i]); detach_buf(vq, i); freed++; > + } > + } > + > + END_USE(vq); > + return freed; Perhaps add a: /* That should have freed everything. */ BUG_ON(vq->num_free != vq->vring.num) > void (*disable_cb)(struct virtqueue *vq); > bool (*enable_cb)(struct virtqueue *vq); > + int (*destroy_bufs)(struct virtqueue *vq, void (*destory)(void *)); destory typo :) Cheers, Rusty. -- 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, I agree with the comments (will have two patches instead of 4 based on Rusty's comments) except below one. On Sun, 2009-12-13 at 12:26 +0200, Michael S. Tsirkin wrote: > That said - do we have to use a callback? > I think destroy_buf which returns data pointer, > and which we call repeatedly until we get NULL > or error, would be an a better, more flexible API. > This is not critical though. The reason to use this is because in virtio_net remove, it has BUG_ON(vi->num != 0), which will be consistent with small skb packet. If we use NULL, error then we lose the track for vi->num, since we don't know how many buffers have been passed to ULPs or still unused. 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 Mon, Dec 14, 2009 at 12:08:05PM -0800, Shirley Ma wrote: > Hello Michael, > > I agree with the comments (will have two patches instead of 4 based on > Rusty's comments) except below one. > > On Sun, 2009-12-13 at 12:26 +0200, Michael S. Tsirkin wrote: > > That said - do we have to use a callback? > > I think destroy_buf which returns data pointer, > > and which we call repeatedly until we get NULL > > or error, would be an a better, more flexible API. > > This is not critical though. > > The reason to use this is because in virtio_net remove, it has > BUG_ON(vi->num != 0), which will be consistent with small skb packet. If > we use NULL, error then we lose the track for vi->num, since we don't > know how many buffers have been passed to ULPs or still unused. > > Thanks > Shirley I dont insist, but my idea was for (;;) { b = vq->destroy(vq); if (!b) break; --vi->num; put_page(b); } so we do not have to lose track of the counter.
Thanks Rusty. I agree with all these comments, will work on them. 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 Mon, 2009-12-14 at 22:22 +0200, Michael S. Tsirkin wrote: > I dont insist, but my idea was > > for (;;) { > b = vq->destroy(vq); > if (!b) > break; > --vi->num; > put_page(b); > } > > so we do not have to lose track of the counter. That's not a bad idea, but to do this, then this fuction will be specific for virtio_net device (vi is virtnet_info) only in a generic virtqueue API. 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 Mon, Dec 14, 2009 at 03:22:22PM -0800, Shirley Ma wrote: > Hello Michael, > > On Mon, 2009-12-14 at 22:22 +0200, Michael S. Tsirkin wrote: > > I dont insist, but my idea was > > > > for (;;) { > > b = vq->destroy(vq); > > if (!b) > > break; > > --vi->num; > > put_page(b); > > } > > > > so we do not have to lose track of the counter. > > That's not a bad idea, but to do this, then this fuction will be > specific for virtio_net device (vi is virtnet_info) only in a generic > virtqueue API. > > Thanks > Shirley No, this code would be in virtio net. destroy would simply be the virtqueue API that returns data pointer.
On Tue, 15 Dec 2009 06:52:53 am Michael S. Tsirkin wrote: > On Mon, Dec 14, 2009 at 12:08:05PM -0800, Shirley Ma wrote: > > Hello Michael, > > > > I agree with the comments (will have two patches instead of 4 based on > > Rusty's comments) except below one. > > > > On Sun, 2009-12-13 at 12:26 +0200, Michael S. Tsirkin wrote: > > > That said - do we have to use a callback? > > > I think destroy_buf which returns data pointer, > > > and which we call repeatedly until we get NULL > > > or error, would be an a better, more flexible API. > > > This is not critical though. > > > > The reason to use this is because in virtio_net remove, it has > > BUG_ON(vi->num != 0), which will be consistent with small skb packet. If > > we use NULL, error then we lose the track for vi->num, since we don't > > know how many buffers have been passed to ULPs or still unused. > > > > Thanks > > Shirley > > I dont insist, but my idea was > > for (;;) { > b = vq->destroy(vq); > if (!b) > break; > --vi->num; > put_page(b); > } In this case it should be called "get_unused_buf" or something. But I like Shirley's approach here; destroy (with callback) accurately reflects the only time this can be validly used. Cheers, Rusty. -- 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 Wed, Dec 16, 2009 at 09:06:12AM +1030, Rusty Russell wrote: > On Tue, 15 Dec 2009 06:52:53 am Michael S. Tsirkin wrote: > > On Mon, Dec 14, 2009 at 12:08:05PM -0800, Shirley Ma wrote: > > > Hello Michael, > > > > > > I agree with the comments (will have two patches instead of 4 based on > > > Rusty's comments) except below one. > > > > > > On Sun, 2009-12-13 at 12:26 +0200, Michael S. Tsirkin wrote: > > > > That said - do we have to use a callback? > > > > I think destroy_buf which returns data pointer, > > > > and which we call repeatedly until we get NULL > > > > or error, would be an a better, more flexible API. > > > > This is not critical though. > > > > > > The reason to use this is because in virtio_net remove, it has > > > BUG_ON(vi->num != 0), which will be consistent with small skb packet. If > > > we use NULL, error then we lose the track for vi->num, since we don't > > > know how many buffers have been passed to ULPs or still unused. > > > > > > Thanks > > > Shirley > > > > I dont insist, but my idea was > > > > for (;;) { > > b = vq->destroy(vq); > > if (!b) > > break; > > --vi->num; > > put_page(b); > > } > > In this case it should be called "get_unused_buf" or something. But I like > Shirley's approach here; destroy (with callback) accurately reflects the only > time this can be validly used. > > Cheers, > Rusty. I guess the actual requirement is that device must be inactive. As I said this is fine with me as well. But I think the callback should get vq pointer besides the data pointer, so that it can e.g. find the device if it needs to. In case of virtio net this makes it possible to decrement the outstanding skb counter in the callback. Makes sense?
On Wed, 16 Dec 2009 09:10:02 am Michael S. Tsirkin wrote: > On Wed, Dec 16, 2009 at 09:06:12AM +1030, Rusty Russell wrote: > > On Tue, 15 Dec 2009 06:52:53 am Michael S. Tsirkin wrote: > > > On Mon, Dec 14, 2009 at 12:08:05PM -0800, Shirley Ma wrote: > > > > Hello Michael, > > > > > > > > I agree with the comments (will have two patches instead of 4 based on > > > > Rusty's comments) except below one. > > > > > > > > On Sun, 2009-12-13 at 12:26 +0200, Michael S. Tsirkin wrote: > > > > > That said - do we have to use a callback? > > > > > I think destroy_buf which returns data pointer, > > > > > and which we call repeatedly until we get NULL > > > > > or error, would be an a better, more flexible API. > > > > > This is not critical though. > > > > > > > > The reason to use this is because in virtio_net remove, it has > > > > BUG_ON(vi->num != 0), which will be consistent with small skb packet. If > > > > we use NULL, error then we lose the track for vi->num, since we don't > > > > know how many buffers have been passed to ULPs or still unused. > > > > > > > > Thanks > > > > Shirley > > > > > > I dont insist, but my idea was > > > > > > for (;;) { > > > b = vq->destroy(vq); > > > if (!b) > > > break; > > > --vi->num; > > > put_page(b); > > > } > > > > In this case it should be called "get_unused_buf" or something. But I like > > Shirley's approach here; destroy (with callback) accurately reflects the only > > time this can be validly used. > > > > Cheers, > > Rusty. > > I guess the actual requirement is that device must be > inactive. Technically, the vq has to be inactive. (This distinction may matter for the multiport virtio_console work). > > As I said this is fine with me as well. > But I think the callback should get vq pointer besides the > data pointer, so that it can e.g. find the device if it needs to. > In case of virtio net this makes it possible > to decrement the outstanding skb counter in the callback. > Makes sense? Sure. I don't really mind either way, and I'm warming to the name detach_buf :) Cheers, Rusty. -- 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
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index c708ecc..bb5eb7b 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -107,6 +107,16 @@ static struct page *get_a_page(struct virtnet_info *vi, gfp_t gfp_mask) return p; } +static void virtio_net_free_pages(void *buf) +{ + struct page *page, *next; + + for (page = buf; page; page = next) { + next = (struct page *)page->private; + __free_pages(page, 0); + } +} + static void skb_xmit_done(struct virtqueue *svq) { struct virtnet_info *vi = svq->vdev->priv; diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index fbd2ecd..db83465 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; } +static int vring_destroy_bufs(struct virtqueue *_vq, void (*destroy)(void *)) +{ + struct vring_virtqueue *vq = to_vvq(_vq); + void *buf; + unsigned int i; + int freed = 0; + + START_USE(vq); + + for (i = 0; i < vq->vring.num; i++) { + if (vq->data[i]) { + /* detach_buf clears data, so grab it now. */ + buf = vq->data[i]; + detach_buf(vq, i); + destroy(buf); + freed++; + } + } + + END_USE(vq); + return freed; +} + 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, + .destroy_bufs = vring_destroy_bufs, }; struct virtqueue *vring_new_virtqueue(unsigned int num, diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 057a2e0..e6d7d77 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); + int (*destroy_bufs)(struct virtqueue *vq, void (*destory)(void *)); }; /**
Signed-off-by: <xma@us.ibm.com> ------------- -- 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