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]

Date:	Sun, 13 Nov 2011 20:53:39 +0200
From:	"Eilon Greenstein" <eilong@...adcom.com>
To:	"Eric Dumazet" <eric.dumazet@...il.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%

On Thu, 2011-11-10 at 08:45 -0800, Eric Dumazet wrote:
> 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.

I’m not sure I’m following the math over here. Assuming L1 is 64 bytes,
we need up to 63 bytes to align the start address (assuming SLOB is
being used) and additional (up to) 63 bytes at the end. That can sum up
to 126 bytes  am I missing something?

> So maybe only BNX2X_FW_RX_ALIGN is twice the needed amount.

I agree that it does not make much sense to optimize for SLOB - after
checking with our FW expert, it seems that we can change the FW to have
two different configuration flags  for start address alignment and end
packet padding. This way, we can only set the end packet padding and add
only 64 bytes. The only down side is that the FW team is pre occupied so
this new FW will be ready for submission only in about a month.

Thanks,
Eilon



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