ip_gre headroom allocation

ip_gre headroom allocation

[Timo Teräs]

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

Re: ip_gre headroom allocation

[David Miller]

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.

Re: ip_gre headroom allocation

[Timo Teräs]

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

Re: ip_gre headroom allocation

[Herbert Xu]

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

Re: ip_gre headroom allocation

[Timo Teräs]

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

Re: ip_gre headroom allocation

[Herbert Xu]

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~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: ip_gre headroom allocation

[Timo Teräs]

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

Re: ip_gre headroom allocation

[Herbert Xu]

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~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt