Sorry, forgot to CC all.
Thanks
Shirley
--- Begin Message ---
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 ---
