Allow IP header alignment to be overriden

Allow IP header alignment to be overriden

[Anton Blanchard]

Hi,

The networking layer currently aligns IP headers in rx packets. It does
this via skb_reserve(,2).

On some architectures (like ppc64) we handle most unaligned accesses in
hardware. This means we gain little from this header alignment. However
forcing this alignment means we attempt to DMA from an unaligned
address.

In the lab we see DMAs beginning at 2 bytes into the page. On some of
our chips we have to do power of 2 writes of increasing size until we
hit a reasonable alignment. It was noticeable on gigabit and now with
10Gbit appearing its becoming a real problem.

Id be surprised if other architectures arent seeing similar issues, with
bridges that disconnect at power of two boundaries.

The following patch creates skb_align and allows an architecture to
override it. Thoughts?

Anton

===== drivers/net/acenic.c 1.44 vs edited =====
--- 1.44/drivers/net/acenic.c	Tue Apr  6 18:01:26 2004
+++ edited/drivers/net/acenic.c	Fri Jun 11 08:09:27 2004
@@ -1695,7 +1695,7 @@
 		/*
 		 * Make sure IP header starts on a fresh cache line.
 		 */
-		skb_reserve(skb, 2 + 16);
+		skb_align(skb, 2 + 16);
 		mapping = pci_map_page(ap->pdev, virt_to_page(skb->data),
 				       offset_in_page(skb->data),
 				       ACE_STD_BUFSIZE - (2 + 16),
@@ -1761,7 +1761,7 @@
 		/*
 		 * Make sure the IP header ends up on a fresh cache line
 		 */
-		skb_reserve(skb, 2 + 16);
+		skb_align(skb, 2 + 16);
 		mapping = pci_map_page(ap->pdev, virt_to_page(skb->data),
 				       offset_in_page(skb->data),
 				       ACE_MINI_BUFSIZE - (2 + 16),
@@ -1822,7 +1822,7 @@
 		/*
 		 * Make sure the IP header ends up on a fresh cache line
 		 */
-		skb_reserve(skb, 2 + 16);
+		skb_align(skb, 2 + 16);
 		mapping = pci_map_page(ap->pdev, virt_to_page(skb->data),
 				       offset_in_page(skb->data),
 				       ACE_JUMBO_BUFSIZE - (2 + 16),
===== drivers/net/e100.c 1.15 vs edited =====
--- 1.15/drivers/net/e100.c	Sat Jun  5 01:49:59 2004
+++ edited/drivers/net/e100.c	Fri Jun 11 08:00:49 2004
@@ -1395,7 +1395,7 @@
 
 	/* Align, init, and map the RFD. */
 	rx->skb->dev = nic->netdev;
-	skb_reserve(rx->skb, rx_offset);
+	skb_align(rx->skb, rx_offset);
 	memcpy(rx->skb->data, &nic->blank_rfd, sizeof(struct rfd));
 	rx->dma_addr = pci_map_single(nic->pdev, rx->skb->data,
 		RFD_BUF_LEN, PCI_DMA_BIDIRECTIONAL);
===== drivers/net/s2io.c 1.5 vs edited =====
--- 1.5/drivers/net/s2io.c	Fri Jun  4 12:00:15 2004
+++ edited/drivers/net/s2io.c	Fri Jun 11 08:06:52 2004
@@ -1431,7 +1431,7 @@
 			DBG_PRINT(ERR_DBG, "memory to allocate SKBs\n");
 			return -ENOMEM;
 		}
-		skb_reserve(skb, HEADER_ALIGN_LAYER_3);
+		skb_align(skb, HEADER_ALIGN_LAYER_3);
 		memset(rxdp, 0, sizeof(RxD_t));
 		rxdp->Buffer0_ptr = pci_map_single
 		    (nic->pdev, skb->data, size, PCI_DMA_FROMDEVICE);
===== drivers/net/tg3.c 1.180 vs edited =====
--- 1.180/drivers/net/tg3.c	Sat Jun  5 01:49:59 2004
+++ edited/drivers/net/tg3.c	Fri Jun 11 08:07:28 2004
@@ -2472,7 +2472,7 @@
 				goto drop_it_no_recycle;
 
 			copy_skb->dev = tp->dev;
-			skb_reserve(copy_skb, 2);
+			skb_align(copy_skb, 2);
 			skb_put(copy_skb, len);
 			pci_dma_sync_single_for_cpu(tp->pdev, dma_addr, len, PCI_DMA_FROMDEVICE);
 			memcpy(copy_skb->data, skb->data, len);
===== drivers/net/e1000/e1000_ethtool.c 1.45 vs edited =====
--- 1.45/drivers/net/e1000/e1000_ethtool.c	Fri May 28 06:59:25 2004
+++ edited/drivers/net/e1000/e1000_ethtool.c	Fri Jun 11 08:10:26 2004
@@ -1008,7 +1008,7 @@
 			ret_val = 6;
 			goto err_nomem;
 		}
-		skb_reserve(skb, 2);
+		skb_align(skb, 2);
 		rxdr->buffer_info[i].skb = skb;
 		rxdr->buffer_info[i].length = E1000_RXBUFFER_2048;
 		rxdr->buffer_info[i].dma =
===== drivers/net/e1000/e1000_main.c 1.118 vs edited =====
--- 1.118/drivers/net/e1000/e1000_main.c	Fri Jun  4 10:59:04 2004
+++ edited/drivers/net/e1000/e1000_main.c	Fri Jun 11 08:05:39 2004
@@ -2387,7 +2387,7 @@
 		 * this will result in a 16 byte aligned IP header after
 		 * the 14 byte MAC header is removed
 		 */
-		skb_reserve(skb, reserve_len);
+		skb_align(skb, reserve_len);
 
 		skb->dev = netdev;
 
===== drivers/net/ixgb/ixgb_main.c 1.13 vs edited =====
--- 1.13/drivers/net/ixgb/ixgb_main.c	Tue Jun  1 10:01:23 2004
+++ edited/drivers/net/ixgb/ixgb_main.c	Fri Jun 11 08:41:13 2004
@@ -1906,7 +1906,7 @@
 		 * this will result in a 16 byte aligned IP header after
 		 * the 14 byte MAC header is removed
 		 */
-		skb_reserve(skb, reserve_len);
+		skb_align(skb, reserve_len);
 
 		skb->dev = netdev;
 
===== include/asm-ppc64/system.h 1.28 vs edited =====
--- 1.28/include/asm-ppc64/system.h	Fri May 21 17:50:12 2004
+++ edited/include/asm-ppc64/system.h	Fri Jun 11 08:39:11 2004
@@ -277,5 +277,15 @@
 				    (unsigned long)_n_, sizeof(*(ptr))); \
   })
 
+/*
+ * We handle most unaligned accesses in hardware. On the other hand 
+ * unaligned DMA can be very expensive on some ppc64 IO chips (it does
+ * powers of 2 writes until it reaches sufficient alignment.
+ *
+ * Based on this we disable the IP header alignment in network drivers.
+ */
+#define ARCH_HAS_SKB_ALIGN
+#define skb_align(SKB, LEN)     do { } while (0)
+
 #endif /* __KERNEL__ */
 #endif
===== include/linux/skbuff.h 1.43 vs edited =====
--- 1.43/include/linux/skbuff.h	Mon May 31 05:09:46 2004
+++ edited/include/linux/skbuff.h	Fri Jun 11 08:27:42 2004
@@ -816,6 +816,20 @@
 	skb->tail += len;
 }
 
+/**
+ *	skb_align - align a buffer
+ *	@skb: buffer to alter
+ *	@len: bytes required to align
+ *
+ * 	Shift a buffer by len bytes for the purposes of alignment. On
+ * 	some architectures that handle unaligned accesses in hardware
+ * 	the effects of unaligned DMA is more costly so we allow it to
+ * 	be overridden. This is only allowed for an empty buffer.
+ */
+#ifndef ARCH_HAS_SKB_ALIGN
+#define skb_align(SKB, LEN)	skb_reserve((SKB), (LEN))
+#endif
+
 extern int ___pskb_trim(struct sk_buff *skb, unsigned int len, int realloc);
 
 static inline void __skb_trim(struct sk_buff *skb, unsigned int len)

Re: Allow IP header alignment to be overriden

[Andi Kleen]

> The following patch creates skb_align and allows an architecture to
> override it. Thoughts?

I like it. While we haven't seen specific networking performance issues
on Opteron yet it's certainly one architectures that doesn't care much
about misalignment. The Intel EM64T CPUs have a bit more penalty, but
also not much. It's certainly one optimization worth trying.

> + *	skb_align - align a buffer
> + *	@skb: buffer to alter
> + *	@len: bytes required to align
> + *
> + * 	Shift a buffer by len bytes for the purposes of alignment. On
> + * 	some architectures that handle unaligned accesses in hardware
> + * 	the effects of unaligned DMA is more costly so we allow it to
> + * 	be overridden. This is only allowed for an empty buffer.
> + */
> +#ifndef ARCH_HAS_SKB_ALIGN
> +#define skb_align(SKB, LEN)	skb_reserve((SKB), (LEN))
> +#endif

But where does this come from? There's no clear asm/ include in skbuff.h
matching it, and relying on indirect ones is probably not a good idea.

-Andi

Re: Allow IP header alignment to be overriden

[Anton Blanchard]

> > +#ifndef ARCH_HAS_SKB_ALIGN
> > +#define skb_align(SKB, LEN)	skb_reserve((SKB), (LEN))
> > +#endif
> 
> But where does this come from? There's no clear asm/ include in skbuff.h
> matching it, and relying on indirect ones is probably not a good idea.

Yeah I wasnt particularly happy about where I put the override.
skbuff.h directly includes:

#include <asm/atomic.h>
#include <asm/types.h>

...

#include <asm/system.h>

system.h gets included way down in the file, so I picked system.h as the 
most reasonable of the three. Im open to ideas as to how to make this neater.

Anton

Re: Allow IP header alignment to be overriden

[David S. Miller]

On Fri, 11 Jun 2004 11:27:27 +1000
Anton Blanchard <anton@samba.org> wrote:

> The following patch creates skb_align and allows an architecture to
> override it. Thoughts?

This transformation is not valid for a lot of drivers, that "2"
in the reserve exists elsewhere in other calculations in the
drivers.  For example, it is added to the RX skb allocation
size.  Sometimes this '2' is there in non-trivial or hard to
see ways (ie. it's implicitly in some DMA alignment value)

That's the only reason I'm against this patch.

Re: Allow IP header alignment to be overriden

[Scott Feldman]

On Thu, 2004-06-10 at 22:35, David S. Miller wrote:
> This transformation is not valid for a lot of drivers, that "2"
> in the reserve exists elsewhere in other calculations in the
> drivers.  For example, it is added to the RX skb allocation
> size.  Sometimes this '2' is there in non-trivial or hard to
> see ways (ie. it's implicitly in some DMA alignment value)

Would replacing "2" with a macro that's defined on a per-arch basis
work?  

Re: Allow IP header alignment to be overriden

[jamal]

On Fri, 2004-06-11 at 01:35, David S. Miller wrote:
> On Fri, 11 Jun 2004 11:27:27 +1000
> Anton Blanchard <anton@samba.org> wrote:
> 
> > The following patch creates skb_align and allows an architecture to
> > override it. Thoughts?
> 
> This transformation is not valid for a lot of drivers, that "2"
> in the reserve exists elsewhere in other calculations in the
> drivers.  For example, it is added to the RX skb allocation
> size.  Sometimes this '2' is there in non-trivial or hard to
> see ways (ie. it's implicitly in some DMA alignment value)

An interesting (annoying one) that falls into that category is
sb1250-mac.c::sbdma_align_skb()
shows up prominently in profiles

cheers,
jamal

Re: Allow IP header alignment to be overriden

[Anton Blanchard]

 
> This transformation is not valid for a lot of drivers, that "2"
> in the reserve exists elsewhere in other calculations in the
> drivers.  For example, it is added to the RX skb allocation
> size.  Sometimes this '2' is there in non-trivial or hard to
> see ways (ie. it's implicitly in some DMA alignment value)
> 
> That's the only reason I'm against this patch.

Yeah I started converting all drivers across to skb_align and quickly
noticed that. We can instead convert drivers over as they are tested,
the Gbit and 10Gbit cards being the most important.

Ive tested the 2 10Gbit cards and one 1Gbit (e1000). I can test the
acenic and should be able to hunt down a sungem and tg3.

That leaves dl2K, myricom, ns83820, hamachi, yellowfin, r8169, and sk98lin
which can be converted across as needed.

Anton

Re: Allow IP header alignment to be overriden

[Anton Blanchard]

 
Hi,

> Would replacing "2" with a macro that's defined on a per-arch basis
> work?

Nice idea. This would avoid us adding useless padding in that case, as
we currently do:

reserve_len = 2;
skb = dev_alloc_skb(adapter->rx_buffer_len + reserve_len);
skb_reserve(skb, reserve_len);

which would be nice if we are using a power of 2 buffer size. Would
creating:

/* 
 * Network drivers want to align IP headers. Since we have 14 bytes of
 * ethernet header, adding 2 bytes will align the IP header.  However
 * this will mean we do unaligned DMA so there is a trade off.
 *
 * We allow this to be overridden per arch as the unaligned DMA cost may
 * outweigh the unaligned CPU cost.
 */
#ifndef NET_IP_ALIGN
#define NET_IP_ALIGN 2
#endif

Instead of skb_align make more sense? It does have the advantage of
removing another magic number.

Anton

Re: Allow IP header alignment to be overriden

[David S. Miller]

On Sat, 12 Jun 2004 00:23:37 +1000
Anton Blanchard <anton@samba.org> wrote:

> Would creating:
> 
> /* 
>  * Network drivers want to align IP headers. Since we have 14 bytes of
>  * ethernet header, adding 2 bytes will align the IP header.  However
>  * this will mean we do unaligned DMA so there is a trade off.
>  *
>  * We allow this to be overridden per arch as the unaligned DMA cost may
>  * outweigh the unaligned CPU cost.
>  */
> #ifndef NET_IP_ALIGN
> #define NET_IP_ALIGN 2
> #endif
> 
> Instead of skb_align make more sense? It does have the advantage of
> removing another magic number.

Yes.  Please add a paragraph to that comment explaining what "unaligned
CPU cost" really means, ie. that the IP/TCP header members are going to
be accessed with alignment less than the types might require on a given
architecture.

Then I'll apply this and we can start beating up the drivers.

Re: Allow IP header alignment to be overriden

[Anton Blanchard]

 
> Yes.  Please add a paragraph to that comment explaining what "unaligned
> CPU cost" really means, ie. that the IP/TCP header members are going to
> be accessed with alignment less than the types might require on a given
> architecture.
> 
> Then I'll apply this and we can start beating up the drivers.

How does this look? The s2io, ixgb and e1000 drivers are converted.

Anton

===== include/linux/skbuff.h 1.43 vs edited =====
--- 1.43/include/linux/skbuff.h	Mon May 31 05:09:46 2004
+++ edited/include/linux/skbuff.h	Wed Jun 16 08:13:57 2004
@@ -816,6 +816,30 @@
 	skb->tail += len;
 }
 
+/*
+ * CPUs often take a performance hit when accessing unaligned memory
+ * locations. The actual performance hit varies, it can be small if the
+ * hardware handles it or large if we have to take an exception and fix it
+ * in software.
+ *
+ * Since an ethernet header is 14 bytes network drivers often end up with
+ * the IP header at an unaligned offset. The IP header can be aligned by
+ * shifting the start of the packet by 2 bytes. Drivers should do this
+ * with:
+ *
+ * skb_reserve(NET_IP_ALIGN);
+ *
+ * The downside to this alignment of the IP header is that the DMA is now
+ * unaligned. On some architectures the cost of an unaligned DMA is high
+ * and this cost outweighs the gains made by aligning the IP header.
+ * 
+ * Since this trade off varies between architectures, we allow NET_IP_ALIGN
+ * to be overridden.
+ */
+#ifndef NET_IP_ALIGN
+#define NET_IP_ALIGN	2
+#endif
+
 extern int ___pskb_trim(struct sk_buff *skb, unsigned int len, int realloc);
 
 static inline void __skb_trim(struct sk_buff *skb, unsigned int lenj
===== include/asm-ppc64/system.h 1.28 vs edited =====
--- 1.28/include/asm-ppc64/system.h	Fri May 21 17:50:12 2004
+++ edited/include/asm-ppc64/system.h	Wed Jun 16 08:15:28 2004
@@ -277,5 +277,14 @@
 				    (unsigned long)_n_, sizeof(*(ptr))); \
   })
 
+/*
+ * We handle most unaligned accesses in hardware. On the other hand 
+ * unaligned DMA can be very expensive on some ppc64 IO chips (it does
+ * powers of 2 writes until it reaches sufficient alignment).
+ *
+ * Based on this we disable the IP header alignment in network drivers.
+ */
+#define NET_IP_ALIGN   0
+
 #endif /* __KERNEL__ */
 #endif
===== drivers/net/s2io.c 1.5 vs edited =====
--- 1.5/drivers/net/s2io.c	Fri Jun  4 12:00:15 2004
+++ edited/drivers/net/s2io.c	Wed Jun 16 08:18:15 2004
@@ -1425,13 +1425,13 @@
 			goto end;
 		}
 
-		skb = dev_alloc_skb(size + HEADER_ALIGN_LAYER_3);
+		skb = dev_alloc_skb(size + NET_IP_ALIGN);
 		if (!skb) {
 			DBG_PRINT(ERR_DBG, "%s: Out of ", dev->name);
 			DBG_PRINT(ERR_DBG, "memory to allocate SKBs\n");
 			return -ENOMEM;
 		}
-		skb_reserve(skb, HEADER_ALIGN_LAYER_3);
+		skb_reserve(skb, NET_IP_ALIGN);
 		memset(rxdp, 0, sizeof(RxD_t));
 		rxdp->Buffer0_ptr = pci_map_single
 		    (nic->pdev, skb->data, size, PCI_DMA_FROMDEVICE);
===== drivers/net/s2io.h 1.4 vs edited =====
--- 1.4/drivers/net/s2io.h	Mon May 31 17:46:35 2004
+++ edited/drivers/net/s2io.h	Wed Jun 16 08:17:23 2004
@@ -411,7 +411,6 @@
 #define HEADER_802_2_SIZE              3
 #define HEADER_SNAP_SIZE               5
 #define HEADER_VLAN_SIZE               4
-#define HEADER_ALIGN_LAYER_3           2
 
 #define MIN_MTU                       46
 #define MAX_PYLD                    1500
===== drivers/net/e1000/e1000_ethtool.c 1.45 vs edited =====
--- 1.45/drivers/net/e1000/e1000_ethtool.c	Fri May 28 06:59:25 2004
+++ edited/drivers/net/e1000/e1000_ethtool.c	Wed Jun 16 08:34:38 2004
@@ -1004,11 +1004,12 @@
 		struct e1000_rx_desc *rx_desc = E1000_RX_DESC(*rxdr, i);
 		struct sk_buff *skb;
 
-		if(!(skb = alloc_skb(E1000_RXBUFFER_2048 + 2, GFP_KERNEL))) {
+		if(!(skb = alloc_skb(E1000_RXBUFFER_2048 + NET_IP_ALIGN,
+				     GFP_KERNEL))) {
 			ret_val = 6;
 			goto err_nomem;
 		}
-		skb_reserve(skb, 2);
+		skb_reserve(skb, NET_IP_ALIGN);
 		rxdr->buffer_info[i].skb = skb;
 		rxdr->buffer_info[i].length = E1000_RXBUFFER_2048;
 		rxdr->buffer_info[i].dma =
===== drivers/net/e1000/e1000_main.c 1.118 vs edited =====
--- 1.118/drivers/net/e1000/e1000_main.c	Fri Jun  4 10:59:04 2004
+++ edited/drivers/net/e1000/e1000_main.c	Wed Jun 16 08:36:58 2004
@@ -2367,7 +2367,6 @@
 	struct e1000_rx_desc *rx_desc;
 	struct e1000_buffer *buffer_info;
 	struct sk_buff *skb;
-	int reserve_len = 2;
 	unsigned int i;
 
 	i = rx_ring->next_to_use;
@@ -2376,7 +2375,7 @@
 	while(!buffer_info->skb) {
 		rx_desc = E1000_RX_DESC(*rx_ring, i);
 
-		skb = dev_alloc_skb(adapter->rx_buffer_len + reserve_len);
+		skb = dev_alloc_skb(adapter->rx_buffer_len + NET_IP_ALIGN);
 
 		if(!skb) {
 			/* Better luck next round */
@@ -2387,7 +2386,7 @@
 		 * this will result in a 16 byte aligned IP header after
 		 * the 14 byte MAC header is removed
 		 */
-		skb_reserve(skb, reserve_len);
+		skb_reserve(skb, NET_IP_ALIGN);
 
 		skb->dev = netdev;
 
===== drivers/net/ixgb/ixgb_main.c 1.13 vs edited =====
--- 1.13/drivers/net/ixgb/ixgb_main.c	Tue Jun  1 10:01:23 2004
+++ edited/drivers/net/ixgb/ixgb_main.c	Wed Jun 16 08:23:28 2004
@@ -1876,7 +1876,6 @@
 	struct ixgb_rx_desc *rx_desc;
 	struct ixgb_buffer *buffer_info;
 	struct sk_buff *skb;
-	int reserve_len = 2;
 	unsigned int i;
 	int num_group_tail_writes;
 	long cleancount;
@@ -1895,7 +1894,7 @@
 	while (--cleancount > 0) {
 		rx_desc = IXGB_RX_DESC(*rx_ring, i);
 
-		skb = dev_alloc_skb(adapter->rx_buffer_len + reserve_len);
+		skb = dev_alloc_skb(adapter->rx_buffer_len + NET_IP_ALIGN);
 
 		if (unlikely(!skb)) {
 			/* Better luck next round */
@@ -1906,7 +1905,7 @@
 		 * this will result in a 16 byte aligned IP header after
 		 * the 14 byte MAC header is removed
 		 */
-		skb_reserve(skb, reserve_len);
+		skb_reserve(skb, NET_IP_ALIGN);
 
 		skb->dev = netdev;
 

Re: Allow IP header alignment to be overriden

[David S. Miller]

On Wed, 16 Jun 2004 09:34:23 +1000
Anton Blanchard <anton@samba.org> wrote:

> > Yes.  Please add a paragraph to that comment explaining what "unaligned
> > CPU cost" really means, ie. that the IP/TCP header members are going to
> > be accessed with alignment less than the types might require on a given
> > architecture.
> > 
> > Then I'll apply this and we can start beating up the drivers.
> 
> How does this look? The s2io, ixgb and e1000 drivers are converted.

Works for me, applied.

Thanks Anton.