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

[netdev-bot+sashiko@kernel.org]

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.

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