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

[Shirley Ma]

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 ---