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