Message-ID: <aG6Y1J9Li2DdjqWv@mini-arch>
Date: Wed, 9 Jul 2025 09:29:08 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Daniel Borkmann <daniel@...earbox.net>
Cc: Jesper Dangaard Brouer <hawk@...nel.org>,
	"Song, Yoong Siang" <yoong.siang.song@...el.com>,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, Jonathan Corbet <corbet@....net>,
	Alexei Starovoitov <ast@...nel.org>,
	John Fastabend <john.fastabend@...il.com>,
	Stanislav Fomichev <sdf@...ichev.me>,
	Andrii Nakryiko <andrii@...nel.org>,
	Martin KaFai Lau <martin.lau@...ux.dev>,
	Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>,
	Yonghong Song <yonghong.song@...ux.dev>,
	KP Singh <kpsingh@...nel.org>, Hao Luo <haoluo@...gle.com>,
	Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>,
	Shuah Khan <shuah@...nel.org>,
	Magnus Karlsson <magnus.karlsson@...il.com>,
	Björn Töpel <bjorn@...nel.org>,
	Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
	Jonathan Lemon <jonathan.lemon@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"bpf@...r.kernel.org" <bpf@...r.kernel.org>,
	"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH bpf-next,v3 2/2] selftests/bpf: Enhance XDP Rx metadata
 handling

On 07/09, Daniel Borkmann wrote:
> On 7/7/25 5:03 PM, Stanislav Fomichev wrote:
> > On 07/04, Daniel Borkmann wrote:
> > > On 7/4/25 11:58 AM, Jesper Dangaard Brouer wrote:
> > > > On 04/07/2025 03.17, Song, Yoong Siang wrote:
> > > > > On Friday, July 4, 2025 1:05 AM, Jesper Dangaard Brouer <hawk@...nel.org> wrote:
> > > > > > On 02/07/2025 18.57, Song Yoong Siang wrote:
> > > > > > > Introduce the XDP_METADATA_SIZE macro as a conservative measure to
> > > > > > > accommodate any metadata areas reserved by Ethernet devices.
> > > > > > 
> > > > > > This seems like a sloppy workaround :-(
> > > > > > 
> > > > > > To me, the problem arise because AF_XDP is lacking the ability to
> > > > > > communicate the size of the data_meta area.  If we had this capability,
> > > > > > then we could allow the IGC driver to take some of the space, have the
> > > > > > BPF-prog expand it futher (bpf_xdp_adjust_meta) and then userspace
> > > > > > AF_XDP would simply be able to see the size of the data_meta area, and
> > > > > > apply the struct xdp_meta at right offset.
> > > > > > 
> > > > > Thanks for your input.
> > > > > 
> > > > > I agree with you that the implementation will be simple if user application
> > > > > able to get the size of data_meta area. The intention of this patch set is to let
> > > > > developer aware of such limitations before we have a perfect solution.
> > > > > 
> > > > > Btw, do you got any suggestion on how to expose the metadata length?
> > > > > I not sure whether xdp_desc.options is a simple and good idea or not?
> > > > 
> > > > That is a question to the AF_XDP maintainers... added them to this email.
> > > > 
> > > > /* Rx/Tx descriptor */
> > > > struct xdp_desc {
> > > >       __u64 addr;
> > > >       __u32 len;
> > > >       __u32 options;
> > > > };
> > > > 
> > > > As far as I know, the xdp_desc.options field isn't used, right?
> > > 
> > > The options holds flags, see also XDP_PKT_CONTD and XDP_TX_METADATA.
> > > 
> > > > (Please AF_XDP experts, please verify below statements:)
> > > > Something else we likely want to document: The available headroom in the
> > > > AF_XDP frame.  When accessing the metadata in userspace AF_XDP we do a
> > > > negative offset from the UMEM packet pointer.  IIRC on RX the available
> > > > headroom will be either 255 or 192 bytes (depending on NIC drivers).
> > > > 
> > > > Slightly confusing when AF_XDP transmitting from userspace the UMEM
> > > > headroom is default zero (XSK_UMEM__DEFAULT_FRAME_HEADROOM is zero).
> > > > This is configurable via xsk_umem_config.frame_headroom, like I did in
> > > > this example[1].
> > > > 
> > > > Maybe I did something wrong in[1], because I see that the new method is
> > > > setting xsk_umem_config.tx_metadata_len + flag XDP_UMEM_TX_METADATA_LEN.
> > > > This is nicely documented in [2]. How does this interact with setting
> > > > xsk_umem_config.frame_headroom ?
> > > 
> > > If you request XDP_UMEM_TX_METADATA_LEN then on TX side you can fill
> > > struct xsk_tx_metadata before the start of packet data, that is,
> > > meta = data - sizeof(struct xsk_tx_metadata). The validity of the
> > > latter is indicated via desc->options |= XDP_TX_METADATA and then
> > > you fill meta->flags with things like XDP_TXMD_FLAGS_CHECKSUM to
> > > tell that the related fields are valid (ex. request.csum_start,
> > > request.csum_offset) and that you expect the driver to do the
> > > offload with this info. This is also what I mentioned in the other
> > > thread some time ago that imho it would make sense to have this also
> > > on RX side somewhat similar to virtio_net_hdr..
> > 
> > Let's at least document the current behavior where some (small minority of)
> > drivers can reuse the rx metadata area for some of its state? If we want
> > to improve on that by adding another knob, we can follow up?
> > (but I remember last time it was discussed, about a year ago, people
> > were not enthusiastic about another parameter exported as uapi)
> 
> But its still fundamentally broken no? Unless there is no harm for BPF devs
> to override that rx metadata area when the pkt later on goes up the stack, but
> it sounds this is not the case here. Iiuc, Yoong is trying a different approach
> now to prepend before data_hard_start [0]? Then if BPF prog needs it, igc
> already implements xmo_rx_timestamp callback which can copy it from there.
> 
>   [0] https://lore.kernel.org/bpf/20250707191742.662aeffb@kernel.org/

True, Jakub mentioned the same thread to me. This is, indeed, a better
idea!

Powered by blists - more mailing lists