: fix lro_gen_skb() alignment

PATCH In review Andrew Gallatin 3 participants 8 messages Nov 27, 2007 – Dec 3, 2007

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Andrew Gallatin

author

2007-11-27 14:47 UTC #0 lore raw

PATCH 1/1 : fix lro_gen_skb() alignment

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 <[email protected]>

Drew

net/ipv4/inet_lro.c +2 -1

index ac3b1d3..91e9371 100644
@@ -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;

Herbert Xu

2007-11-30 09:59 UTC #1 lore raw

net/ipv4/inet_lro.c static struct sk_buff *lro_gen_skb(struc · line 404

Quoted 11 lines · Andrew Gallatin

net/ipv4/inet_lro.c +1 -1

index ac3b1d3..91e9371 100644
@@ -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,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Andrew Gallatin

author

2007-11-30 18:36 UTC #2 lore raw

Quoted 9 lines · Herbert Xu

Quoted 8 lines · Andrew Gallatin

net/ipv4/inet_lro.c +0 -0

index ac3b1d3..91e9371 100644
@@ -401,10 +401,11 @@ static struct sk_buff *lro_gen_skb(struc
       int data_len = len;
       int hdr_len = min(len, hlen);

Quoted 6 lines

net/ipv4/inet_lro.c static struct sk_buff *lro_gen_skb(struc · line 404
Quoted 2 lines
net/ipv4/inet_lro.c +1 -1
-       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 <[email protected]>

Drew

net/ipv4/inet_lro.c +2 -1

index ac3b1d3..35b816e 100644
@@ -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;

Roland Dreier

2007-11-30 19:14 UTC #3 lore raw

Quoted 10 lines

Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-       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 <[email protected]>

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.

Andrew Gallatin

author

2007-11-30 19:36 UTC #4 lore raw

Quoted 11 lines · Roland Dreier

Quoted 10 lines
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +1 -1
-       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 <[email protected]>

Quoted 7 lines

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

Herbert Xu

2007-11-30 22:35 UTC #5 lore raw

Quoted 8 lines · Andrew Gallatin

Quoted 8 lines
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 :)

Quoted 4 lines

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,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Andrew Gallatin

author

2007-12-03 00:08 UTC #6 lore raw

Quoted 14 lines · Herbert Xu

Quoted 7 lines · Andrew Gallatin
Quoted 7 lines
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 :)

Quoted 6 lines

Quoted 4 lines
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

Herbert Xu

2007-12-03 09:29 UTC #7 lore raw

Quoted 5 lines · Andrew Gallatin

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,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt