[PATCH net-next] net: bgmac: use the NAPI skb cache

[PATCH net-next] net: bgmac: use the NAPI skb cache

[Rosen Penev]

Allocate RX skb heads with napi_build_skb() and free completed TX skbs
with napi_consume_skb(). Both run from the NAPI poll, so the heads are
recycled through the per-CPU NAPI cache and freed in bulk instead of
going through the slab allocator for every packet. The recycled heads
are usually still in the cache, which matters on the small cores bgmac
is found on.

On an Asus RT-N16 (BCM4716, 74Kc at 480 MHz, single TCP stream unless
noted, iperf3 median of 3, A/B in the same boot, two rounds each):

                         before       after     (Mbit/s)
  local rx / tx          242 / 210    251 / 213
  routed down            154          156
  routed, 4 streams      69 / 83      71 / 85

Routed upload and software flow offload were unchanged within noise.

Assisted-by: LLM
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/broadcom/bgmac.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index f40fde1d6bce..df2f0a87cbfd 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -229,7 +229,8 @@ static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
 }
 
 /* Free transmitted packets */
-static void bgmac_dma_tx_free(struct bgmac *bgmac, struct bgmac_dma_ring *ring)
+static void bgmac_dma_tx_free(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
+			      int budget)
 {
 	struct device *dma_dev = bgmac->dma_dev;
 	int empty_slot;
@@ -269,7 +270,7 @@ static void bgmac_dma_tx_free(struct bgmac *bgmac, struct bgmac_dma_ring *ring)
 			pkts_compl++;
 
 			/* Free memory! :) */
-			dev_kfree_skb(slot->skb);
+			napi_consume_skb(slot->skb, budget);
 			slot->skb = NULL;
 		}
 
@@ -465,9 +466,9 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 			/* Omit CRC. */
 			len -= ETH_FCS_LEN;
 
-			skb = build_skb(buf, BGMAC_RX_ALLOC_SIZE);
+			skb = napi_build_skb(buf, BGMAC_RX_ALLOC_SIZE);
 			if (unlikely(!skb)) {
-				netdev_err(bgmac->net_dev, "build_skb failed\n");
+				netdev_err(bgmac->net_dev, "napi_build_skb failed\n");
 				put_page(virt_to_head_page(buf));
 				bgmac->net_dev->stats.rx_errors++;
 				break;
@@ -1157,7 +1158,7 @@ static int bgmac_poll(struct napi_struct *napi, int weight)
 	/* Ack */
 	bgmac_write(bgmac, BGMAC_INT_STATUS, ~0);
 
-	bgmac_dma_tx_free(bgmac, &bgmac->tx_ring[0]);
+	bgmac_dma_tx_free(bgmac, &bgmac->tx_ring[0], weight);
 	handled += bgmac_dma_rx_read(bgmac, &bgmac->rx_ring[0], weight);
 
 	/* Poll again if more events arrived in the meantime */
-- 
2.56.0

Re: [PATCH net-next] net: bgmac: use the NAPI skb cache

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In bgmac_dma_rx_read() (drivers/net/ethernet/broadcom/bgmac.c),
  the patch replaces build_skb() with napi_build_skb().

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but bgmac_poll()
  has no zero-budget guard.

--

net: bgmac: use the NAPI skb cache

This commit changes bgmac to allocate RX skb heads with napi_build_skb()
and to free completed TX skbs with napi_consume_skb(). The skb heads are
then recycled through the per-CPU NAPI cache instead of the slab
allocator.

> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 3e9c57196a397..63a01accd7803 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c

[ ... ]

> @@ -466,9 +467,9 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>  			/* Omit CRC. */
>  			len -= ETH_FCS_LEN;
>  
> -			skb = build_skb(buf, BGMAC_RX_ALLOC_SIZE);
> +			skb = napi_build_skb(buf, BGMAC_RX_ALLOC_SIZE);

[Severity: High]
Can netpoll's zero-budget poll reach this call outside softirq context?

poll_one_napi() in net/core/netpoll.c calls napi->poll(napi, 0). A budget
of 0 means only the TX path should be cleaned. The TX side of bgmac_poll()
handles this, because napi_consume_skb() falls back to
dev_consume_skb_any() when budget is 0.

However, bgmac_poll() still calls bgmac_dma_rx_read() with weight 0. The
budget check there only runs after a descriptor has been handled:

bgmac_dma_rx_read() {
    ...
		if (handled >= weight) /* Should never be greater */
			break;
    ...
}

So one ready RX frame always reaches napi_build_skb():

netpoll_send_skb() [local_irq_save]
  __netpoll_send_skb()
    netpoll_poll_dev()
      poll_one_napi()
        bgmac_poll(napi, 0)
          bgmac_dma_rx_read(weight=0)
            napi_build_skb()
              __napi_build_skb()
                napi_skb_cache_get()

napi_skb_cache_get() modifies the per-CPU napi_alloc_cache
(nc->skb_cache[--nc->skb_count]) under local_lock_nested_bh(). On !RT
kernels, that lock is only a lockdep_assert_in_softirq() plus a lockdep
annotation.

Netpoll can be entered from hardirq context, for example by a printk
through netconsole from an IRQ handler. Suppose that hardirq interrupts a
NET_RX softirq on the same CPU while the softirq is inside
napi_skb_cache_get() or napi_skb_cache_put().

Could both contexts then take the same skb head, or corrupt
nc->skb_count?

On PREEMPT_RT, local_lock_nested_bh() is a sleeping per-CPU lock. Here it
would be taken with IRQs disabled. With lockdep enabled,
lockdep_assert_in_softirq() would also warn every time netpoll handles an
RX frame.

The old build_skb() used kmem_cache_alloc(), which is safe in this
context. Zero-budget RX could already reach napi_skb_cache_put() through
the GRO_MERGED_FREE path in gro_skb_finish(). This change makes the cache
access unconditional for every frame handled at budget 0.

Would it make sense to skip RX processing when weight is 0, either in
bgmac_poll() or at the top of bgmac_dma_rx_read()?

[ ... ]

> @@ -1153,7 +1154,7 @@ static int bgmac_poll(struct napi_struct *napi, int weight)
>  	/* Ack */
>  	bgmac_write(bgmac, BGMAC_INT_STATUS, ~0);
>  
> -	bgmac_dma_tx_free(bgmac, &bgmac->tx_ring[0]);
> +	bgmac_dma_tx_free(bgmac, &bgmac->tx_ring[0], weight);
>  	handled += bgmac_dma_rx_read(bgmac, &bgmac->rx_ring[0], weight);

[Severity: Medium]
This isn't a bug introduced by this patch, but bgmac_poll() has no guard
for a zero budget. With weight 0, bgmac_dma_rx_read() handles one RX
descriptor and returns 1. bgmac_poll() then skips the handled < weight
branch and also returns 1.

Doesn't that trigger the WARN_ONCE() in poll_one_napi()?

net/core/netpoll.c:poll_one_napi() {
    ...
	work = napi->poll(napi, 0);
	WARN_ONCE(work, "%pS exceeded budget in poll\n", napi->poll);
    ...
}

This missing guard is also what allows the napi_build_skb() issue above.
The same fix, returning before RX processing when weight is 0, would
cover both.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010070635.97759-1-rosenp%40gmail.com