[PATCH net-next v8 3/5] net: stmmac: drop multi-buffer frames in XDP mode

[Lorenzo Bianconi]

> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The commit message does not describe what the patch actually
>   does, and it hides changes to the non-XDP RX path.
> 
> --
> 
> net: stmmac: drop multi-buffer frames in XDP mode
> 
> When an XDP program is attached, stmmac_rx() now drops any frame that
> spans more than one descriptor, or that uses the second buffer of a
> descriptor. This prevents the XDP program from running on partial
> frames. Both XDP_CONSUMED and XDP_TX/XDP_REDIRECT now finish the packet
> via next instead of re-parsing the following descriptor.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index efa88cd359dac..e929cc0f23125 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5888,18 +5888,23 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> >  		if (priv->extend_desc)
> >  			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
> >  
> > -		if (unlikely(status & discard_frame)) {
> > -			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
> > -			buf->page = NULL;
> > +		if (unlikely(!error && (status & discard_frame))) {
> >  			error = 1;
> >  			if (!priv->hwts_rx_en)
> >  				rx_errors++;
> >  		}
> 
> [Severity: Low]
> The commit message says the patch will:
> 
>   "mark it as errored as soon as a non-last descriptor, or a descriptor
>   using its second buffer, is seen while an XDP program is attached,
>   drain and release every descriptor - including the last."
> 
> Is this description accurate? This hunk removes the only
> page_pool_put_page() call on the error path in stmmac_rx(). After the
> patch, none of the drop paths returns a page to the page_pool. That
> covers the hardware discard_frame drop, the new XDP SG drop and the new
> buf2 drop.

I will fix it in the next iteration.

Regards,
Lorenzo

> 
> The dropped buffers stay in rx_q->buf_pool[], and stmmac_rx_refill()
> reuses them in place because it only allocates when the slot is empty:
> 
> stmmac_rx_refill() {
>     ...
> 		if (!buf->page) {
> 			buf->page = page_pool_alloc_pages(rx_q->page_pool, gfp);
>     ...
> }
> 
> The reuse itself looks safe. All three drop checks run before any
> dma_sync_single_for_cpu() or net_prefetch() on the buffer.
> 
> The new !error guard also changes rx_errors from once per descriptor
> carrying discard_frame to once per frame. Both changes apply whether or
> not an XDP program is attached, but the subject says "in XDP mode".
> 
> On dwmac100/dwmac1000, the previous patch in this series makes non-last
> descriptors report discard_frame | rx_not_ls. So an errored frame
> spread over N descriptors used to add N-1 or N to rx_errors, and now
> adds 1.
> 
> Separately, the new buf2 drop in the later hunk doesn't set error, even
> though the message says the frame is marked as errored:
> 
> 	if (unlikely(stmmac_xdp_is_enabled(priv) && buf2_len)) {
> 		rx_dropped++;
> 		goto next;
> 	}
> 
> It only behaves the same way because, in XDP mode, that descriptor is
> always the last one.
> 
> This patch carries a Fixes: tag for net/stable. Could the commit message
> describe the in-place buffer reuse and the rx_errors accounting change
> for the non-XDP path? The later "net: stmmac: rework stmmac_rx to
> support XDP rx multi-buff" patch keeps both the !error guard and the
> in-place reuse, so it doesn't change the picture here.
> 
> > -		if (unlikely(error && (status & rx_not_ls)))
> > -			goto read_again;
> > +		/* We do not support SG in XDP mode */
> > +		if (unlikely(!error && (status & rx_not_ls) &&
> > +			     stmmac_xdp_is_enabled(priv))) {
> > +			rx_dropped++;
> > +			error = 1;
> > +		}
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018-stmmac-rx-mb-v8-0-5f58c0b25849%40oss.qualcomm.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20261020/3ae28b59/attachment.sig>