[v1] Clarify and Enhance XDP Rx Metadata Handling

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

Posted by Song Yoong Siang 3 months, 1 week ago

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@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;
+	metalen_to_adjust = XDP_METADATA_SIZE - metalen_used;
+	if (metalen_to_adjust < (int)sizeof(struct xdp_meta)) {
+		__sync_add_and_fetch(&pkts_skip, 1);
+		return XDP_PASS;
+	}
+
+	err = bpf_xdp_adjust_meta(ctx, -metalen_to_adjust);
 	if (err) {
 		__sync_add_and_fetch(&pkts_fail, 1);
 		return XDP_PASS;
diff --git a/tools/testing/selftests/bpf/progs/xdp_metadata.c b/tools/testing/selftests/bpf/progs/xdp_metadata.c
index 09bb8a038d52..a0ba4ef4bbd8 100644
--- a/tools/testing/selftests/bpf/progs/xdp_metadata.c
+++ b/tools/testing/selftests/bpf/progs/xdp_metadata.c
@@ -37,6 +37,7 @@ extern int bpf_xdp_metadata_rx_vlan_tag(const struct xdp_md *ctx,
 SEC("xdp")
 int rx(struct xdp_md *ctx)
 {
+	int metalen_used, metalen_to_adjust;
 	void *data, *data_meta, *data_end;
 	struct ipv6hdr *ip6h = NULL;
 	struct ethhdr *eth = NULL;
@@ -73,7 +74,12 @@ int rx(struct xdp_md *ctx)
 
 	/* Reserve enough for all custom metadata. */
 
-	ret = bpf_xdp_adjust_meta(ctx, -(int)sizeof(struct xdp_meta));
+	metalen_used = ctx->data - ctx->data_meta;
+	metalen_to_adjust = XDP_METADATA_SIZE - metalen_used;
+	if (metalen_to_adjust < (int)sizeof(struct xdp_meta))
+		return XDP_DROP;
+
+	ret = bpf_xdp_adjust_meta(ctx, -metalen_to_adjust);
 	if (ret != 0)
 		return XDP_DROP;
 
diff --git a/tools/testing/selftests/bpf/xdp_hw_metadata.c b/tools/testing/selftests/bpf/xdp_hw_metadata.c
index 3d8de0d4c96a..a529d55d4ff4 100644
--- a/tools/testing/selftests/bpf/xdp_hw_metadata.c
+++ b/tools/testing/selftests/bpf/xdp_hw_metadata.c
@@ -223,7 +223,7 @@ static void verify_xdp_metadata(void *data, clockid_t clock_id)
 {
 	struct xdp_meta *meta;
 
-	meta = data - sizeof(*meta);
+	meta = data - XDP_METADATA_SIZE;
 
 	if (meta->hint_valid & XDP_META_FIELD_RSS)
 		printf("rx_hash: 0x%X with RSS type:0x%X\n",
diff --git a/tools/testing/selftests/bpf/xdp_metadata.h b/tools/testing/selftests/bpf/xdp_metadata.h
index 87318ad1117a..2dfd3bf5e7bb 100644
--- a/tools/testing/selftests/bpf/xdp_metadata.h
+++ b/tools/testing/selftests/bpf/xdp_metadata.h
@@ -50,3 +50,10 @@ struct xdp_meta {
 	};
 	enum xdp_meta_field hint_valid;
 };
+
+/* XDP_METADATA_SIZE must be at least the size of struct xdp_meta. An additional
+ * 32 bytes of padding is included as a conservative measure to accommodate any
+ * metadata areas reserved by Ethernet devices. If the device-reserved metadata
+ * exceeds 32 bytes, this value will need adjustment.
+ */
+#define XDP_METADATA_SIZE	(sizeof(struct xdp_meta) + 32)
-- 
2.34.1

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

Posted by Stanislav Fomichev 3 months, 1 week ago

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@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? 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?

My assumptions was that we should just unconditionally do bpf_xdp_adjust_meta
with -XDP_METADATA_SIZE and that should be good enough.

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

Posted by Song, Yoong Siang 3 months, 1 week ago

On Wednesday, July 2, 2025 12:31 AM, Stanislav Fomichev <stfomichev@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.song@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.song@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

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

Posted by Song, Yoong Siang 3 months, 1 week ago

On Wednesday, July 2, 2025 10:23 AM, Song, Yoong Siang <yoong.siang.song@intel.com> wrote:
>On Wednesday, July 2, 2025 12:31 AM, Stanislav Fomichev <stfomichev@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.song@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.song@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?

Thanks & Regards
Siang

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

Posted by Stanislav Fomichev 3 months, 1 week ago

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

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

Posted by Song, Yoong Siang 3 months, 1 week ago

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

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

Posted by Stanislav Fomichev 3 months, 1 week ago

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

Yes, the documentation is super useful, let's keep it!

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

Posted by Song, Yoong Siang 3 months, 1 week ago

On Thursday, July 3, 2025 12:04 AM, Stanislav Fomichev <stfomichev@gmail.com> wrote:
>On 07/02, Song, Yoong Siang wrote:
>> On Wednesday, July 2, 2025 11:19 PM, Stanislav Fomichev
><stfomichev@gmail.com> wrote:
>> >On 07/02, Song, Yoong Siang wrote:
>> >> On Wednesday, July 2, 2025 10:23 AM, Song, Yoong Siang
>> ><yoong.siang.song@intel.com> wrote:
>> >> >On Wednesday, July 2, 2025 12:31 AM, Stanislav Fomichev
>> ><stfomichev@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.song@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.song@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?
>
>Yes, the documentation is super useful, let's keep it!

Sure. I will keep the documentation but submit v3 to remove the portion that
suggest user to use bpf_xdp_adjust_meta with METADATA_SIZE - (xdp_buff->data - xdp_buff->data_meta).

[PATCH bpf-next 1/2] doc: clarify XDP Rx metadata layout and bpf_xdp_adjust_meta usage
[PATCH bpf-next 2/2] selftests/bpf: Enhance XDP Rx Metadata Handling