[PATCH v2] net: avoid the unnecessary kmalloc

[PATCH v2] net: avoid the unnecessary kmalloc

[Changli Gao]

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(-)
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;

Re: [PATCH v2] net: avoid the unnecessary kmalloc

[Eric Dumazet]

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.

Re: [PATCH v2] net: avoid the unnecessary kmalloc

[Changli Gao]

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.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH v2] net: avoid the unnecessary kmalloc

[Eric Dumazet]

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.