mvneta: introduce XDP multi-buffer support

Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer to specify if shared_info area has been properly initialized for non-linear xdp buffers

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

---
 include/net/xdp.h | 8 ++++++--
 net/core/xdp.c    | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

include/net/xdp.h +2 -1

index 3814fb631d52..42f439f9fcda 100644
@@ -72,7 +72,8 @@ struct xdp_buff {
 	void *data_hard_start;
 	struct xdp_rxq_info *rxq;
 	struct xdp_txq_info *txq;
-	u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/
+	u32 frame_sz:31; /* frame size to deduce data_hard_end/reserved tailroom*/
+	u32 mb:1; /* xdp non-linear buffer */
 };
 
 /* Reserve memory area at end-of data area.

Quoted 23 lines · Lorenzo Bianconi

Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer to specify if shared_info area has been properly initialized for non-linear xdp buffers

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

---
 include/net/xdp.h | 8 ++++++--
 net/core/xdp.c    | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

include/net/xdp.h +2 -1

index 3814fb631d52..42f439f9fcda 100644
@@ -72,7 +72,8 @@ struct xdp_buff {
 	void *data_hard_start;
 	struct xdp_rxq_info *rxq;
 	struct xdp_txq_info *txq;
-	u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/
+	u32 frame_sz:31; /* frame size to deduce data_hard_end/reserved tailroom*/
+	u32 mb:1; /* xdp non-linear buffer */
 };
 

include/net/xdp.h struct xdp_frame { · line 101

Quoted 8 lines

include/net/xdp.h +2 -1

 /* Reserve memory area at end-of data area.

@@ -96,7 +97,8 @@ struct xdp_frame {
 	u16 len;
 	u16 headroom;
 	u32 metasize:8;
-	u32 frame_sz:24;
+	u32 frame_sz:23;
+	u32 mb:1; /* xdp non-linear frame */  

Hmm. Last time I checked compilers were generating ugly code with bitfields. Not performant and not efficient.
frame_sz is used in the fast path.
I suspect the first hunk alone will cause performance degradation. Could you use normal u8 or u32 flag field?

Quoted 24 lines · On Thu, 3 Sep 2020 18:07:05 -0700 Alexei Starovoitov…

Quoted 23 lines · Lorenzo Bianconi

Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer to specify if shared_info area has been properly initialized for non-linear xdp buffers

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

---
 include/net/xdp.h | 8 ++++++--
 net/core/xdp.c    | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

include/net/xdp.h +2 -1

index 3814fb631d52..42f439f9fcda 100644
@@ -72,7 +72,8 @@ struct xdp_buff {
 	void *data_hard_start;
 	struct xdp_rxq_info *rxq;
 	struct xdp_txq_info *txq;
-	u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/
+	u32 frame_sz:31; /* frame size to deduce data_hard_end/reserved tailroom*/
+	u32 mb:1; /* xdp non-linear buffer */
 };
 

Quoted 14 lines

include/net/xdp.h struct xdp_frame { · line 101

Quoted 8 lines

include/net/xdp.h +2 -1

 /* Reserve memory area at end-of data area.

@@ -96,7 +97,8 @@ struct xdp_frame {
 	u16 len;
 	u16 headroom;
 	u32 metasize:8;
-	u32 frame_sz:24;
+	u32 frame_sz:23;
+	u32 mb:1; /* xdp non-linear frame */  

Hmm. Last time I checked compilers were generating ugly code with bitfields. Not performant and not efficient.
frame_sz is used in the fast path.
I suspect the first hunk alone will cause performance degradation. Could you use normal u8 or u32 flag field?

For struct xdp_buff sure we can do this. For struct xdp_frame, I'm not sure, as it is a state compressed version of xdp_buff + extra information. The xdp_frame have been called skb-light, and I know people (e.g Ahern) wants to add more info to this, vlan, RX-hash, csum, and we must keep this to 1-cache-line, for performance reasons.

You do make a good point, that these bit-fields might hurt performance more. I guess, we need to test this. As I constantly worry that we will slowly kill XDP performance with a 1000 paper-cuts.

Quoted 48 lines · Jesper Dangaard Brouer

Quoted 24 lines · On Thu, 3 Sep 2020 18:07:05 -0700 Alexei Starovoitov…

Quoted 23 lines · Lorenzo Bianconi

Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer to specify if shared_info area has been properly initialized for non-linear xdp buffers

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

---
 include/net/xdp.h | 8 ++++++--
 net/core/xdp.c    | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

include/net/xdp.h +2 -1

index 3814fb631d52..42f439f9fcda 100644
@@ -72,7 +72,8 @@ struct xdp_buff {
 	void *data_hard_start;
 	struct xdp_rxq_info *rxq;
 	struct xdp_txq_info *txq;
-	u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/
+	u32 frame_sz:31; /* frame size to deduce data_hard_end/reserved tailroom*/
+	u32 mb:1; /* xdp non-linear buffer */
 };
 

Quoted 14 lines

include/net/xdp.h struct xdp_frame { · line 101

Quoted 8 lines

include/net/xdp.h +2 -1

 /* Reserve memory area at end-of data area.

@@ -96,7 +97,8 @@ struct xdp_frame {
 	u16 len;
 	u16 headroom;
 	u32 metasize:8;
-	u32 frame_sz:24;
+	u32 frame_sz:23;
+	u32 mb:1; /* xdp non-linear frame */    

Hmm. Last time I checked compilers were generating ugly code with bitfields. Not performant and not efficient.
frame_sz is used in the fast path.
I suspect the first hunk alone will cause performance degradation. Could you use normal u8 or u32 flag field?

For struct xdp_buff sure we can do this. For struct xdp_frame, I'm not sure, as it is a state compressed version of xdp_buff + extra information. The xdp_frame have been called skb-light, and I know people (e.g Ahern) wants to add more info to this, vlan, RX-hash, csum, and we must keep this to 1-cache-line, for performance reasons.

Quoted 4 lines

You do make a good point, that these bit-fields might hurt performance more. I guess, we need to test this. As I constantly worry that we will slowly kill XDP performance with a 1000 paper-cuts.

That struct is tight on space, and we have to be very smart about additions.

dev_rx for example seems like it could just be the netdev index rather than a pointer or perhaps can be removed completely. I believe it is only used for 1 use case (redirects to CPUMAP); maybe that code can be refactored to handle the dev outside of xdp_frame.

xdp_mem_info is 2 u32's; the type in that struct really could be a u8.

In this case it means removing struct in favor of 2 elements to reclaim the space, but as we reach the 64B limit this is a place to change. e.g., make it a single u32 with the id only 24 bits though the rhashtable key can stay u32 but now with the combined type + id.

As for frame_sz, why does it need to be larger than a u16?

If it really needs to be larger than u16, there are several examples of using a bit (or bits) in the data path. dst metrics for examples uses lowest 4 bits of the dst pointer as a bitfield. It does so using a mask with accessors vs a bitfield. Perhaps that is the way to go here.

Quoted 4 lines

dev_rx for example seems like it could just be the netdev index rather than a pointer or perhaps can be removed completely. I believe it is only used for 1 use case (redirects to CPUMAP); maybe that code can be refactored to handle the dev outside of xdp_frame.

The dev_rx is needed when creating an SKB from a xdp_frame (basically skb->dev = rx_dev). Yes, that is done in cpumap, but I want to generalize this. The veth also creates SKBs from xdp_frame, but use itself as skb->dev.

And yes, we could save some space storing the index instead, and trade space for cycles in a lookup.

Quoted 2 lines

As for frame_sz, why does it need to be larger than a u16?

Because PAGE_SIZE can be 64KiB on some archs.

Quoted 10 lines · Jesper Dangaard Brouer

Quoted 4 lines

dev_rx for example seems like it could just be the netdev index rather than a pointer or perhaps can be removed completely. I believe it is only used for 1 use case (redirects to CPUMAP); maybe that code can be refactored to handle the dev outside of xdp_frame.

The dev_rx is needed when creating an SKB from a xdp_frame (basically skb->dev = rx_dev). Yes, that is done in cpumap, but I want to generalize this. The veth also creates SKBs from xdp_frame, but use itself as skb->dev.

Quoted 2 lines

And yes, we could save some space storing the index instead, and trade space for cycles in a lookup.

I think this can be managed without adding a reference to the xdp_frame. I'll start a separate thread on that.

Quoted 5 lines

Quoted 2 lines

As for frame_sz, why does it need to be larger than a u16?

Because PAGE_SIZE can be 64KiB on some archs.

ok, is there any alignment requirement? can frame_sz be number of 32-bit words? I believe bit shifts are cheap.

Quoted 3 lines

ok, is there any alignment requirement? can frame_sz be number of 32-bit words? I believe bit shifts are cheap.

No that is not possible, because some drivers and generic-XDP have a fully dynamic frame_sz.

Quoted 10 lines · On Thu, 3 Sep 2020 22:58:46 +0200 Lorenzo Bianconi <[email protected]> wrote:

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +1 -0

index 0b675c34ce49..20c8fd3cd4a3 100644
@@ -2298,6 +2298,7 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
 #if (PAGE_SIZE < 8192)
 	xdp.frame_sz = ixgbe_rx_frame_truesize(rx_ring, 0);
 #endif
+	xdp.mb = 0;
 

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, · line 2304

Quoted 2 lines

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0 -0

 	while (likely(total_rx_packets < budget)) {
 		union ixgbe_adv_rx_desc *rx_desc;

In this ixgbe driver you are smart and init the xdp.mb bit outside the
(like xdp.frame_sz, when frame_sz is constant). This is a nice
optimization, but the driver developer that adds XDP multi-buffer support must remember to reset it. The patch itself is okay, it is just something to keep in-mind when reviewing/changing drivers.

Signature · 5 lines

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Update multi-buffer bit (mb) in xdp_buff to notify XDP/eBPF layer and XDP remote drivers if this is a "non-linear" XDP buffer. Access skb_shared_info only if xdp_buff mb is set

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

---
 drivers/net/ethernet/marvell/mvneta.c | 37 
 +++++++++++++++------------
 1 file changed, 21 insertions(+), 16 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +2 -2

index 832bbb8b05c8..4f745a2b702a 100644
@@ -2027,11 +2027,11 @@ mvneta_xdp_put_buff(struct mvneta_port 
 *pp, struct mvneta_rx_queue *rxq,
 		    struct xdp_buff *xdp, int sync_len, bool napi)
 {
 	struct skb_shared_info *sinfo = 
 xdp_get_shared_info_from_buff(xdp);
-	int i;
+	int i, num_frames = xdp->mb ? sinfo->nr_frags : 0;
 
 	page_pool_put_page(rxq->page_pool, 
 virt_to_head_page(xdp->data),
 			   sync_len, napi);
-	for (i = 0; i < sinfo->nr_frags; i++)
+	for (i = 0; i < num_frames; i++)

drivers/net/ethernet/marvell/mvneta.c +2 -3

 		page_pool_put_full_page(rxq->page_pool,
 					skb_frag_page(&sinfo->frags[i]), 
 napi);
 }
...

@@ -2175,6 +2175,7 @@ mvneta_run_xdp(struct mvneta_port *pp, 
 struct mvneta_rx_queue *rxq,
 
 	len = xdp->data_end - xdp->data_hard_start - 
 pp->rx_offset_correction;
 	data_len = xdp->data_end - xdp->data;
-	sinfo = xdp_get_shared_info_from_buff(xdp);
-	sinfo->nr_frags = 0;
+	xdp->mb = 0;
 
 	*size = rx_desc->data_size - len;
 	rx_desc->buf_phys_addr = 0;

@@ -2269,7 +2267,7 @@ mvneta_swbm_add_rx_fragment(struct 
 mvneta_port *pp,
 			    struct mvneta_rx_desc *rx_desc,
 			    struct mvneta_rx_queue *rxq,
 			    struct xdp_buff *xdp, int *size,
-			    struct page *page)
+			    int *nfrags, struct page *page)
 {
 	struct skb_shared_info *sinfo = 

xdp_get_shared_info_from_buff(xdp);

drivers/net/ethernet/marvell/mvneta.c +0 -0

 	struct net_device *dev = pp->dev;

@@ -2288,13 +2286,18 @@ mvneta_swbm_add_rx_fragment(struct 
 mvneta_port *pp,
 				rx_desc->buf_phys_addr,
 				len, dma_dir);
 

drivers/net/ethernet/marvell/mvneta.c +5 -0

+
+		if (rx_desc->status & MVNETA_RXD_LAST_DESC) {
+			sinfo->nr_frags = *nfrags;
+			xdp->mb = true;
+		}
 
 		rx_desc->buf_phys_addr = 0;
...

drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_add_rx_fragment(struct mvneta_port *pp, · line 2295

Quoted 11 lines

drivers/net/ethernet/marvell/mvneta.c +3 -3

 				rx_desc->buf_phys_addr,
 				len, dma_dir);
-	if (data_len > 0 && sinfo->nr_frags < MAX_SKB_FRAGS) {
-		skb_frag_t *frag = &sinfo->frags[sinfo->nr_frags];
+	if (data_len > 0 && *nfrags < MAX_SKB_FRAGS) {
+		skb_frag_t *frag = &sinfo->frags[*nfrags];
 		skb_frag_off_set(frag, pp->rx_offset_correction);
 		skb_frag_size_set(frag, data_len);
 		__skb_frag_set_page(frag, page);
-		sinfo->nr_frags++;
+		*nfrags = *nfrags + 1;

nit, why do you use nfrags variable instead of sinfo->nr_frags (as it was before this patch) ?
You doesn't seem to use the nfrags variable in the
caller function and you update nr_frags as well.
If it's used in a different patch (haven't
reviewed it all yet), maybe move it to that patch ? (sorry in
advance if I missed the logic here)

Quoted 8 lines

drivers/net/ethernet/marvell/mvneta.c +5 -0

+
+		if (rx_desc->status & MVNETA_RXD_LAST_DESC) {
+			sinfo->nr_frags = *nfrags;
+			xdp->mb = true;
+		}
 		rx_desc->buf_phys_addr = 0;
...

Introduce the capability to map non-linear xdp buffer running mvneta_xdp_submit_frame() for XDP_TX and XDP_REDIRECT

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

---
 drivers/net/ethernet/marvell/mvneta.c | 79 
 +++++++++++++++++----------
 1 file changed, 49 insertions(+), 30 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +2 -2

index 4f745a2b702a..65fbed957e4f 100644
@@ -1854,8 +1854,8 @@ static void mvneta_txq_bufs_free(struct 
 mvneta_port *pp,
 			bytes_compl += buf->skb->len;
 			pkts_compl++;
 			dev_kfree_skb_any(buf->skb);
-		} else if (buf->type == MVNETA_TYPE_XDP_TX ||
-			   buf->type == MVNETA_TYPE_XDP_NDO) {
+		} else if ((buf->type == MVNETA_TYPE_XDP_TX ||
+			    buf->type == MVNETA_TYPE_XDP_NDO) && 
 buf->xdpf) {
 			xdp_return_frame(buf->xdpf);

drivers/net/ethernet/marvell/mvneta.c +36 -20

 		}
 	}

@@ -2040,43 +2040,62 @@ static int
 mvneta_xdp_submit_frame(struct mvneta_port *pp, struct 
 mvneta_tx_queue *txq,
 			struct xdp_frame *xdpf, bool dma_map)
 {
-	struct mvneta_tx_desc *tx_desc;
-	struct mvneta_tx_buf *buf;
-	dma_addr_t dma_addr;
+	struct skb_shared_info *sinfo = 
 xdp_get_shared_info_from_frame(xdpf);
+	int i, num_frames = xdpf->mb ? sinfo->nr_frags + 1 : 1;
+	struct mvneta_tx_desc *tx_desc = NULL;
+	struct page *page;
 
-	if (txq->count >= txq->tx_stop_threshold)
+	if (txq->count + num_frames >= txq->tx_stop_threshold)
 		return MVNETA_XDP_DROPPED;
 
-	tx_desc = mvneta_txq_next_desc_get(txq);
+	for (i = 0; i < num_frames; i++) {
+		struct mvneta_tx_buf *buf = 
 &txq->buf[txq->txq_put_index];
+		skb_frag_t *frag = i ? &sinfo->frags[i - 1] : 
 NULL;
+		int len = frag ? skb_frag_size(frag) : xdpf->len;
+		dma_addr_t dma_addr;
 
-	buf = &txq->buf[txq->txq_put_index];
-	if (dma_map) {
-		/* ndo_xdp_xmit */
-		dma_addr = dma_map_single(pp->dev->dev.parent, 
 xdpf->data,
-					  xdpf->len, 
 DMA_TO_DEVICE);
-		if (dma_mapping_error(pp->dev->dev.parent, 
 dma_addr)) {
-			mvneta_txq_desc_put(txq);
-			return MVNETA_XDP_DROPPED;
+		tx_desc = mvneta_txq_next_desc_get(txq);
+		if (dma_map) {
+			/* ndo_xdp_xmit */
+			void *data;
+
+			data = frag ? 
 page_address(skb_frag_page(frag))
+				    : xdpf->data;
+			dma_addr = 
 dma_map_single(pp->dev->dev.parent, data,
+						  len, 
 DMA_TO_DEVICE);
+			if (dma_mapping_error(pp->dev->dev.parent, 
 dma_addr)) {
+				for (; i >= 0; i--)
+					mvneta_txq_desc_put(txq);
+				return MVNETA_XDP_DROPPED;
+			}
+			buf->type = MVNETA_TYPE_XDP_NDO;
+		} else {
+			page = frag ? skb_frag_page(frag)
+				    : virt_to_page(xdpf->data);
+			dma_addr = page_pool_get_dma_addr(page);
+			if (!frag)
+				dma_addr += sizeof(*xdpf) + 
 xdpf->headroom;
+ 
 dma_sync_single_for_device(pp->dev->dev.parent,
+						   dma_addr, len,
+ 
 DMA_BIDIRECTIONAL);
+			buf->type = MVNETA_TYPE_XDP_TX;
 		}
-		buf->type = MVNETA_TYPE_XDP_NDO;
-	} else {
-		struct page *page = virt_to_page(xdpf->data);
+		buf->xdpf = i ? NULL : xdpf;
 
-		dma_addr = page_pool_get_dma_addr(page) +
-			   sizeof(*xdpf) + xdpf->headroom;
-		dma_sync_single_for_device(pp->dev->dev.parent, 
 dma_addr,
-					   xdpf->len, 

DMA_BIDIRECTIONAL);

drivers/net/ethernet/marvell/mvneta.c +9 -5

-		buf->type = MVNETA_TYPE_XDP_TX;
+		if (!i)
+			tx_desc->command = MVNETA_TXD_F_DESC;
+		tx_desc->buf_phys_addr = dma_addr;
+		tx_desc->data_size = len;
+
+		mvneta_txq_inc_put(txq);
 	}
-	buf->xdpf = xdpf;
 
-	tx_desc->command = MVNETA_TXD_FLZ_DESC;
-	tx_desc->buf_phys_addr = dma_addr;
-	tx_desc->data_size = xdpf->len;
+	/*last descriptor */
+	if (tx_desc)
+		tx_desc->command |= MVNETA_TXD_L_DESC | 

MVNETA_TXD_Z_PAD;

drivers/net/ethernet/marvell/mvneta.c +2 -3

-	mvneta_txq_inc_put(txq);
-	txq->pending++;
-	txq->count++;
+	txq->pending += num_frames;
+	txq->count += num_frames;
 
 	return MVNETA_XDP_TX;
 }

Lorenzo Bianconi <[email protected]> writes:

Quoted 5 lines

drivers/net/ethernet/marvell/mvneta.c +1 -4

-	buf->xdpf = xdpf;
-	tx_desc->command = MVNETA_TXD_FLZ_DESC;
-	tx_desc->buf_phys_addr = dma_addr;
-	tx_desc->data_size = xdpf->len;
+	/*last descriptor */

drivers/net/ethernet/marvell/mvneta.c static int · line 2095

Quoted 2 lines

drivers/net/ethernet/marvell/mvneta.c +2 -0

+	if (tx_desc)
+		tx_desc->command |= MVNETA_TXD_L_DESC | MVNETA_TXD_Z_PAD;

When is this condition not taken ? You initialize tx_desc to
NULL, but it seems to me like you always set it inside the for loop
to the output of mvneta_txq_next_desc_get() which doesn't look like
it returns NULL. The for loop runs 1 iteration or sinfo->nr_frage + 1 iterations (which also equals or larger than 1).

Quoted 8 lines

drivers/net/ethernet/marvell/mvneta.c +2 -3

-	mvneta_txq_inc_put(txq);
-	txq->pending++;
-	txq->count++;
+	txq->pending += num_frames;
+	txq->count += num_frames;
 	return MVNETA_XDP_TX;
 }

Introduce bpf_xdp_adjust_mb_header helper in order to adjust frame headers moving *offset* bytes from/to the second buffer to/from the first one.
This helper can be used to move headers when the hw DMA SG is not able to copy all the headers in the first fragment and split header and data pages.

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

---
 include/uapi/linux/bpf.h       | 25 ++++++++++++----
 net/core/filter.c              | 54 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 26 ++++++++++++----
 3 files changed, 95 insertions(+), 10 deletions(-)

include/uapi/linux/bpf.h +0 -0

index 8dda13880957..c4a6d245619c 100644
@@ -3571,11 +3571,25 @@ union bpf_attr {
  *		value.
  *
  * long bpf_copy_from_user(void *dst, u32 size, const void *user_ptr)

Quoted 26 lines · Lorenzo Bianconi

Introduce bpf_xdp_adjust_mb_header helper in order to adjust frame headers moving *offset* bytes from/to the second buffer to/from the first one.
This helper can be used to move headers when the hw DMA SG is not able to copy all the headers in the first fragment and split header and data pages.

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

---
 include/uapi/linux/bpf.h       | 25 ++++++++++++----
 net/core/filter.c              | 54 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 26 ++++++++++++----
 3 files changed, 95 insertions(+), 10 deletions(-)

include/uapi/linux/bpf.h +0 -4

index 8dda13880957..c4a6d245619c 100644
@@ -3571,11 +3571,25 @@ union bpf_attr {
  *		value.
  *
  * long bpf_copy_from_user(void *dst, u32 size, const void *user_ptr)
- * 	Description
- * 		Read *size* bytes from user space address *user_ptr* and store
- * 		the data in *dst*. This is a wrapper of copy_from_user().
- * 	Return

include/uapi/linux/bpf.h union bpf_attr { · line 3578

Quoted 6 lines

include/uapi/linux/bpf.h +5 -1

- * 		0 on success, or a negative error in case of failure.
+ *	Description
+ *		Read *size* bytes from user space address *user_ptr* and store
+ *		the data in *dst*. This is a wrapper of copy_from_user().
+ *
+ * long bpf_xdp_adjust_mb_header(struct xdp_buff *xdp_md, int offset)

botched rebase?

net/core/filter.c +32 -0

+BPF_CALL_2(bpf_xdp_adjust_mb_header, struct  xdp_buff *, xdp,
+	   int, offset)
+{
+	void *data_hard_end, *data_end;
+	struct skb_shared_info *sinfo;
+	int frag_offset, frag_len;
+	u8 *addr;
+
+	if (!xdp->mb)
+		return -EOPNOTSUPP;
+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+
+	frag_len = skb_frag_size(&sinfo->frags[0]);
+	if (offset > frag_len)
+		return -EINVAL;
+
+	frag_offset = skb_frag_off(&sinfo->frags[0]);
+	data_end = xdp->data_end + offset;
+
+	if (offset < 0 && (-offset > frag_offset ||
+			   data_end < xdp->data + ETH_HLEN))
+		return -EINVAL;
+
+	data_hard_end = xdp_data_hard_end(xdp); /* use xdp->frame_sz */
+	if (data_end > data_hard_end)
+		return -EINVAL;
+
+	addr = page_address(skb_frag_page(&sinfo->frags[0])) + frag_offset;
+	if (offset > 0) {
+		memcpy(xdp->data_end, addr, offset);
+	} else {

Quoted 38 lines

net/core/filter.c +38 -0

+BPF_CALL_2(bpf_xdp_adjust_mb_header, struct  xdp_buff *, xdp,
+	   int, offset)
+{
+	void *data_hard_end, *data_end;
+	struct skb_shared_info *sinfo;
+	int frag_offset, frag_len;
+	u8 *addr;
+
+	if (!xdp->mb)
+		return -EOPNOTSUPP;
+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+
+	frag_len = skb_frag_size(&sinfo->frags[0]);
+	if (offset > frag_len)
+		return -EINVAL;
+
+	frag_offset = skb_frag_off(&sinfo->frags[0]);
+	data_end = xdp->data_end + offset;
+
+	if (offset < 0 && (-offset > frag_offset ||
+			   data_end < xdp->data + ETH_HLEN))
+		return -EINVAL;
+
+	data_hard_end = xdp_data_hard_end(xdp); /* use xdp->frame_sz */
+	if (data_end > data_hard_end)
+		return -EINVAL;
+
+	addr = page_address(skb_frag_page(&sinfo->frags[0])) + frag_offset;
+	if (offset > 0) {
+		memcpy(xdp->data_end, addr, offset);
+	} else {
+		memcpy(addr + offset, xdp->data_end + offset, -offset);
+		memset(xdp->data_end + offset, 0, -offset);
+	}
+
+	skb_frag_size_sub(&sinfo->frags[0], offset);
+	skb_frag_off_add(&sinfo->frags[0], offset);

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

Quoted 4 lines

net/core/filter.c +4 -0

+	xdp->data_end = data_end;
+
+	return 0;
+}

wait a sec. Are you saying that multi buffer XDP actually should be skb based? If that's what mvneta driver is doing that's fine, but that is not a reasonable requirement to put on all other drivers.

Quoted 39 lines · Lorenzo Bianconi

Quoted 38 lines

net/core/filter.c +38 -0

+BPF_CALL_2(bpf_xdp_adjust_mb_header, struct  xdp_buff *, xdp,
+	   int, offset)
+{
+	void *data_hard_end, *data_end;
+	struct skb_shared_info *sinfo;
+	int frag_offset, frag_len;
+	u8 *addr;
+
+	if (!xdp->mb)
+		return -EOPNOTSUPP;
+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+
+	frag_len = skb_frag_size(&sinfo->frags[0]);
+	if (offset > frag_len)
+		return -EINVAL;
+
+	frag_offset = skb_frag_off(&sinfo->frags[0]);
+	data_end = xdp->data_end + offset;
+
+	if (offset < 0 && (-offset > frag_offset ||
+			   data_end < xdp->data + ETH_HLEN))
+		return -EINVAL;
+
+	data_hard_end = xdp_data_hard_end(xdp); /* use xdp->frame_sz */
+	if (data_end > data_hard_end)
+		return -EINVAL;
+
+	addr = page_address(skb_frag_page(&sinfo->frags[0])) + frag_offset;
+	if (offset > 0) {
+		memcpy(xdp->data_end, addr, offset);
+	} else {
+		memcpy(addr + offset, xdp->data_end + offset, -offset);
+		memset(xdp->data_end + offset, 0, -offset);
+	}
+
+	skb_frag_size_sub(&sinfo->frags[0], offset);
+	skb_frag_off_add(&sinfo->frags[0], offset);

Quoted 8 lines

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

Quoted 4 lines

net/core/filter.c +4 -0

+	xdp->data_end = data_end;
+
+	return 0;
+}  

wait a sec. Are you saying that multi buffer XDP actually should be skb based? If that's what mvneta driver is doing that's fine, but that is not a reasonable requirement to put on all other drivers.

I did not got what you mean here. The xdp multi-buffer layout uses the skb_shared_info at the end of the first buffer to link subsequent frames [0] and we rely on skb_frag* utilities to set/read offset and length of subsequent buffers.

[0] http://people.redhat.com/lbiancon/conference/NetDevConf2020-0x14/add-xdp-on-driver.html
- XDP multi-buffers section (slide 40)

Quoted 48 lines · On Fri, 4 Sep 2020 09:50:31 +0200 Lorenzo Bianconi <[email protected]> wrote:

Quoted 39 lines · Lorenzo Bianconi

Quoted 38 lines

net/core/filter.c +38 -0

+BPF_CALL_2(bpf_xdp_adjust_mb_header, struct  xdp_buff *, xdp,
+	   int, offset)
+{
+	void *data_hard_end, *data_end;
+	struct skb_shared_info *sinfo;
+	int frag_offset, frag_len;
+	u8 *addr;
+
+	if (!xdp->mb)
+		return -EOPNOTSUPP;
+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+
+	frag_len = skb_frag_size(&sinfo->frags[0]);
+	if (offset > frag_len)
+		return -EINVAL;
+
+	frag_offset = skb_frag_off(&sinfo->frags[0]);
+	data_end = xdp->data_end + offset;
+
+	if (offset < 0 && (-offset > frag_offset ||
+			   data_end < xdp->data + ETH_HLEN))
+		return -EINVAL;
+
+	data_hard_end = xdp_data_hard_end(xdp); /* use xdp->frame_sz */
+	if (data_end > data_hard_end)
+		return -EINVAL;
+
+	addr = page_address(skb_frag_page(&sinfo->frags[0])) + frag_offset;
+	if (offset > 0) {
+		memcpy(xdp->data_end, addr, offset);
+	} else {
+		memcpy(addr + offset, xdp->data_end + offset, -offset);
+		memset(xdp->data_end + offset, 0, -offset);
+	}
+
+	skb_frag_size_sub(&sinfo->frags[0], offset);
+	skb_frag_off_add(&sinfo->frags[0], offset);

Quoted 8 lines

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

Quoted 4 lines

net/core/filter.c +4 -0

+	xdp->data_end = data_end;
+
+	return 0;
+}  

wait a sec. Are you saying that multi buffer XDP actually should be skb based? If that's what mvneta driver is doing that's fine, but that is not a reasonable requirement to put on all other drivers.

Quoted 4 lines

I did not got what you mean here. The xdp multi-buffer layout uses the skb_shared_info at the end of the first buffer to link subsequent frames [0] and we rely on skb_frag* utilities to set/read offset and length of subsequent buffers.

Yes, for now the same layout as "skb_shared_info" is "reuse", but I think we should think of this as "xdp_shared_info" instead, as how it is used for XDP is going to divert from SKBs. We already discussed (in conf call) that we could store the total len of "frags" here, to simplify the other helper.

Using the skb_frag_* helper functions are misleading, and will make it more difficult to divert from how SKB handle frags. What about introducing xdp_frag_* wrappers? (what do others think?)

Quoted 5 lines

[0] http://people.redhat.com/lbiancon/conference/NetDevConf2020-0x14/add-xdp-on-driver.html
- XDP multi-buffers section (slide 40)

Signature · 4 lines

Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Introduce bpf_xdp_adjust_mb_header helper in order to adjust frame headers moving *offset* bytes from/to the second buffer to/from the first one.
This helper can be used to move headers when the hw DMA SG is not able to copy all the headers in the first fragment and split header and data pages.

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

---
 include/uapi/linux/bpf.h       | 25 ++++++++++++----
 net/core/filter.c              | 54 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 26 ++++++++++++----
 3 files changed, 95 insertions(+), 10 deletions(-)

include/uapi/linux/bpf.h +0 -5

index 8dda13880957..c4a6d245619c 100644
@@ -3571,11 +3571,25 @@ union bpf_attr {
  *		value.
  *
  * long bpf_copy_from_user(void *dst, u32 size, const void *user_ptr)
- * 	Description
- * 		Read *size* bytes from user space address *user_ptr* and store
- * 		the data in *dst*. This is a wrapper of copy_from_user().
- * 	Return
- * 		0 on success, or a negative error in case of failure.

include/uapi/linux/bpf.h +10 -0

+ *
+ *		A call to this helper is susceptible to change the underlying
+ *		packet buffer. Therefore, at load time, all checks on pointers
+ *		previously done by the verifier are invalidated and must be
+ *		performed again, if the helper is used in combination with
+ *		direct packet access.
+ *
+ *	Return
+ *		0 on success, or a negative error in case of failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -3727,6 +3741,7 @@ union bpf_attr {
 	FN(inode_storage_delete),	\
 	FN(d_path),			\
 	FN(copy_from_user),		\
+	FN(xdp_adjust_mb_header),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper

net/core/filter.c +6 -0

index 47eef9a0be6a..ae6b10cf062d 100644
@@ -3475,6 +3475,57 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
 	.arg2_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_2(bpf_xdp_adjust_mb_header, struct  xdp_buff *, xdp,
+	   int, offset)
+{
+	void *data_hard_end, *data_end;
+	struct skb_shared_info *sinfo;
+	int frag_offset, frag_len;

net/core/filter.c +5 -0

+
+	frag_offset = skb_frag_off(&sinfo->frags[0]);
+	data_end = xdp->data_end + offset;
+
+	if (offset < 0 && (-offset > frag_offset ||

Quoted 29 lines · Lorenzo Bianconi

Introduce bpf_xdp_adjust_mb_header helper in order to adjust frame headers moving *offset* bytes from/to the second buffer to/from the first one.
This helper can be used to move headers when the hw DMA SG is not able to copy all the headers in the first fragment and split header and data pages.

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

---
 include/uapi/linux/bpf.h       | 25 ++++++++++++----
 net/core/filter.c              | 54 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 26 ++++++++++++----
 3 files changed, 95 insertions(+), 10 deletions(-)

include/uapi/linux/bpf.h +2 -5

index 8dda13880957..c4a6d245619c 100644
@@ -3571,11 +3571,25 @@ union bpf_attr {
  *		value.
  *
  * long bpf_copy_from_user(void *dst, u32 size, const void *user_ptr)
- * 	Description
- * 		Read *size* bytes from user space address *user_ptr* and store
- * 		the data in *dst*. This is a wrapper of copy_from_user().
- * 	Return
- * 		0 on success, or a negative error in case of failure.
+ *	Description
+ *		Read *size* bytes from user space address *user_ptr* and store

include/uapi/linux/bpf.h union bpf_attr { · line 3583

Quoted 8 lines

include/uapi/linux/bpf.h +8 -0

+ *		the data in *dst*. This is a wrapper of copy_from_user().
+ *
+ * long bpf_xdp_adjust_mb_header(struct xdp_buff *xdp_md, int offset)
+ *	Description
+ *		Adjust frame headers moving *offset* bytes from/to the second
+ *		buffer to/from the first one. This helper can be used to move
+ *		headers when the hw DMA SG does not copy all the headers in
+ *		the first fragment.

This is confusing to read. Does this mean I can "move bytes to the second buffer from the first one" or "move bytes from the second buffer to the first one" And what are frame headers? I'm sure I can read below code and work it out, but users reading the header file should be able to parse this.

Also we want to be able to read all data not just headers. Reading the payload of a TCP packet is equally important for many l7 load balancers.

Quoted 34 lines

include/uapi/linux/bpf.h +10 -0

+ *
+ *		A call to this helper is susceptible to change the underlying
+ *		packet buffer. Therefore, at load time, all checks on pointers
+ *		previously done by the verifier are invalidated and must be
+ *		performed again, if the helper is used in combination with
+ *		direct packet access.
+ *
+ *	Return
+ *		0 on success, or a negative error in case of failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -3727,6 +3741,7 @@ union bpf_attr {
 	FN(inode_storage_delete),	\
 	FN(d_path),			\
 	FN(copy_from_user),		\
+	FN(xdp_adjust_mb_header),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper

net/core/filter.c +6 -0

index 47eef9a0be6a..ae6b10cf062d 100644
@@ -3475,6 +3475,57 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
 	.arg2_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_2(bpf_xdp_adjust_mb_header, struct  xdp_buff *, xdp,
+	   int, offset)
+{
+	void *data_hard_end, *data_end;
+	struct skb_shared_info *sinfo;
+	int frag_offset, frag_len;

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

Quoted 10 lines

net/core/filter.c +10 -0

+	u8 *addr;
+
+	if (!xdp->mb)
+		return -EOPNOTSUPP;
+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+
+	frag_len = skb_frag_size(&sinfo->frags[0]);
+	if (offset > frag_len)
+		return -EINVAL;

What if we want data in frags[1] and so on.

Quoted 5 lines

net/core/filter.c +5 -0

+
+	frag_offset = skb_frag_off(&sinfo->frags[0]);
+	data_end = xdp->data_end + offset;
+
+	if (offset < 0 && (-offset > frag_offset ||

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

Quoted 10 lines

net/core/filter.c +10 -0

+			   data_end < xdp->data + ETH_HLEN))
+		return -EINVAL;
+
+	data_hard_end = xdp_data_hard_end(xdp); /* use xdp->frame_sz */
+	if (data_end > data_hard_end)
+		return -EINVAL;
+
+	addr = page_address(skb_frag_page(&sinfo->frags[0])) + frag_offset;
+	if (offset > 0) {
+		memcpy(xdp->data_end, addr, offset);

But this could get expensive for large amount of data? And also limiting because we require the room to do the copy. Presumably the reason we have fargs[1] is because the normal page or half page is in use?

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

Quoted 11 lines

net/core/filter.c +11 -0

+	} else {
+		memcpy(addr + offset, xdp->data_end + offset, -offset);
+		memset(xdp->data_end + offset, 0, -offset);
+	}
+
+	skb_frag_size_sub(&sinfo->frags[0], offset);
+	skb_frag_off_add(&sinfo->frags[0], offset);
+	xdp->data_end = data_end;
+
+	return 0;
+}

So overall I don't see the point of copying bytes from one frag to another. Create an API that adjusts the data pointers and then copies are avoided and manipulating frags is not needed.

Also and even more concerning I think this API requires the driver to populate shinfo. If we use TX_REDIRECT a lot or TX_XMIT this means we need to populate shinfo when its probably not ever used. If our driver is smart L2/L3 headers are in the readable data and prefetched. Writing frags into the end of a page is likely not free.

Did you benchmark this?

In general users of this API should know the bytes they want to fetch. Use an API like this,

bpf_xdp_adjust_bytes(xdp, start, end)

Where xdp is the frame, start is the first byte the user wants and end is the last byte. Then usually you can skip the entire copy part and just move the xdp pointesr around. The ugly case is if the user puts start/end across a frag boundary and a copy is needed. In that case maybe we use end as a hint and not a hard requirement.

The use case I see is I read L2/L3/L4 headers and I need the first N bytes of the payload. I know where the payload starts and I know how many bytes I need so,

  bpf_xdp_adjust_bytes(xdp, payload_offset, bytes_needed);

Then hopefully that is all in one frag. If its not I'll need to do a second helper call. Still nicer than forcing drivers to populate this shinfo I think. If you think I'm wrong try a benchmark. Benchmarks aside I get stuck when data_end and data_hard_end are too close together.

Thanks,
John

Quoted 30 lines

Quoted 29 lines · Lorenzo Bianconi

Introduce bpf_xdp_adjust_mb_header helper in order to adjust frame headers moving *offset* bytes from/to the second buffer to/from the first one.
This helper can be used to move headers when the hw DMA SG is not able to copy all the headers in the first fragment and split header and data pages.

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

---
 include/uapi/linux/bpf.h       | 25 ++++++++++++----
 net/core/filter.c              | 54 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 26 ++++++++++++----
 3 files changed, 95 insertions(+), 10 deletions(-)

include/uapi/linux/bpf.h +2 -5

index 8dda13880957..c4a6d245619c 100644
@@ -3571,11 +3571,25 @@ union bpf_attr {
  *		value.
  *
  * long bpf_copy_from_user(void *dst, u32 size, const void *user_ptr)
- * 	Description
- * 		Read *size* bytes from user space address *user_ptr* and store
- * 		the data in *dst*. This is a wrapper of copy_from_user().
- * 	Return
- * 		0 on success, or a negative error in case of failure.
+ *	Description
+ *		Read *size* bytes from user space address *user_ptr* and store

Quoted 8 lines

include/uapi/linux/bpf.h union bpf_attr { · line 3583

Quoted 8 lines

include/uapi/linux/bpf.h +8 -0

+ *		the data in *dst*. This is a wrapper of copy_from_user().
+ *
+ * long bpf_xdp_adjust_mb_header(struct xdp_buff *xdp_md, int offset)
+ *	Description
+ *		Adjust frame headers moving *offset* bytes from/to the second
+ *		buffer to/from the first one. This helper can be used to move
+ *		headers when the hw DMA SG does not copy all the headers in
+ *		the first fragment.

+ Eric to the discussion

Quoted 5 lines

This is confusing to read. Does this mean I can "move bytes to the second buffer from the first one" or "move bytes from the second buffer to the first one" And what are frame headers? I'm sure I can read below code and work it out, but users reading the header file should be able to parse this.

Our main goal with this helper is to fix the use-case where we request the hw to put L2/L3/L4 headers (and all the TCP options) in the first fragment and TCP data starting from the second fragment (headers split) but for some reasons the hw puts the TCP options in the second fragment (if we understood correctly this issue has been introduced by Eric @ NetDevConf 0x14). bpf_xdp_adjust_mb_header() can fix this use-case moving bytes from the second fragment to the first one (offset > 0) or from the first buffer to the second one (offset < 0).

Quoted 5 lines

Also we want to be able to read all data not just headers. Reading the payload of a TCP packet is equally important for many l7 load balancers.

In order to avoid to slow down performances we require that eBPF sandbox can read/write only buff0 in a xdp multi-buffer. The xdp program can only perform some restricted changes to buff<n> (n >= 1) (e.g. what we did in bpf_xdp_adjust_mb_header()).
You can find the xdp multi-buff design principles here [0][1]

[0] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org
[1] http://people.redhat.com/lbiancon/conference/NetDevConf2020-0x14/add-xdp-on-driver.html - XDP multi-buffers section (slide 40)

Quoted 28 lines

Quoted 28 lines

include/uapi/linux/bpf.h +10 -0

+ *
+ *		A call to this helper is susceptible to change the underlying
+ *		packet buffer. Therefore, at load time, all checks on pointers
+ *		previously done by the verifier are invalidated and must be
+ *		performed again, if the helper is used in combination with
+ *		direct packet access.
+ *
+ *	Return
+ *		0 on success, or a negative error in case of failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -3727,6 +3741,7 @@ union bpf_attr {
 	FN(inode_storage_delete),	\
 	FN(d_path),			\
 	FN(copy_from_user),		\
+	FN(xdp_adjust_mb_header),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper

net/core/filter.c +0 -0

index 47eef9a0be6a..ae6b10cf062d 100644
@@ -3475,6 +3475,57 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
 	.arg2_type	= ARG_ANYTHING,
 };
 

Quoted 10 lines

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

Quoted 10 lines

net/core/filter.c +10 -0

+BPF_CALL_2(bpf_xdp_adjust_mb_header, struct  xdp_buff *, xdp,
+	   int, offset)
+{
+	void *data_hard_end, *data_end;
+	struct skb_shared_info *sinfo;
+	int frag_offset, frag_len;
+	u8 *addr;
+
+	if (!xdp->mb)
+		return -EOPNOTSUPP;

Quoted 30 lines

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

Quoted 6 lines

net/core/filter.c +6 -0

+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+
+	frag_len = skb_frag_size(&sinfo->frags[0]);
+	if (offset > frag_len)
+		return -EINVAL;

What if we want data in frags[1] and so on.

Quoted 5 lines

net/core/filter.c +5 -0

+
+	frag_offset = skb_frag_off(&sinfo->frags[0]);
+	data_end = xdp->data_end + offset;
+
+	if (offset < 0 && (-offset > frag_offset ||

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

Quoted 10 lines

net/core/filter.c +10 -0

+			   data_end < xdp->data + ETH_HLEN))
+		return -EINVAL;
+
+	data_hard_end = xdp_data_hard_end(xdp); /* use xdp->frame_sz */
+	if (data_end > data_hard_end)
+		return -EINVAL;
+
+	addr = page_address(skb_frag_page(&sinfo->frags[0])) + frag_offset;
+	if (offset > 0) {
+		memcpy(xdp->data_end, addr, offset);

But this could get expensive for large amount of data? And also limiting because we require the room to do the copy. Presumably the reason we have fargs[1] is because the normal page or half page is in use?

Quoted 15 lines

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

Quoted 11 lines

net/core/filter.c +11 -0

+	} else {
+		memcpy(addr + offset, xdp->data_end + offset, -offset);
+		memset(xdp->data_end + offset, 0, -offset);
+	}
+
+	skb_frag_size_sub(&sinfo->frags[0], offset);
+	skb_frag_off_add(&sinfo->frags[0], offset);
+	xdp->data_end = data_end;
+
+	return 0;
+}

So overall I don't see the point of copying bytes from one frag to another. Create an API that adjusts the data pointers and then copies are avoided and manipulating frags is not needed.

please see above.

Quoted 8 lines

Also and even more concerning I think this API requires the driver to populate shinfo. If we use TX_REDIRECT a lot or TX_XMIT this means we need to populate shinfo when its probably not ever used. If our driver is smart L2/L3 headers are in the readable data and prefetched. Writing frags into the end of a page is likely not free.

Sorry I did not get what you mean with "populate shinfo" here. We need to properly set shared_info in order to create the xdp multi-buff. Apart of header splits, please consider the main uses-cases for xdp multi-buff are XDP with TSO and Jumbo frames.

Quoted 3 lines

Did you benchmark this?

will do, I need to understand if we can use tiny buffers in mvneta.

Quoted 20 lines

In general users of this API should know the bytes they want to fetch. Use an API like this,

bpf_xdp_adjust_bytes(xdp, start, end)

Where xdp is the frame, start is the first byte the user wants and end is the last byte. Then usually you can skip the entire copy part and just move the xdp pointesr around. The ugly case is if the user puts start/end across a frag boundary and a copy is needed. In that case maybe we use end as a hint and not a hard requirement.

The use case I see is I read L2/L3/L4 headers and I need the first N bytes of the payload. I know where the payload starts and I know how many bytes I need so,

  bpf_xdp_adjust_bytes(xdp, payload_offset, bytes_needed);

Quoted 5 lines

Then hopefully that is all in one frag. If its not I'll need to do a second helper call. Still nicer than forcing drivers to populate this shinfo I think. If you think I'm wrong try a benchmark. Benchmarks aside I get stuck when data_end and data_hard_end are too close together.

IIUC what you mean here is to expose L2/L3/L4 headers + some data to the ebpf program to perform like L7 load-balancing, right?

Let's consider the Jumbo frames use-case (so the data are split in multiple buffers). I can see to issues here:
- since in XDP we can't linearize the buffer if start and end are on the different pages (like we do in bpf_msg_pull_data()), are we ending up in the case where requested data are all in buff0?

- if start and end are in buff<2>, we should report the fragment number to the ebpf program to "fetch" the data. Are we exposing too low-level details to user-space?

Regards,
Lorenzo

Quoted 3 lines

Thanks,
John

Quoted 1 line · Lorenzo Bianconi

Quoted 1 line

Lorenzo Bianconi wrote:

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

+ * Description
+ * Adjust frame headers moving *offset* bytes from/to the second
+ * buffer to/from the first one. This helper can be used to move
+ * headers when the hw DMA SG does not copy all the headers in
+ * the first fragment.

+ Eric to the discussion

Quoted 5 lines

This is confusing to read. Does this mean I can "move bytes to the second buffer from the first one" or "move bytes from the second buffer to the first one" And what are frame headers? I'm sure I can read below code and work it out, but users reading the header file should be able to parse this.

Quoted 7 lines

Our main goal with this helper is to fix the use-case where we request the hw to put L2/L3/L4 headers (and all the TCP options) in the first fragment and TCP data starting from the second fragment (headers split) but for some reasons the hw puts the TCP options in the second fragment (if we understood correctly this issue has been introduced by Eric @ NetDevConf 0x14). bpf_xdp_adjust_mb_header() can fix this use-case moving bytes from the second fragment to the first one (offset > 0) or from the first buffer to the second one (offset < 0).

Ah OK, so the description needs the information about how to use offset then it would have been clear I think. Something like that last line "moving bytes from the second fragment ...."

So this is to fixup header-spit for RX zerocopy? Add that to the commit message then.

Quoted 44 lines

Quoted 6 lines

Also we want to be able to read all data not just headers. Reading the payload of a TCP packet is equally important for many l7 load balancers.

In order to avoid to slow down performances we require that eBPF sandbox can read/write only buff0 in a xdp multi-buffer. The xdp program can only perform some restricted changes to buff<n> (n >= 1) (e.g. what we did in bpf_xdp_adjust_mb_header()).
You can find the xdp multi-buff design principles here [0][1]

[0] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org
[1] http://people.redhat.com/lbiancon/conference/NetDevConf2020-0x14/add-xdp-on-driver.html - XDP multi-buffers section (slide 40)

Quoted 28 lines

Quoted 28 lines

+ *
+ * A call to this helper is susceptible to change the underlying
+ * packet buffer. Therefore, at load time, all checks on pointers
+ * previously done by the verifier are invalidated and must be
+ * performed again, if the helper is used in combination with
+ * direct packet access.
+ *
+ * Return
+ * 0 on success, or a negative error in case of failure.
*/
#define __BPF_FUNC_MAPPER(FN) \

include/uapi/linux/bpf.h +1 -0

 	FN(unspec),			\

@@ -3727,6 +3741,7 @@ union bpf_attr {
 	FN(inode_storage_delete),	\
 	FN(d_path),			\
 	FN(copy_from_user),		\
+	FN(xdp_adjust_mb_header),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper

net/core/filter.c +0 -0

index 47eef9a0be6a..ae6b10cf062d 100644
@@ -3475,6 +3475,57 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
 	.arg2_type	= ARG_ANYTHING,
 };
 

Quoted 10 lines

Quoted 10 lines

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

Quoted 10 lines

net/core/filter.c +10 -0

+BPF_CALL_2(bpf_xdp_adjust_mb_header, struct  xdp_buff *, xdp,
+	   int, offset)
+{
+	void *data_hard_end, *data_end;
+	struct skb_shared_info *sinfo;
+	int frag_offset, frag_len;
+	u8 *addr;
+
+	if (!xdp->mb)
+		return -EOPNOTSUPP;

Not required for this patch necessarily but I think it would be better user experience if instead of EOPNOTSUPP here we did the header split. This would allocate a frag and copy the bytes around as needed. Yes it might be slow if you don't have a frag free in the driver, but if user wants to do header split and their hardware can't do it we would have a way out.

I guess it could be an improvement for later though.

Quoted 48 lines

Quoted 47 lines

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

Quoted 8 lines

net/core/filter.c +6 -0

+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+
+	frag_len = skb_frag_size(&sinfo->frags[0]);
+	if (offset > frag_len)
+		return -EINVAL;

What if we want data in frags[1] and so on.

Quoted 5 lines

net/core/filter.c +5 -0

+
+	frag_offset = skb_frag_off(&sinfo->frags[0]);
+	data_end = xdp->data_end + offset;
+
+	if (offset < 0 && (-offset > frag_offset ||

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

Quoted 10 lines

net/core/filter.c +10 -0

+			   data_end < xdp->data + ETH_HLEN))
+		return -EINVAL;
+
+	data_hard_end = xdp_data_hard_end(xdp); /* use xdp->frame_sz */
+	if (data_end > data_hard_end)
+		return -EINVAL;
+
+	addr = page_address(skb_frag_page(&sinfo->frags[0])) + frag_offset;
+	if (offset > 0) {
+		memcpy(xdp->data_end, addr, offset);

But this could get expensive for large amount of data? And also limiting because we require the room to do the copy. Presumably the reason we have fargs[1] is because the normal page or half page is in use?

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

Quoted 11 lines

net/core/filter.c +11 -0

+	} else {
+		memcpy(addr + offset, xdp->data_end + offset, -offset);
+		memset(xdp->data_end + offset, 0, -offset);
+	}
+
+	skb_frag_size_sub(&sinfo->frags[0], offset);
+	skb_frag_off_add(&sinfo->frags[0], offset);
+	xdp->data_end = data_end;
+
+	return 0;
+}

So overall I don't see the point of copying bytes from one frag to another. Create an API that adjusts the data pointers and then copies are avoided and manipulating frags is not needed.

Quoted 1 line

please see above.

OK it makes more sense with the context. It doesn't have much if anything to do about making data visible to the BPF program. This is about changing the layout of the frags list.

How/when does the header split go wrong on the mvneta device? I guess this is to fix a real bug/issue not some theoritical one? An example in the commit message would make this concrete. Soemthing like, "When using RX zerocopy to mmap data into userspace application if a packet with [all these wild headers] is received rx zerocopy breaks because header split puts headers X in the data frag confusing apps".

Quoted 10 lines

Quoted 9 lines

Also and even more concerning I think this API requires the driver to populate shinfo. If we use TX_REDIRECT a lot or TX_XMIT this means we need to populate shinfo when its probably not ever used. If our driver is smart L2/L3 headers are in the readable data and prefetched. Writing frags into the end of a page is likely not free.

Quoted 4 lines

Sorry I did not get what you mean with "populate shinfo" here. We need to properly set shared_info in order to create the xdp multi-buff. Apart of header splits, please consider the main uses-cases for xdp multi-buff are XDP with TSO and Jumbo frames.

The use case I have in mind is a XDP_TX or XDP_REDIRECT load balancer. I wont know this at the driver level and now I'll have to write into the back of every page with this shinfo just in case. If header split is working I should never need to even touch the page outside the first N bytes that were DMAd and in cache with DDIO. So its extra overhead for something that is unlikely to happen in the LB case.

If you take the simplest possible program that just returns XDP_TX and run a pkt generator against it. I believe (haven't run any tests) that you will see overhead now just from populating this shinfo. I think it needs to only be done when its needed e.g. when user makes this helper call or we need to build the skb and populate the frags there.

I think a smart driver will just keep the frags list in whatever form it has them (rx descriptors?) and push them over to the tx descriptors without having to do extra work with frag lists.

Quoted 6 lines

Quoted 4 lines

Did you benchmark this?

will do, I need to understand if we can use tiny buffers in mvneta.

Why tiny buffers? How does mvneta layout the frags when doing header split? Can we just benchmark what mvneta is doing at the end of this patch series?

Also can you try the basic XDP_TX case mentioned above. I don't want this to degrade existing use cases if at all possible.

Quoted 27 lines

Quoted 26 lines

In general users of this API should know the bytes they want to fetch. Use an API like this,

bpf_xdp_adjust_bytes(xdp, start, end)

Where xdp is the frame, start is the first byte the user wants and end is the last byte. Then usually you can skip the entire copy part and just move the xdp pointesr around. The ugly case is if the user puts start/end across a frag boundary and a copy is needed. In that case maybe we use end as a hint and not a hard requirement.

The use case I see is I read L2/L3/L4 headers and I need the first N bytes of the payload. I know where the payload starts and I know how many bytes I need so,

  bpf_xdp_adjust_bytes(xdp, payload_offset, bytes_needed);

Then hopefully that is all in one frag. If its not I'll need to do a second helper call. Still nicer than forcing drivers to populate this shinfo I think. If you think I'm wrong try a benchmark. Benchmarks aside I get stuck when data_end and data_hard_end are too close together.

Quoted 2 lines

IIUC what you mean here is to expose L2/L3/L4 headers + some data to the ebpf program to perform like L7 load-balancing, right?

Correct, but with extra context I see in this patch you are trying to build an XDP controlled header split. This seems like a different use case from mine.

Quoted 6 lines

Let's consider the Jumbo frames use-case (so the data are split in multiple buffers). I can see to issues here:
- since in XDP we can't linearize the buffer if start and end are on the different pages (like we do in bpf_msg_pull_data()), are we ending up in the case where requested data are all in buff0?

In this case I would expect either the helper returns how many bytes were pulled in, maybe just (start, end_of_frag) or user can find it from data_end pointer. Here end is just a hint.

Quoted 3 lines

- if start and end are in buff<2>, we should report the fragment number to the ebpf program to "fetch" the data. Are we exposing too low-level details to user-space?

Why do you need the frag number? Just say I want bytes (X,Y) if that happens to be on buff<2> let the helper find it.

I think having a helper to read/write any bytes is important and necessary, but the helper implemented in this patch is something else. I get naming is hard what if we called it xdp_header_split().

Quoted 8 lines

Regards,
Lorenzo

Quoted 3 lines

Thanks,
John

Quoted 2 lines

Quoted 1 line · Lorenzo Bianconi

Quoted 1 line

Lorenzo Bianconi wrote:

[...]

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

+ * Description
+ * Adjust frame headers moving *offset* bytes from/to the second
+ * buffer to/from the first one. This helper can be used to move
+ * headers when the hw DMA SG does not copy all the headers in
+ * the first fragment.

+ Eric to the discussion

Quoted 17 lines

Quoted 10 lines

Quoted 10 lines

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

Quoted 10 lines

net/core/filter.c +10 -0

+BPF_CALL_2(bpf_xdp_adjust_mb_header, struct  xdp_buff *, xdp,
+	   int, offset)
+{
+	void *data_hard_end, *data_end;
+	struct skb_shared_info *sinfo;
+	int frag_offset, frag_len;
+	u8 *addr;
+
+	if (!xdp->mb)
+		return -EOPNOTSUPP;

Not required for this patch necessarily but I think it would be better user experience if instead of EOPNOTSUPP here we did the header split. This would allocate a frag and copy the bytes around as needed. Yes it might be slow if you don't have a frag free in the driver, but if user wants to do header split and their hardware can't do it we would have a way out.

Quoted 1 line

I guess it could be an improvement for later though.

I have no a strong opinion on this, I did it in this way to respect the rule "we do not allocate memory for XDP".

@Jesper, David: thoughts?

Quoted 8 lines

How/when does the header split go wrong on the mvneta device? I guess this is to fix a real bug/issue not some theoritical one? An example in the commit message would make this concrete. Soemthing like, "When using RX zerocopy to mmap data into userspace application if a packet with [all these wild headers] is received rx zerocopy breaks because header split puts headers X in the data frag confusing apps".

This issue does not occur with mvneta since the driver is not capable of performing header split AFAIK. The helper has been introduced to cover the "issue" reported by Eric in his NetDevConf presentation. In order to test the helper I modified the mventa rx napi loop in a controlled way (this patch can't be sent upstream, it is for testing only :))
I will improve commit message in v3.

Quoted 16 lines

Quoted 11 lines

Quoted 10 lines

Also and even more concerning I think this API requires the driver to populate shinfo. If we use TX_REDIRECT a lot or TX_XMIT this means we need to populate shinfo when its probably not ever used. If our driver is smart L2/L3 headers are in the readable data and prefetched. Writing frags into the end of a page is likely not free.

Quoted 4 lines

Sorry I did not get what you mean with "populate shinfo" here. We need to properly set shared_info in order to create the xdp multi-buff. Apart of header splits, please consider the main uses-cases for xdp multi-buff are XDP with TSO and Jumbo frames.

Quoted 6 lines

The use case I have in mind is a XDP_TX or XDP_REDIRECT load balancer. I wont know this at the driver level and now I'll have to write into the back of every page with this shinfo just in case. If header split is working I should never need to even touch the page outside the first N bytes that were DMAd and in cache with DDIO. So its extra overhead for something that is unlikely to happen in the LB case.

So far the skb_shared_info in constructed in mvneta only if the hw splits the received data in multiple buffers (so if the MTU is greater than 1 PAGE, please see comments below). Moreover the shared_info is present only in the first buffer.

Quoted 8 lines

If you take the simplest possible program that just returns XDP_TX and run a pkt generator against it. I believe (haven't run any tests) that you will see overhead now just from populating this shinfo. I think it needs to only be done when its needed e.g. when user makes this helper call or we need to build the skb and populate the frags there.

sure, I will carry out some tests.

Quoted 5 lines

I think a smart driver will just keep the frags list in whatever form it has them (rx descriptors?) and push them over to the tx descriptors without having to do extra work with frag lists.

I think there are many use-cases where we want to have this info available in xdp_buff/xdp_frame. E.g: let's consider the following Jumbo frame example:
- MTU > 1 PAGE (so we the driver will split the received data in multiple rx descriptors)
- the driver performs a XDP_REDIRECT to a veth or cpumap

Relying on the proposed architecture we could enable GRO in veth or cpumap I guess since we can build a non-linear skb from the xdp multi-buff, right?

Quoted 11 lines

Quoted 7 lines

Quoted 5 lines

Did you benchmark this?

will do, I need to understand if we can use tiny buffers in mvneta.

Why tiny buffers? How does mvneta layout the frags when doing header split? Can we just benchmark what mvneta is doing at the end of this patch series?

for the moment mvneta can split the received data when the previous buffer is full (e.g. when we the first page is completely written). I want to explore if I can set a tiny buffer (e.g. 128B) as max received buffer to run some performance tests and have some "comparable" results respect to the ones I got when I added XDP support to mvneta.

Quoted 5 lines

Also can you try the basic XDP_TX case mentioned above. I don't want this to degrade existing use cases if at all possible.

sure, will do.

Quoted 13 lines · Lorenzo Bianconi

Quoted 2 lines

Quoted 1 line · Lorenzo Bianconi

Quoted 1 line

Lorenzo Bianconi wrote:

[...]

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

+ * Description
+ * Adjust frame headers moving *offset* bytes from/to the second
+ * buffer to/from the first one. This helper can be used to move
+ * headers when the hw DMA SG does not copy all the headers in
+ * the first fragment.

+ Eric to the discussion

[...]

Quoted 17 lines

Quoted 17 lines

Quoted 10 lines

Quoted 10 lines

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

Quoted 10 lines

net/core/filter.c +10 -0

+BPF_CALL_2(bpf_xdp_adjust_mb_header, struct  xdp_buff *, xdp,
+	   int, offset)
+{
+	void *data_hard_end, *data_end;
+	struct skb_shared_info *sinfo;
+	int frag_offset, frag_len;
+	u8 *addr;
+
+	if (!xdp->mb)
+		return -EOPNOTSUPP;

Not required for this patch necessarily but I think it would be better user experience if instead of EOPNOTSUPP here we did the header split. This would allocate a frag and copy the bytes around as needed. Yes it might be slow if you don't have a frag free in the driver, but if user wants to do header split and their hardware can't do it we would have a way out.

Quoted 6 lines

Quoted 1 line

I guess it could be an improvement for later though.

I have no a strong opinion on this, I did it in this way to respect the rule "we do not allocate memory for XDP".

@Jesper, David: thoughts?

Consider adding a flags field to the helper so we could do this later with a flag. Then users who want the alloc can set the flag and get it.

[...]

Quoted 9 lines

Quoted 8 lines

How/when does the header split go wrong on the mvneta device? I guess this is to fix a real bug/issue not some theoritical one? An example in the commit message would make this concrete. Soemthing like, "When using RX zerocopy to mmap data into userspace application if a packet with [all these wild headers] is received rx zerocopy breaks because header split puts headers X in the data frag confusing apps".

Quoted 6 lines

This issue does not occur with mvneta since the driver is not capable of performing header split AFAIK. The helper has been introduced to cover the "issue" reported by Eric in his NetDevConf presentation. In order to test the helper I modified the mventa rx napi loop in a controlled way (this patch can't be sent upstream, it is for testing only :))
I will improve commit message in v3.

Ah ok so really there is no users for the helper then IMO just drop the patch until we have a user then.

Quoted 24 lines

Quoted 17 lines

Quoted 12 lines

Quoted 11 lines

Also and even more concerning I think this API requires the driver to populate shinfo. If we use TX_REDIRECT a lot or TX_XMIT this means we need to populate shinfo when its probably not ever used. If our driver is smart L2/L3 headers are in the readable data and prefetched. Writing frags into the end of a page is likely not free.

Quoted 4 lines

Sorry I did not get what you mean with "populate shinfo" here. We need to properly set shared_info in order to create the xdp multi-buff. Apart of header splits, please consider the main uses-cases for xdp multi-buff are XDP with TSO and Jumbo frames.

Quoted 6 lines

The use case I have in mind is a XDP_TX or XDP_REDIRECT load balancer. I wont know this at the driver level and now I'll have to write into the back of every page with this shinfo just in case. If header split is working I should never need to even touch the page outside the first N bytes that were DMAd and in cache with DDIO. So its extra overhead for something that is unlikely to happen in the LB case.

Quoted 4 lines

So far the skb_shared_info in constructed in mvneta only if the hw splits the received data in multiple buffers (so if the MTU is greater than 1 PAGE, please see comments below). Moreover the shared_info is present only in the first buffer.

Still in a normal L2/L3/L4 use case I expect all the headers you need to be in the fist buffer so its unlikely for use cases that send most traffic via XDP_TX for example to ever need the extra info. In these cases I think you are paying some penalty for having to do the work of populating the shinfo. Maybe its measurable maybe not I'm not sure.

Also if we make it required for multi-buffer than we also need the shinfo on 40gbps or 100gbps nics and now even small costs matter.

Quoted 11 lines

Quoted 9 lines

If you take the simplest possible program that just returns XDP_TX and run a pkt generator against it. I believe (haven't run any tests) that you will see overhead now just from populating this shinfo. I think it needs to only be done when its needed e.g. when user makes this helper call or we need to build the skb and populate the frags there.

sure, I will carry out some tests.

Thanks!

Quoted 6 lines

Quoted 5 lines

I think a smart driver will just keep the frags list in whatever form it has them (rx descriptors?) and push them over to the tx descriptors without having to do extra work with frag lists.

Quoted 8 lines

I think there are many use-cases where we want to have this info available in xdp_buff/xdp_frame. E.g: let's consider the following Jumbo frame example:
- MTU > 1 PAGE (so we the driver will split the received data in multiple rx descriptors)
- the driver performs a XDP_REDIRECT to a veth or cpumap

Relying on the proposed architecture we could enable GRO in veth or cpumap I guess since we can build a non-linear skb from the xdp multi-buff, right?

I'm not disputing there are use-cases. But, I'm trying to see if we can cover those without introducing additional latency in other cases. Hence the extra benchmarks request ;)

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 5 lines

Did you benchmark this?

will do, I need to understand if we can use tiny buffers in mvneta.

Why tiny buffers? How does mvneta layout the frags when doing header split? Can we just benchmark what mvneta is doing at the end of this patch series?

Quoted 5 lines

for the moment mvneta can split the received data when the previous buffer is full (e.g. when we the first page is completely written). I want to explore if I can set a tiny buffer (e.g. 128B) as max received buffer to run some performance tests and have some "comparable" results respect to the ones I got when I added XDP support to mvneta.

OK would be great.

Quoted 7 lines

Quoted 5 lines

Also can you try the basic XDP_TX case mentioned above. I don't want this to degrade existing use cases if at all possible.

sure, will do.

Thanks!

Quoted 17 lines

Quoted 13 lines · Lorenzo Bianconi

Quoted 2 lines

Quoted 1 line · Lorenzo Bianconi

Quoted 1 line

Lorenzo Bianconi wrote:

[...]

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

+ * Description
+ * Adjust frame headers moving *offset* bytes from/to the second
+ * buffer to/from the first one. This helper can be used to move
+ * headers when the hw DMA SG does not copy all the headers in
+ * the first fragment.

+ Eric to the discussion

[...]

Quoted 11 lines

Still in a normal L2/L3/L4 use case I expect all the headers you need to be in the fist buffer so its unlikely for use cases that send most traffic via XDP_TX for example to ever need the extra info. In these cases I think you are paying some penalty for having to do the work of populating the shinfo. Maybe its measurable maybe not I'm not sure.

Also if we make it required for multi-buffer than we also need the shinfo on 40gbps or 100gbps nics and now even small costs matter.

Now I realized I used the word "split" in a not clear way here,
I apologize for that.
What I mean is not related "header" split, I am referring to the case where the hw is configured with a given rx buffer size (e.g. 1 PAGE) and we have set a higher MTU/max received size (e.g. 9K).
In this case the hw will "split" the jumbo received frame over multiple rx buffers/descriptors. Populating the "xdp_shared_info" we will forward this layout info to the eBPF sandbox and to a remote driver/cpu.
Please note this use case is not currently covered by XDP so if we develop it a proper way I guess we should not get any performance hit for the legacy single-buffer mode since we will not populate the shared_info for it (I think you refer to the "legacy" use-case in your "normal L2/L3/L4" example, right?)
Anyway I will run some tests to verify the performances for the single buffer use-case are not hit.

Regards,
Lorenzo

Quoted 64 lines

Quoted 4 lines

Quoted 4 lines

Quoted 8 lines

Quoted 6 lines

If you take the simplest possible program that just returns XDP_TX and run a pkt generator against it. I believe (haven't run any tests) that you will see overhead now just from populating this shinfo. I think it needs to only be done when its needed e.g. when user makes this helper call or we need to build the skb and populate the frags there.

sure, I will carry out some tests.

Thanks!

Quoted 6 lines

Quoted 5 lines

I think a smart driver will just keep the frags list in whatever form it has them (rx descriptors?) and push them over to the tx descriptors without having to do extra work with frag lists.

Quoted 8 lines

I think there are many use-cases where we want to have this info available in xdp_buff/xdp_frame. E.g: let's consider the following Jumbo frame example:
- MTU > 1 PAGE (so we the driver will split the received data in multiple rx descriptors)
- the driver performs a XDP_REDIRECT to a veth or cpumap

Relying on the proposed architecture we could enable GRO in veth or cpumap I guess since we can build a non-linear skb from the xdp multi-buff, right?

I'm not disputing there are use-cases. But, I'm trying to see if we can cover those without introducing additional latency in other cases. Hence the extra benchmarks request ;)

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 5 lines

Did you benchmark this?

will do, I need to understand if we can use tiny buffers in mvneta.

Why tiny buffers? How does mvneta layout the frags when doing header split? Can we just benchmark what mvneta is doing at the end of this patch series?

Quoted 5 lines

for the moment mvneta can split the received data when the previous buffer is full (e.g. when we the first page is completely written). I want to explore if I can set a tiny buffer (e.g. 128B) as max received buffer to run some performance tests and have some "comparable" results respect to the ones I got when I added XDP support to mvneta.

OK would be great.

Quoted 7 lines

Quoted 5 lines

Also can you try the basic XDP_TX case mentioned above. I don't want this to degrade existing use cases if at all possible.

sure, will do.

Thanks!

From: Sameeh Jubran <[email protected]>

The implementation is based on this [0] draft by Jesper D. Brouer.

Provided two new helpers:

* bpf_xdp_get_frag_count()
* bpf_xdp_get_frags_total_size()

[0] xdp mb design - https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org

Signed-off-by Sameeh Jubran <[email protected]>

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

---
 include/uapi/linux/bpf.h       | 14 ++++++++++++
 net/core/filter.c              | 39 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 14 ++++++++++++
 3 files changed, 67 insertions(+)

include/uapi/linux/bpf.h +14 -0

index c4a6d245619c..53db75095306 100644
@@ -3590,6 +3590,18 @@ union bpf_attr {
  *
  *	Return
  *		0 on success, or a negative error in case of failure.
+ *
+ * int bpf_xdp_get_frag_count(struct xdp_buff *xdp_md)
+ *	Description
+ *		Get the total number of frags for a given packet.
+ *	Return
+ *		The number of frags
+ *
+ * int bpf_xdp_get_frags_total_size(struct xdp_buff *xdp_md)
+ *	Description
+ *		Get the total size of frags for a given packet.
+ *	Return
+ *		The total size of frags for a given packet.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -3742,6 +3754,8 @@ union bpf_attr {
 	FN(d_path),			\
 	FN(copy_from_user),		\
 	FN(xdp_adjust_mb_header),	\
+	FN(xdp_get_frag_count),		\
+	FN(xdp_get_frags_total_size),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper

net/core/filter.c +15 -0

index ae6b10cf062d..ba058fc16440 100644
@@ -3526,6 +3526,41 @@ static const struct bpf_func_proto bpf_xdp_adjust_mb_header_proto = {
 	.arg2_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_1(bpf_xdp_get_frag_count, struct  xdp_buff*, xdp)
+{
+	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
+
+	return xdp->mb ? sinfo->nr_frags : 0;
+}
+
+const struct bpf_func_proto bpf_xdp_get_frag_count_proto = {
+	.func		= bpf_xdp_get_frag_count,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_CTX,
+};
+
+BPF_CALL_1(bpf_xdp_get_frags_total_size, struct  xdp_buff*, xdp)

net/core/filter.c +14 -0

+	return size;
+}
+
+const struct bpf_func_proto bpf_xdp_get_frags_total_size_proto = {
+	.func		= bpf_xdp_get_frags_total_size,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_CTX,
+};
+
 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 */

@@ -6889,6 +6924,10 @@ xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_xdp_adjust_tail_proto;
 	case BPF_FUNC_xdp_adjust_mb_header:
 		return &bpf_xdp_adjust_mb_header_proto;
+	case BPF_FUNC_xdp_get_frag_count:
+		return &bpf_xdp_get_frag_count_proto;
+	case BPF_FUNC_xdp_get_frags_total_size:
+		return &bpf_xdp_get_frags_total_size_proto;
 	case BPF_FUNC_fib_lookup:
 		return &bpf_xdp_fib_lookup_proto;
 #ifdef CONFIG_INET

tools/include/uapi/linux/bpf.h +14 -0

index 392d52a2ecef..dd4669096cbb 100644
@@ -3591,6 +3591,18 @@ union bpf_attr {
  *
  *	Return
  *		0 on success, or a negative error in case of failure.
+ *
+ * int bpf_xdp_get_frag_count(struct xdp_buff *xdp_md)
+ *	Description
+ *		Get the total number of frags for a given packet.
+ *	Return
+ *		The number of frags
+ *
+ * int bpf_xdp_get_frags_total_size(struct xdp_buff *xdp_md)
+ *	Description
+ *		Get the total size of frags for a given packet.
+ *	Return
+ *		The total size of frags for a given packet.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -3743,6 +3755,8 @@ union bpf_attr {
 	FN(d_path),			\
 	FN(copy_from_user),		\
 	FN(xdp_adjust_mb_header),	\
+	FN(xdp_get_frag_count),		\
+	FN(xdp_get_frags_total_size),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper

Signature · 1 line

2.26.2

Quoted 26 lines

From: Sameeh Jubran <[email protected]>

The implementation is based on this [0] draft by Jesper D. Brouer.

Provided two new helpers:

* bpf_xdp_get_frag_count()
* bpf_xdp_get_frags_total_size()

[0] xdp mb design - https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org

Signed-off-by Sameeh Jubran <[email protected]>

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

---
 include/uapi/linux/bpf.h       | 14 ++++++++++++
 net/core/filter.c              | 39 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 14 ++++++++++++
 3 files changed, 67 insertions(+)

include/uapi/linux/bpf.h +0 -0

index c4a6d245619c..53db75095306 100644
@@ -3590,6 +3590,18 @@ union bpf_attr {
  *
  *	Return
  *		0 on success, or a negative error in case of failure.

Quoted 1 line

+

I only quickly jumped through series and IMHO this helper should be rewritten/optimized in a way that we bail out as early as possible if !xdp->mb as the rest of the code on that condition will do nothing and i'm not sure if compiler would optimize it.

	struct skb_shared_info *sinfo;
	int nfrags, i;
	int size = 0;

	if (!xdp->mb)
		return 0;

	sinfo = xdp_get_shared_info_from_buff(xdp);

	nfrags = min(sinfo->nr_frags, MAX_SKB_FRAGS);

	for (i = 0; i < nfrags; i++)
		size += skb_frag_size(&sinfo->frags[i]);

	return size;

Thoughts?

Quoted 61 lines

include/uapi/linux/bpf.h +14 -0

+	return size;
+}
+
+const struct bpf_func_proto bpf_xdp_get_frags_total_size_proto = {
+	.func		= bpf_xdp_get_frags_total_size,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_CTX,
+};
+
 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 */

@@ -6889,6 +6924,10 @@ xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_xdp_adjust_tail_proto;
 	case BPF_FUNC_xdp_adjust_mb_header:
 		return &bpf_xdp_adjust_mb_header_proto;
+	case BPF_FUNC_xdp_get_frag_count:
+		return &bpf_xdp_get_frag_count_proto;
+	case BPF_FUNC_xdp_get_frags_total_size:
+		return &bpf_xdp_get_frags_total_size_proto;
 	case BPF_FUNC_fib_lookup:
 		return &bpf_xdp_fib_lookup_proto;
 #ifdef CONFIG_INET

tools/include/uapi/linux/bpf.h +14 -0

index 392d52a2ecef..dd4669096cbb 100644
@@ -3591,6 +3591,18 @@ union bpf_attr {
  *
  *	Return
  *		0 on success, or a negative error in case of failure.
+ *
+ * int bpf_xdp_get_frag_count(struct xdp_buff *xdp_md)
+ *	Description
+ *		Get the total number of frags for a given packet.
+ *	Return
+ *		The number of frags
+ *
+ * int bpf_xdp_get_frags_total_size(struct xdp_buff *xdp_md)
+ *	Description
+ *		Get the total size of frags for a given packet.
+ *	Return
+ *		The total size of frags for a given packet.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -3743,6 +3755,8 @@ union bpf_attr {
 	FN(d_path),			\
 	FN(copy_from_user),		\
 	FN(xdp_adjust_mb_header),	\
+	FN(xdp_get_frag_count),		\
+	FN(xdp_get_frags_total_size),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper

Signature · 1 line

2.26.2

For the moment we have not implemented any self-test for the introduced the bpf helpers. We can address this in a follow up series if the proposed approach is accepted.

Quoted 3 lines

For the moment we have not implemented any self-test for the introduced the bpf helpers. We can address this in a follow up series if the proposed approach is accepted.

selftest has to be part of the same patch set.

- Finalize XDP multi-buffer support for mvneta driver introducing the capability to map non-linear buffers on tx side.
- Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer to specify if shared_info area has been properly initialized.

- Initialize multi-buffer bit (mb) to 0 in all XDP-capable drivers.
- Add multi-buff support to xdp_return_{buff/frame} utility routines.
- Introduce bpf_xdp_adjust_mb_header helper to adjust frame headers moving *offset* bytes from/to the second buffer to/from the first one. This helper can be used to move headers when the hw DMA SG is not able to copy all the headers in the first fragment and split header and data pages. A possible use case for bpf_xdp_adjust_mb_header is described here [0]

- Introduce bpf_xdp_get_frag_count and bpf_xdp_get_frags_total_size helpers to report the total number/size of frags for a given xdp multi-buff.

XDP multi-buffer design principles are described here [1]
For the moment we have not implemented any self-test for the introduced the bpf helpers. We can address this in a follow up series if the proposed approach is accepted.

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

Lorenzo Bianconi (7):
xdp: introduce mb in xdp_buff/xdp_frame
xdp: initialize xdp_buff mb bit to 0 in all XDP drivers
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
bpf: helpers: add bpf_xdp_adjust_mb_header helper
net: mvneta: enable jumbo frames for XDP

Sameeh Jubran (2):
bpf: helpers: add multibuffer support samples/bpf: add bpf program that uses xdp mb helpers

 drivers/net/ethernet/amazon/ena/ena_netdev.c  |   1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c |   1 +
 .../net/ethernet/cavium/thunder/nicvf_main.c  |   1 +
 .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  |   1 +
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   |   1 +
 drivers/net/ethernet/intel/ice/ice_txrx.c     |   1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   1 +
 .../net/ethernet/intel/ixgbevf/ixgbevf_main.c |   1 +
 drivers/net/ethernet/marvell/mvneta.c         | 126 ++++++------
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   |   1 +
 drivers/net/ethernet/mellanox/mlx4/en_rx.c    |   1 +
 .../net/ethernet/mellanox/mlx5/core/en_rx.c   |   1 +
 .../ethernet/netronome/nfp/nfp_net_common.c   |   1 +
 drivers/net/ethernet/qlogic/qede/qede_fp.c    |   1 +
 drivers/net/ethernet/sfc/rx.c                 |   1 +
 drivers/net/ethernet/socionext/netsec.c       |   1 +
 drivers/net/ethernet/ti/cpsw.c                |   1 +
 drivers/net/ethernet/ti/cpsw_new.c            |   1 +
 drivers/net/hyperv/netvsc_bpf.c               |   1 +
 drivers/net/tun.c                             |   2 +
 drivers/net/veth.c                            |   1 +
 drivers/net/virtio_net.c                      |   2 +
 drivers/net/xen-netfront.c                    |   1 +
 include/net/xdp.h                             |  26 ++-
 include/uapi/linux/bpf.h                      |  39 +++-
 net/core/dev.c                                |   1 +
 net/core/filter.c                             |  93 +++++++++
 net/core/xdp.c                                |  40 ++++
 samples/bpf/Makefile                          |   3 +
 samples/bpf/xdp_mb_kern.c                     |  68 +++++++
 samples/bpf/xdp_mb_user.c                     | 182 ++++++++++++++++++
 tools/include/uapi/linux/bpf.h                |  40 +++-
 32 files changed, 572 insertions(+), 70 deletions(-)
 create mode 100644 samples/bpf/xdp_mb_kern.c
 create mode 100644 samples/bpf/xdp_mb_user.c

Signature · 1 line

2.26.2

Quoted 4 lines · Lorenzo Bianconi

- Finalize XDP multi-buffer support for mvneta driver introducing the capability to map non-linear buffers on tx side.
- Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer to specify if shared_info area has been properly initialized.

Quoted 8 lines

- Initialize multi-buffer bit (mb) to 0 in all XDP-capable drivers.
- Add multi-buff support to xdp_return_{buff/frame} utility routines.
- Introduce bpf_xdp_adjust_mb_header helper to adjust frame headers moving *offset* bytes from/to the second buffer to/from the first one. This helper can be used to move headers when the hw DMA SG is not able to copy all the headers in the first fragment and split header and data pages. A possible use case for bpf_xdp_adjust_mb_header is described here [0]

Are those slides available anywhere? [0] is just a link to the abstract.

Quoted 8 lines

- Introduce bpf_xdp_get_frag_count and bpf_xdp_get_frags_total_size helpers to report the total number/size of frags for a given xdp multi-buff.

XDP multi-buffer design principles are described here [1]
For the moment we have not implemented any self-test for the introduced the bpf helpers. We can address this in a follow up series if the proposed approach is accepted.

Will need to include selftests with series.

Quoted 67 lines

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

Lorenzo Bianconi (7):
xdp: introduce mb in xdp_buff/xdp_frame
xdp: initialize xdp_buff mb bit to 0 in all XDP drivers
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
bpf: helpers: add bpf_xdp_adjust_mb_header helper
net: mvneta: enable jumbo frames for XDP

Sameeh Jubran (2):
bpf: helpers: add multibuffer support samples/bpf: add bpf program that uses xdp mb helpers

 drivers/net/ethernet/amazon/ena/ena_netdev.c  |   1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c |   1 +
 .../net/ethernet/cavium/thunder/nicvf_main.c  |   1 +
 .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  |   1 +
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   |   1 +
 drivers/net/ethernet/intel/ice/ice_txrx.c     |   1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   1 +
 .../net/ethernet/intel/ixgbevf/ixgbevf_main.c |   1 +
 drivers/net/ethernet/marvell/mvneta.c         | 126 ++++++------
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   |   1 +
 drivers/net/ethernet/mellanox/mlx4/en_rx.c    |   1 +
 .../net/ethernet/mellanox/mlx5/core/en_rx.c   |   1 +
 .../ethernet/netronome/nfp/nfp_net_common.c   |   1 +
 drivers/net/ethernet/qlogic/qede/qede_fp.c    |   1 +
 drivers/net/ethernet/sfc/rx.c                 |   1 +
 drivers/net/ethernet/socionext/netsec.c       |   1 +
 drivers/net/ethernet/ti/cpsw.c                |   1 +
 drivers/net/ethernet/ti/cpsw_new.c            |   1 +
 drivers/net/hyperv/netvsc_bpf.c               |   1 +
 drivers/net/tun.c                             |   2 +
 drivers/net/veth.c                            |   1 +
 drivers/net/virtio_net.c                      |   2 +
 drivers/net/xen-netfront.c                    |   1 +
 include/net/xdp.h                             |  26 ++-
 include/uapi/linux/bpf.h                      |  39 +++-
 net/core/dev.c                                |   1 +
 net/core/filter.c                             |  93 +++++++++
 net/core/xdp.c                                |  40 ++++
 samples/bpf/Makefile                          |   3 +
 samples/bpf/xdp_mb_kern.c                     |  68 +++++++
 samples/bpf/xdp_mb_user.c                     | 182 ++++++++++++++++++
 tools/include/uapi/linux/bpf.h                |  40 +++-
 32 files changed, 572 insertions(+), 70 deletions(-)
 create mode 100644 samples/bpf/xdp_mb_kern.c
 create mode 100644 samples/bpf/xdp_mb_user.c

Signature · 1 line

2.26.2