Re: [Intel-wired-lan] [PATCH net v2 3/3] net: stmmac: skip queueMaxSDU check for AF_XDP

On 08/18, Jakub Kicinski wrote:

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: stmmac: skip queueMaxSDU check for AF_XDP

This drops the per-queue max SDU length check from the stmmac AF_XDP
zero-copy transmit path, on the grounds that xsk_tx_peek_desc() has
already reserved a completion ring entry and the old "continue" skipped
both queuing and completion. The change is tagged for stable via a Fixes:
tag on c5c3e1bfc9e0.

quoted

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 62de03e65a90c..4a82f4351303a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

@@ -2710,13 +2710,6 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
 		if (!xsk_tx_peek_desc(pool, &xdp_desc))
 			break;
 
-		if (priv->est && priv->est->enable &&
-		    priv->est->max_sdu[queue] &&
-		    xdp_desc.len > priv->est->max_sdu[queue]) {
-			priv->xstats.max_sdu_txq_drop[queue]++;
-			continue;
-		}
-

[Severity: Medium]
The completion ring imbalance described in the commit message looks real:
xsk_tx_peek_desc() reserves the cq slot with xskq_prod_reserve_addr() and
only xsk_tx_completed() submits it, so the old "continue" left the cq
producer permanently offset.

Was completing the reserved descriptor considered instead of removing the
check, i.e. keeping the length test and doing:

	if (priv->est && priv->est->enable &&
	    priv->est->max_sdu[queue] &&
	    xdp_desc.len > priv->est->max_sdu[queue]) {
		priv->xstats.max_sdu_txq_drop[queue]++;
		xsk_tx_completed(pool, 1);
		continue;
	}

That would fix the ring accounting without dropping the enforcement.

Don't think this suggestion would work. xsk_tx_completed(1) will complete
the first queued entry, no the current rejected one (completions are in order).

If it's not convincing, I can repost only patches 1 and 2. Or if it sounds
reasonable, can try to explain that part in the description.

On stmmac, queueMaxSDU has no hardware table. priv->est->max_sdu[] is
written only by tc_taprio_map_maxsdu_txq() in stmmac_tc.c:

		for (j = offset; j < offset + count; j++)
			priv->est->max_sdu[j] = qopt->max_sdu[i] + ETH_HLEN - ETH_TLEN;

and read only by the length checks in stmmac_main.c. With this hunk gone,
does an AF_XDP zero-copy socket now transmit frames above the configured
per-queue limit while tc_query_caps() still reports:

		caps->gate_mask_per_txq = true;
		caps->supports_queue_max_sdu = true;

That capability is what taprio uses to accept a queueMaxSDU configuration
in the first place, and sch_taprio comments in
taprio_skb_exceeds_queue_max_sdu() that "Devices with full offload are
expected to honor this in hardware".

The "bypasses the qdisc" rationale also applies to stmmac_xdp_xmit_xdpf(),
which serves XDP_TX and ndo_xdp_xmit and does not traverse the qdisc
either, yet still enforces the same limit:

	if (priv->est && priv->est->enable &&
	    priv->est->max_sdu[queue] &&
	    xdpf->len > priv->est->max_sdu[queue]) {
		priv->xstats.max_sdu_txq_drop[queue]++;
		return STMMAC_XDP_CONSUMED;
	}

Should that check be removed as well so the XDP paths agree? The cited igc
precedent checks only in igc_xmit_frame_ring() and in neither of its XDP
paths, so matching igc would mean touching stmmac_xdp_xmit_xdpf() too.

xdp_tx does not expose xsk-like descriptor rings, so it doesn't apply.