[PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Andy Gospodarek]

[-- Attachment #1: Type: text/plain, Size: 2869 bytes --]

This changes the section name for the bpf program embedded in these
files to "xdp.frags" to allow the programs to be loaded on drivers that
are using an MTU greater than PAGE_SIZE.  Rather than directly accessing
the buffers, the packet data is now accessed via xdp helper functions to
provide an example for those who may need to write more complex
programs.

v2: remove new unnecessary variable

Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 samples/bpf/xdp1_kern.c            | 11 ++++++++---
 samples/bpf/xdp2_kern.c            | 11 ++++++++---
 samples/bpf/xdp_tx_iptunnel_kern.c |  2 +-
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/samples/bpf/xdp1_kern.c b/samples/bpf/xdp1_kern.c
index f0c5d95084de..0a5c704badd0 100644
--- a/samples/bpf/xdp1_kern.c
+++ b/samples/bpf/xdp1_kern.c
@@ -39,11 +39,13 @@ static int parse_ipv6(void *data, u64 nh_off, void *data_end)
 	return ip6h->nexthdr;
 }
 
-SEC("xdp1")
+#define XDPBUFSIZE	64
+SEC("xdp.frags")
 int xdp_prog1(struct xdp_md *ctx)
 {
-	void *data_end = (void *)(long)ctx->data_end;
-	void *data = (void *)(long)ctx->data;
+	__u8 pkt[XDPBUFSIZE] = {};
+	void *data_end = &pkt[XDPBUFSIZE-1];
+	void *data = pkt;
 	struct ethhdr *eth = data;
 	int rc = XDP_DROP;
 	long *value;
@@ -51,6 +53,9 @@ int xdp_prog1(struct xdp_md *ctx)
 	u64 nh_off;
 	u32 ipproto;
 
+	if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt)))
+		return rc;
+
 	nh_off = sizeof(*eth);
 	if (data + nh_off > data_end)
 		return rc;
diff --git a/samples/bpf/xdp2_kern.c b/samples/bpf/xdp2_kern.c
index d8a64ab077b0..3332ba6bb95f 100644
--- a/samples/bpf/xdp2_kern.c
+++ b/samples/bpf/xdp2_kern.c
@@ -55,11 +55,13 @@ static int parse_ipv6(void *data, u64 nh_off, void *data_end)
 	return ip6h->nexthdr;
 }
 
-SEC("xdp1")
+#define XDPBUFSIZE	64
+SEC("xdp.frags")
 int xdp_prog1(struct xdp_md *ctx)
 {
-	void *data_end = (void *)(long)ctx->data_end;
-	void *data = (void *)(long)ctx->data;
+	__u8 pkt[XDPBUFSIZE] = {};
+	void *data_end = &pkt[XDPBUFSIZE-1];
+	void *data = pkt;
 	struct ethhdr *eth = data;
 	int rc = XDP_DROP;
 	long *value;
@@ -67,6 +69,9 @@ int xdp_prog1(struct xdp_md *ctx)
 	u64 nh_off;
 	u32 ipproto;
 
+	if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt)))
+		return rc;
+
 	nh_off = sizeof(*eth);
 	if (data + nh_off > data_end)
 		return rc;
diff --git a/samples/bpf/xdp_tx_iptunnel_kern.c b/samples/bpf/xdp_tx_iptunnel_kern.c
index 575d57e4b8d6..0e2bca3a3fff 100644
--- a/samples/bpf/xdp_tx_iptunnel_kern.c
+++ b/samples/bpf/xdp_tx_iptunnel_kern.c
@@ -212,7 +212,7 @@ static __always_inline int handle_ipv6(struct xdp_md *xdp)
 	return XDP_TX;
 }
 
-SEC("xdp_tx_iptunnel")
+SEC("xdp.frags")
 int _xdp_tx_iptunnel(struct xdp_md *xdp)
 {
 	void *data_end = (void *)(long)xdp->data_end;
-- 
2.25.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4222 bytes --]

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Tue, 21 Jun 2022 17:54:02 +0000 you wrote:
> This changes the section name for the bpf program embedded in these
> files to "xdp.frags" to allow the programs to be loaded on drivers that
> are using an MTU greater than PAGE_SIZE.  Rather than directly accessing
> the buffers, the packet data is now accessed via xdp helper functions to
> provide an example for those who may need to write more complex
> programs.
> 
> [...]

Here is the summary with links:
  - [net-next,v2] samples/bpf: fixup some tools to be able to support xdp multibuffer
    https://git.kernel.org/bpf/bpf-next/c/772251742262

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Tariq Toukan]

On 21/06/2022 20:54, Andy Gospodarek wrote:
> This changes the section name for the bpf program embedded in these
> files to "xdp.frags" to allow the programs to be loaded on drivers that
> are using an MTU greater than PAGE_SIZE.  Rather than directly accessing
> the buffers, the packet data is now accessed via xdp helper functions to
> provide an example for those who may need to write more complex
> programs.
> 
> v2: remove new unnecessary variable
> 

Hi,

I'm trying to understand if there are any assumptions/requirements on 
the length of the xdp_buf linear part when passed to XDP multi-buf programs?
Can the linear part be empty, with all data residing in the fragments? 
Is it valid?

Per the proposed pattern below (calling bpf_xdp_load_bytes() to memcpy 
packet data into a local buffer), no such assumption is required, and an 
xdp_buf created by the driver with an empty linear part is valid.

However, in the _xdp_tx_iptunnel example program, it fails (returns 
XDP_DROP) in case the headers are not in the linear part.

Regards,
Tariq

> Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
> Acked-by: John Fastabend <john.fastabend@gmail.com>
> Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>   samples/bpf/xdp1_kern.c            | 11 ++++++++---
>   samples/bpf/xdp2_kern.c            | 11 ++++++++---
>   samples/bpf/xdp_tx_iptunnel_kern.c |  2 +-
>   3 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/samples/bpf/xdp1_kern.c b/samples/bpf/xdp1_kern.c
> index f0c5d95084de..0a5c704badd0 100644
> --- a/samples/bpf/xdp1_kern.c
> +++ b/samples/bpf/xdp1_kern.c
> @@ -39,11 +39,13 @@ static int parse_ipv6(void *data, u64 nh_off, void *data_end)
>   	return ip6h->nexthdr;
>   }
>   
> -SEC("xdp1")
> +#define XDPBUFSIZE	64
> +SEC("xdp.frags")
>   int xdp_prog1(struct xdp_md *ctx)
>   {
> -	void *data_end = (void *)(long)ctx->data_end;
> -	void *data = (void *)(long)ctx->data;
> +	__u8 pkt[XDPBUFSIZE] = {};
> +	void *data_end = &pkt[XDPBUFSIZE-1];
> +	void *data = pkt;
>   	struct ethhdr *eth = data;
>   	int rc = XDP_DROP;
>   	long *value;
> @@ -51,6 +53,9 @@ int xdp_prog1(struct xdp_md *ctx)
>   	u64 nh_off;
>   	u32 ipproto;
>   
> +	if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt)))
> +		return rc;
> +
>   	nh_off = sizeof(*eth);
>   	if (data + nh_off > data_end)
>   		return rc;
> diff --git a/samples/bpf/xdp2_kern.c b/samples/bpf/xdp2_kern.c
> index d8a64ab077b0..3332ba6bb95f 100644
> --- a/samples/bpf/xdp2_kern.c
> +++ b/samples/bpf/xdp2_kern.c
> @@ -55,11 +55,13 @@ static int parse_ipv6(void *data, u64 nh_off, void *data_end)
>   	return ip6h->nexthdr;
>   }
>   
> -SEC("xdp1")
> +#define XDPBUFSIZE	64
> +SEC("xdp.frags")
>   int xdp_prog1(struct xdp_md *ctx)
>   {
> -	void *data_end = (void *)(long)ctx->data_end;
> -	void *data = (void *)(long)ctx->data;
> +	__u8 pkt[XDPBUFSIZE] = {};
> +	void *data_end = &pkt[XDPBUFSIZE-1];
> +	void *data = pkt;
>   	struct ethhdr *eth = data;
>   	int rc = XDP_DROP;
>   	long *value;
> @@ -67,6 +69,9 @@ int xdp_prog1(struct xdp_md *ctx)
>   	u64 nh_off;
>   	u32 ipproto;
>   
> +	if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt)))
> +		return rc;
> +
>   	nh_off = sizeof(*eth);
>   	if (data + nh_off > data_end)
>   		return rc;
> diff --git a/samples/bpf/xdp_tx_iptunnel_kern.c b/samples/bpf/xdp_tx_iptunnel_kern.c
> index 575d57e4b8d6..0e2bca3a3fff 100644
> --- a/samples/bpf/xdp_tx_iptunnel_kern.c
> +++ b/samples/bpf/xdp_tx_iptunnel_kern.c
> @@ -212,7 +212,7 @@ static __always_inline int handle_ipv6(struct xdp_md *xdp)
>   	return XDP_TX;
>   }
>   
> -SEC("xdp_tx_iptunnel")
> +SEC("xdp.frags")
>   int _xdp_tx_iptunnel(struct xdp_md *xdp)
>   {
>   	void *data_end = (void *)(long)xdp->data_end;

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Toke Høiland-Jørgensen]

Tariq Toukan <ttoukan.linux@gmail.com> writes:

> On 21/06/2022 20:54, Andy Gospodarek wrote:
>> This changes the section name for the bpf program embedded in these
>> files to "xdp.frags" to allow the programs to be loaded on drivers that
>> are using an MTU greater than PAGE_SIZE.  Rather than directly accessing
>> the buffers, the packet data is now accessed via xdp helper functions to
>> provide an example for those who may need to write more complex
>> programs.
>> 
>> v2: remove new unnecessary variable
>> 
>
> Hi,
>
> I'm trying to understand if there are any assumptions/requirements on 
> the length of the xdp_buf linear part when passed to XDP multi-buf programs?
> Can the linear part be empty, with all data residing in the fragments? 
> Is it valid?
>
> Per the proposed pattern below (calling bpf_xdp_load_bytes() to memcpy 
> packet data into a local buffer), no such assumption is required, and an 
> xdp_buf created by the driver with an empty linear part is valid.
>
> However, in the _xdp_tx_iptunnel example program, it fails (returns 
> XDP_DROP) in case the headers are not in the linear part.

Hmm, good question! I don't think we've ever explicitly documented any
assumptions one way or the other. My own mental model has certainly
always assumed the first frag would continue to be the same size as in
non-multi-buf packets.

I do seem to recall there was some discussion around this when we were
discussing whether or not we needed programs to explicitly opt-in to
multi-buf support (what ended up being the "xdp.frags" section). The
reason we said it might *not* be necessary to do that was that most
programs would just continue working, and it would only be those that
either tried to access the end of the packet, or to compute the packet
length as data_end-data that would need any changes before enabling
the frags flag. Which also kinda implies that headers etc would continue
to be in the linear part.

This is all from memory, though, so maybe others have different
recollections. In any case this is probably something we should document
somewhere :)

-Toke

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Jakub Kicinski]

On Tue, 03 Jan 2023 16:19:49 +0100 Toke Høiland-Jørgensen wrote:
> Hmm, good question! I don't think we've ever explicitly documented any
> assumptions one way or the other. My own mental model has certainly
> always assumed the first frag would continue to be the same size as in
> non-multi-buf packets.

Interesting! :) My mental model was closer to GRO by frags 
so the linear part would have no data, just headers.

A random datapoint is that bpf_xdp_adjust_head() seems 
to enforce that there is at least ETH_HLEN.

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

> On Tue, 03 Jan 2023 16:19:49 +0100 Toke Høiland-Jørgensen wrote:
> > Hmm, good question! I don't think we've ever explicitly documented any
> > assumptions one way or the other. My own mental model has certainly
> > always assumed the first frag would continue to be the same size as in
> > non-multi-buf packets.
> 
> Interesting! :) My mental model was closer to GRO by frags 
> so the linear part would have no data, just headers.

That is assumption as well.

Regards,
Lorenzo

> 
> A random datapoint is that bpf_xdp_adjust_head() seems 
> to enforce that there is at least ETH_HLEN.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Toke Høiland-Jørgensen]

Lorenzo Bianconi <lorenzo@kernel.org> writes:

>> On Tue, 03 Jan 2023 16:19:49 +0100 Toke Høiland-Jørgensen wrote:
>> > Hmm, good question! I don't think we've ever explicitly documented any
>> > assumptions one way or the other. My own mental model has certainly
>> > always assumed the first frag would continue to be the same size as in
>> > non-multi-buf packets.
>> 
>> Interesting! :) My mental model was closer to GRO by frags 
>> so the linear part would have no data, just headers.
>
> That is assumption as well.

Right, okay, so how many headers? Only Ethernet, or all the way up to
L4 (TCP/UDP)?

I do seem to recall a discussion around the header/data split for TCP
specifically, but I think I mentally put that down as "something people
may way to do at some point in the future", which is why it hasn't made
it into my own mental model (yet?) :)

-Toke

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Jakub Kicinski]

On Wed, 04 Jan 2023 13:28:57 +0100 Toke Høiland-Jørgensen wrote:
> >> Interesting! :) My mental model was closer to GRO by frags 
> >> so the linear part would have no data, just headers.  
> >
> > That is assumption as well.  
> 
> Right, okay, so how many headers? Only Ethernet, or all the way up to
> L4 (TCP/UDP)?

If we're speaking about guarantees or hard requirements - I think that
we can only require / guarantee the Ethernet header. Requiring more
will be defeated by tunnels (i.e. adjust_head() + redirect to a veth).

> I do seem to recall a discussion around the header/data split for TCP
> specifically, but I think I mentally put that down as "something people
> may way to do at some point in the future", which is why it hasn't made
> it into my own mental model (yet?) :)

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Tariq Toukan]

On 04/01/2023 14:28, Toke Høiland-Jørgensen wrote:
> Lorenzo Bianconi <lorenzo@kernel.org> writes:
> 
>>> On Tue, 03 Jan 2023 16:19:49 +0100 Toke Høiland-Jørgensen wrote:
>>>> Hmm, good question! I don't think we've ever explicitly documented any
>>>> assumptions one way or the other. My own mental model has certainly
>>>> always assumed the first frag would continue to be the same size as in
>>>> non-multi-buf packets.
>>>
>>> Interesting! :) My mental model was closer to GRO by frags
>>> so the linear part would have no data, just headers.
>>
>> That is assumption as well.
> 
> Right, okay, so how many headers? Only Ethernet, or all the way up to
> L4 (TCP/UDP)?
> 
> I do seem to recall a discussion around the header/data split for TCP
> specifically, but I think I mentally put that down as "something people
> may way to do at some point in the future", which is why it hasn't made
> it into my own mental model (yet?) :)
> 
> -Toke
> 

I don't think that all the different GRO layers assume having their 
headers/data in the linear part. IMO they will just perform better if 
these parts are already there. Otherwise, the GRO flow manages, and 
pulls the needed amount into the linear part.
As examples, see calls to gro_pull_from_frag0 in net/core/gro.c, and the 
call to pskb_may_pull() from skb_gro_header_slow().

This resembles the bpf_xdp_load_bytes() API used here in the xdp prog.

The context of my questions is that I'm looking for the right memory 
scheme for adding xdp-mb support to mlx5e striding RQ.
In striding RQ, the RX buffer consists of "strides" of a fixed size set 
by the driver. An incoming packet is written to the buffer starting from 
the beginning of the next available stride, consuming as much strides as 
needed.

Due to the need for headroom and tailroom, there's no easy way of 
building the xdp_buf in place (around the packet), so it should go to a 
side buffer.

By using 0-length linear part in a side buffer, I can address two 
challenging issues: (1) save the in-driver headers memcpy (copy might 
still exist in the xdp program though), and (2) conform to the 
"fragments of the same size" requirement/assumption in xdp-mb. 
Otherwise, if we pull from frag[0] into the linear part, frag[0] becomes 
smaller than the next fragments.

Regards,
Tariq

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Toke Høiland-Jørgensen]

Tariq Toukan <ttoukan.linux@gmail.com> writes:

> On 04/01/2023 14:28, Toke Høiland-Jørgensen wrote:
>> Lorenzo Bianconi <lorenzo@kernel.org> writes:
>> 
>>>> On Tue, 03 Jan 2023 16:19:49 +0100 Toke Høiland-Jørgensen wrote:
>>>>> Hmm, good question! I don't think we've ever explicitly documented any
>>>>> assumptions one way or the other. My own mental model has certainly
>>>>> always assumed the first frag would continue to be the same size as in
>>>>> non-multi-buf packets.
>>>>
>>>> Interesting! :) My mental model was closer to GRO by frags
>>>> so the linear part would have no data, just headers.
>>>
>>> That is assumption as well.
>> 
>> Right, okay, so how many headers? Only Ethernet, or all the way up to
>> L4 (TCP/UDP)?
>> 
>> I do seem to recall a discussion around the header/data split for TCP
>> specifically, but I think I mentally put that down as "something people
>> may way to do at some point in the future", which is why it hasn't made
>> it into my own mental model (yet?) :)
>> 
>> -Toke
>> 
>
> I don't think that all the different GRO layers assume having their 
> headers/data in the linear part. IMO they will just perform better if 
> these parts are already there. Otherwise, the GRO flow manages, and 
> pulls the needed amount into the linear part.
> As examples, see calls to gro_pull_from_frag0 in net/core/gro.c, and the 
> call to pskb_may_pull() from skb_gro_header_slow().
>
> This resembles the bpf_xdp_load_bytes() API used here in the xdp prog.

Right, but that is kernel code; what we end up doing with the API here
affects how many programs need to make significant changes to work with
multibuf, and how many can just set the frags flag and continue working.
Which also has a performance impact, see below.

> The context of my questions is that I'm looking for the right memory 
> scheme for adding xdp-mb support to mlx5e striding RQ.
> In striding RQ, the RX buffer consists of "strides" of a fixed size set 
> by pthe driver. An incoming packet is written to the buffer starting from 
> the beginning of the next available stride, consuming as much strides as 
> needed.
>
> Due to the need for headroom and tailroom, there's no easy way of 
> building the xdp_buf in place (around the packet), so it should go to a 
> side buffer.
>
> By using 0-length linear part in a side buffer, I can address two 
> challenging issues: (1) save the in-driver headers memcpy (copy might 
> still exist in the xdp program though), and (2) conform to the 
> "fragments of the same size" requirement/assumption in xdp-mb. 
> Otherwise, if we pull from frag[0] into the linear part, frag[0] becomes 
> smaller than the next fragments.

Right, I see.

So my main concern would be that if we "allow" this, the only way to
write an interoperable XDP program will be to use bpf_xdp_load_bytes()
for every packet access. Which will be slower than DPA, so we may end up
inadvertently slowing down all of the XDP ecosystem, because no one is
going to bother with writing two versions of their programs. Whereas if
you can rely on packet headers always being in the linear part, you can
write a lot of the "look at headers and make a decision" type programs
using just DPA, and they'll work for multibuf as well.

But maybe I'm mistaken and people are just going to use the load_bytes
helper anyway because they want to go deeper than whatever "headers" bit
we'll end up guaranteeing is in the linear part?

-Toke

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Andy Gospodarek]

On Tue, Jan 03, 2023 at 02:55:22PM +0200, Tariq Toukan wrote:
> 
> 
> On 21/06/2022 20:54, Andy Gospodarek wrote:
> > This changes the section name for the bpf program embedded in these
> > files to "xdp.frags" to allow the programs to be loaded on drivers that
> > are using an MTU greater than PAGE_SIZE.  Rather than directly accessing
> > the buffers, the packet data is now accessed via xdp helper functions to
> > provide an example for those who may need to write more complex
> > programs.
> > 
> > v2: remove new unnecessary variable
> > 
> 
> Hi,
> 
> I'm trying to understand if there are any assumptions/requirements on the
> length of the xdp_buf linear part when passed to XDP multi-buf programs?
> Can the linear part be empty, with all data residing in the fragments? Is it
> valid?

That's a great question.  The implementation in bnxt_en was based on the
implementation as I understood it in mvneta where the linear area
contained approx the first 4k of data - xdp headroom - dma_offset.  This
means that you have something that looks like this with a 9k MTU:

skb->data	[~3.6k of packet data]
skb->frag[0]	[4k of paket data]
     frag[1]	[remainder of packet data]

At some point, I'd like to take the opportunity to test something like
this:

skb->data	[header only + space for header expansion]
skb->frag[0]	[first 4k of data]
     frag[1]	[second 4k of data]
     frag[2]	[remainder of packet data]

Though this will use a bit more memory, I think it will be much more
performant for data that is ultimately consumed rather than forwarded
by the host as the actual packet data will be aligned on page boundaries.

With the ability to have packets that are handled by an XDP program
span buffers, I would also like to test out whether or not it would be
worthwhile to have standard MTU packets also look like this:

skb->data	[header only + space for header expansion]
skb->frag[0]	[packet data]

I think the overall system performance would be better in the XDP_PASS
case, but until there is data to back this up, that's just speculation. 

> Per the proposed pattern below (calling bpf_xdp_load_bytes() to memcpy
> packet data into a local buffer), no such assumption is required, and an
> xdp_buf created by the driver with an empty linear part is valid.
> 
> However, in the _xdp_tx_iptunnel example program, it fails (returns
> XDP_DROP) in case the headers are not in the linear part.
> 
> Regards,
> Tariq
> 
> > Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
> > Acked-by: John Fastabend <john.fastabend@gmail.com>
> > Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >   samples/bpf/xdp1_kern.c            | 11 ++++++++---
> >   samples/bpf/xdp2_kern.c            | 11 ++++++++---
> >   samples/bpf/xdp_tx_iptunnel_kern.c |  2 +-
> >   3 files changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/samples/bpf/xdp1_kern.c b/samples/bpf/xdp1_kern.c
> > index f0c5d95084de..0a5c704badd0 100644
> > --- a/samples/bpf/xdp1_kern.c
> > +++ b/samples/bpf/xdp1_kern.c
> > @@ -39,11 +39,13 @@ static int parse_ipv6(void *data, u64 nh_off, void *data_end)
> >   	return ip6h->nexthdr;
> >   }
> > -SEC("xdp1")
> > +#define XDPBUFSIZE	64
> > +SEC("xdp.frags")
> >   int xdp_prog1(struct xdp_md *ctx)
> >   {
> > -	void *data_end = (void *)(long)ctx->data_end;
> > -	void *data = (void *)(long)ctx->data;
> > +	__u8 pkt[XDPBUFSIZE] = {};
> > +	void *data_end = &pkt[XDPBUFSIZE-1];
> > +	void *data = pkt;
> >   	struct ethhdr *eth = data;
> >   	int rc = XDP_DROP;
> >   	long *value;
> > @@ -51,6 +53,9 @@ int xdp_prog1(struct xdp_md *ctx)
> >   	u64 nh_off;
> >   	u32 ipproto;
> > +	if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt)))
> > +		return rc;
> > +
> >   	nh_off = sizeof(*eth);
> >   	if (data + nh_off > data_end)
> >   		return rc;
> > diff --git a/samples/bpf/xdp2_kern.c b/samples/bpf/xdp2_kern.c
> > index d8a64ab077b0..3332ba6bb95f 100644
> > --- a/samples/bpf/xdp2_kern.c
> > +++ b/samples/bpf/xdp2_kern.c
> > @@ -55,11 +55,13 @@ static int parse_ipv6(void *data, u64 nh_off, void *data_end)
> >   	return ip6h->nexthdr;
> >   }
> > -SEC("xdp1")
> > +#define XDPBUFSIZE	64
> > +SEC("xdp.frags")
> >   int xdp_prog1(struct xdp_md *ctx)
> >   {
> > -	void *data_end = (void *)(long)ctx->data_end;
> > -	void *data = (void *)(long)ctx->data;
> > +	__u8 pkt[XDPBUFSIZE] = {};
> > +	void *data_end = &pkt[XDPBUFSIZE-1];
> > +	void *data = pkt;
> >   	struct ethhdr *eth = data;
> >   	int rc = XDP_DROP;
> >   	long *value;
> > @@ -67,6 +69,9 @@ int xdp_prog1(struct xdp_md *ctx)
> >   	u64 nh_off;
> >   	u32 ipproto;
> > +	if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt)))
> > +		return rc;
> > +
> >   	nh_off = sizeof(*eth);
> >   	if (data + nh_off > data_end)
> >   		return rc;
> > diff --git a/samples/bpf/xdp_tx_iptunnel_kern.c b/samples/bpf/xdp_tx_iptunnel_kern.c
> > index 575d57e4b8d6..0e2bca3a3fff 100644
> > --- a/samples/bpf/xdp_tx_iptunnel_kern.c
> > +++ b/samples/bpf/xdp_tx_iptunnel_kern.c
> > @@ -212,7 +212,7 @@ static __always_inline int handle_ipv6(struct xdp_md *xdp)
> >   	return XDP_TX;
> >   }
> > -SEC("xdp_tx_iptunnel")
> > +SEC("xdp.frags")
> >   int _xdp_tx_iptunnel(struct xdp_md *xdp)
> >   {
> >   	void *data_end = (void *)(long)xdp->data_end;

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Andy Gospodarek]

On Tue, Jan 03, 2023 at 05:21:53PM -0800, Jakub Kicinski wrote:
> On Tue, 03 Jan 2023 16:19:49 +0100 Toke Høiland-Jørgensen wrote:
> > Hmm, good question! I don't think we've ever explicitly documented any
> > assumptions one way or the other. My own mental model has certainly
> > always assumed the first frag would continue to be the same size as in
> > non-multi-buf packets.
> 
> Interesting! :) My mental model was closer to GRO by frags 
> so the linear part would have no data, just headers.

As I mentioned in my mail just a few mins ago, I think this would be a good
model to consider.  All headers (including potentially tunnel headers) could be
in the linear area with the actual packet data in frags.

> A random datapoint is that bpf_xdp_adjust_head() seems 
> to enforce that there is at least ETH_HLEN.

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Andy Gospodarek]

On Thu, Jan 05, 2023 at 04:43:28PM +0100, Toke Høiland-Jørgensen wrote:
> Tariq Toukan <ttoukan.linux@gmail.com> writes:
> 
> > On 04/01/2023 14:28, Toke Høiland-Jørgensen wrote:
> >> Lorenzo Bianconi <lorenzo@kernel.org> writes:
> >> 
> >>>> On Tue, 03 Jan 2023 16:19:49 +0100 Toke Høiland-Jørgensen wrote:
> >>>>> Hmm, good question! I don't think we've ever explicitly documented any
> >>>>> assumptions one way or the other. My own mental model has certainly
> >>>>> always assumed the first frag would continue to be the same size as in
> >>>>> non-multi-buf packets.
> >>>>
> >>>> Interesting! :) My mental model was closer to GRO by frags
> >>>> so the linear part would have no data, just headers.
> >>>
> >>> That is assumption as well.
> >> 
> >> Right, okay, so how many headers? Only Ethernet, or all the way up to
> >> L4 (TCP/UDP)?
> >> 
> >> I do seem to recall a discussion around the header/data split for TCP
> >> specifically, but I think I mentally put that down as "something people
> >> may way to do at some point in the future", which is why it hasn't made
> >> it into my own mental model (yet?) :)
> >> 
> >> -Toke
> >> 
> >
> > I don't think that all the different GRO layers assume having their 
> > headers/data in the linear part. IMO they will just perform better if 
> > these parts are already there. Otherwise, the GRO flow manages, and 
> > pulls the needed amount into the linear part.
> > As examples, see calls to gro_pull_from_frag0 in net/core/gro.c, and the 
> > call to pskb_may_pull() from skb_gro_header_slow().
> >
> > This resembles the bpf_xdp_load_bytes() API used here in the xdp prog.
> 
> Right, but that is kernel code; what we end up doing with the API here
> affects how many programs need to make significant changes to work with
> multibuf, and how many can just set the frags flag and continue working.
> Which also has a performance impact, see below.
> 
> > The context of my questions is that I'm looking for the right memory 
> > scheme for adding xdp-mb support to mlx5e striding RQ.
> > In striding RQ, the RX buffer consists of "strides" of a fixed size set 
> > by pthe driver. An incoming packet is written to the buffer starting from 
> > the beginning of the next available stride, consuming as much strides as 
> > needed.
> >
> > Due to the need for headroom and tailroom, there's no easy way of 
> > building the xdp_buf in place (around the packet), so it should go to a 
> > side buffer.
> >
> > By using 0-length linear part in a side buffer, I can address two 
> > challenging issues: (1) save the in-driver headers memcpy (copy might 
> > still exist in the xdp program though), and (2) conform to the 
> > "fragments of the same size" requirement/assumption in xdp-mb. 
> > Otherwise, if we pull from frag[0] into the linear part, frag[0] becomes 
> > smaller than the next fragments.
> 
> Right, I see.
> 
> So my main concern would be that if we "allow" this, the only way to
> write an interoperable XDP program will be to use bpf_xdp_load_bytes()
> for every packet access. Which will be slower than DPA, so we may end up
> inadvertently slowing down all of the XDP ecosystem, because no one is
> going to bother with writing two versions of their programs. Whereas if
> you can rely on packet headers always being in the linear part, you can
> write a lot of the "look at headers and make a decision" type programs
> using just DPA, and they'll work for multibuf as well.

The question I would have is what is really the 'slow down' for
bpf_xdp_load_bytes() vs DPA?  I know you and Jesper can tell me how many
instructions each use. :)

Taking a step back...years ago Dave mentioned wanting to make XDP
programs easy to write and it feels like using these accessor APIs would
help accomplish that.  If the kernel examples use bpf_xdp_load_bytes()
accessors everywhere then that would accomplish that.

> But maybe I'm mistaken and people are just going to use the load_bytes
> helper anyway because they want to go deeper than whatever "headers" bit
> we'll end up guaranteeing is in the linear part?

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Jakub Kicinski]

On Thu, 5 Jan 2023 11:57:32 -0500 Andy Gospodarek wrote:
> > So my main concern would be that if we "allow" this, the only way to
> > write an interoperable XDP program will be to use bpf_xdp_load_bytes()
> > for every packet access. Which will be slower than DPA, so we may end up
> > inadvertently slowing down all of the XDP ecosystem, because no one is
> > going to bother with writing two versions of their programs. Whereas if
> > you can rely on packet headers always being in the linear part, you can
> > write a lot of the "look at headers and make a decision" type programs
> > using just DPA, and they'll work for multibuf as well.  
> 
> The question I would have is what is really the 'slow down' for
> bpf_xdp_load_bytes() vs DPA?  I know you and Jesper can tell me how many
> instructions each use. :)

Until we have an efficient and inlined DPA access to frags an
unconditional memcpy() of the first 2 cachelines-worth of headers
in the driver must be faster than a piece-by-piece bpf_xdp_load_bytes()
onto the stack, right?

> Taking a step back...years ago Dave mentioned wanting to make XDP
> programs easy to write and it feels like using these accessor APIs would
> help accomplish that.  If the kernel examples use bpf_xdp_load_bytes()
> accessors everywhere then that would accomplish that.

I've been pushing for an skb_header_pointer()-like helper but 
the semantics were not universally loved :)

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Toke Høiland-Jørgensen]

Andy Gospodarek <andrew.gospodarek@broadcom.com> writes:

> On Thu, Jan 05, 2023 at 04:43:28PM +0100, Toke Høiland-Jørgensen wrote:
>> Tariq Toukan <ttoukan.linux@gmail.com> writes:
>> 
>> > On 04/01/2023 14:28, Toke Høiland-Jørgensen wrote:
>> >> Lorenzo Bianconi <lorenzo@kernel.org> writes:
>> >> 
>> >>>> On Tue, 03 Jan 2023 16:19:49 +0100 Toke Høiland-Jørgensen wrote:
>> >>>>> Hmm, good question! I don't think we've ever explicitly documented any
>> >>>>> assumptions one way or the other. My own mental model has certainly
>> >>>>> always assumed the first frag would continue to be the same size as in
>> >>>>> non-multi-buf packets.
>> >>>>
>> >>>> Interesting! :) My mental model was closer to GRO by frags
>> >>>> so the linear part would have no data, just headers.
>> >>>
>> >>> That is assumption as well.
>> >> 
>> >> Right, okay, so how many headers? Only Ethernet, or all the way up to
>> >> L4 (TCP/UDP)?
>> >> 
>> >> I do seem to recall a discussion around the header/data split for TCP
>> >> specifically, but I think I mentally put that down as "something people
>> >> may way to do at some point in the future", which is why it hasn't made
>> >> it into my own mental model (yet?) :)
>> >> 
>> >> -Toke
>> >> 
>> >
>> > I don't think that all the different GRO layers assume having their 
>> > headers/data in the linear part. IMO they will just perform better if 
>> > these parts are already there. Otherwise, the GRO flow manages, and 
>> > pulls the needed amount into the linear part.
>> > As examples, see calls to gro_pull_from_frag0 in net/core/gro.c, and the 
>> > call to pskb_may_pull() from skb_gro_header_slow().
>> >
>> > This resembles the bpf_xdp_load_bytes() API used here in the xdp prog.
>> 
>> Right, but that is kernel code; what we end up doing with the API here
>> affects how many programs need to make significant changes to work with
>> multibuf, and how many can just set the frags flag and continue working.
>> Which also has a performance impact, see below.
>> 
>> > The context of my questions is that I'm looking for the right memory 
>> > scheme for adding xdp-mb support to mlx5e striding RQ.
>> > In striding RQ, the RX buffer consists of "strides" of a fixed size set 
>> > by pthe driver. An incoming packet is written to the buffer starting from 
>> > the beginning of the next available stride, consuming as much strides as 
>> > needed.
>> >
>> > Due to the need for headroom and tailroom, there's no easy way of 
>> > building the xdp_buf in place (around the packet), so it should go to a 
>> > side buffer.
>> >
>> > By using 0-length linear part in a side buffer, I can address two 
>> > challenging issues: (1) save the in-driver headers memcpy (copy might 
>> > still exist in the xdp program though), and (2) conform to the 
>> > "fragments of the same size" requirement/assumption in xdp-mb. 
>> > Otherwise, if we pull from frag[0] into the linear part, frag[0] becomes 
>> > smaller than the next fragments.
>> 
>> Right, I see.
>> 
>> So my main concern would be that if we "allow" this, the only way to
>> write an interoperable XDP program will be to use bpf_xdp_load_bytes()
>> for every packet access. Which will be slower than DPA, so we may end up
>> inadvertently slowing down all of the XDP ecosystem, because no one is
>> going to bother with writing two versions of their programs. Whereas if
>> you can rely on packet headers always being in the linear part, you can
>> write a lot of the "look at headers and make a decision" type programs
>> using just DPA, and they'll work for multibuf as well.
>
> The question I would have is what is really the 'slow down' for
> bpf_xdp_load_bytes() vs DPA?  I know you and Jesper can tell me how many
> instructions each use. :)

I can try running some benchmarks to compare the two, sure!

-Toke

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Andy Gospodarek]

On Thu, Jan 05, 2023 at 10:16:42AM -0800, Jakub Kicinski wrote:
> On Thu, 5 Jan 2023 11:57:32 -0500 Andy Gospodarek wrote:
> > > So my main concern would be that if we "allow" this, the only way to
> > > write an interoperable XDP program will be to use bpf_xdp_load_bytes()
> > > for every packet access. Which will be slower than DPA, so we may end up
> > > inadvertently slowing down all of the XDP ecosystem, because no one is
> > > going to bother with writing two versions of their programs. Whereas if
> > > you can rely on packet headers always being in the linear part, you can
> > > write a lot of the "look at headers and make a decision" type programs
> > > using just DPA, and they'll work for multibuf as well.  
> > 
> > The question I would have is what is really the 'slow down' for
> > bpf_xdp_load_bytes() vs DPA?  I know you and Jesper can tell me how many
> > instructions each use. :)
> 
> Until we have an efficient and inlined DPA access to frags an
> unconditional memcpy() of the first 2 cachelines-worth of headers
> in the driver must be faster than a piece-by-piece bpf_xdp_load_bytes()
> onto the stack, right?

100% 

Seems like we are back to speed vs ease of use, then?

> > Taking a step back...years ago Dave mentioned wanting to make XDP
> > programs easy to write and it feels like using these accessor APIs would
> > help accomplish that.  If the kernel examples use bpf_xdp_load_bytes()
> > accessors everywhere then that would accomplish that.
> 
> I've been pushing for an skb_header_pointer()-like helper but 
> the semantics were not universally loved :)

I didn't recall that -- maybe I'll check the archives and see what I can
find.

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Toke Høiland-Jørgensen]

>>> So my main concern would be that if we "allow" this, the only way to
>>> write an interoperable XDP program will be to use bpf_xdp_load_bytes()
>>> for every packet access. Which will be slower than DPA, so we may end up
>>> inadvertently slowing down all of the XDP ecosystem, because no one is
>>> going to bother with writing two versions of their programs. Whereas if
>>> you can rely on packet headers always being in the linear part, you can
>>> write a lot of the "look at headers and make a decision" type programs
>>> using just DPA, and they'll work for multibuf as well.
>>
>> The question I would have is what is really the 'slow down' for
>> bpf_xdp_load_bytes() vs DPA?  I know you and Jesper can tell me how many
>> instructions each use. :)
>
> I can try running some benchmarks to compare the two, sure!

Okay, ran a simple test: a program that just parses the IP header, then
drops the packet. Results as follows:

Baseline (don't touch data):    26.5 Mpps / 37.8 ns/pkt
Touch data (ethernet hdr):      25.0 Mpps / 40.0 ns/pkt
Parse IP (DPA):                 24.1 Mpps / 41.5 ns/pkt
Parse IP (bpf_xdp_load_bytes):  15.3 Mpps / 65.3 ns/pkt

So 2.2 ns of overhead from reading the packet data, another 1.5 ns from
the parsing logic, and a whopping 23.8 ns extra from switching to
bpf_xdp_load_bytes(). This is with two calls to bpf_xdp_load_bytes(),
one to get the Ethernet header, and another to get the IP header.
Dropping one of them also drops the overhead in half, so it seems to fit
with ~12 ns of overhead from a single call to bpf_xdp_load_bytes().

I pushed the code I used for testing here, in case someone else wants to
play around with it:

https://github.com/xdp-project/xdp-tools/tree/xdp-load-bytes

It's part of the 'xdp-bench' utility. Run it as:

./xdp-bench drop <iface> -p parse-ip

for DPA parsing and

./xdp-bench drop <iface> -p parse-ip -l

to use bpf_xdp_load_bytes().

-Toke

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Tariq Toukan]

On 05/01/2023 20:16, Jakub Kicinski wrote:
> On Thu, 5 Jan 2023 11:57:32 -0500 Andy Gospodarek wrote:
>>> So my main concern would be that if we "allow" this, the only way to
>>> write an interoperable XDP program will be to use bpf_xdp_load_bytes()
>>> for every packet access. Which will be slower than DPA, so we may end up
>>> inadvertently slowing down all of the XDP ecosystem, because no one is
>>> going to bother with writing two versions of their programs. Whereas if
>>> you can rely on packet headers always being in the linear part, you can
>>> write a lot of the "look at headers and make a decision" type programs
>>> using just DPA, and they'll work for multibuf as well.
>>
>> The question I would have is what is really the 'slow down' for
>> bpf_xdp_load_bytes() vs DPA?  I know you and Jesper can tell me how many
>> instructions each use. :)
> 
> Until we have an efficient and inlined DPA access to frags an
> unconditional memcpy() of the first 2 cachelines-worth of headers
> in the driver must be faster than a piece-by-piece bpf_xdp_load_bytes()
> onto the stack, right?
> 
>> Taking a step back...years ago Dave mentioned wanting to make XDP
>> programs easy to write and it feels like using these accessor APIs would
>> help accomplish that.  If the kernel examples use bpf_xdp_load_bytes()
>> accessors everywhere then that would accomplish that.
> 
> I've been pushing for an skb_header_pointer()-like helper but
> the semantics were not universally loved :)

Maybe it's time to re-consider.

Is it something like an API that given an offset returns a pointer + 
allowed length to be accessed?

This sounds like a good direction to me, that avoids having any 
linear-part-length assumptions, while preserving good performance.

Maybe we can still require/guarantee that each single header (eth, ip, 
tcp, ...) does not cross a frag/page boundary. For otherwise, a prog 
needs to handle cases where headers span several fragments, so it has to 
reconstruct the header by copying the different parts into some local 
buffer.

This can be achieved by having another assumption that AFAIK already 
holds today: all fragments are of size PAGE_SIZE.

Regards,
Tariq

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Tariq Toukan]

On 08/01/2023 14:33, Tariq Toukan wrote:
> 
> 
> On 05/01/2023 20:16, Jakub Kicinski wrote:
>> On Thu, 5 Jan 2023 11:57:32 -0500 Andy Gospodarek wrote:
>>>> So my main concern would be that if we "allow" this, the only way to
>>>> write an interoperable XDP program will be to use bpf_xdp_load_bytes()
>>>> for every packet access. Which will be slower than DPA, so we may 
>>>> end up
>>>> inadvertently slowing down all of the XDP ecosystem, because no one is
>>>> going to bother with writing two versions of their programs. Whereas if
>>>> you can rely on packet headers always being in the linear part, you can
>>>> write a lot of the "look at headers and make a decision" type programs
>>>> using just DPA, and they'll work for multibuf as well.
>>>
>>> The question I would have is what is really the 'slow down' for
>>> bpf_xdp_load_bytes() vs DPA?  I know you and Jesper can tell me how many
>>> instructions each use. :)
>>
>> Until we have an efficient and inlined DPA access to frags an
>> unconditional memcpy() of the first 2 cachelines-worth of headers
>> in the driver must be faster than a piece-by-piece bpf_xdp_load_bytes()
>> onto the stack, right?
>>
>>> Taking a step back...years ago Dave mentioned wanting to make XDP
>>> programs easy to write and it feels like using these accessor APIs would
>>> help accomplish that.  If the kernel examples use bpf_xdp_load_bytes()
>>> accessors everywhere then that would accomplish that.
>>
>> I've been pushing for an skb_header_pointer()-like helper but
>> the semantics were not universally loved :)
> 
> Maybe it's time to re-consider.
> 
> Is it something like an API that given an offset returns a pointer + 
> allowed length to be accessed?
> 
> This sounds like a good direction to me, that avoids having any 
> linear-part-length assumptions, while preserving good performance.
> 
> Maybe we can still require/guarantee that each single header (eth, ip, 
> tcp, ...) does not cross a frag/page boundary. For otherwise, a prog 
> needs to handle cases where headers span several fragments, so it has to 
> reconstruct the header by copying the different parts into some local 
> buffer.
> 
> This can be achieved by having another assumption that AFAIK already 
> holds today: all fragments are of size PAGE_SIZE.
> 
> Regards,
> Tariq

This can be a good starting point:
static void *bpf_xdp_pointer(struct xdp_buff *xdp, u32 offset, u32 len)

It's currently not exposed as a bpf-helper, and it works a bit 
differently to what I mentioned earlier: It gets the desired length, and 
fails in case it's not continuously accessible (i.e. this piece of data 
spans multiple frags).

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Toke Høiland-Jørgensen]

Tariq Toukan <ttoukan.linux@gmail.com> writes:

> On 08/01/2023 14:33, Tariq Toukan wrote:
>> 
>> 
>> On 05/01/2023 20:16, Jakub Kicinski wrote:
>>> On Thu, 5 Jan 2023 11:57:32 -0500 Andy Gospodarek wrote:
>>>>> So my main concern would be that if we "allow" this, the only way to
>>>>> write an interoperable XDP program will be to use bpf_xdp_load_bytes()
>>>>> for every packet access. Which will be slower than DPA, so we may 
>>>>> end up
>>>>> inadvertently slowing down all of the XDP ecosystem, because no one is
>>>>> going to bother with writing two versions of their programs. Whereas if
>>>>> you can rely on packet headers always being in the linear part, you can
>>>>> write a lot of the "look at headers and make a decision" type programs
>>>>> using just DPA, and they'll work for multibuf as well.
>>>>
>>>> The question I would have is what is really the 'slow down' for
>>>> bpf_xdp_load_bytes() vs DPA?  I know you and Jesper can tell me how many
>>>> instructions each use. :)
>>>
>>> Until we have an efficient and inlined DPA access to frags an
>>> unconditional memcpy() of the first 2 cachelines-worth of headers
>>> in the driver must be faster than a piece-by-piece bpf_xdp_load_bytes()
>>> onto the stack, right?
>>>
>>>> Taking a step back...years ago Dave mentioned wanting to make XDP
>>>> programs easy to write and it feels like using these accessor APIs would
>>>> help accomplish that.  If the kernel examples use bpf_xdp_load_bytes()
>>>> accessors everywhere then that would accomplish that.
>>>
>>> I've been pushing for an skb_header_pointer()-like helper but
>>> the semantics were not universally loved :)
>> 
>> Maybe it's time to re-consider.
>> 
>> Is it something like an API that given an offset returns a pointer + 
>> allowed length to be accessed?
>> 
>> This sounds like a good direction to me, that avoids having any 
>> linear-part-length assumptions, while preserving good performance.
>> 
>> Maybe we can still require/guarantee that each single header (eth, ip, 
>> tcp, ...) does not cross a frag/page boundary. For otherwise, a prog 
>> needs to handle cases where headers span several fragments, so it has to 
>> reconstruct the header by copying the different parts into some local 
>> buffer.
>> 
>> This can be achieved by having another assumption that AFAIK already 
>> holds today: all fragments are of size PAGE_SIZE.
>> 
>> Regards,
>> Tariq
>
> This can be a good starting point:
> static void *bpf_xdp_pointer(struct xdp_buff *xdp, u32 offset, u32 len)
>
> It's currently not exposed as a bpf-helper, and it works a bit 
> differently to what I mentioned earlier: It gets the desired length, and 
> fails in case it's not continuously accessible (i.e. this piece of data 
> spans multiple frags).

Did a bit of digging through the mail archives. Exposing
bpf_xdp_pointer() as a helper was proposed back in March last year:

https://lore.kernel.org/r/20220306234311.452206-1-memxor@gmail.com

The discussion of this seems to have ended on "let's use dynptrs
instead". There was a patch series posted for this as well, which seems
to have stalled out with this comment from Alexei in October:

https://lore.kernel.org/r/CAADnVQKhv2YBrUAQJq6UyqoZJ-FGNQbKenGoPySPNK+GaOjBOg@mail.gmail.com

-Toke

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Maxim Mikityanskiy]

On Tue, Jan 03, 2023 at 05:21:53PM -0800, Jakub Kicinski wrote:
> On Tue, 03 Jan 2023 16:19:49 +0100 Toke Høiland-Jørgensen wrote:
> > Hmm, good question! I don't think we've ever explicitly documented any
> > assumptions one way or the other. My own mental model has certainly
> > always assumed the first frag would continue to be the same size as in
> > non-multi-buf packets.
> 
> Interesting! :) My mental model was closer to GRO by frags 
> so the linear part would have no data, just headers.
> 
> A random datapoint is that bpf_xdp_adjust_head() seems 
> to enforce that there is at least ETH_HLEN.

Also bpf_xdp_frags_increase_tail has the following check:

	if (!rxq->frag_size || rxq->frag_size > xdp->frame_sz)
		return -EOPNOTSUPP;

However, I can't seem to find where the `frag_size > frame_sz` part is
actually used. Maybe this condition can be dropped? Can someone shed
some light?

BTW, Tariq, we seem to have missed setting frag_size to a non-zero
value. Could you check that increasing the tail indeed doesn't work on
fragmented packets on mlx5e? I can send a oneliner to fix that.

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Tariq Toukan]

On 10/01/2023 22:59, Maxim Mikityanskiy wrote:
> On Tue, Jan 03, 2023 at 05:21:53PM -0800, Jakub Kicinski wrote:
>> On Tue, 03 Jan 2023 16:19:49 +0100 Toke Høiland-Jørgensen wrote:
>>> Hmm, good question! I don't think we've ever explicitly documented any
>>> assumptions one way or the other. My own mental model has certainly
>>> always assumed the first frag would continue to be the same size as in
>>> non-multi-buf packets.
>>
>> Interesting! :) My mental model was closer to GRO by frags
>> so the linear part would have no data, just headers.
>>
>> A random datapoint is that bpf_xdp_adjust_head() seems
>> to enforce that there is at least ETH_HLEN.
> 
> Also bpf_xdp_frags_increase_tail has the following check:
> 
> 	if (!rxq->frag_size || rxq->frag_size > xdp->frame_sz)
> 		return -EOPNOTSUPP;
> 
> However, I can't seem to find where the `frag_size > frame_sz` part is
> actually used. Maybe this condition can be dropped? Can someone shed
> some light?
> 
> BTW, Tariq, we seem to have missed setting frag_size to a non-zero
> value.

Hey Maxim,
Indeed. We use xdp_rxq_info_reg, it passes 0 as frag_size.

> Could you check that increasing the tail indeed doesn't work on
> fragmented packets on mlx5e? I can send a oneliner to fix that.

I can test early next week, and update.

Re: [PATCH net-next v2] samples/bpf: fixup some tools to be able to support xdp multibuffer

[Tariq Toukan]

On 13/01/2023 23:07, Tariq Toukan wrote:
> 
> 
> On 10/01/2023 22:59, Maxim Mikityanskiy wrote:
>> On Tue, Jan 03, 2023 at 05:21:53PM -0800, Jakub Kicinski wrote:
>>> On Tue, 03 Jan 2023 16:19:49 +0100 Toke Høiland-Jørgensen wrote:
>>>> Hmm, good question! I don't think we've ever explicitly documented any
>>>> assumptions one way or the other. My own mental model has certainly
>>>> always assumed the first frag would continue to be the same size as in
>>>> non-multi-buf packets.
>>>
>>> Interesting! :) My mental model was closer to GRO by frags
>>> so the linear part would have no data, just headers.
>>>
>>> A random datapoint is that bpf_xdp_adjust_head() seems
>>> to enforce that there is at least ETH_HLEN.
>>
>> Also bpf_xdp_frags_increase_tail has the following check:
>>
>>     if (!rxq->frag_size || rxq->frag_size > xdp->frame_sz)
>>         return -EOPNOTSUPP;
>>
>> However, I can't seem to find where the `frag_size > frame_sz` part is
>> actually used. Maybe this condition can be dropped? Can someone shed
>> some light?
>>
>> BTW, Tariq, we seem to have missed setting frag_size to a non-zero
>> value.
> 
> Hey Maxim,
> Indeed. We use xdp_rxq_info_reg, it passes 0 as frag_size.
> 
>> Could you check that increasing the tail indeed doesn't work on
>> fragmented packets on mlx5e? I can send a oneliner to fix that.
> 
> I can test early next week, and update.

Hi Maxim,

Hacking the existing adjust_tail sample program, in between other high 
prio tasks, to make it work with frags took me some time.
But I can approve the fix below (or equivalent) is needed. Please go on 
and submit it.

Regards,
Tariq

--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -600,7 +600,7 @@ static int mlx5e_init_rxq_rq(struct mlx5e_channel 
*c, struct mlx5e_params *param
         if (err)
                 return err;

-       return xdp_rxq_info_reg(&rq->xdp_rxq, rq->netdev, rq->ix, 
c->napi.napi_id);
+       return __xdp_rxq_info_reg(&rq->xdp_rxq, rq->netdev, rq->ix, 
c->napi.napi_id, PAGE_SIZE);
  }

  static int mlx5_rq_shampo_alloc(struct mlx5_core_dev *mdev,