Re: [PATCH v2 2/4] Defer skb allocation -- new skb_set calls & chain pages in virtio_net

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Dec 14, 2009 at 01:23:45PM -0800, Shirley Ma wrote:
> On Sun, 2009-12-13 at 13:24 +0200, Michael S. Tsirkin wrote:
> > Hmm, this scans the whole list each time.
> > OTOH, the caller probably can easily get list tail as well as head.
> > If we ask caller to give us list tail, and chain them at head, then
> > 1. we won't have to scan the list each time
> > 2. we get better memory locality reusing same pages over and over
> > again
> 
> I could use page private to point to a list_head which will have a head
> and a tail, but it will induce more alloc, and free, when this page is
> passed to ULPs as a part of skb frags, it would induce more overhead.

Yes, we don't want that. But I think caller already has
list tail available because he built up the list,
so it should be possible to chain our pages
at head: head -> new pages -> old pages.

Is this call a rare one? Maybe we do not need
to optimize this list scan, but if so let's
add a comment explaining why it does not matter.

If we are going to change data structures,
I think we should replace the linked list
simply with an array acting as a circular buffer.
But I am not asking you to implement it as
part of this patchset.

> > So this comment does not explain why this = 0 is here.
> > clearly = 0 does not chain anything.
> > Please add a bigger comment.
> > I think you also want to extend the comment at top of
> > file, where datastructure is, that explains the deferred
> > alogorigthm and how pages are chained.
> Ok, will do.
> 
> > Use min for clarity instead of opencoded if.
> > This will make it obvious that len won't ever become
> > negative below.
> Ok.
> 
> > > +static struct sk_buff *skb_goodcopy(struct virtnet_info *vi, struct
> > page **page,
> > 
> > I know you got this name from GOOD_COPY_LEN, but it's not
> > very good for a function :) and skb_ prefix is also confusing.
> > Just copy_small_skb or something like that?
> > 
> > > +                                 unsigned int *len)
> Ok.
> 
> > Comments about splitting patches  apply here as well.
> > No way to understand what this should do and whether it
> > does it correctly just by looking at patch.
> > I think reader will still wonder about is "why does it
> > need to be 16 byte aligned?". And this is what
> > comment should explain I think.
> 
> Ok, will put more comments.
> 
> > So you are overriding *len here? why bother calculating it
> > then?
> I can remove the overriding part.
> 
> > Also - this does *not* always copy all of data, and *page
> > tells us whether it did a copy or not? This is pretty confusing,
> > as APIs go. Also, if we have scatter/gather anyway,
> > why do we bother copying the head?
> 
> If receiving buffer in mergeable buf and big packets, the packet is
> small, then there is no scatter/gather, we can release the page for new
> receiving, that was the reason to copy skb head. *page will be only used
> by big packets path to indicate whether/where to start next skb frag if
> any.

I guess the main complaint is that if a function
has copy in the name, one expects it to copy data.
Maybe split it up to functions that copy
different packet types?

> > Also, before skb_set_frag skb is linear, right?
> > So in fact you do not need generic skb_set_frag,
> > you can just put stuff in the first fragment.
> > For example, pass the fragment number to skb_set_frag,
> > compiler will be able to better optimize.
> 
> You meant to reuse skb_put_frags() in ipoib_cm.c?
> 
> Thanks
> Shirley

I just mean we can pass fragment number to skb_set_frag.
In your code nr_fragments is always 0, right?

-- 
MST