ip_gre headroom allocation

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)?

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))) {

From: Timo Teräs <redacted>
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.

David Miller wrote:

From: Timo Teräs <redacted>
Date: Thu, 11 Jun 2009 11:22:34 +0300

quoted

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

On Thu, Jun 11, 2009 at 01:17:14PM +0300, Timo Teräs wrote:

quoted

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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Herbert Xu wrote:

On Thu, Jun 11, 2009 at 01:17:14PM +0300, Timo Teräs wrote:

quoted

quoted

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

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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Herbert Xu wrote:

On Thu, Jun 11, 2009 at 02:29:46PM +0300, Timo Teräs wrote:

quoted

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.

quoted

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

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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt