On 07/02, Song, Yoong Siang wrote: > On Wednesday, July 2, 2025 11:19 PM, Stanislav Fomichev > <[email protected]> wrote: > >On 07/02, Song, Yoong Siang wrote: > >> On Wednesday, July 2, 2025 10:23 AM, Song, Yoong Siang > ><[email protected]> wrote: > >> >On Wednesday, July 2, 2025 12:31 AM, Stanislav Fomichev > ><[email protected]> > >> >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 <[email protected]> > >> >>> --- > >> >>> 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/[email protected]/ > >> > > >> >> > >> >>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? > > Thank you for your feedback. I agree that we don't have a strong justification > for making the metadata length user-visible at this time. I concur with your > preference to keep everything as is and proceed with -(int)XDP_METADATA_SIZE. > > Btw, do you think whether my first patch which changes the documentation is > still needed or not?
Yes, the documentation is super useful, let's keep it!
