Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH net-next v2 5/6] bcm63xx_enet: convert to build_skb
To:     Sieng Piaw Liew <liew.s.piaw@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>
Cc:     bcm-kernel-feedback-list@broadcom.com, netdev@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20201224142421.32350-1-liew.s.piaw@gmail.com>
 <20201224142421.32350-6-liew.s.piaw@gmail.com>
From:   Florian Fainelli <f.fainelli@gmail.com>
Message-ID: <49b174e9-df1c-a2fc-f5d5-adeabd940692@gmail.com>
Date:   Mon, 28 Dec 2020 19:24:03 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <20201224142421.32350-6-liew.s.piaw@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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