[PATCH]: fix lro_gen_skb() alignment

[PATCH]: fix lro_gen_skb() alignment

[Andrew Gallatin]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]

The inet_lro.c:lro_gen_skb() function fails to include
NET_IP_ALIGN padding at the front of the sk_buffs it creates,
leading to alignment warnings on architectures which require
strict alignment (seen on sparc64).  The attached patch
adds NET_IP_ALIGN padding.

Signed off by: Andrew Gallatin <gallatin@myri.com>

Drew


[-- Attachment #2: lro_align.diff --]
[-- Type: text/plain, Size: 508 bytes --]

diff --git a/net/ipv4/inet_lro.c b/net/ipv4/inet_lro.c
index ac3b1d3..91e9371 100644
--- a/net/ipv4/inet_lro.c
+++ b/net/ipv4/inet_lro.c
@@ -401,10 +401,11 @@ static struct sk_buff *lro_gen_skb(struc
 	int data_len = len;
 	int hdr_len = min(len, hlen);
 
-	skb = netdev_alloc_skb(lro_mgr->dev, hlen);
+	skb = netdev_alloc_skb(lro_mgr->dev, hlen + NET_IP_ALIGN);
 	if (!skb)
 		return NULL;
 
+	skb_reserve(skb, NET_IP_ALIGN);
 	skb->len = len;
 	skb->data_len = len - hdr_len;
 	skb->truesize += true_size;

Re: [PATCH]: fix lro_gen_skb() alignment

[Herbert Xu]

Andrew Gallatin <gallatin@myri.com> wrote:
>
> diff --git a/net/ipv4/inet_lro.c b/net/ipv4/inet_lro.c
> index ac3b1d3..91e9371 100644
> --- a/net/ipv4/inet_lro.c
> +++ b/net/ipv4/inet_lro.c
> @@ -401,10 +401,11 @@ static struct sk_buff *lro_gen_skb(struc
>        int data_len = len;
>        int hdr_len = min(len, hlen);
> 
> -       skb = netdev_alloc_skb(lro_mgr->dev, hlen);
> +       skb = netdev_alloc_skb(lro_mgr->dev, hlen + NET_IP_ALIGN);

NET_IP_ALIGN should only be used if you're DMAing into the skb head.
Otherwise you should say 2.  It would be nice to have another macro
for that I suppose.

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: [PATCH]: fix lro_gen_skb() alignment

[Andrew Gallatin]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]

Herbert Xu wrote:
> Andrew Gallatin <gallatin@myri.com> wrote:
>> diff --git a/net/ipv4/inet_lro.c b/net/ipv4/inet_lro.c
>> index ac3b1d3..91e9371 100644
>> --- a/net/ipv4/inet_lro.c
>> +++ b/net/ipv4/inet_lro.c
>> @@ -401,10 +401,11 @@ static struct sk_buff *lro_gen_skb(struc
>>        int data_len = len;
>>        int hdr_len = min(len, hlen);
>>
>> -       skb = netdev_alloc_skb(lro_mgr->dev, hlen);
>> +       skb = netdev_alloc_skb(lro_mgr->dev, hlen + NET_IP_ALIGN);
> 
> NET_IP_ALIGN should only be used if you're DMAing into the skb head.
> Otherwise you should say 2.  It would be nice to have another macro
> for that I suppose.

It is certainly simple enough to say 2.  Thank you for pointing
this out.  I have attached a patch to do that..

Signed off by: Andrew Gallatin <gallatin@myri.com>

Drew

[-- Attachment #2: lro_align_2.diff --]
[-- Type: text/plain, Size: 486 bytes --]

diff --git a/net/ipv4/inet_lro.c b/net/ipv4/inet_lro.c
index ac3b1d3..35b816e 100644
--- a/net/ipv4/inet_lro.c
+++ b/net/ipv4/inet_lro.c
@@ -401,10 +401,11 @@ static struct sk_buff *lro_gen_skb(struc
 	int data_len = len;
 	int hdr_len = min(len, hlen);
 
-	skb = netdev_alloc_skb(lro_mgr->dev, hlen);
+	skb = netdev_alloc_skb(lro_mgr->dev, hlen + 2);
 	if (!skb)
 		return NULL;
 
+	skb_reserve(skb, 2);
 	skb->len = len;
 	skb->data_len = len - hdr_len;
 	skb->truesize += true_size;

Re: [PATCH]: fix lro_gen_skb() alignment

[Roland Dreier]

 > >> -       skb = netdev_alloc_skb(lro_mgr->dev, hlen);
 > >> +       skb = netdev_alloc_skb(lro_mgr->dev, hlen + NET_IP_ALIGN);
 > > NET_IP_ALIGN should only be used if you're DMAing into the skb head.
 > > Otherwise you should say 2.  It would be nice to have another macro
 > > for that I suppose.
 > 
 > It is certainly simple enough to say 2.  Thank you for pointing
 > this out.  I have attached a patch to do that..
 > 
 > Signed off by: Andrew Gallatin <gallatin@myri.com>

Isn't the value of 2 ethernet-specific (to round the 14-byte header up
to 16)?  Given that the rest of the lro code is fairly careful to
calculate mac_hdr_len etc it seems as if it would be cleaner to make
this independent of the specific L2 being used.

(And I plan on using the LRO module for IP-over-InfiniBand so this is
not completely theoretical)

 - R.

Re: [PATCH]: fix lro_gen_skb() alignment

[Andrew Gallatin]

Roland Dreier wrote:
>  > >> -       skb = netdev_alloc_skb(lro_mgr->dev, hlen);
>  > >> +       skb = netdev_alloc_skb(lro_mgr->dev, hlen + NET_IP_ALIGN);
>  > > NET_IP_ALIGN should only be used if you're DMAing into the skb head.
>  > > Otherwise you should say 2.  It would be nice to have another macro
>  > > for that I suppose.
>  > 
>  > It is certainly simple enough to say 2.  Thank you for pointing
>  > this out.  I have attached a patch to do that..
>  > 
>  > Signed off by: Andrew Gallatin <gallatin@myri.com>
> 
> Isn't the value of 2 ethernet-specific (to round the 14-byte header up
> to 16)?  Given that the rest of the lro code is fairly careful to
> calculate mac_hdr_len etc it seems as if it would be cleaner to make
> this independent of the specific L2 being used.
> 
> (And I plan on using the LRO module for IP-over-InfiniBand so this is
> not completely theoretical)

Good point.  I tend to think all the world is ethernet.
Perhaps the better way would be to simply add an alignment pad
field to lro_mgr?  When the driver initializes it, it specifies
any padding needed.  Ethernet drivers would specify 2.

Is this acceptable?

Drew

Re: [PATCH]: fix lro_gen_skb() alignment

[Herbert Xu]

On Fri, Nov 30, 2007 at 02:35:43PM -0500, Andrew Gallatin wrote:
>
> >Isn't the value of 2 ethernet-specific (to round the 14-byte header up
> >to 16)?  Given that the rest of the lro code is fairly careful to
> >calculate mac_hdr_len etc it seems as if it would be cleaner to make
> >this independent of the specific L2 being used.
> >
> >(And I plan on using the LRO module for IP-over-InfiniBand so this is
> >not completely theoretical)

Good point!

We really should rename NET_IP_ALIGN so that both Ethernet and DMA
occur in it somehow :)

> Good point.  I tend to think all the world is ethernet.
> Perhaps the better way would be to simply add an alignment pad
> field to lro_mgr?  When the driver initializes it, it specifies
> any padding needed.  Ethernet drivers would specify 2.

Just pass in the mac_hdr_len, and calculate the padding as

	ALIGN(mac_hdr_len, 4) - mac_hdr_len

or even simpler

	~(mac_hdr_len - 1) & 3

Bonus points for putting this in a macro alongside NET_IP_ALIGN :)

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: [PATCH]: fix lro_gen_skb() alignment

[Andrew Gallatin]

Herbert Xu wrote:
> On Fri, Nov 30, 2007 at 02:35:43PM -0500, Andrew Gallatin wrote:
>>> Isn't the value of 2 ethernet-specific (to round the 14-byte header up
>>> to 16)?  Given that the rest of the lro code is fairly careful to
>>> calculate mac_hdr_len etc it seems as if it would be cleaner to make
>>> this independent of the specific L2 being used.
>>>
>>> (And I plan on using the LRO module for IP-over-InfiniBand so this is
>>> not completely theoretical)
> 
> Good point!
> 
> We really should rename NET_IP_ALIGN so that both Ethernet and DMA
> occur in it somehow :)
> 
>> Good point.  I tend to think all the world is ethernet.
>> Perhaps the better way would be to simply add an alignment pad
>> field to lro_mgr?  When the driver initializes it, it specifies
>> any padding needed.  Ethernet drivers would specify 2.
> 
> Just pass in the mac_hdr_len, and calculate the padding as

That was my first thought as well, but it turns out that
when lro_gen_skb() is called via the out1 label, mac_hdr_len
may not be known.  It seemed simplest and cleanest to just
make it a field in lro_mgr.

Drew

Re: [PATCH]: fix lro_gen_skb() alignment

[Herbert Xu]

On Sun, Dec 02, 2007 at 07:07:26PM -0500, Andrew Gallatin wrote:
> 
> That was my first thought as well, but it turns out that
> when lro_gen_skb() is called via the out1 label, mac_hdr_len
> may not be known.  It seemed simplest and cleanest to just
> make it a field in lro_mgr.

Fair enough.

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