[PATCH net-next v6 4/4] net: stmmac: rework stmmac_rx to support XDP rx multi-buff
Lorenzo Bianconi
lorenzo.bianconi at oss.qualcomm.com
Tue Oct 13 02:12:05 PDT 2026
> 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 at 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
-------------- 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/20261013/dd30446c/attachment-0001.sig>
More information about the linux-arm-kernel
mailing list