| Message ID | 1291077629-6339-1-git-send-email-xiaosuo@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Le mardi 30 novembre 2010 à 08:40 +0800, Changli Gao a écrit : > If the old memory allocated by kmalloc() is larger than the new requested, > pskb_expand_head() doesn't need to allocate a new one, unless the skb->head > is shared. > > Signed-off-by: Changli Gao <xiaosuo@gmail.com> Seems fine to me, but patch title and changelog are a bit uninformative. skb head being allocated by kmalloc(), it might be larger than what actually requested because of discrete kmem caches sizes. Before reallocating a new skb head, check if the current one has the needed extra size. Do this check only if skb head is not shared. > + > + if (fastpath && > + size + sizeof(struct skb_shared_info) <= ksize(skb->head)) { > + memmove(skb->head + size, skb_shinfo(skb), > + offsetof(struct skb_shared_info, > + frags[skb_shinfo(skb)->nr_frags])); > + memmove(skb->head + nhead, skb->head, > + skb_tail_pointer(skb) - skb->head); > + off = nhead; > + goto adjust_others; > + } > + I suggest doing the max possible resize at this stage ? Ie moving skb_shared_info at the edge of memory block. Maybe its not necessary, and a given skb is not expanded multiple times in our stack, I dont really know. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 30, 2010 at 2:52 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le mardi 30 novembre 2010 à 08:40 +0800, Changli Gao a écrit : >> If the old memory allocated by kmalloc() is larger than the new requested, >> pskb_expand_head() doesn't need to allocate a new one, unless the skb->head >> is shared. >> >> Signed-off-by: Changli Gao <xiaosuo@gmail.com> > > Seems fine to me, but patch title and changelog are a bit uninformative. > > > skb head being allocated by kmalloc(), it might be larger than what > actually requested because of discrete kmem caches sizes. Before > reallocating a new skb head, check if the current one has the needed > extra size. > > Do this check only if skb head is not shared. > > OK, I'll refine it with your comment. Thanks. > >> + >> + if (fastpath && >> + size + sizeof(struct skb_shared_info) <= ksize(skb->head)) { >> + memmove(skb->head + size, skb_shinfo(skb), >> + offsetof(struct skb_shared_info, >> + frags[skb_shinfo(skb)->nr_frags])); >> + memmove(skb->head + nhead, skb->head, >> + skb_tail_pointer(skb) - skb->head); >> + off = nhead; >> + goto adjust_others; >> + } >> + > > I suggest doing the max possible resize at this stage ? > > Ie moving skb_shared_info at the edge of memory block. > > Maybe its not necessary, and a given skb is not expanded multiple times > in our stack, I dont really know. > I don't think it is a good idea, If it is, why not use ksize(skb->head) to set the tail pointer of skb when allocating a new skb. If we do sth. like this, more cache lines maybe involved.
Le mardi 30 novembre 2010 à 15:45 +0800, Changli Gao a écrit : > I don't think it is a good idea, If it is, why not use > ksize(skb->head) to set the tail pointer of skb when allocating a new > skb. > Calling ksize() for all allocated skb would be overkill, since most of them are correctly sized and dont need expand() at all. In what situation do you notice this being expand_head() ever called ? > If we do sth. like this, more cache lines maybe involved. > This has same number of _touched_ cache lines actually, and this is what matters. Actually I dont have strong feeling, this is a detail. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 104f844..8814a9a 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -778,6 +778,28 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, size = SKB_DATA_ALIGN(size); + /* Check if we can avoid taking references on fragments if we own + * the last reference on skb->head. (see skb_release_data()) + */ + if (!skb->cloned) + fastpath = true; + else { + int delta = skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1; + + fastpath = atomic_read(&skb_shinfo(skb)->dataref) == delta; + } + + if (fastpath && + size + sizeof(struct skb_shared_info) <= ksize(skb->head)) { + memmove(skb->head + size, skb_shinfo(skb), + offsetof(struct skb_shared_info, + frags[skb_shinfo(skb)->nr_frags])); + memmove(skb->head + nhead, skb->head, + skb_tail_pointer(skb) - skb->head); + off = nhead; + goto adjust_others; + } + data = kmalloc(size + sizeof(struct skb_shared_info), gfp_mask); if (!data) goto nodata; @@ -791,17 +813,6 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, skb_shinfo(skb), offsetof(struct skb_shared_info, frags[skb_shinfo(skb)->nr_frags])); - /* Check if we can avoid taking references on fragments if we own - * the last reference on skb->head. (see skb_release_data()) - */ - if (!skb->cloned) - fastpath = true; - else { - int delta = skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1; - - fastpath = atomic_read(&skb_shinfo(skb)->dataref) == delta; - } - if (fastpath) { kfree(skb->head); } else { @@ -816,6 +827,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, off = (data + nhead) - skb->head; skb->head = data; +adjust_others: skb->data += off; #ifdef NET_SKBUFF_DATA_USES_OFFSET skb->end = size;
If the old memory allocated by kmalloc() is larger than the new requested, pskb_expand_head() doesn't need to allocate a new one, unless the skb->head is shared. Signed-off-by: Changli Gao <xiaosuo@gmail.com> --- v2: apply this trick for the cloned but not shared skb net/core/skbuff.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html