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

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

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

> Hi Lorenzo
> 
> On 12.10.2026 18:44, Lorenzo Bianconi wrote:
> > Build the xdp_buff by accumulating all the descriptors that make up a
> > frame, so the XDP program runs on the full (possibly fragmented) packet
> > instead of just the first buffer. When the frame is not consumed by the
> > program, assemble the skb from the head buffer and the collected
> > fragments via napi_build_skb()/xdp_update_skb_frags_info().
> > 
> > Note that XDP still rejects an MTU larger than ETH_DATA_LEN
> > (stmmac_xdp_set_prog(), stmmac_change_mtu()) and disables SPH, so an
> > ordinary frame fits in a single buffer. This path is therefore exercised
> > by frames that do span several descriptors (for example jumbo frames the
> > MAC admits through its jumbo-enable bit) and is groundwork for extending
> > XDP to jumbo/SPH; it does not enable XDP jumbo support by itself.
> > 
> > For those multi-descriptor frames the old code was buggy as well: it ran
> > the XDP program once per descriptor buffer, so for a multi-descriptor
> > frame the program was also invoked on continuation buffers holding
> > mid-frame payload. It also subtracted ETH_FCS_LEN from the first buffer
> > even when it was not the last descriptor (its second buffer empty),
> > dropping four bytes from the middle of the frame while marking the FCS
> > as
> > already stripped and skipping the final pskb_trim(). Accumulating the
> > whole frame before running XDP fixes both.
> 
> With patch 3/4 applied, these frames are already dropped in XDP mode,
> so this paragraph no longer matches the code this patch changes?

ack, I will remove it in v7.

> 
> > 
> > To do so, store the in-progress xdp_buff in rx_q->state instead of the
> > partially built skb, so the accumulated head and fragments survive a
> > NAPI poll boundary (mid-frame dma_own or dirty_rx break). The state is
> > saved only while a frame is in progress and cleared once it completes,
> > leaving it untouched when the poll does not process anything (e.g.
> > netpoll invoked with a zero budget).
> > 
> > In addition:
> > - advertise NETDEV_XDP_ACT_RX_SG, since the RX path can now deliver
> >   non-linear buffers to XDP;
> > - drop multi-buffer frames when the attached program was not loaded with
> >   BPF_F_XDP_HAS_FRAGS, since a single-buffer program must not be handed
> > a
> >   non-linear xdp_buff;
> > - build the skb head with napi_build_skb() passing xdp->frame_sz, so
> >   skb_shinfo() lands on the same shared_info the fragments were
> >   accumulated into;
> > - 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;
> > - attach fragments with xdp_buff_add_frag(), which initializes all the
> >   shared_info fields and takes care of the pfmemalloc bit;
> > - release the buffers collected in the xdp_buff when a frame is dropped
> >   on RX errors; the previous code relied on dev_kfree_skb() recycling
> > the
> >   skb frags, which no longer applies now that the head and fragments
> > live
> >   in an xdp_buff rather than a partially built skb;
> > - drop the whole frame when it would exceed MAX_SKB_FRAGS, where the
> >   previous skb_add_rx_frag() calls were unbounded;
> > - strip the Ethernet FCS from the accumulated xdp_buff with a
> >   driver-local tail shrink (stmmac_xdp_shrink_tail()) for both linear
> >   and fragmented frames, instead of trimming the skb or the first
> >   descriptor length.
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> > ---
> 
> > [...]
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> 
> > [...]
> 
> > +static void
> > +stmmac_xdp_put_buff(struct stmmac_rx_queue *rx_q, struct xdp_buff *xdp,
> > +		    int sync_len, bool allow_direct)
> 
> This looks like xdp_return_buff(), apart from the sync length of the
> first buffer. Could the NAPI paths (XDP_CONSUMED and error_free_frag)
> use xdp_return_buff() instead? The teardown paths need allow_direct =
> false, so they would still need something local.

Theoretically I agree, but, since we still need stmmac_xdp_put_buff()
for the teardown path, I guess we can keep it since it is even an
optimized version with respect to xdp_return_buff(). What do yo think?

> 
> > +{
> > +	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> > +	int i;
> > +
> > +	if (likely(!xdp_buff_has_frags(xdp)))
> > +		goto out;
> > +
> > +	for (i = 0; i < sinfo->nr_frags; i++)
> > +		page_pool_put_full_page(rx_q->page_pool,
> > +					skb_frag_page(&sinfo->frags[i]),
> > +					allow_direct);
> > +out:
> > +	page_pool_put_page(rx_q->page_pool, virt_to_head_page(xdp->data),
> > +			   sync_len, allow_direct);
> > +}
> > +
> > +static struct sk_buff *stmmac_build_skb(struct xdp_buff *xdp)
> > +{
> 
> This is very close to xdp_build_skb_from_buff(). Have you checked if this
> might
> be a fit?

yes, but we can't use xdp_build_skb_from_buff() since it runs eth_type_trans()
modifying the skb->data pointer (in this way we will corrupt stmmac_rx_vlan()).
Agree?

> 
> > +	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> > +	u32 metasize = xdp->data - xdp->data_meta;
> > +	struct sk_buff *skb;
> > +	u8 num_frags = 0;
> > +
> > +	if (unlikely(xdp_buff_has_frags(xdp)))
> > +		num_frags = sinfo->nr_frags;
> > +
> > +	skb = napi_build_skb(xdp->data_hard_start, xdp->frame_sz);
> > +	if (!skb)
> > +		return NULL;
> > +
> > +	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);
> > +
> > +	if (unlikely(xdp_buff_has_frags(xdp)))
> > +		xdp_update_skb_frags_info(skb, num_frags, sinfo->xdp_frags_size,
> > +					  num_frags * xdp->frame_sz,
> > +					  xdp_buff_get_skb_flags(xdp));
> > +	return skb;
> > +}
> 
> > [...]
> 
> > +static int stmmac_xdp_shrink_tail(struct stmmac_rx_queue *rx_q,
> > +				  struct xdp_buff *xdp, int offset)
> 
> This is only used to drop the FCS. On the last descriptor we already
> know the frame length, so could the 4 bytes be taken off buf1_len /
> buf2_len before the buffers are added? Then only the rare case where
> the last buffer has fewer than 4 bytes would need to trim the previous
> frag, and this helper could go away.

uhm, right. I guess we could do something like (and drop
stmmac_xdp_shrink_tail()):

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e80793bd6553..5cb8a6883f44 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6027,6 +6027,19 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		buf2_len = stmmac_rx_buf2_len(priv, p, status, len);
 		len += buf2_len;
 
+		if (!(status & rx_not_ls)) {
+			/* ACS is disabled; strip manually. */
+			if (buf2_len > ETH_FCS_LEN) {
+				buf2_len -= ETH_FCS_LEN;
+			} else if (!buf2_len) {
+				buf1_len -= ETH_FCS_LEN;
+			} else {
+				buf1_len -= (ETH_FCS_LEN - buf2_len);
+				buf1_len = 0;
+			}
+			len -= ETH_FCS_LEN;
+		}
+
 		if (!frames) {
 			dma_sync_single_for_cpu(priv->device, buf->addr,
 						buf1_len, dma_dir);
@@ -6064,8 +6077,6 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		if (likely(status & rx_not_ls))
 			goto read_again;
 
-		/* ACS is disabled; strip manually. */
-		len -= ETH_FCS_LEN;
 		if (stmmac_xdp_shrink_tail(rx_q, &ctx.xdp, ETH_FCS_LEN)) {
 			if (!error)
 				rx_dropped++;

What do you think?

Regards,
Lorenzo

> 
> > [...]
> 
> Thanks,
> Nicolai

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