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

[Eric Dumazet <eric.dumazet@gmail.com>]

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.