netdev - Re: [PATCH net-next] bnx2x: reduce skb truesize by 50%

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1320943512.10042.14.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
Date:	Thu, 10 Nov 2011 17:45:12 +0100
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	eilong@...adcom.com
Cc:	David Miller <davem@...emloft.net>,
	"bhutchings@...arflare.com" <bhutchings@...arflare.com>,
	"pstaszewski@...are.pl" <pstaszewski@...are.pl>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] bnx2x: reduce skb truesize by 50%

Le jeudi 10 novembre 2011 à 18:27 +0200, Eilon Greenstein a écrit :
> On Thu, 2011-11-10 at 07:27 -0800, Eric Dumazet wrote:
> > Le jeudi 10 novembre 2011 à 17:05 +0200, Eilon Greenstein a écrit :
> > > > --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> > > > +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> > > > @@ -1185,9 +1185,14 @@ struct bnx2x {
> > > >  #define ETH_MAX_PACKET_SIZE		1500
> > > >  #define ETH_MAX_JUMBO_PACKET_SIZE	9600
> > > >  
> > > > -	/* Max supported alignment is 256 (8 shift) */
> > > > -#define BNX2X_RX_ALIGN_SHIFT		((L1_CACHE_SHIFT < 8) ? \
> > > > -					 L1_CACHE_SHIFT : 8)
> > > > +/* Max supported alignment is 256 (8 shift)
> > > > + * It should ideally be min(L1_CACHE_SHIFT, 8)
> > > > + * Choosing 5 (32 bytes) permits to get skb heads of 2048 bytes
> > > > + * instead of 4096 bytes.
> > > > + * With SLUB/SLAB allocators, data will be cache line aligned anyway.
> > > > + */
> > > > +#define BNX2X_RX_ALIGN_SHIFT		5
> > > > +
> > > 
> > > Hi Eric,
> > > 
> > > This can seriously hurt the PCI utilization. So in scenarios in which
> > > the PCI is the bottle neck, you will see performance degradation. We are
> > > looking at alternatives to reduce the allocation, but it is taking a
> > > while. Please hold off with this patch.
> > 
> > What do you mean exactly ?
> > 
> > This patch doesnt change skb->data alignment, its still 64 bytes
> > aligned. (cqe_fp->placement_offset == 2). PCI utilization is the same.
> > 
> > Only SLOB could get a misalignement, but who uses SLOB for performance ?
> 
> Obviously you are right... But the FW is configured to the wrong
> alignment and that will affect the end alignment (padding) which is
> significant in small packets scenarios where the PCI is the bottle neck.

Yes, I fully understand.

> 
> > Alternative would be to check why hardware need 2*L1_CACHE_BYTES extra
> > room for alignment... Normaly it could be 1*L1_CACHE_BYTES ?
> 
> Again - you are a mind reader :) This is what we are looking into right
> now. The problem is that `if` the buffer is not aligned (SLOB) we can
> overstep the allocated boundaries by configuring the FW to align.
> 
> >  	/* FW use 2 Cache lines Alignment for start packet and size  */
> > -#define BNX2X_FW_RX_ALIGN              (2 << BNX2X_RX_ALIGN_SHIFT)
> > +#define BNX2X_FW_RX_ALIGN              (1 << BNX2X_RX_ALIGN_SHIFT)
> > 
> > 

I did a SLOB test (and my patch included as well)

skb->len=66 pad=26 wkb->data=0xffff8801194da048 truesize=2304

So skb->data + pad -> 0xffff8801194da062 : So a 32bytes alignement + 2
bytes to align IP header. (BTW we dont really need it, NET_IP_ALIGN is
now 0 on most x86 platforms ?)

In the end, we get 98 bytes of 'skb reserve', and also 64 bytes of extra
headroom _after_ the end of full frame.

In my understanding, hardware alignement should be between 0 and 63, not
0 and 127.

So maybe only BNX2X_FW_RX_ALIGN is twice the needed amount.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html