Re: [PATCH net-next v9] net: stmmac: rework stmmac_rx to support XDP rx multi-buff

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 7126 bytes --]

> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (1) · Medium: 1 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The skb head truesize grows on every received frame, including
>   with no XDP program loaded, and the commit message does not say so.
> - [Low] The commit message overstates one change.
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the patch
>   makes it wider.
> 
> --
> 
> net: stmmac: rework stmmac_rx to support XDP rx multi-buff
> 
> This reworks stmmac_rx() to collect all the descriptors of a frame into an
> xdp_buff, so programs with frags support see the whole packet. On XDP_PASS
> the skb is built from the xdp_buff head and its fragments. The patch also
> saves the in-progress xdp_buff in rx_q->state across NAPI polls, advertises
> NETDEV_XDP_ACT_RX_SG, and strips the FCS with a driver-local tail shrink.
> 
> > - carry the XDP metadata written with bpf_xdp_adjust_meta() into the skb
> >   via skb_metadata_set(), so it is no longer dropped and TC/BPF programs
> >   can read it through data_meta;
> 
> [Severity: Low]
> Is this item accurate? The XDP_PASS path in stmmac_rx() already did this
> before the patch. The patch removes these lines from stmmac_rx():
> 
> -			metasize = ctx.xdp.data - ctx.xdp.data_meta;
> -			if (metasize)
> -				skb_metadata_set(skb, metasize);
> 
> The same logic now lives in stmmac_build_skb().
> 
> Metadata on linear frames was not being dropped before. The only frames
> that never got metadata were multi-descriptor or buf2 frames, and XDP mode
> dropped those outright. The commit message already covers that elsewhere.
> 
> Could this item be reworded so it does not read as a fix for a metadata
> loss bug?

I guess this is minor point, I can update the commit message if I need to
repost.

> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index a2177777727ef..c215cc70caa19 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> 
> [ ... ]
> 
> > @@ -2290,10 +2294,6 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
> >  			 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> >  	num_pages = DIV_ROUND_UP(dma_buf_sz_pad, PAGE_SIZE);
> >  
> > -	rx_q->queue_index = queue;
> > -	rx_q->priv_data = priv;
> > -	rx_q->napi_skb_frag_size = num_pages * PAGE_SIZE;
> > -
> >  	pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
> >  	pp_params.pool_size = dma_conf->dma_rx_size;
> >  	pp_params.order = order_base_2(num_pages);
> > @@ -2321,6 +2321,10 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
> >  	if (!addr)
> >  		return -ENOMEM;
> >  
> > +	rx_q->queue_index = queue;
> > +	rx_q->priv_data = priv;
> > +	rx_q->napi_skb_frag_size = PAGE_SIZE << pp_params.order;
> > +
> 
> [Severity: Medium]
> Does this change the skb head truesize for every received frame, even when
> no XDP program is attached?
> 
> Before the patch the head size was num_pages * PAGE_SIZE. Now it is rounded
> up to the page_pool allocation order, and skb->truesize is derived from it
> here:
> 
> stmmac_rx()
> 	bufsz = rx_q->napi_skb_frag_size;
> 	...
> 	xdp_init_buff(&ctx.xdp, bufsz, &rx_q->xdp_rxq);
> 
> stmmac_build_skb()
> 	skb = napi_build_skb(xdp->data_hard_start, xdp->frame_sz);
> 
> On 4K pages, a dma_buf_sz of 8188 (BUF_SIZE_8KiB) gives num_pages = 3, so
> the head grows from 12K to 16K. A dma_buf_sz of 16368 (BUF_SIZE_16KiB)
> gives num_pages = 5, so the head grows from 20K to 32K.
> 
> That shrinks the effective socket receive buffer for jumbo MTU users who
> never load XDP. The commit message calls out the fragment truesize change
> as a behavior change, but not this one. Should this be mentioned too?

Same here, I guess this is minor point, I can update the commit message if
I need to repost.

> 
> [ ... ]
> 
> > @@ -5962,120 +6073,79 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> 
> [ ... ]
> 
> > +		skb = stmmac_xdp_run_prog(priv, &ctx.xdp);
> > +		/* For Not XDP_PASS verdict */
> > +		if (IS_ERR(skb)) {
> > +			unsigned int xdp_res = -PTR_ERR(skb);
> > +
> > +			if (xdp_res & STMMAC_XDP_CONSUMED) {
> > +				stmmac_xdp_put_buff(rx_q, &ctx.xdp, true);
> > +				rx_dropped++;
> > +			} else if (xdp_res & (STMMAC_XDP_TX |
> > +					      STMMAC_XDP_REDIRECT)) {
> > +				xdp_status |= xdp_res;
> > +			}
> > +			goto next;
> > +		}
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but multi-buffer frames can now
> reach XDP_TX, which makes it worse. Does XDP_TX completion recycle pages
> into this RX queue's page_pool direct cache from the wrong NAPI context?

As pointed out by sashiko, this issue has not been introduced by this patch and
it should be fixed with a dedicated patch.

Regards,
Lorenzo

> 
> stmmac_xdp_xmit_back() picks the TX queue by CPU, not by RX queue:
> 
> stmmac_xdp_get_tx_queue()
> 	while (index >= priv->plat->tx_queues_to_use)
> 		index -= priv->plat->tx_queues_to_use;
> 
> Completion then runs in that channel's tx_napi:
> 
> stmmac_napi_poll_tx()
>   stmmac_tx_clean()
>     xdp_return_frame_rx_napi(xdpf)
>       __xdp_return(..., napi_direct=true)    /* head and every frag */
>         page_pool_put_unrefed_netmem(..., allow_direct=true)
>           page_pool_recycle_in_cache()
> 
> stmmac_tx_clean() {
> 	...
> 		if (xdpf &&
> 		    tx_q->tx_skbuff_dma[entry].buf_type == STMMAC_TXBUF_T_XDP_TX) {
> 			xdp_return_frame_rx_napi(xdpf);
> 	...
> }
> 
> When allow_direct is already true, page_pool_put_unrefed_netmem() skips the
> page_pool_napi_local() check. That means pool->alloc.cache and
> pool->alloc.count are updated without a lock.
> 
> The tx_napi is a separate NAPI instance from the rx_napi that owns the
> pool. Meanwhile the owning rx_napi can be on another CPU running
> stmmac_rx_refill() or the allow_direct=true puts in stmmac_xdp_put_buff(),
> stmmac_build_xdp_frags() and stmmac_xdp_shrink_tail().
> 
> Can this corrupt the alloc cache? If so, the same page could be handed to
> two RX descriptors, or recycled while an skb still holds it.
> 
> The race dates back to XDP_TX support. With this patch, though, each
> XDP_TX frame can push the head plus several fragment pages down this path.
> Would returning XDP_TX frames with xdp_return_frame() in stmmac_tx_clean()
> avoid this?
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-stmmac-rx-mb-v9-1-c099c923664b%40oss.qualcomm.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]