On Wednesday, July 2, 2025 11:19 PM, Stanislav Fomichev <stfomic...@gmail.com>
wrote:
>On 07/02, Song, Yoong Siang wrote:
>> On Wednesday, July 2, 2025 10:23 AM, Song, Yoong Siang
><yoong.siang.s...@intel.com> wrote:
>> >On Wednesday, July 2, 2025 12:31 AM, Stanislav Fomichev
><stfomic...@gmail.com>
>> >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.s...@intel.com>
>> >>> ---
>> >>> 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.s...@intel.com/
>> >
>> >>
>> >>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?
