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

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

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

> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (1) · Medium: 2 (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 patch fixes a DMA under-sync on the XDP drop path but does
>   not say so.
> - [Medium] Fragment truesize is undercounted when num_pages is not a
>   power of two.
> - [Low] The patch advertises NETDEV_XDP_ACT_RX_SG (line 8189) and now
>   runs frags-aware programs on multi-buffer xdp_buffs…
> 
> Pre-existing issues:
> - [High] XDP_TX completions are recycled into the RX page_pool's lockless
>   alloc cache from a different NAPI context.
> - [Medium] The AF_XDP zero-copy XDP_PASS path reports XDP metadata whose
>   bytes it never copies.
> 
> --
> 
> net: stmmac: rework stmmac_rx to support XDP rx multi-buff
> 
> This reworks stmmac_rx() to collect every descriptor of a frame in an
> xdp_buff, so the XDP program runs on the whole frame, including any
> fragments. The skb is then built from the xdp_buff with napi_build_skb()
> and xdp_update_skb_frags_info(). The in-progress xdp_buff is saved in
> rx_q->state across NAPI polls, and NETDEV_XDP_ACT_RX_SG is advertised.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 04dc6dcaa435d..e9c61524fc740 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> 
> [ ... ]
> 
> > @@ -5806,6 +5812,119 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
> 
> [ ... ]
> 
> > +static struct sk_buff *stmmac_build_skb(struct xdp_buff *xdp)
> > +{
> > +	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> > +	u32 metasize = xdp->data - xdp->data_meta;
> 
> [ ... ]
> 
> > +	skb_mark_for_recycle(skb);
> > +	skb_reserve(skb, xdp->data - xdp->data_hard_start);
> > +	skb_put(skb, xdp->data_end - xdp->data);
> > +	if (metasize)
> > +		skb_metadata_set(skb, metasize);
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch. Now that the page_pool path
> keeps the metadata, though, the AF_XDP zero-copy XDP_PASS path no longer
> matches it. Does stmmac_construct_skb_zc() pass stale headroom bytes to
> TC/BPF as metadata?

Since this issue has not been introduced by this patch I guess we can fix it
with a dedicated patch.
> 
> stmmac_construct_skb_zc() {
>     ...
> 	skb_reserve(skb, xdp->data - xdp->data_hard_start);
> 	memcpy(__skb_put(skb, datasize), xdp->data, datasize);
> 	if (metasize)
> 		skb_metadata_set(skb, metasize);
>     ...
> }
> 
> Only [data, data_end) is copied into the new skb, but the metasize bytes
> in front of skb->data are still marked as metadata. A TC/BPF program that
> reads data_meta on frames from stmmac_rx_zc()->stmmac_dispatch_skb_zc()
> would see leftover bytes from the napi_alloc_skb() headroom instead of
> what the XDP program wrote.
> 
> Other zero-copy drivers copy metasize + datasize bytes starting at
> xdp->data_meta and then call __skb_pull(skb, metasize). Would the same
> approach work here?
> 
> > +
> > +	if (unlikely(xdp_buff_has_frags(xdp)))
> > +		xdp_update_skb_frags_info(skb, num_frags, sinfo->xdp_frags_size,
> > +					  sinfo->xdp_frags_truesize,
> > +					  xdp_buff_get_skb_flags(xdp));
> > +	return skb;
> > +}
> > +
> > +static bool stmmac_build_xdp_frags(struct stmmac_priv *priv,
> > +				   struct stmmac_rx_queue *rx_q,
> > +				   unsigned int len, struct page *page,
> > +				   unsigned int offset,
> > +				   enum dma_data_direction dma_dir,
> > +				   struct xdp_buff *xdp)
> > +{
> > +	dma_addr_t dma_addr = page_pool_get_dma_addr(page) + offset;
> > +
> > +	dma_sync_single_for_cpu(priv->device, dma_addr, len, dma_dir);
> > +	if (!xdp_buff_add_frag(xdp, page_to_netmem(page), offset, len,
> > +			       xdp->frame_sz)) {
> 
> [Severity: Medium]
> The commit message says:
> 
>   charge fragments the whole page_pool page size (xdp->frame_sz) as
>   truesize instead of the old dma_buf_sz
> 
> Is xdp->frame_sz the page_pool page size in every configuration?
> 
> frame_sz comes from bufsz = rx_q->napi_skb_frag_size in stmmac_rx(). That
> value is set in __alloc_dma_rx_desc_resources():
> 
> 	rx_q->napi_skb_frag_size = num_pages * PAGE_SIZE;
> 	...
> 	pp_params.order = order_base_2(num_pages);
> 
> When num_pages is not a power of two, each page_pool buffer is
> PAGE_SIZE << order, which is larger than napi_skb_frag_size. Two examples
> with 4K pages and no XDP:
> 
> - dma_buf_sz of 8KiB: num_pages = 3, order 2, so each 16K page is charged
>   as 12K.
> - dma_buf_sz of 16KiB: num_pages = 5, order 3, so each 32K page is charged
>   as 20K.
> 
> stmmac_xdp_shrink_tail() subtracts the same xdp->frame_sz. Then
> stmmac_build_skb() adds sinfo->xdp_frags_truesize to skb->truesize
> through xdp_update_skb_frags_info(). As a result, skb->truesize is lower
> than the memory the fragments actually pin.
> 
> Could the fragment truesize use PAGE_SIZE << pp_params.order instead? Or
> could num_pages be rounded up to a power of two?

I will fix it in the next iteration.

> 
> [ ... ]
> 
> > @@ -5927,129 +6039,89 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> 
> [ ... ]
> 
> > +		pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start;
> > +
> > +		ctx.desc = p;
> > +		ctx.ndesc = np;
> > +
> > +		skb = stmmac_xdp_run_prog(priv, &ctx.xdp);
> > +		/* Due xdp_adjust_tail: DMA sync for_device
> > +		 * cover max len CPU touch
> > +		 */
> > +		sync_len = ctx.xdp.data_end - ctx.xdp.data_hard_start;
> > +		sync_len = max(sync_len, pre_len);
> 
> [Severity: Medium]
> The old pre_len and sync_len subtracted buf->page_offset:
> 
> 	sync_len = ctx.xdp.data_end - ctx.xdp.data_hard_start -
> 		   buf->page_offset;
> 
> The new ones don't. This looks like a separate DMA sync fix, and the
> commit message doesn't mention it.
> 
> Commit 88095728511e ("net: stmmac: fix rx Scatter-Gather support") set
> pp_params.offset = 0, and __page_pool_dma_sync_for_device() syncs from
> dma_addr + pool->p.offset. So with the old formula, the XDP_CONSUMED path
> synced [0, L) for an L-byte frame at XDP_PACKET_HEADROOM. The bytes the
> CPU had touched were at [256, 256 + L).
> 
> Take a non-coherent platform and an XDP program that writes the packet,
> grows the tail or writes metadata, and then returns XDP_DROP or
> XDP_ABORTED, or fails a redirect. Could that leave dirty cache lines in
> the recycled page? Those lines could later be written back over a frame
> the device has DMA'd into the page.
> 
> The new formula looks like the correct one. Stable kernels that picked up
> 88095728511e would keep the under-sync. Could this go out as a separate
> net fix with:
> 
> Fixes: 88095728511e ("net: stmmac: fix rx Scatter-Gather support")
> 
> or at least be described in the commit message?

I will post a dedicated patch to fix it.

> 
> > +
> > +		/* 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, sync_len,
> > +						    true);
> > +				rx_dropped++;
> > +			} else if (xdp_res & (STMMAC_XDP_TX |
> > +					      STMMAC_XDP_REDIRECT)) {
> > +				xdp_status |= xdp_res;
> > +			}
> 
> [Severity: High]
> This is a pre-existing issue, but this patch sends fragment pages down the
> same path. Is it safe for tx_napi to recycle XDP_TX completions directly
> into this RX queue's page_pool?

Since this issue has not been introduced by this patch I guess we can fix it
with a dedicated patch.

> 
> stmmac_tx_clean() runs from ch->tx_napi and does:
> 
> 	if (xdpf &&
> 	    tx_q->tx_skbuff_dma[entry].buf_type == STMMAC_TXBUF_T_XDP_TX) {
> 		xdp_return_frame_rx_napi(xdpf);
> 		tx_q->xdpf[entry] = NULL;
> 	}
> 
> That call ends up here:
> 
> xdp_return_frame_rx_napi()
>   __xdp_return(..., napi_direct=true)
>     page_pool_put_full_netmem(pp, netmem, true)
>       page_pool_recycle_in_cache()
>         pool->alloc.cache[pool->alloc.count++] = netmem;
> 
> The page_pool belongs to ch->rx_napi, which is a separate napi_struct.
> stmmac_xdp_get_tx_queue() picks the XDP_TX queue by CPU. tx_napi can also
> be scheduled on another CPU by stmmac_tx_timer() or by a per-channel TX
> MSI vector. stmmac does not set pp_params.napi, so page_pool cannot catch
> this.
> 
> At the same time, stmmac_rx() on rx_napi uses the same alloc cache. It
> does so through stmmac_rx_refill() and through the new allow_direct=true
> puts in stmmac_xdp_put_buff(), stmmac_build_xdp_frags() and
> stmmac_xdp_shrink_tail().
> 
> Can these two contexts race on alloc.count and the cache slots? That
> could lose a page, or cache the same page twice so that it gets handed to
> two RX descriptors.
> 
> With multi-buffer frames, xdp_return_frame_rx_napi() now also returns
> every fragment page with napi_direct=true, so more pages go through this
> race.
> 
> [ ... ]
> 
> > @@ -8106,6 +8186,7 @@ static int __stmmac_dvr_probe(struct device *device,
> >  	ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> >  			    NETIF_F_RXCSUM;
> >  	ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
> > +			     NETDEV_XDP_ACT_RX_SG |
> >  			     NETDEV_XDP_ACT_XSK_ZEROCOPY;
> 
> [Severity: Low]
> With NETDEV_XDP_ACT_RX_SG advertised, frags-aware programs now see
> multi-buffer frames. However, __alloc_dma_rx_desc_resources() still
> registers the rxq with:
> 
> 	ret = xdp_rxq_info_reg(&rx_q->xdp_rxq, priv->dev, queue, napi_id);
> 
> That leaves frag_size at 0, so bpf_xdp_frags_increase_tail() always
> returns early:
> 
> 	if (!rxq->frag_size || rxq->frag_size > xdp->frame_sz)
> 		return -EOPNOTSUPP;
> 
> Is it intended that a positive bpf_xdp_adjust_tail() on a multi-buffer
> stmmac frame always fails, even when the last fragment page has room?
> Other RX_SG drivers such as bnxt behave the same way, so this may just be
> a limitation. Would registering with __xdp_rxq_info_reg() and a real
> frag_size be worth considering?

I will fix it in the next revision.

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018-stmmac-rx-mb-v8-0-5f58c0b25849%40oss.qualcomm.com

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