[v5,10/17] net: sgi: ioc3-eth: rework skb rx handling

Buffers alloacted by alloc_skb() are already cache aligned so there
is no need for an extra align done by ioc3_alloc_skb. And instead
of skb_put/skb_trim simply use one skb_put after frame size is known
during receive.

Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
---
 drivers/net/ethernet/sgi/ioc3-eth.c | 50 ++++++++-----------------------------
 1 file changed, 11 insertions(+), 39 deletions(-)

On Mon, 19 Aug 2019 18:31:33 +0200, Thomas Bogendoerfer wrote:
> Buffers alloacted by alloc_skb() are already cache aligned so there
> is no need for an extra align done by ioc3_alloc_skb. And instead
> of skb_put/skb_trim simply use one skb_put after frame size is known
> during receive.
> 
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> ---
>  drivers/net/ethernet/sgi/ioc3-eth.c | 50 ++++++++-----------------------------
>  1 file changed, 11 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sgi/ioc3-eth.c b/drivers/net/ethernet/sgi/ioc3-eth.c
> index c875640926d6..d862f28887f9 100644
> --- a/drivers/net/ethernet/sgi/ioc3-eth.c
> +++ b/drivers/net/ethernet/sgi/ioc3-eth.c
> @@ -11,7 +11,6 @@
>   *
>   * To do:
>   *
> - *  o Handle allocation failures in ioc3_alloc_skb() more gracefully.
>   *  o Handle allocation failures in ioc3_init_rings().
>   *  o Use prefetching for large packets.  What is a good lower limit for
>   *    prefetching?
> @@ -72,6 +71,12 @@
>  #define TX_RING_ENTRIES		128
>  #define TX_RING_MASK		(TX_RING_ENTRIES - 1)
>  
> +/* BEWARE: The IOC3 documentation documents the size of rx buffers as
> + * 1644 while it's actually 1664.  This one was nasty to track down...
> + */
> +#define RX_OFFSET		10
> +#define RX_BUF_SIZE		1664
> +
>  #define ETCSR_FD   ((17 << ETCSR_IPGR2_SHIFT) | (11 << ETCSR_IPGR1_SHIFT) | 21)
>  #define ETCSR_HD   ((21 << ETCSR_IPGR2_SHIFT) | (21 << ETCSR_IPGR1_SHIFT) | 21)
>  
> @@ -111,31 +116,6 @@ static void ioc3_init(struct net_device *dev);
>  static const char ioc3_str[] = "IOC3 Ethernet";
>  static const struct ethtool_ops ioc3_ethtool_ops;
>  
> -/* We use this to acquire receive skb's that we can DMA directly into. */
> -
> -#define IOC3_CACHELINE	128UL

Is the cache line on the platform this driver works on 128B?
This looks like a DMA engine alignment requirement, more than an
optimization.

The comment in __alloc_skb() says:

	/* We do our best to align skb_shared_info on a separate cache
	 * line. It usually works because kmalloc(X > SMP_CACHE_BYTES) gives
	 * aligned memory blocks, unless SLUB/SLAB debug is enabled.
	 * Both skb->head and skb_shared_info are cache line aligned.
	 */

note the "unless".

> -static inline unsigned long aligned_rx_skb_addr(unsigned long addr)
> -{
> -	return (~addr + 1) & (IOC3_CACHELINE - 1UL);
> -}
> -
> -static inline struct sk_buff *ioc3_alloc_skb(unsigned long length,
> -					     unsigned int gfp_mask)
> -{
> -	struct sk_buff *skb;
> -
> -	skb = alloc_skb(length + IOC3_CACHELINE - 1, gfp_mask);
> -	if (likely(skb)) {
> -		int offset = aligned_rx_skb_addr((unsigned long)skb->data);
> -
> -		if (offset)
> -			skb_reserve(skb, offset);
> -	}
> -
> -	return skb;
> -}
> -
>  static inline unsigned long ioc3_map(void *ptr, unsigned long vdev)
>  {
>  #ifdef CONFIG_SGI_IP27
> @@ -148,12 +128,6 @@ static inline unsigned long ioc3_map(void *ptr, unsigned long vdev)
>  #endif
>  }
>  
> -/* BEWARE: The IOC3 documentation documents the size of rx buffers as
> - * 1644 while it's actually 1664.  This one was nasty to track down ...
> - */
> -#define RX_OFFSET		10
> -#define RX_BUF_ALLOC_SIZE	(1664 + RX_OFFSET + IOC3_CACHELINE)
> -
>  #define IOC3_SIZE 0x100000
>  
>  static inline u32 mcr_pack(u32 pulse, u32 sample)
> @@ -534,10 +508,10 @@ static inline void ioc3_rx(struct net_device *dev)
>  		err = be32_to_cpu(rxb->err);		/* It's valid ...  */
>  		if (err & ERXBUF_GOODPKT) {
>  			len = ((w0 >> ERXBUF_BYTECNT_SHIFT) & 0x7ff) - 4;
> -			skb_trim(skb, len);
> +			skb_put(skb, len);
>  			skb->protocol = eth_type_trans(skb, dev);
>  
> -			new_skb = ioc3_alloc_skb(RX_BUF_ALLOC_SIZE, GFP_ATOMIC);
> +			new_skb = alloc_skb(RX_BUF_SIZE, GFP_ATOMIC);
>  			if (!new_skb) {
>  				/* Ouch, drop packet and just recycle packet
>  				 * to keep the ring filled.
> @@ -546,6 +520,7 @@ static inline void ioc3_rx(struct net_device *dev)
>  				new_skb = skb;
>  				goto next;
>  			}
> +			new_skb->dev = dev;

Assigning dev pointer seems unrelated to the rest of the patch?

>  			if (likely(dev->features & NETIF_F_RXCSUM))
>  				ioc3_tcpudp_checksum(skb,
> @@ -556,8 +531,6 @@ static inline void ioc3_rx(struct net_device *dev)
>  
>  			ip->rx_skbs[rx_entry] = NULL;	/* Poison  */
>  
> -			/* Because we reserve afterwards. */
> -			skb_put(new_skb, (1664 + RX_OFFSET));
>  			rxb = (struct ioc3_erxbuf *)new_skb->data;
>  			skb_reserve(new_skb, RX_OFFSET);
>  
> @@ -846,16 +819,15 @@ static void ioc3_alloc_rings(struct net_device *dev)
>  		for (i = 0; i < RX_BUFFS; i++) {
>  			struct sk_buff *skb;
>  
> -			skb = ioc3_alloc_skb(RX_BUF_ALLOC_SIZE, GFP_ATOMIC);
> +			skb = alloc_skb(RX_BUF_SIZE, GFP_ATOMIC);
>  			if (!skb) {
>  				show_free_areas(0, NULL);
>  				continue;
>  			}
> +			skb->dev = dev;
>  
>  			ip->rx_skbs[i] = skb;
>  
> -			/* Because we reserve afterwards. */
> -			skb_put(skb, (1664 + RX_OFFSET));
>  			rxb = (struct ioc3_erxbuf *)skb->data;
>  			rxr[i] = cpu_to_be64(ioc3_map(rxb, 1));
>  			skb_reserve(skb, RX_OFFSET);

On Mon, 19 Aug 2019 16:55:22 -0700
Jakub Kicinski <jakub.kicinski@netronome.com> wrote:

> On Mon, 19 Aug 2019 18:31:33 +0200, Thomas Bogendoerfer wrote:
> > Buffers alloacted by alloc_skb() are already cache aligned so there
> > is no need for an extra align done by ioc3_alloc_skb. And instead
> > of skb_put/skb_trim simply use one skb_put after frame size is known
> > during receive.
> [...]  
> > -/* We use this to acquire receive skb's that we can DMA directly into. */
> > -
> > -#define IOC3_CACHELINE	128UL
> 
> Is the cache line on the platform this driver works on 128B?

right now yes, in theory IOC3 CAD DUO cards might work in SGI O2 systems,
but the current Linux PCI implementation for O2 will not detect that card.
On X86 usually the BIOS will choke up on that cards.

> This looks like a DMA engine alignment requirement, more than an
> optimization.

that true, there are two constraints for the rx buffers, start must be aligned
to 128 bytes and a buffer must not cross a 16kbyte boundary. I was already
thinking of allocating pages and chop them up. Is there a Linux API available,
which could help for implementing this ?

I'll probably drop this patch or only change the skb_put stuff plus RX_BUF_SIZE
define.

Thomas.

On Wed, 21 Aug 2019 16:28:47 +0200, Thomas Bogendoerfer wrote:
> > This looks like a DMA engine alignment requirement, more than an
> > optimization.  
> 
> that true, there are two constraints for the rx buffers, start must be aligned
> to 128 bytes and a buffer must not cross a 16kbyte boundary. I was already
> thinking of allocating pages and chop them up. Is there a Linux API available,
> which could help for implementing this ?
> 
> I'll probably drop this patch or only change the skb_put stuff plus RX_BUF_SIZE
> define.

Sounds a little like frag allocator (napi_alloc_frag()/
netdev_alloc_frag()), but I'm not sure you'd have sufficient control
to skip over the 16k boundary.. Perhaps others have better suggestions.