Re: [PATCH bpf-next 2/2] selftests/bpf: Enhance XDP Rx Metadata Handling

[Stanislav Fomichev]

On 07/02, Song, Yoong Siang wrote:
> On Wednesday, July 2, 2025 10:23 AM, Song, Yoong Siang <yoong.siang.song@xxxxxxxxx> wrote:
> >On Wednesday, July 2, 2025 12:31 AM, Stanislav Fomichev <stfomichev@xxxxxxxxx>
> >wrote:
> >>On 07/01, Song Yoong Siang wrote:
> >>> Introduce the XDP_METADATA_SIZE macro to ensure that user applications can
> >>> consistently retrieve the correct location of struct xdp_meta.
> >>>
> >>> Prior to this commit, the XDP program adjusted the data_meta backward by
> >>> the size of struct xdp_meta, while the user application retrieved the data
> >>> by calculating backward from the data pointer. This approach only worked if
> >>> xdp_buff->data_meta was equal to xdp_buff->data before calling
> >>> bpf_xdp_adjust_meta.
> >>>
> >>> With the introduction of XDP_METADATA_SIZE, both the XDP program and user
> >>> application now calculate and identify the location of struct xdp_meta from
> >>> the data pointer. This ensures the implementation remains functional even
> >>> when there is device-reserved metadata, making the tests more portable
> >>> across different NICs.
> >>>
> >>> Signed-off-by: Song Yoong Siang <yoong.siang.song@xxxxxxxxx>
> >>> ---
> >>>  tools/testing/selftests/bpf/prog_tests/xdp_metadata.c |  2 +-
> >>>  tools/testing/selftests/bpf/progs/xdp_hw_metadata.c   | 10 +++++++++-
> >>>  tools/testing/selftests/bpf/progs/xdp_metadata.c      |  8 +++++++-
> >>>  tools/testing/selftests/bpf/xdp_hw_metadata.c         |  2 +-
> >>>  tools/testing/selftests/bpf/xdp_metadata.h            |  7 +++++++
> >>>  5 files changed, 25 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> >>b/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> >>> index 19f92affc2da..8d6c2633698b 100644
> >>> --- a/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> >>> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> >>> @@ -302,7 +302,7 @@ static int verify_xsk_metadata(struct xsk *xsk, bool
> >>sent_from_af_xdp)
> >>>
> >>>  	/* custom metadata */
> >>>
> >>> -	meta = data - sizeof(struct xdp_meta);
> >>> +	meta = data - XDP_METADATA_SIZE;
> >>>
> >>>  	if (!ASSERT_NEQ(meta->rx_timestamp, 0, "rx_timestamp"))
> >>>  		return -1;
> >>> diff --git a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> >>b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> >>> index 330ece2eabdb..72242ac1cdcd 100644
> >>> --- a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> >>> +++ b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> >>> @@ -27,6 +27,7 @@ extern int bpf_xdp_metadata_rx_vlan_tag(const struct
> >>xdp_md *ctx,
> >>>  SEC("xdp.frags")
> >>>  int rx(struct xdp_md *ctx)
> >>>  {
> >>> +	int metalen_used, metalen_to_adjust;
> >>>  	void *data, *data_meta, *data_end;
> >>>  	struct ipv6hdr *ip6h = NULL;
> >>>  	struct udphdr *udp = NULL;
> >>> @@ -72,7 +73,14 @@ int rx(struct xdp_md *ctx)
> >>>  		return XDP_PASS;
> >>>  	}
> >>>
> >>> -	err = bpf_xdp_adjust_meta(ctx, -(int)sizeof(struct xdp_meta));
> >>
> >>[..]
> >>
> >>> +	metalen_used = ctx->data - ctx->data_meta;
> >>
> >>Is the intent here to query how much metadata has been consumed/reserved
> >>by the driver?
> >Yes.
> >
> >>Looking at IGC it has the following code/comment:
> >>
> >>	bi->xdp->data += IGC_TS_HDR_LEN;
> >>
> >>	/* HW timestamp has been copied into local variable. Metadata
> >>	 * length when XDP program is called should be 0.
> >>	 */
> >>	bi->xdp->data_meta += IGC_TS_HDR_LEN;
> >>
> >>Are you sure that metadata size is correctly exposed to the bpf program?
> >You are right, the current igc driver didn't expose the metadata size correctly.
> >I submitted [1] to fix it.
> >
> >[1] https://patchwork.ozlabs.org/project/intel-wired-
> >lan/patch/20250701080955.3273137-1-yoong.siang.song@xxxxxxxxx/
> >
> >>
> >>My assumptions was that we should just unconditionally do bpf_xdp_adjust_meta
> >>with -XDP_METADATA_SIZE and that should be good enough.
> >
> >The checking is just for precautions. No problem if directly adjust the meta
> >unconditionally.
> >That will save processing time for each packet as well.
> >I will remove the checking and submit v2.
> >
> >Thanks & Regards
> >Siang
> >
> 
> Hi Stanislav Fomichev,
> 
> I submitted v2. But after that, I think twice. IMHO, 
> err = bpf_xdp_adjust_meta(ctx, (int)(ctx->data - ctx->data_meta - XDP_METADATA_SIZE));
> is better than
> err = bpf_xdp_adjust_meta(ctx, -(int)XDP_METADATA_SIZE);
> because it is more robust.
> 
> Any thoughts?

My preference is on keeping everything as is and converting to
-(int)XDP_METADATA_SIZE. Making IGC properly expose (temporary) metadata len
is a user visible change, not sure we have a good justification?