| Message ID | 1269088078-7343-1-git-send-email-timo.teras@iki.fi |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, Mar 20, 2010 at 02:27:58PM +0200, Timo Teras wrote: > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > index f47c9f7..f78402d 100644 > --- a/net/ipv4/ip_gre.c > +++ b/net/ipv4/ip_gre.c > @@ -810,11 +810,13 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev > tunnel->err_count = 0; > } > > - max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen; > + max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen + rt->u.dst.header_len; > > if (skb_headroom(skb) < max_headroom || skb_shared(skb)|| > (skb_cloned(skb) && !skb_clone_writable(skb, 0))) { > struct sk_buff *new_skb = skb_realloc_headroom(skb, max_headroom); > + if (max_headroom > dev->needed_headroom) > + dev->needed_headroom = max_headroom; Are you sure about this? LL_RESERVED_SPACE already includes dev->needed_headroom so won't this get bigger each time? Cheers,
Herbert Xu wrote: > On Sat, Mar 20, 2010 at 02:27:58PM +0200, Timo Teras wrote: >> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c >> index f47c9f7..f78402d 100644 >> --- a/net/ipv4/ip_gre.c >> +++ b/net/ipv4/ip_gre.c >> @@ -810,11 +810,13 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev >> tunnel->err_count = 0; >> } >> >> - max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen; >> + max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen + rt->u.dst.header_len; >> >> if (skb_headroom(skb) < max_headroom || skb_shared(skb)|| >> (skb_cloned(skb) && !skb_clone_writable(skb, 0))) { >> struct sk_buff *new_skb = skb_realloc_headroom(skb, max_headroom); >> + if (max_headroom > dev->needed_headroom) >> + dev->needed_headroom = max_headroom; > > Are you sure about this? LL_RESERVED_SPACE already includes > dev->needed_headroom so won't this get bigger each time? Whoops. Must've been after-midnight issue. And not noticed since the max_headroom won't change many times. dev->needed_headroom should be compared against gre_hlen+rt->u.dst.header_len. I'll send a fixed patch in a jiffy. - timo -- 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
Timo Teräs wrote: > Herbert Xu wrote: >> On Sat, Mar 20, 2010 at 02:27:58PM +0200, Timo Teras wrote: >>> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c >>> index f47c9f7..f78402d 100644 >>> --- a/net/ipv4/ip_gre.c >>> +++ b/net/ipv4/ip_gre.c >>> @@ -810,11 +810,13 @@ static netdev_tx_t ipgre_tunnel_xmit(struct >>> sk_buff *skb, struct net_device *dev >>> tunnel->err_count = 0; >>> } >>> >>> - max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen; >>> + max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen + >>> rt->u.dst.header_len; >>> >>> if (skb_headroom(skb) < max_headroom || skb_shared(skb)|| >>> (skb_cloned(skb) && !skb_clone_writable(skb, 0))) { >>> struct sk_buff *new_skb = skb_realloc_headroom(skb, >>> max_headroom); >>> + if (max_headroom > dev->needed_headroom) >>> + dev->needed_headroom = max_headroom; >> >> Are you sure about this? LL_RESERVED_SPACE already includes >> dev->needed_headroom so won't this get bigger each time? > > Whoops. Must've been after-midnight issue. And not noticed since the > max_headroom won't change many times. > > dev->needed_headroom should be compared against > gre_hlen+rt->u.dst.header_len. > I'll send a fixed patch in a jiffy. Actually, isn't the above right? max_headroom is calculated with LL_RESERVED_SPACE of the tdev, which is the interface to which the gre packet is being sent to, not the gre interface. Thus, max_headroom won't include gre devices previous needed_headroom. - Timo -- 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 Sat, Mar 20, 2010 at 04:47:28PM +0200, Timo Teräs wrote: > > Actually, isn't the above right? > > max_headroom is calculated with LL_RESERVED_SPACE of the tdev, which > is the interface to which the gre packet is being sent to, not the > gre interface. Thus, max_headroom won't include gre devices > previous needed_headroom. Indeed you're right. Sorry for the confusion. Acked-by: Herbert Xu <herbert@gondor.apana.org.au> Cheers,
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Sat, 20 Mar 2010 23:00:49 +0800 > On Sat, Mar 20, 2010 at 04:47:28PM +0200, Timo Teräs wrote: >> >> Actually, isn't the above right? >> >> max_headroom is calculated with LL_RESERVED_SPACE of the tdev, which >> is the interface to which the gre packet is being sent to, not the >> gre interface. Thus, max_headroom won't include gre devices >> previous needed_headroom. > > Indeed you're right. Sorry for the confusion. > > Acked-by: Herbert Xu <herbert@gondor.apana.org.au> Applied, thanks everyone. -- 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/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index f47c9f7..f78402d 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -810,11 +810,13 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev tunnel->err_count = 0; } - max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen; + max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen + rt->u.dst.header_len; if (skb_headroom(skb) < max_headroom || skb_shared(skb)|| (skb_cloned(skb) && !skb_clone_writable(skb, 0))) { struct sk_buff *new_skb = skb_realloc_headroom(skb, max_headroom); + if (max_headroom > dev->needed_headroom) + dev->needed_headroom = max_headroom; if (!new_skb) { ip_rt_put(rt); txq->tx_dropped++;
Taking route's header_len into account, and updating gre device needed_headroom will give better hints on upper bound of required headroom. This is useful if the gre traffic is xfrm'ed. Signed-off-by: Timo Teras <timo.teras@iki.fi> Cc: Herbert Xu <herbert@gondor.apana.org.au> --- net/ipv4/ip_gre.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) This was earlier discussed in netdev thread: http://marc.info/?t=124470870200004&r=1&w=2 I just never got to writing the patch until now.