Skip to contentSkip to conversation

net: macb: replace literal constant with NET_IP_ALIGN

PATCH In review Nicolae Rosia 6 participants 15 messages Jun 30, 2015 – Jul 6, 2015

Subsystems (3)
Networking Network drivers Atmel MACB Ethernet driver
lore
Share & tools
lore

Nicolae Rosia

author
#0 lore raw

PATCH 1/1 net: macb: replace literal constant with NET_IP_ALIGN

Signed-off-by Nicolae Rosia <[email protected]>
---
 drivers/net/ethernet/cadence/macb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/cadence/macb.c +2 -2
index caeb395..dbb5160 100644
@@ -2554,9 +2554,9 @@ static void at91ether_rx(struct net_device *dev)
 	while (lp->rx_ring[lp->rx_tail].addr & MACB_BIT(RX_USED)) {
 		p_recv = lp->rx_buffers + lp->rx_tail * AT91ETHER_MAX_RBUFF_SZ;
 		pktlen = MACB_BF(RX_FRMLEN, lp->rx_ring[lp->rx_tail].ctrl);
-		skb = netdev_alloc_skb(dev, pktlen + 2);
+		skb = netdev_alloc_skb(dev, pktlen + NET_IP_ALIGN);
 		if (skb) {
-			skb_reserve(skb, 2);
+			skb_reserve(skb, NET_IP_ALIGN);
 			memcpy(skb_put(skb, pktlen), p_recv, pktlen);
 
 			skb->protocol = eth_type_trans(skb, dev);
Signature · 1 line
2.1.0
Acked-by on v1 1/1
#1 lore raw
Quoted 2 lines · Nicolae Rosia
Signed-off-by Nicolae Rosia <[email protected]>
Acked-by Nicolas Ferre <[email protected]>

Thanks, bye.

Quoted 21 lines
---
 drivers/net/ethernet/cadence/macb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/cadence/macb.c +2 -2
index caeb395..dbb5160 100644
@@ -2554,9 +2554,9 @@ static void at91ether_rx(struct net_device *dev)
 	while (lp->rx_ring[lp->rx_tail].addr & MACB_BIT(RX_USED)) {
 		p_recv = lp->rx_buffers + lp->rx_tail * AT91ETHER_MAX_RBUFF_SZ;
 		pktlen = MACB_BF(RX_FRMLEN, lp->rx_ring[lp->rx_tail].ctrl);
-		skb = netdev_alloc_skb(dev, pktlen + 2);
+		skb = netdev_alloc_skb(dev, pktlen + NET_IP_ALIGN);
 		if (skb) {
-			skb_reserve(skb, 2);
+			skb_reserve(skb, NET_IP_ALIGN);
 			memcpy(skb_put(skb, pktlen), p_recv, pktlen);
 
 			skb->protocol = eth_type_trans(skb, dev);
Signature · 1 line
Nicolas Ferre
#2 lore raw
In reply to Nicolae Rosia
Quoted 11 lines · Nicolae Rosia
Signed-off-by Nicolae Rosia <[email protected]>
---
 drivers/net/ethernet/cadence/macb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/cadence/macb.c +0 -0
index caeb395..dbb5160 100644
@@ -2554,9 +2554,9 @@ static void at91ether_rx(struct net_device *dev)
 	while (lp->rx_ring[lp->rx_tail].addr & MACB_BIT(RX_USED)) {

drivers/net/ethernet/cadence/macb.c static void at91ether_rx(struct net_device *dev) · line 2562

Quoted 10 lines
drivers/net/ethernet/cadence/macb.c +2 -2
 		p_recv = lp->rx_buffers + lp->rx_tail * AT91ETHER_MAX_RBUFF_SZ;
 		pktlen = MACB_BF(RX_FRMLEN, lp->rx_ring[lp->rx_tail].ctrl);
-		skb = netdev_alloc_skb(dev, pktlen + 2);
+		skb = netdev_alloc_skb(dev, pktlen + NET_IP_ALIGN);
 		if (skb) {
-			skb_reserve(skb, 2);
+			skb_reserve(skb, NET_IP_ALIGN);
 			memcpy(skb_put(skb, pktlen), p_recv, pktlen);
 
 			skb->protocol = eth_type_trans(skb, dev);

Then please use netdev_alloc_skb_ip_align(), so that you get rid of skb_reserve()

author
#3 lore raw
Quoted 2 lines · Eric Dumazet

Then please use netdev_alloc_skb_ip_align(), so that you get rid of skb_reserve()

Thank you for the suggestion.
I can do that.
A related question, should I also replace netdev_alloc with napi_alloc_skb in places where I have a napi struct?

#4 lore raw
Quoted 7 lines · Nicolae Rosia
Quoted 2 lines · Eric Dumazet

Then please use netdev_alloc_skb_ip_align(), so that you get rid of skb_reserve()

Thank you for the suggestion.
I can do that.
A related question, should I also replace netdev_alloc with napi_alloc_skb in places where I have a napi struct?

I really doubt this adapter can process millions of packets per second ?

I would rather enable GRO, it would be more useful.

author
#5 lore raw
Quoted 1 line · Eric Dumazet

I really doubt this adapter can process millions of packets per second ?

I was suggesting this since I was taking into consideration the comment from skbuff.c, "we can save several CPU cycles by avoiding having to disable and re-enable IRQs." Is there a downside to this?

Quoted 2 lines

I would rather enable GRO, it would be more useful.

I had no idea what GRO is, so I have read about it [0] and looked at a couple of drivers which use it. They all seem to replace netif_receive_skb with napi_gro_receive and when there are no more packets in napi_pool they call napi_gro_flush. Is it that simple?

Regards

[0] https://lwn.net/Articles/358910/

#6 lore raw
Quoted 6 lines · Nicolae Rosia
Quoted 1 line · Eric Dumazet

I really doubt this adapter can process millions of packets per second ?

I was suggesting this since I was taking into consideration the comment from skbuff.c, "we can save several CPU cycles by avoiding having to disable and re-enable IRQs."
Is there a downside to this?

This only matters in terms of few nano seconds per packet, so for 10Gb+ NIC anyway. Absolute noise for most NIC.

Quoted 8 lines
Quoted 3 lines

I would rather enable GRO, it would be more useful.

I had no idea what GRO is, so I have read about it [0] and looked at a couple of drivers which use it. They all seem to
replace netif_receive_skb with napi_gro_receive and when there are no more packets in napi_pool they call napi_gro_flush.
Is it that simple?

Yes, but main question is : Do you have the hardware to test your changes ?

#7 lore raw

On Wed, Jul 1, 2015 at 6:33 PM, Eric Dumazet <[email protected]> wrote:
[...]

Quoted 3 lines

This only matters in terms of few nano seconds per packet, so for 10Gb+ NIC anyway. Absolute noise for most NIC.

I'll give it a try and benchmark.
I achieved a huge speedup by moving TX into napi [0], but my hardware doesn't support multiple TX queues and I can't test that situation.

Quoted 2 lines

Yes, but main question is : Do you have the hardware to test your changes ?

Yes, I have a Xilinx ZC706 board with a Zynq7 XC7Z045 processor [1]

[0] https://patchwork.ozlabs.org/patch/488949/
[1] http://www.xilinx.com/products/boards-and-kits/ek-z7-zc706-g.html

#8 lore raw
Quoted 9 lines · Nicolae Rosia

On Wed, Jul 1, 2015 at 6:33 PM, Eric Dumazet <[email protected]> wrote:
[...]

Quoted 3 lines

This only matters in terms of few nano seconds per packet, so for 10Gb+ NIC anyway. Absolute noise for most NIC.

I'll give it a try and benchmark.
I achieved a huge speedup by moving TX into napi [0], but my hardware doesn't support multiple TX queues and I can't test that situation.

Quoted 6 lines
Quoted 2 lines

Yes, but main question is : Do you have the hardware to test your changes ?

Yes, I have a Xilinx ZC706 board with a Zynq7 XC7Z045 processor [1]

[0] https://patchwork.ozlabs.org/patch/488949/
[1] http://www.xilinx.com/products/boards-and-kits/ek-z7-zc706-g.html

OK, then enabling GRO should be quite easy, given driver already has most of it.

drivers/net/ethernet/cadence/macb.c +2 -2
index caeb39561567237261ac0d50befebad666cfbeb3..24a93c769caa5430ca61efe002b458fef7281e99 100644
@@ -815,7 +815,7 @@ static int gem_rx(struct macb *bp, int budget)
 			       skb->data, 32, true);
 #endif
 
-		netif_receive_skb(skb);
+		napi_gro_receive(&bp->napi, skb);
 	}
 
 	gem_rx_refill(bp);

@@ -896,7 +896,7 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 	bp->stats.rx_bytes += skb->len;
 	netdev_vdbg(bp->dev, "received skb of length %u, csum: %08x\n",
 		   skb->len, skb->csum);
-	netif_receive_skb(skb);
+	napi_gro_receive(&bp->napi, skb);
 
 	return 0;
 }
#9 lore raw
In reply to Eric Dumazet
Quoted 2 lines · Eric Dumazet

drivers/net/ethernet/cadence/macb.c

Quoted 1 line · Nicolae Rosia
drivers/net/ethernet/cadence/macb.c +0 -0

[]

Quoted 11 lines
Quoted 11 lines
drivers/net/ethernet/cadence/macb.c +2 -2
@@ -2554,9 +2554,9 @@ static void at91ether_rx(struct net_device *dev)
 	while (lp->rx_ring[lp->rx_tail].addr & MACB_BIT(RX_USED)) {
 		p_recv = lp->rx_buffers + lp->rx_tail * AT91ETHER_MAX_RBUFF_SZ;
 		pktlen = MACB_BF(RX_FRMLEN, lp->rx_ring[lp->rx_tail].ctrl);
-		skb = netdev_alloc_skb(dev, pktlen + 2);
+		skb = netdev_alloc_skb(dev, pktlen + NET_IP_ALIGN);
 		if (skb) {
-			skb_reserve(skb, 2);
+			skb_reserve(skb, NET_IP_ALIGN);
 			memcpy(skb_put(skb, pktlen), p_recv, pktlen);
Quoted 4 lines

drivers/net/ethernet/cadence/macb.c static void at91ether_rx(struct net_device *dev) · line 2562

Quoted 1 line
drivers/net/ethernet/cadence/macb.c +0 -0
 			skb->protocol = eth_type_trans(skb, dev);

Then please use netdev_alloc_skb_ip_align(), so that you get rid of skb_reserve()

It seems there are ~50 of these in the kernel tree that could be converted.

#10 lore raw
Quoted 15 lines · Joe Perches
Quoted 2 lines · Eric Dumazet

drivers/net/ethernet/cadence/macb.c

Quoted 1 line · Nicolae Rosia
drivers/net/ethernet/cadence/macb.c +0 -0

[]

Quoted 11 lines
Quoted 11 lines
drivers/net/ethernet/cadence/macb.c +2 -2
@@ -2554,9 +2554,9 @@ static void at91ether_rx(struct net_device *dev)
 	while (lp->rx_ring[lp->rx_tail].addr & MACB_BIT(RX_USED)) {
 		p_recv = lp->rx_buffers + lp->rx_tail * AT91ETHER_MAX_RBUFF_SZ;
 		pktlen = MACB_BF(RX_FRMLEN, lp->rx_ring[lp->rx_tail].ctrl);
-		skb = netdev_alloc_skb(dev, pktlen + 2);
+		skb = netdev_alloc_skb(dev, pktlen + NET_IP_ALIGN);
 		if (skb) {
-			skb_reserve(skb, 2);
+			skb_reserve(skb, NET_IP_ALIGN);
 			memcpy(skb_put(skb, pktlen), p_recv, pktlen);
Quoted 8 lines
Quoted 4 lines

drivers/net/ethernet/cadence/macb.c static void at91ether_rx(struct net_device *dev) · line 2562

Quoted 1 line
drivers/net/ethernet/cadence/macb.c +0 -0
 			skb->protocol = eth_type_trans(skb, dev);

Then please use netdev_alloc_skb_ip_align(), so that you get rid of skb_reserve()

It seems there are ~50 of these in the kernel tree that could be converted.

Make sure the 2 is really NET_IP_ALIGN

Some hardwares need 2, even if NET_IP_ALIGN is 0 (on x86 arches for example)

I would rather not touch this without testing the change on real hardware.

#11 lore raw
Quoted 25 lines · Eric Dumazet
Quoted 15 lines · Joe Perches
Quoted 2 lines · Eric Dumazet

drivers/net/ethernet/cadence/macb.c

Quoted 1 line · Nicolae Rosia
drivers/net/ethernet/cadence/macb.c +0 -0

[]

Quoted 11 lines
Quoted 11 lines
drivers/net/ethernet/cadence/macb.c +2 -2
@@ -2554,9 +2554,9 @@ static void at91ether_rx(struct net_device *dev)
 	while (lp->rx_ring[lp->rx_tail].addr & MACB_BIT(RX_USED)) {
 		p_recv = lp->rx_buffers + lp->rx_tail * AT91ETHER_MAX_RBUFF_SZ;
 		pktlen = MACB_BF(RX_FRMLEN, lp->rx_ring[lp->rx_tail].ctrl);
-		skb = netdev_alloc_skb(dev, pktlen + 2);
+		skb = netdev_alloc_skb(dev, pktlen + NET_IP_ALIGN);
 		if (skb) {
-			skb_reserve(skb, 2);
+			skb_reserve(skb, NET_IP_ALIGN);
 			memcpy(skb_put(skb, pktlen), p_recv, pktlen);
Quoted 8 lines
Quoted 4 lines

drivers/net/ethernet/cadence/macb.c static void at91ether_rx(struct net_device *dev) · line 2562

Quoted 1 line
drivers/net/ethernet/cadence/macb.c +0 -0
 			skb->protocol = eth_type_trans(skb, dev);

Then please use netdev_alloc_skb_ip_align(), so that you get rid of skb_reserve()

It seems there are ~50 of these in the kernel tree that could be converted.

Quoted 7 lines

Make sure the 2 is really NET_IP_ALIGN

Some hardwares need 2, even if NET_IP_ALIGN is 0 (on x86 arches for example)

I would rather not touch this without testing the change on real hardware.

Nor I really.

Most all of those are in fairly old hardware drivers.

I just wanted to point out that more exist.

#12 lore raw
In reply to Eric Dumazet

From: Eric Dumazet

Quoted 2 lines

Sent: 01 July 2015 23:14
To: Joe Perches

...

Quoted 11 lines
Quoted 6 lines
Quoted 2 lines

Then please use netdev_alloc_skb_ip_align(), so that you get rid of skb_reserve()

It seems there are ~50 of these in the kernel tree that could be converted.

Make sure the 2 is really NET_IP_ALIGN

Some hardwares need 2, even if NET_IP_ALIGN is 0 (on x86 arches for example)

Even on x86 aligning the ethernet receive data on a 4n+2 boundary is likely to give marginally better performance than aligning on a 4n boundary.

David

Acked-by H. Peter Anvin
#13 lore raw
Quoted 3 lines · David Laight

Even on x86 aligning the ethernet receive data on a 4n+2 boundary is likely to give marginally better performance than aligning on a 4n boundary.

You are coming late to the party.

Intel guys decided to change NET_IP_ALIGN to 0 (it was 2 in the past)

commit ea812ca1b06113597adcd8e70c0f84a413d97544

Author: Alexander Duyck <[email protected]>
Date:   Tue Jun 29 18:38:00 2010 +0000

x86: Align skb w/ start of cacheline on newer core 2/Xeon Arch

x86 architectures can handle unaligned accesses in hardware, and it has been shown that unaligned DMA accesses can be expensive on Nehalem architectures. As such we should overwrite NET_IP_ALIGN to resolve this issue.

Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc [email protected]
Signed-off-by Alexander Duyck <[email protected]>
Signed-off-by Jeff Kirsher <[email protected]>
Acked-by H. Peter Anvin <[email protected]>
Signed-off-by David S. Miller <[email protected]>
#14 lore raw

From: Eric Dumazet [mailto:[email protected]]

Quoted 8 lines

Sent: 03 July 2015 17:39

Quoted 3 lines · David Laight

Even on x86 aligning the ethernet receive data on a 4n+2 boundary is likely to give marginally better performance than aligning on a 4n boundary.

You are coming late to the party.

I've been to many parties at many different times....

Going back many years, Sun's original sbus DMA part generated a lot of single sbus transfers for 4n+2 aligned buffers - so it was necessary to do a 'realignment' copy. The later DMA+ (definitely the DMA2) part did sbus burst transfers even when the buffer was 4n+2 aligned. So with the later parts you could correctly align the buffer.

Quoted 1 line

Intel guys decided to change NET_IP_ALIGN to 0 (it was 2 in the past)

...

Quoted 6 lines

x86: Align skb w/ start of cacheline on newer core 2/Xeon Arch

x86 architectures can handle unaligned accesses in hardware, and it has been shown that unaligned DMA accesses can be expensive on Nehalem architectures. As such we should overwrite NET_IP_ALIGN to resolve this issue.

My 2 cents:

I'd have thought it would depend on the nature of the 'DMA' requests generated by the hardware - so ethernet hardware dependant.

The above may be correct for PCI masters - especially those that do paired 16bit accesses for every 32bit word.
If the hardware generated cache line aligned PCI bursts I wouldn't have thought it would matter.

I doubt it is valid for PCIe transfers - where the ethernet frame will be split into (probably) 128byte TLPs. Even if it starts on a 64n+2 boundary the splits will be on 64 byte boundaries since the first and last 32bit words of the TLP have separate byte enables.

So I'd expect to see a cache line RMW for the first and last cache lines - That may, or may not, be slower than the misaligned accesses for the entire frame (1 clock data delay per access?)

Of course, modern nics will write 2 bytes of 'crap' before the frame. Rounding up the transfer to the end of a cache line might also help (especially if only a few extra words are needed).

David