* [RFC] skb: avoid unnecessary reallocations in __skb_cow @ 2012-05-27 15:26 Felix Fietkau 2012-05-29 12:34 ` Eric Dumazet 0 siblings, 1 reply; 6+ messages in thread From: Felix Fietkau @ 2012-05-27 15:26 UTC (permalink / raw) To: netdev 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 <nbd@openwrt.org> --- include/linux/skbuff.h | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 0e50171..1898471 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -1894,12 +1894,15 @@ static inline int skb_clone_writable(const struct sk_buff *skb, unsigned int len static inline int __skb_cow(struct sk_buff *skb, unsigned int headroom, int cloned) { + unsigned int alloc_headroom = headroom; int delta = 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) return pskb_expand_head(skb, ALIGN(delta, NET_SKB_PAD), 0, -- 1.7.3.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC] skb: avoid unnecessary reallocations in __skb_cow 2012-05-27 15:26 [RFC] skb: avoid unnecessary reallocations in __skb_cow Felix Fietkau @ 2012-05-29 12:34 ` Eric Dumazet 2012-05-29 12:41 ` Felix Fietkau 0 siblings, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2012-05-29 12:34 UTC (permalink / raw) To: Felix Fietkau; +Cc: netdev On Sun, 2012-05-27 at 17:26 +0200, Felix Fietkau wrote: > 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 <nbd@openwrt.org> > --- > include/linux/skbuff.h | 9 ++++++--- > 1 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index 0e50171..1898471 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -1894,12 +1894,15 @@ static inline int skb_clone_writable(const struct sk_buff *skb, unsigned int len > static inline int __skb_cow(struct sk_buff *skb, unsigned int headroom, > int cloned) > { > + unsigned int alloc_headroom = headroom; > int delta = 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) > return pskb_expand_head(skb, ALIGN(delta, NET_SKB_PAD), 0, Nice catch. Scratching my head on this one. Why not the obvious fix ? diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 0e50171..b534a1b 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -1896,8 +1896,6 @@ static inline int __skb_cow(struct sk_buff *skb, unsigned int headroom, { int delta = 0; - if (headroom < NET_SKB_PAD) - headroom = NET_SKB_PAD; if (headroom > skb_headroom(skb)) delta = headroom - skb_headroom(skb); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC] skb: avoid unnecessary reallocations in __skb_cow 2012-05-29 12:34 ` Eric Dumazet @ 2012-05-29 12:41 ` Felix Fietkau 2012-05-29 12:59 ` Eric Dumazet 0 siblings, 1 reply; 6+ messages in thread From: Felix Fietkau @ 2012-05-29 12:41 UTC (permalink / raw) To: Eric Dumazet; +Cc: netdev On 2012-05-29 2:34 PM, Eric Dumazet wrote: > On Sun, 2012-05-27 at 17:26 +0200, Felix Fietkau wrote: >> 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 <nbd@openwrt.org> >> --- >> include/linux/skbuff.h | 9 ++++++--- >> 1 files changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h >> index 0e50171..1898471 100644 >> --- a/include/linux/skbuff.h >> +++ b/include/linux/skbuff.h >> @@ -1894,12 +1894,15 @@ static inline int skb_clone_writable(const struct sk_buff *skb, unsigned int len >> static inline int __skb_cow(struct sk_buff *skb, unsigned int headroom, >> int cloned) >> { >> + unsigned int alloc_headroom = headroom; >> int delta = 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) >> return pskb_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. - Felix ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] skb: avoid unnecessary reallocations in __skb_cow 2012-05-29 12:41 ` Felix Fietkau @ 2012-05-29 12:59 ` Eric Dumazet 2012-05-29 13:10 ` Felix Fietkau 0 siblings, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2012-05-29 12:59 UTC (permalink / raw) To: Felix Fietkau; +Cc: netdev On Tue, 2012-05-29 at 14:41 +0200, Felix Fietkau wrote: > On 2012-05-29 2:34 PM, Eric Dumazet wrote: > > On Sun, 2012-05-27 at 17:26 +0200, Felix Fietkau wrote: > >> 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 <nbd@openwrt.org> > >> --- > >> include/linux/skbuff.h | 9 ++++++--- > >> 1 files changed, 6 insertions(+), 3 deletions(-) > >> > >> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > >> index 0e50171..1898471 100644 > >> --- a/include/linux/skbuff.h > >> +++ b/include/linux/skbuff.h > >> @@ -1894,12 +1894,15 @@ static inline int skb_clone_writable(const struct sk_buff *skb, unsigned int len > >> static inline int __skb_cow(struct sk_buff *skb, unsigned int headroom, > >> int cloned) > >> { > >> + unsigned int alloc_headroom = headroom; > >> int delta = 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) > >> return pskb_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 ? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] skb: avoid unnecessary reallocations in __skb_cow 2012-05-29 12:59 ` Eric Dumazet @ 2012-05-29 13:10 ` Felix Fietkau 2012-05-29 13:26 ` Eric Dumazet 0 siblings, 1 reply; 6+ messages in thread From: Felix Fietkau @ 2012-05-29 13:10 UTC (permalink / raw) To: Eric Dumazet; +Cc: netdev On 2012-05-29 2:59 PM, Eric Dumazet wrote: > On Tue, 2012-05-29 at 14:41 +0200, Felix Fietkau wrote: >> On 2012-05-29 2:34 PM, Eric Dumazet wrote: >> > On Sun, 2012-05-27 at 17:26 +0200, Felix Fietkau wrote: >> >> 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 <nbd@openwrt.org> >> >> --- >> >> include/linux/skbuff.h | 9 ++++++--- >> >> 1 files changed, 6 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h >> >> index 0e50171..1898471 100644 >> >> --- a/include/linux/skbuff.h >> >> +++ b/include/linux/skbuff.h >> >> @@ -1894,12 +1894,15 @@ static inline int skb_clone_writable(const struct sk_buff *skb, unsigned int len >> >> static inline int __skb_cow(struct sk_buff *skb, unsigned int headroom, >> >> int cloned) >> >> { >> >> + unsigned int alloc_headroom = headroom; >> >> int delta = 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) >> >> return pskb_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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] skb: avoid unnecessary reallocations in __skb_cow 2012-05-29 13:10 ` Felix Fietkau @ 2012-05-29 13:26 ` Eric Dumazet 0 siblings, 0 replies; 6+ messages in thread From: Eric Dumazet @ 2012-05-29 13:26 UTC (permalink / raw) To: Felix Fietkau; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-05-29 13:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-27 15:26 [RFC] skb: avoid unnecessary reallocations in __skb_cow Felix Fietkau 2012-05-29 12:34 ` Eric Dumazet 2012-05-29 12:41 ` Felix Fietkau 2012-05-29 12:59 ` Eric Dumazet 2012-05-29 13:10 ` Felix Fietkau 2012-05-29 13:26 ` Eric Dumazet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).