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 +0 -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*/

include/net/xdp.h +1 -0

+	u32 frame_sz:31; /* frame size to deduce 

data_hard_end/reserved tailroom*/

include/net/xdp.h +1 -0

+	u32 mb:1; /* xdp non-linear buffer */
 };
 
 /* Reserve memory area at end-of data area.

include/net/xdp.h +0 -0

 	/* Lifetime of xdp_rxq_info is limited to NAPI/enqueue 
 time,
 	 * while mem info is valid on remote CPU.

include/net/xdp.h +1 -0

 	 */

@@ -141,6 +143,7 @@ void xdp_convert_frame_to_buff(struct 
 xdp_frame *frame, struct xdp_buff *xdp)
 	xdp->data_end = frame->data + frame->len;
 	xdp->data_meta = frame->data - frame->metasize;
 	xdp->frame_sz = frame->frame_sz;
+	xdp->mb = frame->mb;
 }
 

static inline

include/net/xdp.h +1 -0

@@ -167,6 +170,7 @@ int xdp_update_frame_from_buff(struct 
 xdp_buff *xdp,
 	xdp_frame->headroom = headroom - sizeof(*xdp_frame);
 	xdp_frame->metasize = metasize;
 	xdp_frame->frame_sz = xdp->frame_sz;
+	xdp_frame->mb = xdp->mb;
 
 	return 0;

}

net/core/xdp.c +1 -0

index 48aba933a5a8..884f140fc3be 100644
@@ -454,6 +454,7 @@ struct xdp_frame 
 *xdp_convert_zc_to_xdp_frame(struct xdp_buff *xdp)
 	xdpf->headroom = 0;
 	xdpf->metasize = metasize;
 	xdpf->frame_sz = PAGE_SIZE;
+	xdpf->mb = xdp->mb;
 	xdpf->mem.type = MEM_TYPE_PAGE_ORDER0;
 

xsk_buff_free(xdp);

Quoted 15 lines

include/net/xdp.h +0 -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*/

include/net/xdp.h +1 -0

+	u32 frame_sz:31; /* frame size to deduce 

data_hard_end/reserved tailroom*/

include/net/xdp.h +1 -0

+	u32 mb:1; /* xdp non-linear buffer */
 };

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

Quoted 8 lines

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

include/net/xdp.h +2 -1

@@ -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 */  

Although this issue wasn't introduced with this patch, why not make frame_sz field to be the same size in xdp_buff and xdp_frame ?

Quoted 15 lines · On Sun, 23 Aug 2020 17:08:30 +0300 Shay Agroskin <[email protected]> wrote:

Quoted 15 lines

include/net/xdp.h +0 -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*/

include/net/xdp.h +1 -0

+	u32 frame_sz:31; /* frame size to deduce 

data_hard_end/reserved tailroom*/

include/net/xdp.h +1 -0

+	u32 mb:1; /* xdp non-linear buffer */
 };

Quoted 10 lines

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

Quoted 8 lines

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

include/net/xdp.h +2 -1

@@ -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 */  

Although this issue wasn't introduced with this patch, why not make frame_sz field to be the same size in xdp_buff and xdp_frame ?

This is all about struct layout and saving memory size, due to cacheline access. Please read up on this and use the tool pahole to inspect the struct memory layout.

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

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

---
 drivers/net/ethernet/marvell/mvneta.c | 3 +++
 1 file changed, 3 insertions(+)

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

index 832bbb8b05c8..36a3defa63fa 100644
@@ -2170,11 +2170,14 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
 	       struct bpf_prog *prog, struct xdp_buff *xdp,
 	       u32 frame_sz, struct mvneta_stats *stats)

Quoted 20 lines · On Wed, 19 Aug 2020 15:13:48 +0200 Lorenzo Bianconi <[email protected]> wrote:

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

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

---
 drivers/net/ethernet/marvell/mvneta.c | 3 +++
 1 file changed, 3 insertions(+)

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

index 832bbb8b05c8..36a3defa63fa 100644
@@ -2170,11 +2170,14 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
 	       struct bpf_prog *prog, struct xdp_buff *xdp,
 	       u32 frame_sz, struct mvneta_stats *stats)
 {
+	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
 	unsigned int len, data_len, sync;
 	u32 ret, act;
 

drivers/net/ethernet/marvell/mvneta.c mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq, · line 2181

Quoted 5 lines

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

 	len = xdp->data_end - xdp->data_hard_start - pp->rx_offset_correction;
 	data_len = xdp->data_end - xdp->data;
+
+	xdp->mb = !!sinfo->nr_frags;
 	act = bpf_prog_run_xdp(prog, xdp);

Reading the memory sinfo->nr_frags could be a performance issue for our baseline case of no-multi-buffer. As you are reading a cache-line that you don't need to (and driver have not touch yet).

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

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

---
 drivers/net/ethernet/marvell/mvneta.c | 3 +++
 1 file changed, 3 insertions(+)

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

index 832bbb8b05c8..36a3defa63fa 100644
@@ -2170,11 +2170,14 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
 	       struct bpf_prog *prog, struct xdp_buff *xdp,

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

 	act = bpf_prog_run_xdp(prog, xdp);
 
 	/* Due xdp_adjust_tail: DMA sync for_device cover max len CPU touch */

Signature · 1 line

2.26.2

Quoted 20 lines · Lorenzo Bianconi

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

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

---
 drivers/net/ethernet/marvell/mvneta.c | 3 +++
 1 file changed, 3 insertions(+)

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

index 832bbb8b05c8..36a3defa63fa 100644
@@ -2170,11 +2170,14 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
 	       struct bpf_prog *prog, struct xdp_buff *xdp,
 	       u32 frame_sz, struct mvneta_stats *stats)
 {
+	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
 	unsigned int len, data_len, sync;
 	u32 ret, act;
 

drivers/net/ethernet/marvell/mvneta.c mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq, · line 2180

Quoted 4 lines

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

 	len = xdp->data_end - xdp->data_hard_start - pp->rx_offset_correction;
 	data_len = xdp->data_end - xdp->data;
+
+	xdp->mb = !!sinfo->nr_frags;

But this set is not utilizing it from BPF side in any way. Personally I would like to see this as a part of work where BPF program would actually be taught how to rely on xdp->mb. Especially after John's comment in other patch.

Quoted 6 lines

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

 	act = bpf_prog_run_xdp(prog, xdp);
 
 	/* Due xdp_adjust_tail: DMA sync for_device cover max len CPU touch */

Signature · 1 line

2.26.2

net/core/xdp.c +8 -0

+	if (!xdpf->mb)
+		return;
+
+	sinfo = xdp_get_shared_info_from_frame(xdpf);
+	for (i = 0; i < sinfo->nr_frags; i++) {
+		struct page *page = skb_frag_page(&sinfo->frags[i]);
+
+		__xdp_return(page_address(page), &xdpf->mem, false);

net/core/xdp.c +8 -0

+	if (!xdpf->mb)
+		return;
+
+	sinfo = xdp_get_shared_info_from_frame(xdpf);
+	for (i = 0; i < sinfo->nr_frags; i++) {
+		struct page *page = skb_frag_page(&sinfo->frags[i]);
+
+		__xdp_return(page_address(page), &xdpf->mem, true);

net/core/xdp.c +9 -0

+	if (!xdp->mb)
+		return;
+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+	for (i = 0; i < sinfo->nr_frags; i++) {
+		struct page *page = skb_frag_page(&sinfo->frags[i]);
+
+		__xdp_return(page_address(page), &xdp->rxq->mem, true);
+	}
 }

Quoted 6 lines · On Wed, 19 Aug 2020 15:13:49 +0200 Lorenzo Bianconi <[email protected]> wrote:

net/core/xdp.c +0 -0

index 884f140fc3be..006b24b5d276 100644
@@ -370,19 +370,55 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct)
 

net/core/xdp.c static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct) · line 376

Quoted 6 lines

net/core/xdp.c +3 -0

 void xdp_return_frame(struct xdp_frame *xdpf)
 {
+	struct skb_shared_info *sinfo;
+	int i;
+
 	__xdp_return(xdpf->data, &xdpf->mem, false);

There is a use-after-free race here. The xdpf->data contains the shared_info (xdp_get_shared_info_from_frame(xdpf)). Thus you cannot free/return the page and use this data area below.

Quoted 9 lines

net/core/xdp.c +8 -0

+	if (!xdpf->mb)
+		return;
+
+	sinfo = xdp_get_shared_info_from_frame(xdpf);
+	for (i = 0; i < sinfo->nr_frags; i++) {
+		struct page *page = skb_frag_page(&sinfo->frags[i]);
+
+		__xdp_return(page_address(page), &xdpf->mem, false);

net/core/xdp.c static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct) · line 394

Quoted 10 lines

net/core/xdp.c +4 -0

+	}
 }
 EXPORT_SYMBOL_GPL(xdp_return_frame);
 
 void xdp_return_frame_rx_napi(struct xdp_frame *xdpf)
 {
+	struct skb_shared_info *sinfo;
+	int i;
+
 	__xdp_return(xdpf->data, &xdpf->mem, true);

Same issue.

Quoted 8 lines

net/core/xdp.c +8 -0

+	if (!xdpf->mb)
+		return;
+
+	sinfo = xdp_get_shared_info_from_frame(xdpf);
+	for (i = 0; i < sinfo->nr_frags; i++) {
+		struct page *page = skb_frag_page(&sinfo->frags[i]);
+
+		__xdp_return(page_address(page), &xdpf->mem, true);

net/core/xdp.c static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct) · line 412

Quoted 10 lines

net/core/xdp.c +4 -0

+	}
 }
 EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi);
 
 void xdp_return_buff(struct xdp_buff *xdp)
 {
+	struct skb_shared_info *sinfo;
+	int i;
+
 	__xdp_return(xdp->data, &xdp->rxq->mem, true);

Same issue.

Quoted 10 lines

net/core/xdp.c +9 -0

+	if (!xdp->mb)
+		return;
+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+	for (i = 0; i < sinfo->nr_frags; i++) {
+		struct page *page = skb_frag_page(&sinfo->frags[i]);
+
+		__xdp_return(page_address(page), &xdp->rxq->mem, true);
+	}
 }

Signature · 4 lines

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

Enable the capability to receive jumbo frames even if the interface is running in XDP mode

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

Quoted 4 lines

Enable the capability to receive jumbo frames even if the interface is running in XDP mode

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

Hm, already? Is all the infra in place? Or does it not imply multi-buffer.

Quoted 8 lines · On Wed, 19 Aug 2020 15:13:51 +0200 Lorenzo Bianconi wrote:

Quoted 4 lines

Enable the capability to receive jumbo frames even if the interface is running in XDP mode

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

Hm, already? Is all the infra in place? Or does it not imply multi-buffer.

with this series mvneta supports xdp multi-buff on both rx and tx sides (XDP_TX and ndo_xpd_xmit()) so we can remove MTU limitation.

Quoted 6 lines · On Wed, 19 Aug 2020 22:22:23 +0200 Lorenzo Bianconi wrote:

Quoted 6 lines · On Wed, 19 Aug 2020 15:13:51 +0200 Lorenzo Bianconi wrote:

Quoted 4 lines

Enable the capability to receive jumbo frames even if the interface is running in XDP mode

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

Quoted 5 lines

Quoted 2 lines

Hm, already? Is all the infra in place? Or does it not imply multi-buffer.

with this series mvneta supports xdp multi-buff on both rx and tx sides (XDP_TX and ndo_xpd_xmit()) so we can remove MTU limitation.

Is there an API for programs to access the multi-buf frames?

Quoted 7 lines · Jakub Kicinski

Quoted 6 lines · On Wed, 19 Aug 2020 22:22:23 +0200 Lorenzo Bianconi wrote:

Quoted 6 lines · On Wed, 19 Aug 2020 15:13:51 +0200 Lorenzo Bianconi wrote:

Quoted 4 lines

Enable the capability to receive jumbo frames even if the interface is running in XDP mode

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

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Hm, already? Is all the infra in place? Or does it not imply multi-buffer.

with this series mvneta supports xdp multi-buff on both rx and tx sides (XDP_TX and ndo_xpd_xmit()) so we can remove MTU limitation.

Is there an API for programs to access the multi-buf frames?

Hi Lorenzo,

This is not enough to support multi-buffer in my opinion. I have the same comment as Jakub. We need an API to pull in the multiple buffers otherwise we break the ability to parse the packets and that is a hard requirement to me. I don't want to lose visibility to get jumbo frames.

At minimum we need a bpf_xdp_pull_data() to adjust pointer. In the skmsg case we use this,

bpf_msg_pull_data(u32 start, u32 end, u64 flags)

Where start is the offset into the packet and end is the last byte we want to adjust start/end pointers to. This way we can walk pages if we want and avoid having to linearize the data unless the user actual asks us for a block that crosses a page range. Smart users then never do a start/end that crosses a page boundary if possible. I think the same would apply here.

XDP by default gives you the first page start/end to use freely. If you need to parse deeper into the payload then you call bpf_msg_pull_data with the byte offsets needed.

Quoted 10 lines · Jakub Kicinski

Quoted 9 lines · On Wed, 19 Aug 2020 22:22:23 +0200 Lorenzo Bianconi wrote:

Quoted 8 lines · On Wed, 19 Aug 2020 15:13:51 +0200 Lorenzo Bianconi wrote:

Quoted 4 lines

Enable the capability to receive jumbo frames even if the interface is running in XDP mode

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

Hm, already? Is all the infra in place? Or does it not imply multi-buffer.

Quoted 4 lines

Quoted 2 lines

with this series mvneta supports xdp multi-buff on both rx and tx sides (XDP_TX and ndo_xpd_xmit()) so we can remove MTU limitation.

Is there an API for programs to access the multi-buf frames?

Hi Lorenzo,

This is not enough to support multi-buffer in my opinion. I have the same comment as Jakub. We need an API to pull in the multiple buffers otherwise we break the ability to parse the packets and that is a hard requirement to me. I don't want to lose visibility to get jumbo frames.

At minimum we need a bpf_xdp_pull_data() to adjust pointer. In the skmsg case we use this,

bpf_msg_pull_data(u32 start, u32 end, u64 flags)

Where start is the offset into the packet and end is the last byte we want to adjust start/end pointers to. This way we can walk pages if we want and avoid having to linearize the data unless the user actual asks us for a block that crosses a page range. Smart users then never do a start/end that crosses a page boundary if possible. I think the same would apply here.

XDP by default gives you the first page start/end to use freely. If you need to parse deeper into the payload then you call bpf_msg_pull_data with the byte offsets needed.

Also we would want performance numbers to see how good/bad this is compared to the base case.

Thanks,
John

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.

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

General issue (that I think must be resolved/discussed as part of this initial patchset).

When XDP_REDIRECT'ing a multi-buffer xdp_frame out of another driver's ndo_xdp_xmit(), what happens if the remote driver doesn't understand the multi-buffer format?

My guess it that it will only send the first part of the packet (in the main page). Fortunately we don't leak memory, because xdp_return_frame() handle freeing the other segments. I assume this isn't acceptable behavior... or maybe it is?

What are our options for handling this:

1. Add mb support in ndo_xdp_xmit in every driver?

2. Drop xdp->mb frames inside ndo_xdp_xmit (in every driver without support)?

3. Add core-code check before calling ndo_xdp_xmit()?

--Jesper

Quoted 10 lines · On Wed, 19 Aug 2020 15:13:45 +0200 Lorenzo Bianconi <[email protected]> wrote:

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.

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

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