From: Felix Fietkau <hidden> Date: 2012-05-27 15:26:16
At the beginning of __skb_cow, headroom gets set to a minimum of
NET_SKB_PAD. This causes unnecessary reallocations if the buffer was not
cloned and the headroom is just below NET_SKB_PAD, but still more than the
amount requested by the caller.
This was showing up frequently in my tests on VLAN tx, where
vlan_insert_tag calls skb_cow_head(skb, VLAN_HLEN).
Fix this by only setting the headroom delta if either there is less
headroom than specified by the caller, or if reallocation has to be done
anyway because the skb was cloned.
Signed-off-by: Felix Fietkau <redacted>
---
include/linux/skbuff.h | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
From: Eric Dumazet <hidden> Date: 2012-05-29 12:34:14
On Sun, 2012-05-27 at 17:26 +0200, Felix Fietkau wrote:
quoted hunk
At the beginning of __skb_cow, headroom gets set to a minimum of
NET_SKB_PAD. This causes unnecessary reallocations if the buffer was not
cloned and the headroom is just below NET_SKB_PAD, but still more than the
amount requested by the caller.
This was showing up frequently in my tests on VLAN tx, where
vlan_insert_tag calls skb_cow_head(skb, VLAN_HLEN).
Fix this by only setting the headroom delta if either there is less
headroom than specified by the caller, or if reallocation has to be done
anyway because the skb was cloned.
Signed-off-by: Felix Fietkau <redacted>
---
include/linux/skbuff.h | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
From: Felix Fietkau <hidden> Date: 2012-05-29 12:41:14
On 2012-05-29 2:34 PM, Eric Dumazet wrote:
On Sun, 2012-05-27 at 17:26 +0200, Felix Fietkau wrote:
quoted
At the beginning of __skb_cow, headroom gets set to a minimum of
NET_SKB_PAD. This causes unnecessary reallocations if the buffer was not
cloned and the headroom is just below NET_SKB_PAD, but still more than the
amount requested by the caller.
This was showing up frequently in my tests on VLAN tx, where
vlan_insert_tag calls skb_cow_head(skb, VLAN_HLEN).
Fix this by only setting the headroom delta if either there is less
headroom than specified by the caller, or if reallocation has to be done
anyway because the skb was cloned.
Signed-off-by: Felix Fietkau <redacted>
---
include/linux/skbuff.h | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
From: Eric Dumazet <hidden> Date: 2012-05-29 12:59:27
On Tue, 2012-05-29 at 14:41 +0200, Felix Fietkau wrote:
On 2012-05-29 2:34 PM, Eric Dumazet wrote:
quoted
On Sun, 2012-05-27 at 17:26 +0200, Felix Fietkau wrote:
quoted
At the beginning of __skb_cow, headroom gets set to a minimum of
NET_SKB_PAD. This causes unnecessary reallocations if the buffer was not
cloned and the headroom is just below NET_SKB_PAD, but still more than the
amount requested by the caller.
This was showing up frequently in my tests on VLAN tx, where
vlan_insert_tag calls skb_cow_head(skb, VLAN_HLEN).
Fix this by only setting the headroom delta if either there is less
headroom than specified by the caller, or if reallocation has to be done
anyway because the skb was cloned.
Signed-off-by: Felix Fietkau <redacted>
---
include/linux/skbuff.h | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
@@ -1894,12 +1894,15 @@ static inline int skb_clone_writable(const struct sk_buff *skb, unsigned int lenstaticinlineint__skb_cow(structsk_buff*skb,unsignedintheadroom,intcloned){+unsignedintalloc_headroom=headroom;intdelta=0;if(headroom<NET_SKB_PAD)-headroom=NET_SKB_PAD;-if(headroom>skb_headroom(skb))-delta=headroom-skb_headroom(skb);+alloc_headroom=NET_SKB_PAD;+if(headroom>skb_headroom(skb)||+(cloned&&alloc_headroom>skb_headroom(skb))){+delta=alloc_headroom-skb_headroom(skb);+}if(delta||cloned)returnpskb_expand_head(skb,ALIGN(delta,NET_SKB_PAD),0,
Nice catch.
Scratching my head on this one. Why not the obvious fix ?
If we're reallocating anyway, we might as well put in more headroom than
requested, in case something else needs even more than that.
Locally generated packets should have enough headroom, and for forward
paths, we already have NET_SKB_PAD bytes of headroom.
Adding yet another NET_SKB_PAD extra space is overkill, unless you have
a real use case in mind ?
From: Felix Fietkau <hidden> Date: 2012-05-29 13:10:11
On 2012-05-29 2:59 PM, Eric Dumazet wrote:
On Tue, 2012-05-29 at 14:41 +0200, Felix Fietkau wrote:
quoted
On 2012-05-29 2:34 PM, Eric Dumazet wrote:
quoted
On Sun, 2012-05-27 at 17:26 +0200, Felix Fietkau wrote:
quoted
At the beginning of __skb_cow, headroom gets set to a minimum of
NET_SKB_PAD. This causes unnecessary reallocations if the buffer was not
cloned and the headroom is just below NET_SKB_PAD, but still more than the
amount requested by the caller.
This was showing up frequently in my tests on VLAN tx, where
vlan_insert_tag calls skb_cow_head(skb, VLAN_HLEN).
Fix this by only setting the headroom delta if either there is less
headroom than specified by the caller, or if reallocation has to be done
anyway because the skb was cloned.
Signed-off-by: Felix Fietkau <redacted>
---
include/linux/skbuff.h | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
@@ -1894,12 +1894,15 @@ static inline int skb_clone_writable(const struct sk_buff *skb, unsigned int lenstaticinlineint__skb_cow(structsk_buff*skb,unsignedintheadroom,intcloned){+unsignedintalloc_headroom=headroom;intdelta=0;if(headroom<NET_SKB_PAD)-headroom=NET_SKB_PAD;-if(headroom>skb_headroom(skb))-delta=headroom-skb_headroom(skb);+alloc_headroom=NET_SKB_PAD;+if(headroom>skb_headroom(skb)||+(cloned&&alloc_headroom>skb_headroom(skb))){+delta=alloc_headroom-skb_headroom(skb);+}if(delta||cloned)returnpskb_expand_head(skb,ALIGN(delta,NET_SKB_PAD),0,
Nice catch.
Scratching my head on this one. Why not the obvious fix ?
If we're reallocating anyway, we might as well put in more headroom than
requested, in case something else needs even more than that.
Locally generated packets should have enough headroom, and for forward
paths, we already have NET_SKB_PAD bytes of headroom.
Adding yet another NET_SKB_PAD extra space is overkill, unless you have
a real use case in mind ?
I don't have any real use case in mind, but it's not really adding an
extra NET_SKB_PAD, it simply fills up the headroom to NET_SKB_PAD, but I
guess that's probably unnecessary as well.
I'll resend the patch without the extra padding later.
- Felix
From: Eric Dumazet <hidden> Date: 2012-05-29 13:26:14
On Tue, 2012-05-29 at 15:10 +0200, Felix Fietkau wrote:
I don't have any real use case in mind, but it's not really adding an
extra NET_SKB_PAD, it simply fills up the headroom to NET_SKB_PAD,
This is not what is doing your patch.
If cloned is true, and current skb headroom less than 64, you add an
extra 64 bytes of headroom.
Just keep it simple, this is inline code and should be kept as small as
possible.