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

[Jakub Kicinski]

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.

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

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.

There is also a hardware side to this. An oversized frame on an EST-gated
queue raises MTL_EST_STATUS.HLBF, and est_irq_status() in stmmac_est.c only
counts it and clears the error register:

	if (status & EST_HLBF) {
		value = readl(est_addr + EST_FRM_SZ_ERR);
	...
				x->mtl_est_txq_hlbf[i]++;
		/* Clear Interrupt */
		writel(feqn, est_addr + EST_FRM_SZ_ERR);

There is no queue flush or EST reprogramming. est_configure() programs only:

	if (cfg->enable)
		ctrl |= EST_EEST | EST_SSWL | EST_DFBS;

and no DDBF (drop-on-frame-size-error) definition exists in the tree, so
the drop-versus-retain response is left at the reset value. If the IP
retains the frame after the doorbell was rung and cur_tx advanced, does
dirty_tx stop advancing and xsk_tx_completed() never run for that
descriptor, reintroducing the same ring imbalance from the hardware side?

One related side effect worth mentioning in the changelog: for zero-copy
traffic, taprio window_drops attribution moves from max_sdu_txq_drop to
the HLBF counters reported through stmmac_tc.c.

Also, in the commit message:

>     The assumptions is that AF_XDP zero-copy traffic bypasses the qdisc, so

"The assumptions is" should be "The assumption is".
-- 
pw-bot: cr