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

2020-12-28 Thread Florian Fainelli 



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/secreceiver
> 
> 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/secreceiver

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 = >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

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

2020-12-24 Thread Sieng Piaw Liew 
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/secreceiver

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/secreceiver

Signed-off-by: Sieng Piaw Liew 
---
 drivers/net/ethernet/broadcom/bcm63xx_enet.c | 157 +--
 drivers/net/ethernet/broadcom/bcm63xx_enet.h |  14 +-
 2 files changed, 80 insertions(+), 91 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcm63xx_enet.c 
b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
index 51976ed87d2d..8c2e97311a2c 100644
--- a/drivers/net/ethernet/broadcom/bcm63xx_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
@@ -220,7 +220,7 @@ static void bcm_enet_mdio_write_mii(struct net_device *dev, 
int mii_id,
 /*
  * refill rx queue
  */
-static int bcm_enet_refill_rx(struct net_device *dev)
+static int bcm_enet_refill_rx(struct net_device *dev, bool napi_mode)
 {
struct bcm_enet_priv *priv;
 
@@ -228,29 +228,29 @@ static int bcm_enet_refill_rx(struct net_device *dev)
 
while (priv->rx_desc_count < priv->rx_ring_size) {
struct bcm_enet_desc *desc;
-   struct sk_buff *skb;
-   dma_addr_t p;
int desc_idx;
u32 len_stat;
 
desc_idx = priv->rx_dirty_desc;
desc = >rx_desc_cpu[desc_idx];
 
-   if (!priv->rx_skb[desc_idx]) {
-   if (priv->enet_is_sw)
-   skb = netdev_alloc_skb_ip_align(dev, 
priv->rx_skb_size);
+   if (!priv->rx_buf[desc_idx]) {
+   void *buf;
+
+   if (likely(napi_mode))
+   buf = napi_alloc_frag(priv->rx_frag_size);
else
-   skb = netdev_alloc_skb(dev, priv->rx_skb_size);
-   if (!skb)
+   buf = netdev_alloc_frag(priv->rx_frag_size);
+   if (unlikely(!buf))
break;
-   priv->rx_skb[desc_idx] = skb;
-   p = dma_map_single(>pdev->dev, skb->data,
-  priv->rx_skb_size,
-  DMA_FROM_DEVICE);
-   desc->address = p;
+   priv->rx_buf[desc_idx] = buf;
+   desc->address = dma_map_single(>pdev->dev,
+  buf + 
priv->rx_buf_offset,
+  priv->rx_buf_size,
+  DMA_FROM_DEVICE);
}
 
-   len_stat = priv->rx_skb_size << DMADESC_LENGTH_SHIFT;
+   len_stat = priv->rx_buf_size << DMADESC_LENGTH_SHIFT;
len_stat |= DMADESC_OWNER_MASK;
if (priv->rx_dirty_desc == priv->rx_ring_size - 1) {
len_stat |= (DMADESC_WRAP_MASK >> priv->dma_desc_shift);
@@ -290,7 +290,7 @@ static void bcm_enet_refill_rx_timer(struct timer_list *t)
struct net_device *dev = priv->net_dev;
 
spin_lock(>rx_lock);
-   bcm_enet_refill_rx(dev);
+   bcm_enet_refill_rx(dev, false);
spin_unlock(>rx_lock);
 }
 
@@ -320,6 +320,7 @@ static int bcm_enet_receive_queue(struct net_device *dev, 
int budget)
int desc_idx;
u32 len_stat;
unsigned int len;
+   void *buf;
 
desc_idx = priv->rx_curr_desc;
desc = >rx_desc_cpu[desc_idx];
@@ -365,16 +366,14 @@ static int bcm_enet_receive_queue(struct net_device *dev, 
int budget)
}
 
/* valid packet */
-   skb = priv->rx_skb[desc_idx];
+   buf = priv->rx_buf[desc_idx];
len = (len_stat & DMADESC_LENGTH_MASK) >> DMADESC_LENGTH_SHIFT;
/* don't include FCS */
len -= 4;
 
if (len < copybreak) {
-   struct sk_buff *nskb;
-
-   nskb = napi_alloc_skb(>napi,