On Wed Mar 5, 2025 at 4:24 PM CET, Alexander Lobakin wrote:
> From: Arthur <arthur@xxxxxxxxxxxxxxx>
> Date: Wed, 05 Mar 2025 15:32:04 +0100
>
> > From: Arthur Fabre <afabre@xxxxxxxxxxxxxx>
> >
> > xdp_buff stores whether metadata is supported by a NIC by setting
> > data_meta to be greater than data.
> >
> > But xdp_frame only stores the metadata size (as metasize), so converting
> > between xdp_frame and xdp_buff is lossy.
> >
> > Steal an unused bit in xdp_frame to track whether metadata is supported
> > or not.
> >
> > This will lets us have "generic" functions for setting skb fields from
> > either xdp_frame or xdp_buff from drivers.
> >
> > Signed-off-by: Arthur Fabre <afabre@xxxxxxxxxxxxxx>
> > ---
> > include/net/xdp.h | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > index 58019fa299b56dbd45c104fdfa807f73af6e4fa4..84afe07d09efdb2ab0cb78b904f02cb74f9a56b6 100644
> > --- a/include/net/xdp.h
> > +++ b/include/net/xdp.h
> > @@ -116,6 +116,9 @@ static __always_inline void xdp_buff_set_frag_pfmemalloc(struct xdp_buff *xdp)
> > xdp->flags |= XDP_FLAGS_FRAGS_PF_MEMALLOC;
> > }
> >
> > +static bool xdp_data_meta_unsupported(const struct xdp_buff *xdp);
> > +static void xdp_set_data_meta_invalid(struct xdp_buff *xdp);
> > +
> > static __always_inline void *xdp_buff_traits(const struct xdp_buff *xdp)
> > {
> > return xdp->data_hard_start + _XDP_FRAME_SIZE;
> > @@ -270,7 +273,9 @@ struct xdp_frame {
> > void *data;
> > u32 len;
> > u32 headroom;
> > - u32 metasize; /* uses lower 8-bits */
> > + u32 :23, /* unused */
> > + meta_unsupported:1,
> > + metasize:8;
>
> See the history of this structure how we got rid of using bitfields here
> and why.
>
> ...because of performance.
>
> Even though metasize uses only 8 bits, 1-byte access is slower than
> 32-byte access.
Interesting, thanks!
> I was going to write "you can use the fact that metasize is always a
> multiple of 4 or that it's never > 252, for example, you could reuse LSB
> as a flag indicating that meta is not supported", but first of all
>
> Do we still have drivers which don't support metadata?
> Why don't they do that? It's not HW-specific or even driver-specific.
> They don't reserve headroom? Then they're invalid, at least XDP_REDIRECT
> won't work.
>
> So maybe we need to fix those drivers first, if there are any.
Most drivers don't support metadata unfortunately:
> rg -U "xdp_prepare_buff\([^)]*false\);" drivers/net/
drivers/net/tun.c
1712: xdp_prepare_buff(&xdp, buf, pad, len, false);
drivers/net/ethernet/microsoft/mana/mana_bpf.c
94: xdp_prepare_buff(xdp, buf_va, XDP_PACKET_HEADROOM, pkt_len, false);
drivers/net/ethernet/marvell/mvneta.c
2344: xdp_prepare_buff(xdp, data, pp->rx_offset_correction + MVNETA_MH_SIZE,
2345: data_len, false);
drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
1436: xdp_prepare_buff(&xdp, hard_start, OTX2_HEAD_ROOM,
1437: cqe->sg.seg_size, false);
drivers/net/ethernet/socionext/netsec.c
1021: xdp_prepare_buff(&xdp, desc->addr, NETSEC_RXBUF_HEADROOM,
1022: pkt_len, false);
drivers/net/ethernet/google/gve/gve_rx.c
740: xdp_prepare_buff(&new, frame, headroom, len, false);
859: xdp_prepare_buff(&xdp, page_info->page_address +
860: page_info->page_offset, GVE_RX_PAD,
861: len, false);
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
3984: xdp_prepare_buff(&xdp, data,
3985: MVPP2_MH_SIZE + MVPP2_SKB_HEADROOM,
3986: rx_bytes, false);
drivers/net/ethernet/aquantia/atlantic/aq_ring.c
794: xdp_prepare_buff(&xdp, hard_start, rx_ring->page_offset,
795: buff->len, false);
drivers/net/ethernet/cavium/thunder/nicvf_main.c
554: xdp_prepare_buff(&xdp, hard_start, data - hard_start, len, false);
drivers/net/ethernet/ti/cpsw_new.c
348: xdp_prepare_buff(&xdp, pa, headroom, size, false);
drivers/net/ethernet/freescale/enetc/enetc.c
1710: xdp_prepare_buff(xdp_buff, hard_start - rx_ring->buffer_offset,
1711: rx_ring->buffer_offset, size, false);
drivers/net/ethernet/ti/am65-cpsw-nuss.c
1335: xdp_prepare_buff(&xdp, page_addr, AM65_CPSW_HEADROOM,
1336: pkt_len, false);
drivers/net/ethernet/ti/cpsw.c
403: xdp_prepare_buff(&xdp, pa, headroom, size, false);
drivers/net/ethernet/sfc/rx.c
289: xdp_prepare_buff(&xdp, *ehp - EFX_XDP_HEADROOM, EFX_XDP_HEADROOM,
290: rx_buf->len, false);
drivers/net/ethernet/mediatek/mtk_eth_soc.c
2097: xdp_prepare_buff(&xdp, data, MTK_PP_HEADROOM, pktlen,
2098: false);
drivers/net/ethernet/sfc/siena/rx.c
291: xdp_prepare_buff(&xdp, *ehp - EFX_XDP_HEADROOM, EFX_XDP_HEADROOM,
292: rx_buf->len, false)
I don't know if it's just because no one has added calls to
skb_metadata_set() in yet, or if there's a more fundamental reason.
I think they all reserve some amount of headroom, but not always the
full XDP_PACKET_HEADROOM. Eg sfc:
drivers/net/ethernet/sfc/net_driver.h:
/* Non-standard XDP_PACKET_HEADROOM and tailroom to satisfy XDP_REDIRECT and
* still fit two standard MTU size packets into a single 4K page.
*/
#define EFX_XDP_HEADROOM 128
If it's just because skb_metadata_set() is missing, I can take the
patches from this series that adds a "generic" XDP -> skb hook ("trait:
Propagate presence of traits to sk_buff"), have it call
skb_metadata_set(), and try to add it to all the drivers in a separate
series.
> > /* Lifetime of xdp_rxq_info is limited to NAPI/enqueue time,
> > * while mem_type is valid on remote CPU.
> > */
> > @@ -369,6 +374,8 @@ void xdp_convert_frame_to_buff(const struct xdp_frame *frame,
> > xdp->data = frame->data;
> > xdp->data_end = frame->data + frame->len;
> > xdp->data_meta = frame->data - frame->metasize;
> > + if (frame->meta_unsupported)
> > + xdp_set_data_meta_invalid(xdp);
> > xdp->frame_sz = frame->frame_sz;
> > xdp->flags = frame->flags;
> > }
> > @@ -396,6 +403,7 @@ int xdp_update_frame_from_buff(const struct xdp_buff *xdp,
> > xdp_frame->len = xdp->data_end - xdp->data;
> > xdp_frame->headroom = headroom - sizeof(*xdp_frame);
> > xdp_frame->metasize = metasize;
> > + xdp_frame->meta_unsupported = xdp_data_meta_unsupported(xdp);
> > xdp_frame->frame_sz = xdp->frame_sz;
> > xdp_frame->flags = xdp->flags;
>
> Thanks,
> Olek
