Re: [PATCH v2 1/4] Defer skb allocation -- add destroy buffers function for virtio
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. -- 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
[Fwd: Re: [PATCH v2 1/4] Defer skb allocation -- add destroy buffers function for virtio]
Sorry, forgot to CC all. Thanks Shirley ---BeginMessage--- On Tue, Dec 15, 2009 at 07:59:42AM -0800, Shirley Ma wrote: Hello Michael, On Tue, 2009-12-15 at 12:57 +0200, Michael S. Tsirkin wrote: No, this code would be in virtio net. destroy would simply be the virtqueue API that returns data pointer. Since virtio_net doesn't maintain the descriptors of vring, to return the data pointer from vq destroy will go through vq-vring.num. It's a little expensive. Since it's shutdown code, it might be OK. Rusty, How do you think? If I change the code something like this, not tested. Yes, this is basically what I had in mind. Looks slightly easier to understand than callbacks to me. But far from critical of course. +static void *vring_destroy_bufs(struct virtqueue *_vq, void (*destroy)(void *)) +{ + 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_USED(vq); + return vq-data[i]; + } + } + /* That should have freed everything. */ + BUG_ON(vq-num_free != vq-vring.num); + END_USED(vq); + return NULL; +} +static void virtnet_free_bufs(struct virtqueue *rvq) +{ + void *buf; + for (;;) { + buf = rvq-destroy(rvq); + if (!buf) { + BUG_ON(vi-num != 0); + return; + } else { Since we have return above, you can put the following code in the outer block and reduce nesting (without else). + if (vi-mergeable_rx_bufs || vi-big_packets) + struct page *page, *next; + + for (page = buf; page; page = next) { +next = (struct page *)page-private; *)page-private; +__free_pages(page, 0); + } else + kfree_skb(buf); + } + --vi-num; +} Thanks Shirley ---End Message---
Re: [PATCH v2 1/4] Defer skb allocation -- add destroy buffers function for virtio
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? -- 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: [PATCH v2 1/4] Defer skb allocation -- add destroy buffers function for virtio
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 kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] Defer skb allocation -- add destroy buffers function for virtio
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 kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] Defer skb allocation -- add destroy buffers function for virtio
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. -- 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: [PATCH v2 1/4] Defer skb allocation -- add destroy buffers function for virtio
Thanks Rusty. I agree with all these comments, will work on them. 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: [PATCH v2 1/4] Defer skb allocation -- add destroy buffers function for virtio
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 kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] Defer skb allocation -- add destroy buffers function for virtio
On Fri, Dec 11, 2009 at 04:33:25AM -0800, Shirley Ma wrote: Signed-off-by: x...@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 kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] Defer skb allocation -- add destroy buffers function for virtio
On Fri, 11 Dec 2009 11:03:25 pm Shirley Ma wrote: Signed-off-by: x...@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 kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html