Messages in this thread | | | Subject | Re: [PATCH net-next v2 5/6] bcm63xx_enet: convert to build_skb | From | Florian Fainelli <> | Date | Mon, 28 Dec 2020 19:24:03 -0800 |
| |
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
| |