mvneta: introduce XDP multi-buffer support

data_len field will be used for paged frame len for xdp_buff/xdp_frame. This is a preliminary patch to properly support xdp-multibuff

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/linux/skbuff.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

include/linux/skbuff.h +0 -0

index dbf820a50a39..332ec56c200d 100644

Quoted 19 lines

data_len field will be used for paged frame len for xdp_buff/xdp_frame. This is a preliminary patch to properly support xdp-multibuff

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/linux/skbuff.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

include/linux/skbuff.h +1 -1

index dbf820a50a39..332ec56c200d 100644
@@ -522,7 +522,10 @@ struct skb_shared_info {
        struct sk_buff  *frag_list;
        struct skb_shared_hwtstamps hwtstamps;
        unsigned int    gso_type;
-       u32             tskey;
+       union {

include/linux/skbuff.h struct skb_shared_info { · line 529

Quoted 4 lines

include/linux/skbuff.h +3 -0

+               u32     tskey;
+               u32     data_len;
+       };
 

Rather than use the tskey field why not repurpose the gso_size field? I would think in the XDP paths that the gso fields would be unused since LRO and HW_GRO would be incompatible with XDP anyway.

Quoted 20 lines · Lorenzo Bianconi

Quoted 19 lines

data_len field will be used for paged frame len for xdp_buff/xdp_frame. This is a preliminary patch to properly support xdp-multibuff

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/linux/skbuff.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

include/linux/skbuff.h +1 -1

index dbf820a50a39..332ec56c200d 100644
@@ -522,7 +522,10 @@ struct skb_shared_info {
        struct sk_buff  *frag_list;
        struct skb_shared_hwtstamps hwtstamps;
        unsigned int    gso_type;
-       u32             tskey;
+       union {

Quoted 8 lines

include/linux/skbuff.h struct skb_shared_info { · line 529

Quoted 4 lines

include/linux/skbuff.h +3 -0

+               u32     tskey;
+               u32     data_len;
+       };
 

Rather than use the tskey field why not repurpose the gso_size field? I would think in the XDP paths that the gso fields would be unused since LRO and HW_GRO would be incompatible with XDP anyway.

ack, I agree. I will fix it in v10.

Quoted 29 lines · On Tue, 29 Jun 2021 14:44:56 +0200 Lorenzo Bianconi wrote:

Quoted 28 lines · Lorenzo Bianconi

Quoted 14 lines

data_len field will be used for paged frame len for xdp_buff/xdp_frame. This is a preliminary patch to properly support xdp-multibuff

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/linux/skbuff.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

include/linux/skbuff.h +0 -0

index dbf820a50a39..332ec56c200d 100644

include/linux/skbuff.h struct skb_shared_info { · line 529

Quoted 9 lines

include/linux/skbuff.h +4 -1

@@ -522,7 +522,10 @@ struct skb_shared_info {
        struct sk_buff  *frag_list;
        struct skb_shared_hwtstamps hwtstamps;
        unsigned int    gso_type;
-       u32             tskey;
+       union {
+               u32     tskey;
+               u32     data_len;
+       };
 

Rather than use the tskey field why not repurpose the gso_size field? I would think in the XDP paths that the gso fields would be unused since LRO and HW_GRO would be incompatible with XDP anyway.

Quoted 1 line

ack, I agree. I will fix it in v10.

Why is XDP mb incompatible with LRO? I thought that was one of the use cases (mentioned by Willem IIRC).

Quoted 4 lines · Jakub Kicinski

Quoted 1 line

ack, I agree. I will fix it in v10.

Why is XDP mb incompatible with LRO? I thought that was one of the use cases (mentioned by Willem IIRC).

XDP is meant to be a per packet operation with support for TX and REDIRECT, and LRO isn't routable. So we could put together a large LRO frame but we wouldn't be able to break it apart again. If we allow that then we are going to need a ton more exception handling added to the XDP paths.

As far as GSO it would require setting many more fields in order to actually make it offloadable by any hardware.

My preference would be
to make use of gso_segs and gso_size to store the truesize and datalen of the pages. That way we keep all of the data fields used in the shared info in the first 8 bytes assuming we don't end up having to actually use multiple buffers.

Quoted 10 lines · On Tue, 29 Jun 2021 11:18:38 -0700 Alexander Duyck wrote:

Quoted 3 lines · Jakub Kicinski

Quoted 1 line

ack, I agree. I will fix it in v10.

Why is XDP mb incompatible with LRO? I thought that was one of the use cases (mentioned by Willem IIRC).

XDP is meant to be a per packet operation with support for TX and REDIRECT, and LRO isn't routable. So we could put together a large LRO frame but we wouldn't be able to break it apart again. If we allow that then we are going to need a ton more exception handling added to the XDP paths.

Quoted 2 lines

As far as GSO it would require setting many more fields in order to actually make it offloadable by any hardware.

It would require more work, but TSO seems to be explicitly stated as what the series builds towards (in the cover letter). It's fine to make choices we'd need to redo later, I guess, I'm just trying to understand the why.

Quoted 11 lines · On Tue, 29 Jun 2021 11:18:38 -0700 Alexander Duyck wrote:

Quoted 10 lines · Jakub Kicinski

Quoted 3 lines

Quoted 1 line

ack, I agree. I will fix it in v10.

Why is XDP mb incompatible with LRO? I thought that was one of the use cases (mentioned by Willem IIRC).

XDP is meant to be a per packet operation with support for TX and REDIRECT, and LRO isn't routable. So we could put together a large LRO frame but we wouldn't be able to break it apart again. If we allow that then we are going to need a ton more exception handling added to the XDP paths.

Quoted 6 lines

Quoted 2 lines

As far as GSO it would require setting many more fields in order to actually make it offloadable by any hardware.

It would require more work, but TSO seems to be explicitly stated as what the series builds towards (in the cover letter). It's fine to make choices we'd need to redo later, I guess, I'm just trying to understand the why.

This is also my understanding that LRO and TSO is what this patchset is working towards.

Sorry, I don't agree or understand this requested change.

Quoted 24 lines · Jakub Kicinski

Quoted 11 lines · On Tue, 29 Jun 2021 11:18:38 -0700 Alexander Duyck wrote:

Quoted 10 lines · Jakub Kicinski

Quoted 3 lines

Quoted 1 line

ack, I agree. I will fix it in v10.

Why is XDP mb incompatible with LRO? I thought that was one of the use cases (mentioned by Willem IIRC).

XDP is meant to be a per packet operation with support for TX and REDIRECT, and LRO isn't routable. So we could put together a large LRO frame but we wouldn't be able to break it apart again. If we allow that then we are going to need a ton more exception handling added to the XDP paths.

Quoted 6 lines

Quoted 2 lines

As far as GSO it would require setting many more fields in order to actually make it offloadable by any hardware.

It would require more work, but TSO seems to be explicitly stated as what the series builds towards (in the cover letter). It's fine to make choices we'd need to redo later, I guess, I'm just trying to understand the why.

This is also my understanding that LRO and TSO is what this patchset is working towards.

Quoted 3 lines

Sorry, I don't agree or understand this requested change.

My understanding here is to use gso_size to store paged length of the xdp multi-buffer. When converting the xdp_frame to a skb we will need to overwrite it to support gro/lro. Is my understanding correct?

Introduce flags field in xdp_frame/xdp_buffer data structure to define additional buffer features. At the moment the only supported buffer feature is multi-buffer bit (mb). Multi-buffer bit is used to specify if this is a linear buffer (mb = 0) or a multi-buffer frame (mb = 1). In the latter case the shared_info area at the end of the first buffer will be properly initialized to link together subsequent buffers.

Signed-off-by Lorenzo Bianconi <[email protected]>

Quoted 9 lines

Introduce flags field in xdp_frame/xdp_buffer data structure to define additional buffer features. At the moment the only supported buffer feature is multi-buffer bit (mb). Multi-buffer bit is used to specify if this is a linear buffer (mb = 0) or a multi-buffer frame (mb = 1). In the latter case the shared_info area at the end of the first buffer will be properly initialized to link together subsequent buffers.

Quoted 1 line

Signed-off-by Lorenzo Bianconi <[email protected]>

Instead of passing this between buffers and frames I wonder if this wouldn't be better to place in something like the xdp_mem_info structure since this is something that would be specific to how the device is handling memory anyway. You could probably split the type field into a 16b type and a 16b flags field. Then add your bit where 0 is linear/legacy and 1 is scatter-gather/multi-buffer.

Quoted 11 lines · Lorenzo Bianconi

Quoted 9 lines

Introduce flags field in xdp_frame/xdp_buffer data structure to define additional buffer features. At the moment the only supported buffer feature is multi-buffer bit (mb). Multi-buffer bit is used to specify if this is a linear buffer (mb = 0) or a multi-buffer frame (mb = 1). In the latter case the shared_info area at the end of the first buffer will be properly initialized to link together subsequent buffers.

Quoted 9 lines

Quoted 1 line

Signed-off-by Lorenzo Bianconi <[email protected]>

Instead of passing this between buffers and frames I wonder if this wouldn't be better to place in something like the xdp_mem_info structure since this is something that would be specific to how the device is handling memory anyway. You could probably split the type field into a 16b type and a 16b flags field. Then add your bit where 0 is linear/legacy and 1 is scatter-gather/multi-buffer.

ack, this should be fine but I put the flag field in xdp_buff/xdp_frame in order to reuse it for some xdp hw-hints (e.g rx checksum type). We can put it in xdp_mem_info too but I guess it would be less intuitive, what do you think?

Quoted 21 lines · Lorenzo Bianconi

Quoted 11 lines · Lorenzo Bianconi

Quoted 9 lines

Introduce flags field in xdp_frame/xdp_buffer data structure to define additional buffer features. At the moment the only supported buffer feature is multi-buffer bit (mb). Multi-buffer bit is used to specify if this is a linear buffer (mb = 0) or a multi-buffer frame (mb = 1). In the latter case the shared_info area at the end of the first buffer will be properly initialized to link together subsequent buffers.

Quoted 9 lines

Quoted 1 line

Signed-off-by Lorenzo Bianconi <[email protected]>

Instead of passing this between buffers and frames I wonder if this wouldn't be better to place in something like the xdp_mem_info structure since this is something that would be specific to how the device is handling memory anyway. You could probably split the type field into a 16b type and a 16b flags field. Then add your bit where 0 is linear/legacy and 1 is scatter-gather/multi-buffer.

Quoted 4 lines

ack, this should be fine but I put the flag field in xdp_buff/xdp_frame in order to reuse it for some xdp hw-hints (e.g rx checksum type). We can put it in xdp_mem_info too but I guess it would be less intuitive, what do you think?

I think it makes the most sense in xdp_mem_info. It already tells us what to expect in some respect in regards to memory layout as it tells us if we are dealing with shared pages or whole pages and how to recycle them. I would think that applies almost identically to scatter-gather XDP the same way.

As far as the addition of flags there is still time for that later as we still have the 32b of unused space after frame_sz.

Quoted 21 lines · Lorenzo Bianconi

Quoted 11 lines · Lorenzo Bianconi

Quoted 9 lines

Introduce flags field in xdp_frame/xdp_buffer data structure to define additional buffer features. At the moment the only supported buffer feature is multi-buffer bit (mb). Multi-buffer bit is used to specify if this is a linear buffer (mb = 0) or a multi-buffer frame (mb = 1). In the latter case the shared_info area at the end of the first buffer will be properly initialized to link together subsequent buffers.

Quoted 9 lines

Quoted 1 line

Signed-off-by Lorenzo Bianconi <[email protected]>

Instead of passing this between buffers and frames I wonder if this wouldn't be better to place in something like the xdp_mem_info structure since this is something that would be specific to how the device is handling memory anyway. You could probably split the type field into a 16b type and a 16b flags field. Then add your bit where 0 is linear/legacy and 1 is scatter-gather/multi-buffer.

Quoted 4 lines

ack, this should be fine but I put the flag field in xdp_buff/xdp_frame in order to reuse it for some xdp hw-hints (e.g rx checksum type). We can put it in xdp_mem_info too but I guess it would be less intuitive, what do you think?

I think it makes the most sense in xdp_mem_info. It already tells us what to expect in some respect in regards to memory layout as it tells us if we are dealing with shared pages or whole pages and how to recycle them. I would think that applies almost identically to scatter-gather XDP the same way.

As far as the addition of flags there is still time for that later as we still have the 32b of unused space after frame_sz.

Quoted 29 lines

Quoted 21 lines · Lorenzo Bianconi

Quoted 11 lines · Lorenzo Bianconi

Quoted 9 lines

Introduce flags field in xdp_frame/xdp_buffer data structure to define additional buffer features. At the moment the only supported buffer feature is multi-buffer bit (mb). Multi-buffer bit is used to specify if this is a linear buffer (mb = 0) or a multi-buffer frame (mb = 1). In the latter case the shared_info area at the end of the first buffer will be properly initialized to link together subsequent buffers.

Quoted 9 lines

Quoted 1 line

Signed-off-by Lorenzo Bianconi <[email protected]>

Instead of passing this between buffers and frames I wonder if this wouldn't be better to place in something like the xdp_mem_info structure since this is something that would be specific to how the device is handling memory anyway. You could probably split the type field into a 16b type and a 16b flags field. Then add your bit where 0 is linear/legacy and 1 is scatter-gather/multi-buffer.

Quoted 4 lines

ack, this should be fine but I put the flag field in xdp_buff/xdp_frame in order to reuse it for some xdp hw-hints (e.g rx checksum type). We can put it in xdp_mem_info too but I guess it would be less intuitive, what do you think?

Quoted 5 lines

I think it makes the most sense in xdp_mem_info. It already tells us what to expect in some respect in regards to memory layout as it tells us if we are dealing with shared pages or whole pages and how to recycle them. I would think that applies almost identically to scatter-gather XDP the same way.

Hi Alex,

Reviewing the code to address this comment I think I spotted a corner case where we can't use this approach. Whenever we run dev_map_bpf_prog_run() we loose mb info converting xdp_frame to xdp_buff since xdp_convert_frame_to_buff() does not copy it and we have no xdp_rxq_info there. Do you think we should add a rxq_info there similar to what we did for cpumap? I think it is better to keep the previous approach since it seems cleaner and reusable in the future. What do you think?

Quoted 3 lines · Lorenzo Bianconi

Quoted 3 lines

On Tue, Jun 29, 2021 at 5:43 AM Lorenzo Bianconi
<[email protected]> wrote:

Hi Lorenzo,

What about doing something like breaking up the type value in xdp_mem_info? The fact is having it as an enum doesn't get us much since we have a 32b type field but are only storing 4 possible values there currently

The way I see it, scatter-gather is just another memory model attribute rather than being something entirely new. It makes as much sense to have a bit there for MEM_TYPE_PAGE_SG as it does for MEM_TYPE_PAGE_SHARED. I would consider either splitting the type field into two 16b fields. For example you might have one field that describes the source pool which is currently either allocated page (ORDER0, SHARED), page_pool (PAGE_POOL), or XSK pool (XSK_BUFF_POOL), and then two flags for type with there being either shared and/or scatter-gather.

Also, looking over the code I don't see any reason why current ORDER0/SHARED couldn't be merged as the free paths are essentially identical since the MEM_TYPE_PAGE_SHARED path would function perfectly fine to free MEM_TYPE_PAGE_ORDER0 pages.

Thanks,

- Alex

Quoted 6 lines

Quoted 3 lines · Lorenzo Bianconi

Quoted 3 lines

On Tue, Jun 29, 2021 at 5:43 AM Lorenzo Bianconi
<[email protected]> wrote:

[...]

Quoted 8 lines

Hi Lorenzo,

What about doing something like breaking up the type value in xdp_mem_info? The fact is having it as an enum doesn't get us much since we have a 32b type field but are only storing 4 possible values there currently

Quoted 9 lines

The way I see it, scatter-gather is just another memory model attribute rather than being something entirely new. It makes as much sense to have a bit there for MEM_TYPE_PAGE_SG as it does for MEM_TYPE_PAGE_SHARED. I would consider either splitting the type field into two 16b fields. For example you might have one field that describes the source pool which is currently either allocated page (ORDER0, SHARED), page_pool (PAGE_POOL), or XSK pool (XSK_BUFF_POOL), and then two flags for type with there being either shared and/or scatter-gather.

Hi Alex,

I am fine reducing the xdp_mem_info size defining type field as u16 instead of u32 but I think mb is a per-xdp_buff/xdp_frame property since at runtime we can receive a tiny single page xdp_buff/xdp_frame and a "jumbo" xdp_buff/xdp_frame composed by multiple pages. According to the documentation available in include/net/xdp.h, xdp_rxq_info (where xdp_mem_info is contained for xdp_buff) is "associated with the driver level RX-ring queues and it is information that is specific to how the driver have configured a given RX-ring queue" so I guess it is a little bit counterintuitive to add this info there.

Moreover we have the "issue" for devmap in dev_map_bpf_prog_run() when we perform XDP_REDIRECT with the approach you proposed and last we can reuse this new flags filed for XDP hw-hints support.
What about reducing xdp_mem_info and add the flags field in xdp_buff/xdp_frame in order to avoid increasing the xdp_buff/xdp_frame size? Am I missing something?

Quoted 28 lines · Lorenzo Bianconi

Quoted 6 lines

Quoted 3 lines · Lorenzo Bianconi

Quoted 3 lines

On Tue, Jun 29, 2021 at 5:43 AM Lorenzo Bianconi
<[email protected]> wrote:

[...]

Quoted 8 lines

Hi Lorenzo,

What about doing something like breaking up the type value in xdp_mem_info? The fact is having it as an enum doesn't get us much since we have a 32b type field but are only storing 4 possible values there currently

Quoted 9 lines

The way I see it, scatter-gather is just another memory model attribute rather than being something entirely new. It makes as much sense to have a bit there for MEM_TYPE_PAGE_SG as it does for MEM_TYPE_PAGE_SHARED. I would consider either splitting the type field into two 16b fields. For example you might have one field that describes the source pool which is currently either allocated page (ORDER0, SHARED), page_pool (PAGE_POOL), or XSK pool (XSK_BUFF_POOL), and then two flags for type with there being either shared and/or scatter-gather.

Hi Alex,

Quoted 8 lines

I am fine reducing the xdp_mem_info size defining type field as u16 instead of u32 but I think mb is a per-xdp_buff/xdp_frame property since at runtime we can receive a tiny single page xdp_buff/xdp_frame and a "jumbo" xdp_buff/xdp_frame composed by multiple pages. According to the documentation available in include/net/xdp.h, xdp_rxq_info (where xdp_mem_info is contained for xdp_buff) is "associated with the driver level RX-ring queues and it is information that is specific to how the driver have configured a given RX-ring queue" so I guess it is a little bit counterintuitive to add this info there.

It isn't really all that counterintuitive. However it does put the onus on the driver to be consistent about things. So even a single-buffer xdp_buff would technically have to be a scatter-gather buff, but it would have no fragments in it. So the requirement would be to initialize the frags and data_len fields to 0 for all xdp_buff structures.

Quoted 7 lines

Moreover we have the "issue" for devmap in dev_map_bpf_prog_run() when we perform XDP_REDIRECT with the approach you proposed and last we can reuse this new flags filed for XDP hw-hints support.
What about reducing xdp_mem_info and add the flags field in xdp_buff/xdp_frame in order to avoid increasing the xdp_buff/xdp_frame size? Am I missing something?

The problem is there isn't a mem_info field in the xdp_buff. It is in the Rx queue info structure.

Thanks,

- Alex

Introduce xdp multi-buff support to __xdp_build_skb_from_frame/xdp_build_skb_from_frame utility routines.

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 net/core/xdp.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

net/core/xdp.c +6 -0

index f61c63115c95..71bedf6049a1 100644
@@ -582,9 +582,15 @@ struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf,
                                           struct sk_buff *skb,
                                           struct net_device *dev)
 {
+       struct skb_shared_info *sinfo = xdp_get_shared_info_from_frame(xdpf);
        unsigned int headroom, frame_size;
+       int i, num_frags = 0;
        void *hard_start;
 
+       /* xdp multi-buff frame */
+       if (unlikely(xdp_frame_is_mb(xdpf)))
+               num_frags = sinfo->nr_frags;
+
        /* Part of headroom was reserved to xdpf */
        headroom = sizeof(*xdpf) + xdpf->headroom;
 

Quoted 34 lines

Introduce xdp multi-buff support to __xdp_build_skb_from_frame/xdp_build_skb_from_frame utility routines.

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 net/core/xdp.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

net/core/xdp.c +6 -0

index f61c63115c95..71bedf6049a1 100644
@@ -582,9 +582,15 @@ struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf,
                                           struct sk_buff *skb,
                                           struct net_device *dev)
 {
+       struct skb_shared_info *sinfo = xdp_get_shared_info_from_frame(xdpf);
        unsigned int headroom, frame_size;
+       int i, num_frags = 0;
        void *hard_start;
 
+       /* xdp multi-buff frame */
+       if (unlikely(xdp_frame_is_mb(xdpf)))
+               num_frags = sinfo->nr_frags;
+
        /* Part of headroom was reserved to xdpf */
        headroom = sizeof(*xdpf) + xdpf->headroom;
 

@@ -603,6 +609,13 @@ struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf,
        if (xdpf->metasize)
                skb_metadata_set(skb, xdpf->metasize);
 

net/core/xdp.c struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf, · line 618

Quoted 7 lines

net/core/xdp.c +7 -0

+       for (i = 0; i < num_frags; i++)
+               skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
+                               skb_frag_page(&sinfo->frags[i]),
+                               skb_frag_off(&sinfo->frags[i]),
+                               skb_frag_size(&sinfo->frags[i]),
+                               xdpf->frame_sz);
+

So this is assuming the header frame and all of the frags are using the same size. Rather than reading the frags out and then writing them back, why not just directly rewrite the nr_frags, add the total size to skb->len and skb->data_len, and then update the truesize?

Actually, I think you might need to store the truesize somewhere in addition to the data_len that you were storing in the shared info.

From: Eelco Chaudron <[email protected]>

This change adds support for tail growing and shrinking for XDP multi-buff.

Signed-off-by Eelco Chaudron <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/net/xdp.h |  7 ++++++
 net/core/filter.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
 net/core/xdp.c    |  5 ++--
 3 files changed, 72 insertions(+), 2 deletions(-)

include/net/xdp.h +7 -0

index 935a6f83115f..3525801c6ed5 100644
@@ -132,6 +132,11 @@ xdp_get_shared_info_from_buff(struct xdp_buff *xdp)
 	return (struct skb_shared_info *)xdp_data_hard_end(xdp);
 }
 
+static inline unsigned int xdp_get_frag_tailroom(const skb_frag_t *frag)
+{
+	return PAGE_SIZE - skb_frag_size(frag) - skb_frag_off(frag);
+}
+
 struct xdp_frame {
 	void *data;
 	u16 len;

@@ -259,6 +264,8 @@ struct xdp_frame *xdp_convert_buff_to_frame(struct xdp_buff *xdp)
 	return xdp_frame;
 }
 
+void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
+		  struct xdp_buff *xdp);
 void xdp_return_frame(struct xdp_frame *xdpf);
 void xdp_return_frame_rx_napi(struct xdp_frame *xdpf);
 void xdp_return_buff(struct xdp_buff *xdp);

net/core/filter.c +9 -0

index caa88955562e..05f574a3d690 100644
@@ -3859,11 +3859,73 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
 	.arg2_type	= ARG_ANYTHING,
 };
 
+static int bpf_xdp_mb_adjust_tail(struct xdp_buff *xdp, int offset)
+{
+	struct skb_shared_info *sinfo;
+
+	if (unlikely(!xdp_buff_is_mb(xdp)))
+		return -EINVAL;
+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+	if (offset >= 0) {

net/core/filter.c +1 -0

+	} else {

Quoted 4 lines · Lorenzo Bianconi

From: Eelco Chaudron <[email protected]>

This change adds support for tail growing and shrinking for XDP multi-buff.

It would be nice if the commit message gave us some details on how the growing/shrinking works in the multi-buff support.

Quoted 58 lines

Signed-off-by Eelco Chaudron <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/net/xdp.h |  7 ++++++
 net/core/filter.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
 net/core/xdp.c    |  5 ++--
 3 files changed, 72 insertions(+), 2 deletions(-)

include/net/xdp.h +7 -0

index 935a6f83115f..3525801c6ed5 100644
@@ -132,6 +132,11 @@ xdp_get_shared_info_from_buff(struct xdp_buff *xdp)
 	return (struct skb_shared_info *)xdp_data_hard_end(xdp);
 }
 
+static inline unsigned int xdp_get_frag_tailroom(const skb_frag_t *frag)
+{
+	return PAGE_SIZE - skb_frag_size(frag) - skb_frag_off(frag);
+}
+
 struct xdp_frame {
 	void *data;
 	u16 len;

@@ -259,6 +264,8 @@ struct xdp_frame *xdp_convert_buff_to_frame(struct xdp_buff *xdp)
 	return xdp_frame;
 }
 
+void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
+		  struct xdp_buff *xdp);
 void xdp_return_frame(struct xdp_frame *xdpf);
 void xdp_return_frame_rx_napi(struct xdp_frame *xdpf);
 void xdp_return_buff(struct xdp_buff *xdp);

net/core/filter.c +17 -0

index caa88955562e..05f574a3d690 100644
@@ -3859,11 +3859,73 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
 	.arg2_type	= ARG_ANYTHING,
 };
 
+static int bpf_xdp_mb_adjust_tail(struct xdp_buff *xdp, int offset)
+{
+	struct skb_shared_info *sinfo;
+
+	if (unlikely(!xdp_buff_is_mb(xdp)))
+		return -EINVAL;
+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+	if (offset >= 0) {
+		skb_frag_t *frag = &sinfo->frags[sinfo->nr_frags - 1];
+		int size;
+
+		if (unlikely(offset > xdp_get_frag_tailroom(frag)))
+			return -EINVAL;
+
+		size = skb_frag_size(frag);
+		memset(skb_frag_address(frag) + size, 0, offset);

net/core/filter.c static const struct bpf_func_proto bpf_xdp_adjust_head_proto = { · line 3880

Quoted 2 lines

net/core/filter.c +2 -0

+		skb_frag_size_set(frag, size + offset);
+		sinfo->data_len += offset;

Can you add some comment on how this works? So today I call bpf_xdp_adjust_tail() to add some trailer to my packet. This looks like it adds tailroom to the last frag? But, then how do I insert my trailer? I don't think we can without the extra multi-buffer access support right.

Also data_end will be unchanged yet it will return 0 so my current programs will likely be a bit confused by this.

Quoted 1 line

net/core/filter.c +1 -0

+	} else {

Quoted 8 lines · John Fastabend

Quoted 4 lines · Lorenzo Bianconi

From: Eelco Chaudron <[email protected]>

This change adds support for tail growing and shrinking for XDP multi-buff.

It would be nice if the commit message gave us some details on how the growing/shrinking works in the multi-buff support.

Will add this to the next rev.

Quoted 58 lines

Quoted 58 lines

Signed-off-by Eelco Chaudron <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/net/xdp.h |  7 ++++++
 net/core/filter.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
 net/core/xdp.c    |  5 ++--
 3 files changed, 72 insertions(+), 2 deletions(-)

include/net/xdp.h +7 -0

index 935a6f83115f..3525801c6ed5 100644
@@ -132,6 +132,11 @@ xdp_get_shared_info_from_buff(struct xdp_buff *xdp)
 	return (struct skb_shared_info *)xdp_data_hard_end(xdp);
 }
 
+static inline unsigned int xdp_get_frag_tailroom(const skb_frag_t *frag)
+{
+	return PAGE_SIZE - skb_frag_size(frag) - skb_frag_off(frag);
+}
+
 struct xdp_frame {
 	void *data;
 	u16 len;

@@ -259,6 +264,8 @@ struct xdp_frame *xdp_convert_buff_to_frame(struct xdp_buff *xdp)
 	return xdp_frame;
 }
 
+void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
+		  struct xdp_buff *xdp);
 void xdp_return_frame(struct xdp_frame *xdpf);
 void xdp_return_frame_rx_napi(struct xdp_frame *xdpf);
 void xdp_return_buff(struct xdp_buff *xdp);

net/core/filter.c +17 -0

index caa88955562e..05f574a3d690 100644
@@ -3859,11 +3859,73 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
 	.arg2_type	= ARG_ANYTHING,
 };
 
+static int bpf_xdp_mb_adjust_tail(struct xdp_buff *xdp, int offset)
+{
+	struct skb_shared_info *sinfo;
+
+	if (unlikely(!xdp_buff_is_mb(xdp)))
+		return -EINVAL;
+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+	if (offset >= 0) {
+		skb_frag_t *frag = &sinfo->frags[sinfo->nr_frags - 1];
+		int size;
+
+		if (unlikely(offset > xdp_get_frag_tailroom(frag)))
+			return -EINVAL;
+
+		size = skb_frag_size(frag);
+		memset(skb_frag_address(frag) + size, 0, offset);

Quoted 8 lines

net/core/filter.c static const struct bpf_func_proto bpf_xdp_adjust_head_proto = { · line 3880

Quoted 2 lines

net/core/filter.c +2 -0

+		skb_frag_size_set(frag, size + offset);
+		sinfo->data_len += offset;

Can you add some comment on how this works? So today I call bpf_xdp_adjust_tail() to add some trailer to my packet. This looks like it adds tailroom to the last frag? But, then how do I insert my trailer? I don't think we can without the extra multi-buffer access support right.

You are right, we need some kind of multi-buffer access helpers.

Quoted 2 lines

Also data_end will be unchanged yet it will return 0 so my current programs will likely be a bit confused by this.

Guess this is the tricky part, applications need to be multi-buffer aware. If current applications rely on bpf_xdp_adjust_tail(+) to determine maximum frame length this approach might not work. In this case, we might need an additional helper to do tail expansion with multi buffer support.

But then the question arrives how would mb unaware application behave in general when an mb packet is supplied?? It would definitely not determine the correct packet length.

Quoted 2 lines

Quoted 2 lines

net/core/filter.c +1 -0

+	} else {

Quoted 87 lines · Eelco Chaudron

Quoted 8 lines · John Fastabend

Quoted 4 lines · Lorenzo Bianconi

From: Eelco Chaudron <[email protected]>

This change adds support for tail growing and shrinking for XDP multi-buff.

It would be nice if the commit message gave us some details on how the growing/shrinking works in the multi-buff support.

Will add this to the next rev.

Quoted 58 lines

Quoted 58 lines

Signed-off-by Eelco Chaudron <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/net/xdp.h |  7 ++++++
 net/core/filter.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
 net/core/xdp.c    |  5 ++--
 3 files changed, 72 insertions(+), 2 deletions(-)

include/net/xdp.h +7 -0

index 935a6f83115f..3525801c6ed5 100644
@@ -132,6 +132,11 @@ xdp_get_shared_info_from_buff(struct xdp_buff *xdp)
   return (struct skb_shared_info *)xdp_data_hard_end(xdp);
 }
 
+static inline unsigned int xdp_get_frag_tailroom(const skb_frag_t *frag)
+{
+  return PAGE_SIZE - skb_frag_size(frag) - skb_frag_off(frag);
+}
+
 struct xdp_frame {
   void *data;
   u16 len;

@@ -259,6 +264,8 @@ struct xdp_frame *xdp_convert_buff_to_frame(struct xdp_buff *xdp)
   return xdp_frame;
 }
 
+void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
+            struct xdp_buff *xdp);
 void xdp_return_frame(struct xdp_frame *xdpf);
 void xdp_return_frame_rx_napi(struct xdp_frame *xdpf);
 void xdp_return_buff(struct xdp_buff *xdp);

net/core/filter.c +17 -0

index caa88955562e..05f574a3d690 100644
@@ -3859,11 +3859,73 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
   .arg2_type      = ARG_ANYTHING,
 };
 
+static int bpf_xdp_mb_adjust_tail(struct xdp_buff *xdp, int offset)
+{
+  struct skb_shared_info *sinfo;
+
+  if (unlikely(!xdp_buff_is_mb(xdp)))
+          return -EINVAL;
+
+  sinfo = xdp_get_shared_info_from_buff(xdp);
+  if (offset >= 0) {
+          skb_frag_t *frag = &sinfo->frags[sinfo->nr_frags - 1];
+          int size;
+
+          if (unlikely(offset > xdp_get_frag_tailroom(frag)))
+                  return -EINVAL;
+
+          size = skb_frag_size(frag);
+          memset(skb_frag_address(frag) + size, 0, offset);

Quoted 8 lines

net/core/filter.c static const struct bpf_func_proto bpf_xdp_adjust_head_proto = { · line 3880

Quoted 2 lines

net/core/filter.c +2 -0

+          skb_frag_size_set(frag, size + offset);
+          sinfo->data_len += offset;

Can you add some comment on how this works? So today I call bpf_xdp_adjust_tail() to add some trailer to my packet. This looks like it adds tailroom to the last frag? But, then how do I insert my trailer? I don't think we can without the extra multi-buffer access support right.

You are right, we need some kind of multi-buffer access helpers.

Quoted 2 lines

Also data_end will be unchanged yet it will return 0 so my current programs will likely be a bit confused by this.

Quoted 3 lines

Guess this is the tricky part, applications need to be multi-buffer aware. If current applications rely on bpf_xdp_adjust_tail(+) to determine maximum frame length this approach might not work. In this case, we might need an additional helper to do tail expansion with multi buffer support.

But then the question arrives how would mb unaware application behave in general when an mb packet is supplied?? It would definitely not determine the correct packet length.

Right that was my conclusion as well. Existing programs might have subtle side effects if they start running on multibuffer drivers as is. I don't have any good ideas though on how to handle this.

Quoted 3 lines

Quoted 2 lines

Quoted 2 lines

net/core/filter.c +1 -0

+  } else {

Quoted 12 lines · Eelco Chaudron

Quoted 8 lines · John Fastabend

Quoted 4 lines · Lorenzo Bianconi

From: Eelco Chaudron <[email protected]>

This change adds support for tail growing and shrinking for XDP multi-buff.

It would be nice if the commit message gave us some details on how the growing/shrinking works in the multi-buff support.

Quoted 3 lines

Guess this is the tricky part, applications need to be multi-buffer aware. If current applications rely on bpf_xdp_adjust_tail(+) to determine maximum frame length this approach might not work. In this case, we might need an additional helper to do tail expansion with multi buffer support.

But then the question arrives how would mb unaware application behave in general when an mb packet is supplied?? It would definitely not determine the correct packet length.

Right that was my conclusion as well. Existing programs might have subtle side effects if they start running on multibuffer drivers as is. I don't have any good ideas though on how to handle this.

Quoted 4 lines

Quoted 3 lines

Quoted 3 lines

(quoted diff) +1 -0

+	} else {

Quoted 13 lines

Quoted 12 lines · Eelco Chaudron

Quoted 8 lines · John Fastabend

Quoted 4 lines · Lorenzo Bianconi

From: Eelco Chaudron <[email protected]>

This change adds support for tail growing and shrinking for XDP multi-buff.

It would be nice if the commit message gave us some details on how the growing/shrinking works in the multi-buff support.

[...]

Quoted 4 lines

Quoted 3 lines

Guess this is the tricky part, applications need to be multi-buffer aware. If current applications rely on bpf_xdp_adjust_tail(+) to determine maximum frame length this approach might not work. In this case, we might need an additional helper to do tail expansion with multi buffer support.

But then the question arrives how would mb unaware application behave in general when an mb packet is supplied?? It would definitely not determine the correct packet length.

Quoted 4 lines

Right that was my conclusion as well. Existing programs might have subtle side effects if they start running on multibuffer drivers as is. I don't have any good ideas though on how to handle this.

what about checking the program capabilities at load time (e.g. with a special program type) and disable mb feature if the bpf program is not mb-aware? (e.g. forbid to set the MTU greater than 1500B in xdp mode).

Right that was my conclusion as well. Existing programs might have subtle side effects if they start running on multibuffer drivers as is. I don't have any good ideas though on how to handle this.

From: Eelco Chaudron <[email protected]>

This patch adds support for multi-buffer for the following helpers:
- bpf_xdp_output()
- bpf_perf_event_output()

Signed-off-by Eelco Chaudron <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---

 kernel/trace/bpf_trace.c                      |   3 +
 net/core/filter.c                             |  72 +++++++++-
 .../selftests/bpf/prog_tests/xdp_bpf2bpf.c    | 127 ++++++++++++------
 .../selftests/bpf/progs/test_xdp_bpf2bpf.c    |   2 +-
 4 files changed, 160 insertions(+), 44 deletions(-)

kernel/trace/bpf_trace.c +3 -0

index d2d7cf6cfe83..ee926ec64f78 100644
@@ -1365,6 +1365,7 @@ static const struct bpf_func_proto bpf_perf_event_output_proto_raw_tp = {
 
 extern const struct bpf_func_proto bpf_skb_output_proto;
 extern const struct bpf_func_proto bpf_xdp_output_proto;
+extern const struct bpf_func_proto bpf_xdp_get_buff_len_trace_proto;
 
 BPF_CALL_3(bpf_get_stackid_raw_tp, struct bpf_raw_tracepoint_args *, args,
 	   struct bpf_map *, map, u64, flags)

@@ -1460,6 +1461,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_sock_from_file_proto;
 	case BPF_FUNC_get_socket_cookie:
 		return &bpf_get_socket_ptr_cookie_proto;
+	case BPF_FUNC_xdp_get_buff_len:
+		return &bpf_xdp_get_buff_len_trace_proto;
 #endif
 	case BPF_FUNC_seq_printf:
 		return prog->expected_attach_type == BPF_TRACE_ITER ?

net/core/filter.c +45 -2

index b0855f2d4726..f7211b7908a9 100644
@@ -3939,6 +3939,15 @@ const struct bpf_func_proto bpf_xdp_get_buff_len_proto = {
 	.arg1_type	= ARG_PTR_TO_CTX,
 };
 
+BTF_ID_LIST_SINGLE(bpf_xdp_get_buff_len_bpf_ids, struct, xdp_buff)
+
+const struct bpf_func_proto bpf_xdp_get_buff_len_trace_proto = {
+	.func		= bpf_xdp_get_buff_len,
+	.gpl_only	= false,
+	.arg1_type	= ARG_PTR_TO_BTF_ID,
+	.arg1_btf_id	= &bpf_xdp_get_buff_len_bpf_ids[0],
+};
+
 BPF_CALL_2(bpf_xdp_adjust_tail, struct xdp_buff *, xdp, int, offset)
 {
 	void *data_hard_end = xdp_data_hard_end(xdp); /* use xdp->frame_sz */

@@ -4606,10 +4615,56 @@ static const struct bpf_func_proto bpf_sk_ancestor_cgroup_id_proto = {
 };
 #endif
 
-static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
+static unsigned long bpf_xdp_copy(void *dst_buff, const void *ctx,
 				  unsigned long off, unsigned long len)
 {
-	memcpy(dst_buff, src_buff + off, len);
+	struct xdp_buff *xdp = (struct xdp_buff *)ctx;
+	struct skb_shared_info *sinfo;
+	unsigned long base_len;
+
+	if (likely(!xdp_buff_is_mb(xdp))) {
+		memcpy(dst_buff, xdp->data + off, len);
+		return 0;
+	}
+
+	base_len = xdp->data_end - xdp->data;
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+	do {
+		const void *src_buff = NULL;
+		unsigned long copy_len = 0;
+
+		if (off < base_len) {
+			src_buff = xdp->data + off;
+			copy_len = min(len, base_len - off);
+		} else {
+			unsigned long frag_off_total = base_len;
+			int i;
+
+			for (i = 0; i < sinfo->nr_frags; i++) {
+				skb_frag_t *frag = &sinfo->frags[i];
+				unsigned long frag_len, frag_off;
+
+				frag_len = skb_frag_size(frag);
+				frag_off = off - frag_off_total;
+				if (frag_off < frag_len) {
+					src_buff = skb_frag_address(frag) +
+						   frag_off;
+					copy_len = min(len,
+						       frag_len - frag_off);
+					break;
+				}

net/core/filter.c +2 -0

+	} while (len);
+
 	return 0;
 }

Quoted 6 lines · Lorenzo Bianconi

From: Eelco Chaudron <[email protected]>

This patch adds support for multi-buffer for the following helpers:
- bpf_xdp_output()
- bpf_perf_event_output()

Quoted 3 lines

Signed-off-by Eelco Chaudron <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---

Ah ok so at least xdp_output will work with all bytes. But this is getting close to having access into the frags so I think doing the last bit shouldn't be too hard?

Quoted 92 lines

 kernel/trace/bpf_trace.c                      |   3 +
 net/core/filter.c                             |  72 +++++++++-
 .../selftests/bpf/prog_tests/xdp_bpf2bpf.c    | 127 ++++++++++++------
 .../selftests/bpf/progs/test_xdp_bpf2bpf.c    |   2 +-
 4 files changed, 160 insertions(+), 44 deletions(-)

kernel/trace/bpf_trace.c +3 -0

index d2d7cf6cfe83..ee926ec64f78 100644
@@ -1365,6 +1365,7 @@ static const struct bpf_func_proto bpf_perf_event_output_proto_raw_tp = {
 
 extern const struct bpf_func_proto bpf_skb_output_proto;
 extern const struct bpf_func_proto bpf_xdp_output_proto;
+extern const struct bpf_func_proto bpf_xdp_get_buff_len_trace_proto;
 
 BPF_CALL_3(bpf_get_stackid_raw_tp, struct bpf_raw_tracepoint_args *, args,
 	   struct bpf_map *, map, u64, flags)

@@ -1460,6 +1461,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_sock_from_file_proto;
 	case BPF_FUNC_get_socket_cookie:
 		return &bpf_get_socket_ptr_cookie_proto;
+	case BPF_FUNC_xdp_get_buff_len:
+		return &bpf_xdp_get_buff_len_trace_proto;
 #endif
 	case BPF_FUNC_seq_printf:
 		return prog->expected_attach_type == BPF_TRACE_ITER ?

net/core/filter.c +45 -2

index b0855f2d4726..f7211b7908a9 100644
@@ -3939,6 +3939,15 @@ const struct bpf_func_proto bpf_xdp_get_buff_len_proto = {
 	.arg1_type	= ARG_PTR_TO_CTX,
 };
 
+BTF_ID_LIST_SINGLE(bpf_xdp_get_buff_len_bpf_ids, struct, xdp_buff)
+
+const struct bpf_func_proto bpf_xdp_get_buff_len_trace_proto = {
+	.func		= bpf_xdp_get_buff_len,
+	.gpl_only	= false,
+	.arg1_type	= ARG_PTR_TO_BTF_ID,
+	.arg1_btf_id	= &bpf_xdp_get_buff_len_bpf_ids[0],
+};
+
 BPF_CALL_2(bpf_xdp_adjust_tail, struct xdp_buff *, xdp, int, offset)
 {
 	void *data_hard_end = xdp_data_hard_end(xdp); /* use xdp->frame_sz */

@@ -4606,10 +4615,56 @@ static const struct bpf_func_proto bpf_sk_ancestor_cgroup_id_proto = {
 };
 #endif
 
-static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
+static unsigned long bpf_xdp_copy(void *dst_buff, const void *ctx,
 				  unsigned long off, unsigned long len)
 {
-	memcpy(dst_buff, src_buff + off, len);
+	struct xdp_buff *xdp = (struct xdp_buff *)ctx;
+	struct skb_shared_info *sinfo;
+	unsigned long base_len;
+
+	if (likely(!xdp_buff_is_mb(xdp))) {
+		memcpy(dst_buff, xdp->data + off, len);
+		return 0;
+	}
+
+	base_len = xdp->data_end - xdp->data;
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+	do {
+		const void *src_buff = NULL;
+		unsigned long copy_len = 0;
+
+		if (off < base_len) {
+			src_buff = xdp->data + off;
+			copy_len = min(len, base_len - off);
+		} else {
+			unsigned long frag_off_total = base_len;
+			int i;
+
+			for (i = 0; i < sinfo->nr_frags; i++) {
+				skb_frag_t *frag = &sinfo->frags[i];
+				unsigned long frag_len, frag_off;
+
+				frag_len = skb_frag_size(frag);
+				frag_off = off - frag_off_total;
+				if (frag_off < frag_len) {
+					src_buff = skb_frag_address(frag) +
+						   frag_off;
+					copy_len = min(len,
+						       frag_len - frag_off);
+					break;
+				}

net/core/filter.c static const struct bpf_func_proto bpf_sk_ancestor_cgroup_id_proto = { · line 4665

Quoted 10 lines

net/core/filter.c +10 -0

+				frag_off_total += frag_len;
+			}
+		}
+		if (!src_buff)
+			break;
+
+		memcpy(dst_buff, src_buff, copy_len);
+		off += copy_len;
+		len -= copy_len;
+		dst_buff += copy_len;

This block reads odd to be because it requires looping over the frags multiple times? Why not something like this,

  if (off < base_len) {
   src_buff = xdp->data + off
   copy_len = min...
   memcpy(dst_buff, src_buff, copy_len)
   off += copylen
   len -= copylen
   dst_buff += copylen;
  }

  for (i = 0; i , nr_frags; i++) {
     frag = ...
     ...
     if frag_off < fraglen
        ...
        memcpy()
        update(off, len, dst_buff)
  }

Maybe use a helper to set off,len and dst_buff if worried about the duplication. Seems cleaner than walking through 0..n-1 frags for each copy.

Quoted 4 lines

net/core/filter.c +2 -0

+	} while (len);
+
 	return 0;
 }

Quoted 11 lines · John Fastabend

Quoted 9 lines · Lorenzo Bianconi

From: Eelco Chaudron <[email protected]>

This patch adds support for multi-buffer for the following helpers:
- bpf_xdp_output()
- bpf_perf_event_output()

Signed-off-by Eelco Chaudron <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---

Quoted 3 lines

Ah ok so at least xdp_output will work with all bytes. But this is getting close to having access into the frags so I think doing the last bit shouldn't be too hard?

Guess you are talking about multi-buffer access in the XDP program?

I did suggest an API a while back, https://lore.kernel.org/bpf/[email protected]/ but I had/have not time to work on it. Guess the difficult part is to convince the verifier to allow the data to be accessed.

Quoted 15 lines · Eelco Chaudron

Quoted 11 lines · John Fastabend

Quoted 9 lines · Lorenzo Bianconi

From: Eelco Chaudron <[email protected]>

This patch adds support for multi-buffer for the following helpers:
- bpf_xdp_output()
- bpf_perf_event_output()

Signed-off-by Eelco Chaudron <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---

Quoted 8 lines

Quoted 3 lines

Ah ok so at least xdp_output will work with all bytes. But this is getting close to having access into the frags so I think doing the last bit shouldn't be too hard?

Guess you are talking about multi-buffer access in the XDP program?

I did suggest an API a while back, https://lore.kernel.org/bpf/[email protected]/ but I had/have not time to work on it. Guess the difficult part is to convince the verifier to allow the data to be accessed.

Ah great I think we had the same idea I called it xdp_pull_data() though.

Whats the complication though it looks like it can be done by simply moving the data and data_end pointers around then marking them invalidated. This way the verifier knows the program needs to rewrite them. I can probably look more into next week.

From my first glance it looks relatively straight forward to do now. I really would like to avoid yet another iteration of programs features I have to discover and somehow work around if we can get the helper into this series. If you really don't have time I can probably take a look early next week on an RFC for something like above helper.

.John

Quoted 15 lines · John Fastabend

Quoted 12 lines

Quoted 9 lines · Lorenzo Bianconi

From: Eelco Chaudron <[email protected]>

This patch adds support for multi-buffer for the following helpers:
- bpf_xdp_output()
- bpf_perf_event_output()

Signed-off-by Eelco Chaudron <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---

Quoted 8 lines

Quoted 3 lines

Ah ok so at least xdp_output will work with all bytes. But this is getting close to having access into the frags so I think doing the last bit shouldn't be too hard?

Guess you are talking about multi-buffer access in the XDP program?

I did suggest an API a while back, https://lore.kernel.org/bpf/[email protected]/ but I had/have not time to work on it. Guess the difficult part is to convince the verifier to allow the data to be accessed.

Ah great I think we had the same idea I called it xdp_pull_data() though.

Whats the complication though it looks like it can be done by simply moving the data and data_end pointers around then marking them invalidated. This way the verifier knows the program needs to rewrite them. I can probably look more into next week.

From my first glance it looks relatively straight forward to do now. I really would like to avoid yet another iteration of programs features I have to discover and somehow work around if we can get the helper into this series. If you really don't have time I can probably take a look early next week on an RFC for something like above helper.

.John

Quoted 15 lines · Eelco Chaudron

Quoted 11 lines · John Fastabend

Quoted 9 lines · Lorenzo Bianconi

From: Eelco Chaudron <[email protected]>

This patch adds support for multi-buffer for the following helpers:
- bpf_xdp_output()
- bpf_perf_event_output()

Signed-off-by Eelco Chaudron <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---

Quoted 8 lines

Quoted 3 lines

Ah ok so at least xdp_output will work with all bytes. But this is getting close to having access into the frags so I think doing the last bit shouldn't be too hard?

Guess you are talking about multi-buffer access in the XDP program?

I did suggest an API a while back, https://lore.kernel.org/bpf/[email protected]/ but I had/have not time to work on it. Guess the difficult part is to convince the verifier to allow the data to be accessed.

Ah great I think we had the same idea I called it xdp_pull_data() though.

Whats the complication though it looks like it can be done by simply moving the data and data_end pointers around then marking them invalidated. This way the verifier knows the program needs to rewrite them. I can probably look more into next week.

From my first glance it looks relatively straight forward to do now. I really would like to avoid yet another iteration of programs features I have to discover and somehow work around if we can get the helper into this series. If you really don't have time I can probably take a look early next week on an RFC for something like above helper.

This series introduce XDP multi-buffer support. The mvneta driver is the first to support these new "non-linear" xdp_{buff,frame}. Reviewers please focus on how these new types of xdp_{buff,frame} packets traverse the different layers and the layout design. It is on purpose that BPF-helpers are kept simple, as we don't want to expose the internal layout to allow later changes.

For now, to keep the design simple and to maintain performance, the XDP BPF-prog (still) only have access to the first-buffer. It is left for later (another patchset) to add payload access across multiple buffers. This patchset should still allow for these future extensions. The goal is to lift the XDP MTU restriction that comes with XDP, but maintain same performance as before.

The main idea for the new multi-buffer layout is to reuse the same layout used for non-linear SKB. This rely on the "skb_shared_info" struct at the end of the first buffer to link together subsequent buffers. Keeping the layout compatible with SKBs is also done to ease and speedup creating an SKB from an xdp_{buff,frame}. Converting xdp_frame to SKB and deliver it to the network stack is shown in patch 07/14 (e.g. cpumaps).

A multi-buffer bit (mb) has been introduced in the flags field of xdp_{buff,frame} structure to notify the bpf/network layer if this is a xdp multi-buffer frame (mb = 1) or not (mb = 0).
The mb bit will be set by a xdp multi-buffer capable driver only for non-linear frames maintaining the capability to receive linear frames without any extra cost since the skb_shared_info structure at the end of the first buffer will be initialized only if mb is set.
Moreover the flags field in xdp_{buff,frame} will be reused even for xdp rx csum offloading in future series.

Typical use cases for this series are:
- Jumbo-frames
- Packet header split (please see Google���s use-case @ NetDevConf 0x14, [0])
- TSO

A new bpf helper (bpf_xdp_get_buff_len) has been introduce in order to notify the eBPF layer about the total frame size (linear + paged parts).

bpf_xdp_adjust_tail and bpf_xdp_copy helpers have been modified to take into account xdp multi-buff frames.

More info about the main idea behind this approach can be found here [1][2].

Changes since v8:
- add proper dma unmapping if XDP_TX fails on mvneta for a xdp multi-buff
- switch back to skb_shared_info implementation from previous xdp_shared_info one
- avoid using a bietfield in xdp_buff/xdp_frame since it introduces performance regressions. Tested now on 10G NIC (ixgbe) to verify there are no performance penalties for regular codebase
- add bpf_xdp_get_buff_len helper and remove frame_length field in xdp ctx
- add data_len field in skb_shared_info struct

Changes since v7:
- rebase on top of bpf-next
- fix sparse warnings
- improve comments for frame_length in include/net/xdp.h

Changes since v6:
- the main difference respect to previous versions is the new approach proposed by Eelco to pass full length of the packet to eBPF layer in XDP context
- reintroduce multi-buff support to eBPF kself-tests
- reintroduce multi-buff support to bpf_xdp_adjust_tail helper
- introduce multi-buffer support to bpf_xdp_copy helper
- rebase on top of bpf-next

Changes since v5:
- rebase on top of bpf-next
- initialize mb bit in xdp_init_buff() and drop per-driver initialization
- drop xdp->mb initialization in xdp_convert_zc_to_xdp_frame()
- postpone introduction of frame_length field in XDP ctx to another series
- minor changes

Changes since v4:
- rebase ontop of bpf-next
- introduce xdp_shared_info to build xdp multi-buff instead of using the skb_shared_info struct
- introduce frame_length in xdp ctx
- drop previous bpf helpers
- fix bpf_xdp_adjust_tail for xdp multi-buff
- introduce xdp multi-buff self-tests for bpf_xdp_adjust_tail
- fix xdp_return_frame_bulk for xdp multi-buff

Changes since v3:
- rebase ontop of bpf-next
- add patch 10/13 to copy back paged data from a xdp multi-buff frame to userspace buffer for xdp multi-buff selftests

Changes since v2:
- add throughput measurements
- drop bpf_xdp_adjust_mb_header bpf helper
- introduce selftest for xdp multibuffer
- addressed comments on bpf_xdp_get_frags_count
- introduce xdp multi-buff support to cpumaps

Changes since v1:
- Fix use-after-free in xdp_return_{buff/frame}
- Introduce bpf helpers
- Introduce xdp_mb sample program
- access skb_shared_info->nr_frags only on the last fragment

Changes since RFC:
- squash multi-buffer bit initialization in a single patch
- add mvneta non-linear XDP buff support for tx side

[0] https://netdevconf.info/0x14/session.html?talk-the-path-to-tcp-4k-mtu-and-rx-zerocopy
[1] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org
[2] https://netdevconf.info/0x14/session.html?tutorial-add-XDP-support-to-a-NIC-driver (XDPmulti-buffers section)

Eelco Chaudron (3):
bpf: add multi-buff support to the bpf_xdp_adjust_tail() API
bpf: add multi-buffer support to xdp copy helpers
bpf: update xdp_adjust_tail selftest to include multi-buffer

Lorenzo Bianconi (11):
net: skbuff: add data_len field to skb_shared_info
xdp: introduce flags field in xdp_buff/xdp_frame
net: mvneta: update mb bit before passing the xdp buffer to eBPF layer
xdp: add multi-buff support to xdp_return_{buff/frame}
net: mvneta: add multi buffer support to XDP_TX
net: mvneta: enable jumbo frames for XDP
net: xdp: add multi-buff support to xdp_build_skb_from_frame
bpf: introduce bpf_xdp_get_buff_len helper
bpf: move user_size out of bpf_test_init
bpf: introduce multibuff support to bpf_prog_test_run_xdp()
bpf: test_run: add xdp_shared_info pointer in bpf_test_finish
signature

 drivers/net/ethernet/marvell/mvneta.c         | 143 ++++++++++------
 include/linux/skbuff.h                        |   5 +-
 include/net/xdp.h                             |  56 ++++++-
 include/uapi/linux/bpf.h                      |   7 +
 kernel/trace/bpf_trace.c                      |   3 +
 net/bpf/test_run.c                            | 108 +++++++++---
 net/core/filter.c                             | 157 +++++++++++++++++-
 net/core/xdp.c                                |  72 +++++++-
 tools/include/uapi/linux/bpf.h                |   7 +
 .../bpf/prog_tests/xdp_adjust_tail.c          | 105 ++++++++++++
 .../selftests/bpf/prog_tests/xdp_bpf2bpf.c    | 127 +++++++++-----
 .../bpf/progs/test_xdp_adjust_tail_grow.c     |  10 +-
 .../bpf/progs/test_xdp_adjust_tail_shrink.c   |  32 +++-
 .../selftests/bpf/progs/test_xdp_bpf2bpf.c    |   2 +-
 14 files changed, 705 insertions(+), 129 deletions(-)

Signature · 1 line

2.31.1

At this point I don't think we can have a partial implementation. At the moment we have packet capture applications and protocol parsers running in production. If we allow this to go in staged we are going to break those applications that make the fundamental assumption they have access to all the data in the packet.

Quoted 5 lines · John Fastabend

At this point I don't think we can have a partial implementation. At the moment we have packet capture applications and protocol parsers running in production. If we allow this to go in staged we are going to break those applications that make the fundamental assumption they have access to all the data in the packet.

What about cases like netgpu where headers are accessible but data is not (e.g., gpu memory)? If the API indicates limited buffer access, is that sufficient?

Quoted 7 lines · David Ahern

Quoted 5 lines · John Fastabend

At this point I don't think we can have a partial implementation. At the moment we have packet capture applications and protocol parsers running in production. If we allow this to go in staged we are going to break those applications that make the fundamental assumption they have access to all the data in the packet.

Quoted 3 lines

What about cases like netgpu where headers are accessible but data is not (e.g., gpu memory)? If the API indicates limited buffer access, is that sufficient?

I never consider netgpus and I guess I don't fully understand the architecture to say. But, I would try to argue that an XDP API should allow XDP to reach into the payload of these GPU packets as well. Of course it might be slow.

I'm not really convinced just indicating its a limited buffer is enough. I think we want to be able to read/write any byte in the packet. I see two ways to do it,

/* xdp_pull_data moves data and data_end pointers into the frag
* containing the byte offset start.
*
* returns negative value on error otherwise returns offset of
* data pointer into payload.
*/
int xdp_pull_data(int start)

This would be a helper call to push the xdp->data{_end} pointers into the correct frag and then normal verification should work. From my side this works because I can always find the next frag by starting at 'xdp_pull_data(xdp->data_end+1)'. And by returning offset we can always figure out where we are in the payload. This is the easiest thing I could come up with. And hopefully for _most_ cases the bytes we need are in the initial data. Also I don't see how extending tail works without something like this.

My other thought, but requires some verifier work would be to extend 'struct xdp_md' with a frags[] pointer.

 struct xdp_md {
   __u32 data;
   __u32 data_end;
   __u32 data_meta;
   /* metadata stuff */

struct _xdp_md frags[]
__u32 frags_end;
}

Then a XDP program could read access a frag like so,

  if (i < xdp->frags_end) {
     frag = xdp->frags[i];
     if (offset + hdr_size < frag->data_end)
         memcpy(dst, frag->data[offset], hdr_size);
  }

The nice bit about above is you avoid the call, but maybe it doesn't matter if you are already looking into frags pps is probably not at 64B sizes anyways.

My main concern here is we hit a case where the driver doesn't pull in the bytes we need and then we are stuck without a workaround. The helper looks fairly straightforward to me could we try that?

Also I thought we had another driver in the works? Any ideas where that went...

Last, I'll add thanks for working on this everyone.

.John

Quoted 12 lines · John Fastabend

Quoted 10 lines · David Ahern

Quoted 5 lines · John Fastabend

At this point I don't think we can have a partial implementation. At the moment we have packet capture applications and protocol parsers running in production. If we allow this to go in staged we are going to break those applications that make the fundamental assumption they have access to all the data in the packet.

What about cases like netgpu where headers are accessible but data is not (e.g., gpu memory)? If the API indicates limited buffer access, is that sufficient?

Quoted 4 lines

I never consider netgpus and I guess I don't fully understand the architecture to say. But, I would try to argue that an XDP API should allow XDP to reach into the payload of these GPU packets as well. Of course it might be slow.

AIUI S/W on the host can not access gpu memory, so that is not a possibility at all.

Another use case is DDP and ZC. Mellanox has a proposal for NVME (with intentions to extend to iscsi) to do direct data placement. This is really just an example of zerocopy (and netgpu has morphed into zctap with current prototype working for host memory) which will become more prominent. XDP programs accessing memory already mapped to user space will be racy.

To me these proposals suggest a trend and one that XDP APIs should be ready to handle - like indicating limited access or specifying length that can be accessed.

Quoted 7 lines · Lorenzo Bianconi

This series introduce XDP multi-buffer support. The mvneta driver is the first to support these new "non-linear" xdp_{buff,frame}. Reviewers please focus on how these new types of xdp_{buff,frame} packets traverse the different layers and the layout design. It is on purpose that BPF-helpers are kept simple, as we don't want to expose the internal layout to allow later changes.

Quoted 6 lines

For now, to keep the design simple and to maintain performance, the XDP BPF-prog (still) only have access to the first-buffer. It is left for later (another patchset) to add payload access across multiple buffers. This patchset should still allow for these future extensions. The goal is to lift the XDP MTU restriction that comes with XDP, but maintain same performance as before.

At this point I don't think we can have a partial implementation. At the moment we have packet capture applications and protocol parsers running in production. If we allow this to go in staged we are going to break those applications that make the fundamental assumption they have access to all the data in the packet.

There will be no way to fix it when it happens. The teams running the applications wont necessarily be able to change the network MTU. Now it doesn't work, hard stop. This is better than it sort of works some of the time. Worse if we get in a situation where some drivers support partial access and others support full access the support matrix gets worse.

I think we need to get full support and access to all bytes. I believe I said this earlier, but now we've deployed apps that really do need access to the payloads so its not a theoritical concern anymore, but rather a real one based on deployed BPF programs.

Quoted 19 lines

The main idea for the new multi-buffer layout is to reuse the same layout used for non-linear SKB. This rely on the "skb_shared_info" struct at the end of the first buffer to link together subsequent buffers. Keeping the layout compatible with SKBs is also done to ease and speedup creating an SKB from an xdp_{buff,frame}. Converting xdp_frame to SKB and deliver it to the network stack is shown in patch 07/14 (e.g. cpumaps).

A multi-buffer bit (mb) has been introduced in the flags field of xdp_{buff,frame} structure to notify the bpf/network layer if this is a xdp multi-buffer frame (mb = 1) or not (mb = 0).
The mb bit will be set by a xdp multi-buffer capable driver only for non-linear frames maintaining the capability to receive linear frames without any extra cost since the skb_shared_info structure at the end of the first buffer will be initialized only if mb is set.
Moreover the flags field in xdp_{buff,frame} will be reused even for xdp rx csum offloading in future series.

Quoted 7 lines

Typical use cases for this series are:
- Jumbo-frames
- Packet header split (please see Google���s use-case @ NetDevConf 0x14, [0])
- TSO

A new bpf helper (bpf_xdp_get_buff_len) has been introduce in order to notify the eBPF layer about the total frame size (linear + paged parts).

Is it possible to make currently working programs continue to work? For a simple packet capture example a program might capture the entire packet of bytes '(data_end - data_start)'. With above implementation the program will continue to run, but will no longer be capturing all the bytes... so its a silent failure. Otherwise I'll need to backport fixes into my BPF programs and releases to ensure they don't walk onto a new kernel with multi-buffer support enabled. Its not ideal.

Quoted 4 lines

bpf_xdp_adjust_tail and bpf_xdp_copy helpers have been modified to take into account xdp multi-buff frames.

Quoted 1 line

More info about the main idea behind this approach can be found here [1][2].

Will read [1],[2].

Where did the perf data for the 40gbps NIC go? I think we want that done again on this series with at least 40gbps NICs and better yet 100gbps drivers. If its addressed in a patch commit message I'm reading the series now.

Quoted 105 lines

Changes since v8:
- add proper dma unmapping if XDP_TX fails on mvneta for a xdp multi-buff
- switch back to skb_shared_info implementation from previous xdp_shared_info one
- avoid using a bietfield in xdp_buff/xdp_frame since it introduces performance regressions. Tested now on 10G NIC (ixgbe) to verify there are no performance penalties for regular codebase
- add bpf_xdp_get_buff_len helper and remove frame_length field in xdp ctx
- add data_len field in skb_shared_info struct

Changes since v7:
- rebase on top of bpf-next
- fix sparse warnings
- improve comments for frame_length in include/net/xdp.h

Changes since v6:
- the main difference respect to previous versions is the new approach proposed by Eelco to pass full length of the packet to eBPF layer in XDP context
- reintroduce multi-buff support to eBPF kself-tests
- reintroduce multi-buff support to bpf_xdp_adjust_tail helper
- introduce multi-buffer support to bpf_xdp_copy helper
- rebase on top of bpf-next

Changes since v5:
- rebase on top of bpf-next
- initialize mb bit in xdp_init_buff() and drop per-driver initialization
- drop xdp->mb initialization in xdp_convert_zc_to_xdp_frame()
- postpone introduction of frame_length field in XDP ctx to another series
- minor changes

Changes since v4:
- rebase ontop of bpf-next
- introduce xdp_shared_info to build xdp multi-buff instead of using the skb_shared_info struct
- introduce frame_length in xdp ctx
- drop previous bpf helpers
- fix bpf_xdp_adjust_tail for xdp multi-buff
- introduce xdp multi-buff self-tests for bpf_xdp_adjust_tail
- fix xdp_return_frame_bulk for xdp multi-buff

Changes since v3:
- rebase ontop of bpf-next
- add patch 10/13 to copy back paged data from a xdp multi-buff frame to userspace buffer for xdp multi-buff selftests

Changes since v2:
- add throughput measurements
- drop bpf_xdp_adjust_mb_header bpf helper
- introduce selftest for xdp multibuffer
- addressed comments on bpf_xdp_get_frags_count
- introduce xdp multi-buff support to cpumaps

Changes since v1:
- Fix use-after-free in xdp_return_{buff/frame}
- Introduce bpf helpers
- Introduce xdp_mb sample program
- access skb_shared_info->nr_frags only on the last fragment

Changes since RFC:
- squash multi-buffer bit initialization in a single patch
- add mvneta non-linear XDP buff support for tx side

[0] https://netdevconf.info/0x14/session.html?talk-the-path-to-tcp-4k-mtu-and-rx-zerocopy
[1] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org
[2] https://netdevconf.info/0x14/session.html?tutorial-add-XDP-support-to-a-NIC-driver (XDPmulti-buffers section)

Eelco Chaudron (3):
bpf: add multi-buff support to the bpf_xdp_adjust_tail() API
bpf: add multi-buffer support to xdp copy helpers
bpf: update xdp_adjust_tail selftest to include multi-buffer

Lorenzo Bianconi (11):
net: skbuff: add data_len field to skb_shared_info
xdp: introduce flags field in xdp_buff/xdp_frame
net: mvneta: update mb bit before passing the xdp buffer to eBPF layer
xdp: add multi-buff support to xdp_return_{buff/frame}
net: mvneta: add multi buffer support to XDP_TX
net: mvneta: enable jumbo frames for XDP
net: xdp: add multi-buff support to xdp_build_skb_from_frame
bpf: introduce bpf_xdp_get_buff_len helper
bpf: move user_size out of bpf_test_init
bpf: introduce multibuff support to bpf_prog_test_run_xdp()
bpf: test_run: add xdp_shared_info pointer in bpf_test_finish
signature

 drivers/net/ethernet/marvell/mvneta.c         | 143 ++++++++++------
 include/linux/skbuff.h                        |   5 +-
 include/net/xdp.h                             |  56 ++++++-
 include/uapi/linux/bpf.h                      |   7 +
 kernel/trace/bpf_trace.c                      |   3 +
 net/bpf/test_run.c                            | 108 +++++++++---
 net/core/filter.c                             | 157 +++++++++++++++++-
 net/core/xdp.c                                |  72 +++++++-
 tools/include/uapi/linux/bpf.h                |   7 +
 .../bpf/prog_tests/xdp_adjust_tail.c          | 105 ++++++++++++
 .../selftests/bpf/prog_tests/xdp_bpf2bpf.c    | 127 +++++++++-----
 .../bpf/progs/test_xdp_adjust_tail_grow.c     |  10 +-
 .../bpf/progs/test_xdp_adjust_tail_shrink.c   |  32 +++-
 .../selftests/bpf/progs/test_xdp_bpf2bpf.c    |   2 +-
 14 files changed, 705 insertions(+), 129 deletions(-)

--
2.31.1