[PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[Nicolae Rosia]

Signed-off-by: Nicolae Rosia <nicolae.rosia@certsign.ro>
---
 drivers/net/ethernet/cadence/macb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index caeb395..dbb5160 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -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);
-- 
2.1.0

Re: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[Nicolas Ferre]

Le 30/06/2015 19:25, Nicolae Rosia a écrit :
> 
> Signed-off-by: Nicolae Rosia <nicolae.rosia@certsign.ro>

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Thanks, bye.

> ---
>  drivers/net/ethernet/cadence/macb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index caeb395..dbb5160 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -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);
> 


-- 
Nicolas Ferre

Re: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[Eric Dumazet]

On Tue, 2015-06-30 at 20:25 +0300, Nicolae Rosia wrote:
> Signed-off-by: Nicolae Rosia <nicolae.rosia@certsign.ro>
> ---
>  drivers/net/ethernet/cadence/macb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index caeb395..dbb5160 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -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);

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

Re: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[Nicolae Rosia]

On 07/01/2015 01:56 PM, Eric Dumazet wrote:
> 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?

Re: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[Eric Dumazet]

On Wed, 2015-07-01 at 16:29 +0300, Nicolae Rosia wrote:
> On 07/01/2015 01:56 PM, Eric Dumazet wrote:
> > 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.

Re: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[Nicolae Rosia]

On 07/01/2015 04:44 PM, Eric Dumazet wrote:
> 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?

>
> 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/

Re: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[Eric Dumazet]

On Wed, 2015-07-01 at 17:29 +0300, Nicolae Rosia wrote:
> On 07/01/2015 04:44 PM, Eric Dumazet wrote:
> > 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.

> 
> >
> > 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 ?

Re: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[Nicolae Rosia]

On Wed, Jul 1, 2015 at 6:33 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
[...]
> 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.

> 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

Re: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[Eric Dumazet]

On Wed, 2015-07-01 at 18:53 +0300, Nicolae Rosia wrote:
> On Wed, Jul 1, 2015 at 6:33 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> [...]
> > 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.
> 
> > 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.

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index caeb39561567237261ac0d50befebad666cfbeb3..24a93c769caa5430ca61efe002b458fef7281e99 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -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;
 }

Re: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[Joe Perches]

On Wed, 2015-07-01 at 12:56 +0200, Eric Dumazet wrote:
> On Tue, 2015-06-30 at 20:25 +0300, Nicolae Rosia wrote:
> > diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
[]
> > @@ -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);
> 
> 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.

Re: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[Eric Dumazet]

On Wed, 2015-07-01 at 10:06 -0700, Joe Perches wrote:
> On Wed, 2015-07-01 at 12:56 +0200, Eric Dumazet wrote:
> > On Tue, 2015-06-30 at 20:25 +0300, Nicolae Rosia wrote:
> > > diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> []
> > > @@ -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);
> > 
> > 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.

Re: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[Joe Perches]

On Thu, 2015-07-02 at 00:13 +0200, Eric Dumazet wrote:
> On Wed, 2015-07-01 at 10:06 -0700, Joe Perches wrote:
> > On Wed, 2015-07-01 at 12:56 +0200, Eric Dumazet wrote:
> > > On Tue, 2015-06-30 at 20:25 +0300, Nicolae Rosia wrote:
> > > > diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> > []
> > > > @@ -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);
> > > 
> > > 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.

Nor I really.

Most all of those are in fairly old hardware drivers.

I just wanted to point out that more exist.

RE: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[David Laight]

From: Eric Dumazet
> Sent: 01 July 2015 23:14
> To: Joe Perches
...
> > > 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

Re: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[Eric Dumazet]

On Fri, 2015-07-03 at 16:18 +0000, David Laight wrote:

> 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 <alexander.h.duyck@intel.com>
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 <tglx@linutronix.de>
    Cc: Ingo Molnar <mingo@redhat.com>
    Cc: "H. Peter Anvin" <hpa@zytor.com>
    Cc: x86@kernel.org
    Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
    Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
    Acked-by: H. Peter Anvin <hpa@zytor.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

RE: [PATCH net-next] net: macb: replace literal constant with NET_IP_ALIGN

[David Laight]

From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: 03 July 2015 17:39
> On Fri, 2015-07-03 at 16:18 +0000, David Laight wrote:
> 
> > 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.

> Intel guys decided to change NET_IP_ALIGN to 0 (it was 2 in the past)
...
>     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