[PATCH net] net: axienet: bound TX completion cleanup by the NAPI budget

[PATCH net] net: axienet: bound TX completion cleanup by the NAPI budget

[Sagi Maimon]

axienet_tx_poll() passes lp->tx_bd_num to axienet_free_tx_chain() as
@nr_bds, and @budget is only forwarded to napi_consume_skb() as its
bulk-free hint.  Nothing limits the cleanup loop to the NAPI budget, so
the number of packets returned is bounded by the TX ring size rather
than by the budget, and the poll can report more work than it was
given:

  eth0: NAPI poll function axienet_tx_poll+0x0/0x180 [xilinx_emac]
        returned 96, exceeding its budget of 64.

Returning more than the budget breaks the NAPI contract.  It also makes
the "packets < budget" test in axienet_tx_poll() false, so
napi_complete_done() is skipped and TX completion interrupts are not
re-enabled on that pass.  NAPI reschedules the poll, so this recovers,
but the accounting is wrong either way.

In steady state fewer descriptors complete per poll than the budget
allows, which is why this is rarely observed.  Triggering it needs more
than @budget completions outstanding at once - for example when TX
completion interrupts have not been taken for a while and a full ring is
reclaimed in one go.

Stop the loop once the budget is spent.  cur_p->skb is only set on a
packet's last descriptor, so breaking there never leaves a packet
half-freed.  The check is skipped on the @force path, which cleans up
after a DMA mapping failure with a @budget of 0.

Fixes: 9e2bc267e780 ("net: axienet: Use NAPI for TX completion path")
Signed-off-by: Sagi Maimon <maimon.sagi@gmail.com>
---
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 782f903d318f..37309fa355ab 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -788,6 +788,13 @@ static int axienet_free_tx_chain(struct axienet_local *lp, u32 first_bd,
 	dma_addr_t phys;
 
 	for (i = 0; i < nr_bds; i++) {
+		/* A NAPI poll must not return more than its budget.  Stop on a
+		 * packet boundary once it is spent - cur_p->skb is only set on
+		 * a packet's last descriptor, so no packet is left half-freed.
+		 */
+		if (!force && packets >= budget)
+			break;
+
 		cur_p = &lp->tx_bd_v[(first_bd + i) % lp->tx_bd_num];
 		status = cur_p->status;
 
-- 
2.47.0

Re: [PATCH net] net: axienet: bound TX completion cleanup by the NAPI budget

[netdev-bot+sashiko]

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

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

- [High] drivers/net/ethernet/xilinx/xilinx_axienet_main.c:795 — the
  newly added `if (!force && packets >= budget) break;` is evaluated at…
- [Low] The kernel-doc block of axienet_free_tx_chain()
  (drivers/net/ethernet/xilinx/xilinx_axienet_main.c:768-781) was not…

--

net: axienet: bound TX completion cleanup by the NAPI budget

This adds a break in axienet_free_tx_chain() so the TX completion loop
stops once the NAPI budget has been spent, instead of being bounded only
by the TX ring size.  The check is skipped when @force is set.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 1722b7038f340..4381643227139 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -788,6 +788,13 @@ static int axienet_free_tx_chain(struct axienet_local *lp, u32 first_bd,
>  	dma_addr_t phys;
>  
>  	for (i = 0; i < nr_bds; i++) {
> +		/* A NAPI poll must not return more than its budget.  Stop on a
> +		 * packet boundary once it is spent - cur_p->skb is only set on
> +		 * a packet's last descriptor, so no packet is left half-freed.
> +		 */
> +		if (!force && packets >= budget)
> +			break;
> +

[Severity: High]
What happens on this path when budget is 0 and force is false?  The test
is evaluated before any descriptor is looked at, and 0 >= 0 is true, so
the loop breaks at i == 0.

netpoll calls the poll function with a budget of exactly 0:

net/core/netpoll.c:poll_one_napi() {
	...
	/* We explicitly pass the polling call a budget of 0 to
	 * indicate that we are clearing the Tx path only.
	 */
	work = napi->poll(napi, 0);
	...
}

lp->napi_tx is registered with netif_napi_add() in axienet_probe(), so it
is on dev->napi_list and poll_napi() reaches axienet_tx_poll(), which
forwards the budget unchanged with force = false:

	packets = axienet_free_tx_chain(lp, lp->tx_bd_ci, lp->tx_bd_num, false,
					&size, budget);

With the new break taken at i == 0, does this turn the netpoll TX reclaim
into a no-op?  No dma_unmap_single() or napi_consume_skb() runs, and
lp->tx_bd_ci += i leaves the consumer index unchanged.

Since packets is then 0, axienet_tx_poll() also skips the if (packets)
block containing netdev_completed_queue() and netif_wake_queue(), and
packets < budget (0 < 0) is false, so napi_complete_done() and the
re-write of lp->tx_dma_cr that re-enables TX completion interrupts are
skipped too.

Can this stall netconsole/kgdboe output when interrupts are disabled, for
example during an oops, where the TX completion interrupt and the NAPI
softirq will never run?  Ring-full detection depends only on software
clearing cur_p->cntrl:

drivers/net/ethernet/xilinx/xilinx_axienet_main.c:axienet_check_tx_bd_space() {
	...
	if (cur_p->cntrl)
		return NETDEV_TX_BUSY;
	...
}

and cur_p->cntrl is cleared solely by axienet_free_tx_chain(), so
netif_xmit_stopped(txq) stays true and __netpoll_send_skb()'s bounded
retry loop, which calls netpoll_poll_dev() to "tickle device maybe there
is some cleanup", exhausts its tries with the skbs still pinned on the
ring.

Before this patch the same call cleaned every completed descriptor, with
budget only used as the napi_consume_skb() bulk-free hint, and
napi_consume_skb(skb, 0) degrades to dev_consume_skb_any().

Would it work to make budget 0 mean "clean everything, report no work",
for instance:

	if (!force && budget && packets >= budget)
		break;

or alternatively leave the loop unbounded and clamp the value returned by
axienet_tx_poll() to the budget?

The commit message reasons about budget 0 only for the force path:

  "The check is skipped on the @force path, which cleans up
   after a DMA mapping failure with a @budget of 0."

Could the netpoll caller be covered as well, since it uses budget 0 with
force = false?

[Severity: Low]
Should the kernel-doc block just above axienet_free_tx_chain() be updated
in the same patch?  It still reads:

 * @nr_bds:	Max number of descriptors to clean up
 ...
 * @budget:	NAPI budget (use 0 when not called from NAPI poll)

@nr_bds is no longer the only cleanup bound for the !force path, and
@budget is no longer just a napi_consume_skb() hint.  A caller that
follows the documented advice of passing 0 outside NAPI poll with
force = false now reclaims nothing and does not advance lp->tx_bd_ci.
Could the @budget line state that cleanup stops after @budget completed
packets when @force is false?

>  		cur_p = &lp->tx_bd_v[(first_bd + i) % lp->tx_bd_num];
>  		status = cur_p->status;
>

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