Re: [PATCH net-next v2 5/6] bcm63xx_enet: convert to build_skb

[Florian Fainelli <f.fainelli@gmail.com>]

On 12/24/2020 6:24 AM, Sieng Piaw Liew wrote:
> We can increase the efficiency of rx path by using buffers to receive
> packets then build SKBs around them just before passing into the network
> stack. In contrast, preallocating SKBs too early reduces CPU cache
> efficiency.
> 
> Check if we're in NAPI context when refilling RX. Normally we're almost
> always running in NAPI context. Dispatch to napi_alloc_frag directly
> instead of relying on netdev_alloc_frag which does the same but
> with the overhead of local_bh_disable/enable.
> 
> Tested on BCM6328 320 MHz and iperf3 -M 512 to measure packet/sec
> performance. Included netif_receive_skb_list and NET_IP_ALIGN
> optimizations.
> 
> Before:
> [ ID] Interval           Transfer     Bandwidth       Retr
> [  4]   0.00-10.00  sec  49.9 MBytes  41.9 Mbits/sec  197         sender
> [  4]   0.00-10.00  sec  49.3 MBytes  41.3 Mbits/sec            receiver
> 
> After:
> [ ID] Interval           Transfer     Bandwidth       Retr
> [  4]   0.00-30.00  sec   171 MBytes  47.8 Mbits/sec  272         sender
> [  4]   0.00-30.00  sec   170 MBytes  47.6 Mbits/sec            receiver

This looks good, however there are a few nits and suggestions below:

[snip]

> @@ -862,6 +868,24 @@ static void bcm_enet_adjust_link(struct net_device *dev)
>  		priv->pause_tx ? "tx" : "off");
>  }
>  
> +static void bcm_enet_free_rx_buf_ring(struct device *kdev, struct bcm_enet_priv *priv)
> +{
> +	int i;
> +
> +	for (i = 0; i < priv->rx_ring_size; i++) {
> +		struct bcm_enet_desc *desc;
> +
> +		if (!priv->rx_buf[i])
> +			continue;
> +
> +		desc = &priv->rx_desc_cpu[i];
> +		dma_unmap_single(kdev, desc->address, priv->rx_buf_size,
> +				 DMA_FROM_DEVICE);
> +		skb_free_frag(priv->rx_buf[i]);
> +	}
> +	kfree(priv->rx_buf);
> +}

This is a good helper to introduced, however I would introduce it as a
preliminary patch that way it becomes clear when you are doing the
sk_buff to buf substitution in the next patch.

[snip]

> @@ -1640,9 +1641,12 @@ static int bcm_enet_change_mtu(struct net_device *dev, int new_mtu)
>  	 * align rx buffer size to dma burst len, account FCS since
>  	 * it's appended
>  	 */
> -	priv->rx_skb_size = ALIGN(actual_mtu + ETH_FCS_LEN,
> +	priv->rx_buf_size = ALIGN(actual_mtu + ETH_FCS_LEN,
>  				  priv->dma_maxburst * 4);
>  
> +	priv->rx_frag_size = SKB_DATA_ALIGN(priv->rx_buf_offset + priv->rx_buf_size)
> +						+ SKB_DATA_ALIGN(sizeof(struct skb_shared_info));

The alignment of the second line is a bit off and you should aim for the
+ operator to be on the preceding line, and have SKB_DATA_ALIGN() start
on the opening parenthesis of the preceding line.
-- 
Florian