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
