| Message ID | 4A30BECA.8030205@iki.fi |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Timo Teräs <timo.teras@iki.fi> Date: Thu, 11 Jun 2009 11:22:34 +0300 > I'm not entirely sure about this, but shouldn't we do something like > the below to make ip_gre allocation enough headroom for paths where > e.g. xfrm transformations take place (as a speed optimization)? This should be handled by the code one level up that allocates the SKB. And as routes get stacked, the MTUs and headroom values get adjusted up to the root. At least that's how it's supposed to work :-) Otherwise we'd need a similar hack in ipip.c too. -- 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
David Miller wrote: > From: Timo Teräs <timo.teras@iki.fi> > Date: Thu, 11 Jun 2009 11:22:34 +0300 > >> I'm not entirely sure about this, but shouldn't we do something like >> the below to make ip_gre allocation enough headroom for paths where >> e.g. xfrm transformations take place (as a speed optimization)? > > This should be handled by the code one level up that allocates > the SKB. > > And as routes get stacked, the MTUs and headroom values get adjusted > up to the root. > > At least that's how it's supposed to work :-) Ok, that makes sense. I was just wondering if there's any corner cases were this would have helped. > Otherwise we'd need a similar hack in ipip.c too. I was under the impression that as we need to do the route lookup anyway we could take advantage of the information available from there fully. But yes, your explanation makes perfect sense. Thanks, 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 Thu, Jun 11, 2009 at 01:17:14PM +0300, Timo Teräs wrote: > >> Otherwise we'd need a similar hack in ipip.c too. > > I was under the impression that as we need to do the route lookup anyway > we could take advantage of the information available from there fully. > > But yes, your explanation makes perfect sense. I think you're both right :) We should include the IPsec head room here, but also in the dev->needed_headroom calculation so that most packets don't get reallocated in the first place. And yes all tunnels are deficient in this regard. Cheers,
Herbert Xu wrote: > On Thu, Jun 11, 2009 at 01:17:14PM +0300, Timo Teräs wrote: >>> Otherwise we'd need a similar hack in ipip.c too. >> I was under the impression that as we need to do the route lookup anyway >> we could take advantage of the information available from there fully. >> >> But yes, your explanation makes perfect sense. > > I think you're both right :) > > We should include the IPsec head room here, but also in the > dev->needed_headroom calculation so that most packets don't > get reallocated in the first place. I guess this is easy for point-to-point tunnels. But how about the multipoint gre tunnels? The needed_headroom can vary on destination basis depending on if they are NATted or not, and on the cipher/hash used. Can we change the needed_headroom on-the-fly? Increase it when ever we encounter a larger path? But this also means some packages would get extra headroom allocated. - 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 Thu, Jun 11, 2009 at 02:29:46PM +0300, Timo Teräs wrote: > > I guess this is easy for point-to-point tunnels. But how about the > multipoint gre tunnels? The needed_headroom can vary on destination > basis depending on if they are NATted or not, and on the cipher/hash > used. I don't think we need to worry about NAT, well at least we don't worry about it on the normal path :) > Can we change the needed_headroom on-the-fly? Increase it when ever > we encounter a larger path? But this also means some packages would > get extra headroom allocated. Just make it the maximum. It's OK to have extra head space. If the head space is huge then you've got bigger problems than wasted memory :) Cheers,
Herbert Xu wrote: > On Thu, Jun 11, 2009 at 02:29:46PM +0300, Timo Teräs wrote: >> I guess this is easy for point-to-point tunnels. But how about the >> multipoint gre tunnels? The needed_headroom can vary on destination >> basis depending on if they are NATted or not, and on the cipher/hash >> used. > > I don't think we need to worry about NAT, well at least we don't > worry about it on the normal path :) It affects if ESP or UDP-over-ESP encapsulation is used. This can vary on destination IP basis. This different needed_headroom size. This the case only for multi-point tunnels. But yes, we don't need to worry about it, it just one factor that can affect what the actual needed_headroom can end up being. >> Can we change the needed_headroom on-the-fly? Increase it when ever >> we encounter a larger path? But this also means some packages would >> get extra headroom allocated. > > Just make it the maximum. It's OK to have extra head space. > If the head space is huge then you've got bigger problems than > wasted memory :) I guess so. The difference is not too many bytes. Is it ok to just change the dev->needed_headroom from xmit routine? - 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 Thu, Jun 11, 2009 at 02:51:26PM +0300, Timo Teräs wrote: > > I guess so. The difference is not too many bytes. Is it ok to just > change the dev->needed_headroom from xmit routine? I don't see why not. Also we only need to change it in xmit if we hit an skb that needs to be reallocated. Cheers,
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index e62510d..eb78dd9 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -812,7 +812,7 @@ static int 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))) {