lkml.org 
[lkml]   [2020]   [Dec]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH net-next v2 5/6] bcm63xx_enet: convert to build_skb
From
Date


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

\
 
 \ /
  Last update: 2020-12-29 04:26    [W:0.079 / U:1.736 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site