Re: [PATCH net v3 3/3] net: stmmac: document oversized AF_XDP frame handling

On Thu, Aug 20, 2026 at 06:29:16PM -0700, Stanislav Fomichev wrote:
> On 08/20, Maciej Fijalkowski wrote:
> > On Wed, Aug 19, 2026 at 09:05:35AM -0700, Stanislav Fomichev wrote:
> > > stmmac drops AF_XDP zero-copy frames that exceed taprio's queueMaxSDU
> > > after xsk_tx_peek_desc() has reserved their completion entries.
> > > 
> > > Completing a rejected descriptor is unsafe because AF_XDP completions are
> > > ordered: xsk_tx_completed(pool, 1) would complete the oldest outstanding
> > > descriptor, which may still be owned by hardware. Instead, leave the
> > > completion pending so the ring eventually wedges and increment the drop
> > > counter to expose the application error without risking hardware
> > > misbehavior.
> > > 
> > > Document this intentional ring imbalance at the check.
> > > 
> > > Signed-off-by: Stanislav Fomichev <[email protected]>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > index 62de03e65a90..6a532747c039 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > @@ -2713,6 +2713,10 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
> > >  		if (priv->est && priv->est->enable &&
> > >  		    priv->est->max_sdu[queue] &&
> > >  		    xdp_desc.len > priv->est->max_sdu[queue]) {
> > > +			/* Completions are ordered, so this descriptor cannot
> > > +			 * be completed safely. Wedge the ring to expose the
> > > +			 * application error instead.
> > > +			 */
> > >  			priv->xstats.max_sdu_txq_drop[queue]++;
> > >  			continue;
> > 
> > Hmm. I read the discussion on v2. Maybe we could cancel cq entry here in
> > this branch? Also it feels like something achievable at bind time when
> > taprio is configured and vice versa?
> > 
> > Otherwise we over-commit cq entries.
> 
> What do you want to achieve with the cancel here? IIUC it will make it look
> as if some (if the user has posted many) tx descriptor has not been consumed
> by the kernel?

Oof. My bad. I meant completely different thing :D

Right now the semantics are that we post invalid/dropped addrs to cq (the
rationale was that dropped descs are gone and unreachable which might
eventually lead to dying traffic).

We should submit xdp_desc's addr to cq.

Regarding the comment included in code I must disagree. CQ entries no
longer imply that 'this particular descriptor has been successfully sent
by HW'.

> 
> I do agree that a better idea is to probably do these checks during control
> paths, but it's a bit more involved (and not sure if it's possible? if we
> have a bunch of xsk sockets and we change that max_sdu, do we go over all
> sockets on the system somehow?). My main motivation with this patch was
> to make our LLM reviewers less chatty about preexisting issues.

I hear you, however I feel like we do not know this driver too much and
probably we don't have a HW to test out such changes, so maybe let us try
to fix existing behavior?