[Fwd: Re: [PATCH v2 1/4] Defer skb allocation -- add destroy buffers function for virtio]

[Shirley Ma <mashirle@us.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 41 bytes --]

Sorry, forgot to CC all.

Thanks
Shirley

[-- Attachment #2: Forwarded message - Re: [PATCH v2 1/4] Defer skb allocation -- add destroy buffers function for virtio --]
[-- Type: message/rfc822, Size: 6254 bytes --]

From: "Michael S. Tsirkin" <mst@redhat.com>
To: Shirley Ma <mashirle@us.ibm.com>
Subject: Re: [PATCH v2 1/4] Defer skb allocation -- add destroy buffers function for virtio
Date: Tue, 15 Dec 2009 18:02:05 +0200
Message-ID: <20091215160205.GA23883@redhat.com>

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
>