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: 4135 bytes --]

> On Fri, 06 Nov 2026 11:54:19 +0100 Lorenzo Bianconi wrote:
> > +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;
> > +	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,
> > +					  sinfo->xdp_frags_truesize,
> > +					  xdp_buff_get_skb_flags(xdp));
> > +	return skb;
> > +}
> 
> This is almost identical to xdp_build_skb_from_buff(), why not use the
> common code?

Right, this is almost xdp_build_skb_from_buff() but we can't use directly it
here since it runs eth_type_trans() and we would need to run stmmac_rx_vlan().
I guess we can refactor xdp_build_skb_from_buff() in something like:

struct sk_buff *__xdp_build_skb_from_buff(const struct xdp_buff *xdp)
{
	// Do not run eth_type_trans()
	....
	return skb;
}

static inline struct sk_buff *dp_build_skb_from_buff(const struct xdp_buff *xdp)
{
	__xdp_build_skb_from_buff()
	skb->protocol = eth_type_trans();

	return skb;
}


> 
> > +static void
> > +stmmac_xdp_put_buff(struct stmmac_rx_queue *rx_q, struct xdp_buff *xdp,
> > +		    bool allow_direct)
> > +{
> > +	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_full_page(rx_q->page_pool, virt_to_head_page(xdp->data),
> > +				allow_direct);
> > +}

ack, we can rework xdp_return_buff() adding the capability to specify
napi_direct.

> > +
> > +static int stmmac_xdp_shrink_tail(struct stmmac_rx_queue *rx_q,
> > +				  struct xdp_buff *xdp, int offset)
> > +{
> > +	struct skb_shared_info *sinfo;
> > +	int i;
> > +
> > +	if (unlikely(offset < 0 ||
> > +		     offset > (int)xdp_get_buff_len(xdp) - ETH_HLEN))
> > +		return -EINVAL;
> > +
> > +	if (likely(!xdp_buff_has_frags(xdp))) {
> > +		xdp->data_end -= offset;
> > +		return 0;
> > +	}
> > +
> > +	sinfo = xdp_get_shared_info_from_buff(xdp);
> > +	for (i = sinfo->nr_frags - 1; i >= 0 && offset > 0; i--) {
> > +		skb_frag_t *frag = &sinfo->frags[i];
> > +		int delta = min_t(int, offset, skb_frag_size(frag));
> > +
> > +		if (delta == skb_frag_size(frag)) {
> > +			/* The whole frag is consumed by the strip: return it
> > +			 * to the page pool right away. Its ring slot still
> > +			 * carries the page DMA address until stmmac_rx_refill()
> > +			 * re-arms it at the end of the NAPI poll. This is
> > +			 * safe because HW cannot touch the slot until then.
> > +			 */
> > +			page_pool_put_full_page(rx_q->page_pool,
> > +						skb_frag_page(frag), true);
> > +			sinfo->nr_frags--;
> > +			sinfo->xdp_frags_truesize -= xdp->frame_sz;
> > +		} else {
> > +			skb_frag_size_sub(frag, delta);
> > +		}
> > +
> > +		sinfo->xdp_frags_size -= delta;
> > +		offset -= delta;
> > +	}
> > +
> > +	if (unlikely(!sinfo->nr_frags)) {
> > +		xdp_buff_clear_frags_flag(xdp);
> > +		xdp_buff_clear_frag_pfmemalloc(xdp);
> > +		xdp->data_end -= offset;
> > +	}
> > +
> > +	return 0;
> > +}

I guess we can exctract the logic from
bpf_xdp_adjust_tail()/bpf_xdp_frags_shrink_tail() and create a helper we can
reuse it in stmmac and filter.c. What do you think?

Regards,
Lorenzo

> 
> These two OTOH could easily become new common helpers, they seem to do
> nothing stmmac specific?

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