Messages in this thread | | | Subject | Re: [PATCH v2 2/4] Defer skb allocation -- new skb_set calls & chain pages in virtio_net | From | Shirley Ma <> | Date | Mon, 14 Dec 2009 13:23:45 -0800 |
| |
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.
> 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.
> 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
| |