mvneta: introduce XDP multi-buffer support

Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer data structure in order to specify if this is a linear buffer (mb = 0) or a multi-buffer frame (mb = 1). In the latter case the shared_info area at the end of the first buffer is been properly initialized to link together subsequent 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 -0

index 700ad5db7f5d..70559720ff44 100644

include/net/xdp.h +2 -0

@@ -154,6 +156,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

@@ -180,6 +183,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 +0 -0

index 17ffd33c6b18..79dd45234e4d 100644

Quoted 30 lines · Lorenzo Bianconi

Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer data structure
in order to specify if this is a linear buffer (mb = 0) or a multi- buffer
frame (mb = 1). In the latter case the shared_info area at the end of the
first buffer is been properly initialized to link together subsequent
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 700ad5db7f5d..70559720ff44 100644
@@ -73,7 +73,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 1 line

If we are really going to do something like this I say we should just rip a swath of bits out instead of just grabbing one. We are already cutting the size down then we should just decide on the minimum size that is acceptable and just jump to that instead of just stealing one bit at a time. It looks like we already have differences between the size here and frame_size in xdp_frame.

If we have to steal a bit why not look at something like one of the lower 2/3 bits in rxq? You could then do the same thing using dev_rx in a similar fashion instead of stealing from a bit that is likely to be used in multiple spots and modifying like this adds extra overhead to?

Quoted 32 lines · Alexander Duyck

Quoted 30 lines · Lorenzo Bianconi

Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer data structure
in order to specify if this is a linear buffer (mb = 0) or a multi- buffer
frame (mb = 1). In the latter case the shared_info area at the end of the
first buffer is been properly initialized to link together subsequent
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 700ad5db7f5d..70559720ff44 100644
@@ -73,7 +73,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 9 lines

Quoted 1 line

If we are really going to do something like this I say we should just rip a swath of bits out instead of just grabbing one. We are already cutting the size down then we should just decide on the minimum size that is acceptable and just jump to that instead of just stealing one bit at a time. It looks like we already have differences between the size here and frame_size in xdp_frame.

+1

Quoted 6 lines

If we have to steal a bit why not look at something like one of the lower 2/3 bits in rxq? You could then do the same thing using dev_rx in a similar fashion instead of stealing from a bit that is likely to be used in multiple spots and modifying like this adds extra overhead to?

What do you mean in rxq ? from the pointer ?

Quoted 64 lines · Saeed Mahameed

Quoted 40 lines · Alexander Duyck

Quoted 27 lines · Lorenzo Bianconi

Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer data structure
in order to specify if this is a linear buffer (mb = 0) or a multi-
buffer
frame (mb = 1). In the latter case the shared_info area at the end
of the
first buffer is been properly initialized to link together subsequent
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 700ad5db7f5d..70559720ff44 100644
@@ -73,7 +73,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

Quoted 6 lines

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 */
};

If we are really going to do something like this I say we should just
rip a swath of bits out instead of just grabbing one. We are

Quoted 9 lines

already
cutting the size down then we should just decide on the minimum size
that is acceptable and just jump to that instead of just stealing one
bit at a time. It looks like we already have differences between the
size here and frame_size in xdp_frame.

+1

Quoted 10 lines

If we have to steal a bit why not look at something like one of the
lower 2/3 bits in rxq? You could then do the same thing using dev_rx
in a similar fashion instead of stealing from a bit that is likely to
be used in multiple spots and modifying like this adds extra overhead
to?

Quoted 1 line

What do you mean in rxq ? from the pointer ?

Yeah, the pointers have a few bits that are guaranteed 0 and in my mind reusing the lower bits from a 4 or 8 byte aligned pointer would make more sense then stealing the upper bits from the size of the frame.

Quoted 66 lines · Alexander Duyck

Quoted 64 lines · Saeed Mahameed

Quoted 40 lines · Alexander Duyck

Quoted 27 lines · Lorenzo Bianconi

Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer data structure
in order to specify if this is a linear buffer (mb = 0) or a multi-
buffer
frame (mb = 1). In the latter case the shared_info area at the end
of the
first buffer is been properly initialized to link together subsequent
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 700ad5db7f5d..70559720ff44 100644
@@ -73,7 +73,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

Quoted 6 lines

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 */
};

If we are really going to do something like this I say we should just
rip a swath of bits out instead of just grabbing one. We are

Quoted 9 lines

already
cutting the size down then we should just decide on the minimum size
that is acceptable and just jump to that instead of just stealing one
bit at a time. It looks like we already have differences between the
size here and frame_size in xdp_frame.

+1

Quoted 10 lines

If we have to steal a bit why not look at something like one of the
lower 2/3 bits in rxq? You could then do the same thing using dev_rx
in a similar fashion instead of stealing from a bit that is likely to
be used in multiple spots and modifying like this adds extra overhead
to?

Quoted 6 lines

Quoted 1 line

What do you mean in rxq ? from the pointer ?

Yeah, the pointers have a few bits that are guaranteed 0 and in my mind reusing the lower bits from a 4 or 8 byte aligned pointer would make more sense then stealing the upper bits from the size of the frame.

Ha, i can't imagine how accessing that pointer would look like .. is possible to define the pointer as a bit-field and just access it normally ? or do we need to fix it up every time we need to access it ? will gcc/static checkers complain about wrong pointer type ?

Initialize multi-buffer bit (mb) to 0 in all XDP-capable drivers. This is a preliminary patch to enable xdp multi-buffer support.

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

---
 drivers/net/ethernet/amazon/ena/ena_netdev.c        | 1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c       | 1 +
 drivers/net/ethernet/cavium/thunder/nicvf_main.c    | 1 +
 drivers/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 +
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c   | 1 +
 drivers/net/ethernet/marvell/mvneta.c               | 1 +
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c     | 1 +
 drivers/net/ethernet/mellanox/mlx4/en_rx.c          | 1 +
 drivers/net/ethernet/mellanox/mlx5/core/en_rx.c     | 1 +
 drivers/net/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 +
 net/core/dev.c                                      | 1 +
 24 files changed, 26 insertions(+)

Quoted 5 lines · Lorenzo Bianconi

Initialize multi-buffer bit (mb) to 0 in all XDP-capable drivers. This is a preliminary patch to enable xdp multi-buffer support.

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

I'm really not a fan of this design. Having to update every driver in order to initialize a field that was fragmented is a pain. At a minimum it seems like it might be time to consider introducing some sort of initializer function for this so that you can update things in one central place the next time you have to add a new field instead of having to update every individual driver that supports XDP. Otherwise this isn't going to scale going forward.

Quoted 28 lines

---
 drivers/net/ethernet/amazon/ena/ena_netdev.c        | 1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c       | 1 +
 drivers/net/ethernet/cavium/thunder/nicvf_main.c    | 1 +
 drivers/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 +
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c   | 1 +
 drivers/net/ethernet/marvell/mvneta.c               | 1 +
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c     | 1 +
 drivers/net/ethernet/mellanox/mlx4/en_rx.c          | 1 +
 drivers/net/ethernet/mellanox/mlx5/core/en_rx.c     | 1 +
 drivers/net/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 +
 net/core/dev.c                                      | 1 +
 24 files changed, 26 insertions(+)

Quoted 7 lines · Alexander Duyck

On Mon, Dec 7, 2020 at 8:36 AM Lorenzo Bianconi <[email protected]

Quoted 5 lines

wrote:
Initialize multi-buffer bit (mb) to 0 in all XDP-capable drivers. This is a preliminary patch to enable xdp multi-buffer support.

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

Quoted 11 lines

I'm really not a fan of this design. Having to update every driver in
order to initialize a field that was fragmented is a pain. At a minimum it seems like it might be time to consider introducing some sort of initializer function for this so that you can update things in
one central place the next time you have to add a new field instead of
having to update every individual driver that supports XDP. Otherwise
this isn't going to scale going forward.

Also, a good example of why this might be bothering for us is a fact that
in the meantime the dpaa driver got XDP support and this patch hasn't been
updated to include mb setting in that driver.

Quoted 20 lines · Alexander Duyck

Quoted 7 lines

On Mon, Dec 7, 2020 at 8:36 AM Lorenzo Bianconi <[email protected]

Quoted 5 lines

wrote:
Initialize multi-buffer bit (mb) to 0 in all XDP-capable drivers. This is a preliminary patch to enable xdp multi-buffer support.

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

Quoted 11 lines

I'm really not a fan of this design. Having to update every driver in
order to initialize a field that was fragmented is a pain. At a minimum it seems like it might be time to consider introducing some sort of initializer function for this so that you can update things in
one central place the next time you have to add a new field instead of
having to update every individual driver that supports XDP. Otherwise
this isn't going to scale going forward.

Quoted 6 lines

Also, a good example of why this might be bothering for us is a fact that
in the meantime the dpaa driver got XDP support and this patch hasn't been
updated to include mb setting in that driver.

something like
init_xdp_buff(hard_start, headroom, len, frame_sz, rxq);

would work for most of the drivers.

Quoted 9 lines · Maciej Fijalkowski

Quoted 8 lines · Alexander Duyck

Quoted 7 lines

On Mon, Dec 7, 2020 at 8:36 AM Lorenzo Bianconi <[email protected]

Quoted 5 lines

wrote:
Initialize multi-buffer bit (mb) to 0 in all XDP-capable drivers. This is a preliminary patch to enable xdp multi-buffer support.

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

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

I'm really not a fan of this design. Having to update every driver in order to initialize a field that was fragmented is a pain. At a minimum it seems like it might be time to consider introducing some sort of initializer function for this so that you can update things in one central place the next time you have to add a new field instead of having to update every individual driver that supports XDP. Otherwise this isn't going to scale going forward.

Quoted 9 lines

Quoted 4 lines

Also, a good example of why this might be bothering for us is a fact that in the meantime the dpaa driver got XDP support and this patch hasn't been updated to include mb setting in that driver.

something like
init_xdp_buff(hard_start, headroom, len, frame_sz, rxq);

would work for most of the drivers.

ack, agree. I will add init_xdp_buff() in v6.

Introduce xdp_shared_info data structure to contain info about "non-linear" xdp frame. xdp_shared_info will alias skb_shared_info allowing to keep most of the frags in the same cache-line. Introduce some xdp_shared_info helpers aligned to skb_frag* ones

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

---
 drivers/net/ethernet/marvell/mvneta.c | 62 +++++++++++++++--------
----
 include/net/xdp.h                     | 52 ++++++++++++++++++++--
 2 files changed, 82 insertions(+), 32 deletions(-)

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

index 1e5b5c69685a..d635463609ad 100644
@@ -2033,14 +2033,17 @@ int mvneta_rx_refill_queue(struct mvneta_port
 *pp, struct mvneta_rx_queue *rxq)
 

static void

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

@@ -2278,7 +2281,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 skb_shared_info *xdp_sinfo,
+			    struct xdp_shared_info *xdp_sinfo,
 			    struct page *page)
 {

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

 	struct net_device *dev = pp->dev;

@@ -2301,13 +2304,13 @@ mvneta_swbm_add_rx_fragment(struct
 mvneta_port *pp,
 	if (data_len > 0 && xdp_sinfo->nr_frags < MAX_SKB_FRAGS) {

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

 		/* last fragment */
 		if (len == *size) {
-			struct skb_shared_info *sinfo;
+			struct xdp_shared_info *sinfo;
 

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

 			sinfo = xdp_get_shared_info_from_buff(xdp);
 			sinfo->nr_frags = xdp_sinfo->nr_frags;

@@ -2324,10 +2327,13 @@ static struct sk_buff *
 mvneta_swbm_build_skb(struct mvneta_port *pp, struct mvneta_rx_queue
 *rxq,
 		      struct xdp_buff *xdp, u32 desc_status)
 {
-	struct skb_shared_info *sinfo =
 xdp_get_shared_info_from_buff(xdp);
-	int i, num_frags = sinfo->nr_frags;
+	struct xdp_shared_info *xdp_sinfo =
 xdp_get_shared_info_from_buff(xdp);
+	int i, num_frags = xdp_sinfo->nr_frags;
+	skb_frag_t frag_list[MAX_SKB_FRAGS];
 	struct sk_buff *skb;
 
+	memcpy(frag_list, xdp_sinfo->frags, sizeof(skb_frag_t) *

num_frags);

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

+
 	skb = build_skb(xdp->data_hard_start, PAGE_SIZE);
 	if (!skb)
 		return ERR_PTR(-ENOMEM);

@@ -2339,12 +2345,12 @@ mvneta_swbm_build_skb(struct mvneta_port *pp,
 struct mvneta_rx_queue *rxq,
 	mvneta_rx_csum(pp, desc_status, skb);
 
 	for (i = 0; i < num_frags; i++) {
-		skb_frag_t *frag = &sinfo->frags[i];
+		struct page *page = xdp_get_frag_page(&frag_list[i]);
 
 		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
-				skb_frag_page(frag),
 skb_frag_off(frag),
-				skb_frag_size(frag), PAGE_SIZE);
-		page_pool_release_page(rxq->page_pool,
 skb_frag_page(frag));
+				page,

xdp_get_frag_offset(&frag_list[i]),

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

+				xdp_get_frag_size(&frag_list[i]),

PAGE_SIZE);

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

+		page_pool_release_page(rxq->page_pool, page);
 	}
 
 	return skb;

@@ -2357,7 +2363,7 @@ static int mvneta_rx_swbm(struct napi_struct
 *napi,
 {
 	int rx_proc = 0, rx_todo, refill, size = 0;
 	struct net_device *dev = pp->dev;
-	struct skb_shared_info sinfo;
+	struct xdp_shared_info xdp_sinfo;
 	struct mvneta_stats ps = {};
 	struct bpf_prog *xdp_prog;

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

 	u32 desc_status, frame_sz;

@@ -2368,7 +2374,7 @@ static int mvneta_rx_swbm(struct napi_struct
 *napi,
 	xdp_buf.rxq = &rxq->xdp_rxq;
 	xdp_buf.mb = 0;
 
-	sinfo.nr_frags = 0;
+	xdp_sinfo.nr_frags = 0;
 
 	/* Get number of received packets */

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

 	rx_todo = mvneta_rxq_busy_desc_num_get(pp, rxq);

@@ -2412,7 +2418,7 @@ static int mvneta_rx_swbm(struct napi_struct
 *napi,
 			}
 
 			mvneta_swbm_add_rx_fragment(pp, rx_desc, rxq,
 &xdp_buf,
-						    &size, &sinfo,
 page);
+						    &size, &xdp_sinfo,

page);

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

 		} /* Middle or Last descriptor */
 
 		if (!(rx_status & MVNETA_RXD_LAST_DESC))

@@ -2420,7 +2426,7 @@ static int mvneta_rx_swbm(struct napi_struct
 *napi,
 			continue;
 
 		if (size) {
-			mvneta_xdp_put_buff(pp, rxq, &xdp_buf, &sinfo,
-1);
+			mvneta_xdp_put_buff(pp, rxq, &xdp_buf,
 &xdp_sinfo, -1);
 			goto next;
 		}

@@ -2432,7 +2438,7 @@ static int mvneta_rx_swbm(struct napi_struct
 *napi,
 		if (IS_ERR(skb)) {
 			struct mvneta_pcpu_stats *stats =
 this_cpu_ptr(pp->stats);
 
-			mvneta_xdp_put_buff(pp, rxq, &xdp_buf, &sinfo,
-1);
+			mvneta_xdp_put_buff(pp, rxq, &xdp_buf,

&xdp_sinfo, -1);

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

 			u64_stats_update_begin(&stats->syncp);
 			stats->es.skb_alloc_error++;

@@ -2449,12 +2455,12 @@ static int mvneta_rx_swbm(struct napi_struct
 *napi,
 		napi_gro_receive(napi, skb);
 next:
 		xdp_buf.data_hard_start = NULL;
-		sinfo.nr_frags = 0;
+		xdp_sinfo.nr_frags = 0;
 	}
 	rcu_read_unlock();
 
 	if (xdp_buf.data_hard_start)
-		mvneta_xdp_put_buff(pp, rxq, &xdp_buf, &sinfo, -1);
+		mvneta_xdp_put_buff(pp, rxq, &xdp_buf, &xdp_sinfo, -1);
 
 	if (ps.xdp_redirect)

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

 		xdp_do_flush_map();

include/net/xdp.h +0 -0

index 70559720ff44..614f66d35ee8 100644

include/net/xdp.h +34 -0

+static inline struct page *xdp_get_frag_page(const skb_frag_t *frag)
+{
+	return frag->bv_page;
+}
+
+static inline unsigned int xdp_get_frag_offset(const skb_frag_t
 *frag)
+{
+	return frag->bv_offset;
+}
+
+static inline unsigned int xdp_get_frag_size(const skb_frag_t *frag)
+{
+	return frag->bv_len;
+}
+
+static inline void *xdp_get_frag_address(const skb_frag_t *frag)
+{
+	return page_address(xdp_get_frag_page(frag)) +
+	       xdp_get_frag_offset(frag);
+}
+
+static inline void xdp_set_frag_page(skb_frag_t *frag, struct page
 *page)
+{
+	frag->bv_page = page;
+}
+
+static inline void xdp_set_frag_offset(skb_frag_t *frag, u32 offset)
+{
+	frag->bv_offset = offset;
+}
+
+static inline void xdp_set_frag_size(skb_frag_t *frag, u32 size)
+{
+	frag->bv_len = size;
 }

struct xdp_frame {

include/net/xdp.h +1 -2

@@ -120,12 +164,12 @@ static __always_inline void
 xdp_frame_bulk_init(struct xdp_frame_bulk *bq)
 	bq->xa = NULL;
 }
 
-static inline struct skb_shared_info *
+static inline struct xdp_shared_info *
 xdp_get_shared_info_from_frame(struct xdp_frame *frame)
 {
 	void *data_hard_start = frame->data - frame->headroom -
 sizeof(*frame);
 
-	return (struct skb_shared_info *)(data_hard_start + frame-

Quoted 1 line

frame_sz -

include/net/xdp.h +1 -0

+	return (struct xdp_shared_info *)(data_hard_start + frame-

Quoted 1 line

frame_sz -

SKB_DATA_ALIGN(sizeof(struct
skb_shared_info)));
}

Quoted 5 lines

Introduce xdp_shared_info data structure to contain info about "non-linear" xdp frame. xdp_shared_info will alias skb_shared_info allowing to keep most of the frags in the same cache-line. Introduce some xdp_shared_info helpers aligned to skb_frag* ones

is there or will be a more general purpose use to this xdp_shared_info ? other than hosting frags ?

Quoted 16 lines

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

---
 drivers/net/ethernet/marvell/mvneta.c | 62 +++++++++++++++--------
----
 include/net/xdp.h                     | 52 ++++++++++++++++++++--
 2 files changed, 82 insertions(+), 32 deletions(-)

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

index 1e5b5c69685a..d635463609ad 100644
@@ -2033,14 +2033,17 @@ int mvneta_rx_refill_queue(struct mvneta_port
 *pp, struct mvneta_rx_queue *rxq)
 

[...]

Quoted 17 lines

static void

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

@@ -2278,7 +2281,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 skb_shared_info *xdp_sinfo,
+			    struct xdp_shared_info *xdp_sinfo,
 			    struct page *page)
 {

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

 	struct net_device *dev = pp->dev;

@@ -2301,13 +2304,13 @@ mvneta_swbm_add_rx_fragment(struct
 mvneta_port *pp,
 	if (data_len > 0 && xdp_sinfo->nr_frags < MAX_SKB_FRAGS) {
 		skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo-
 >nr_frags++];
 

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

Quoted 7 lines

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

-		skb_frag_off_set(frag, pp->rx_offset_correction);
-		skb_frag_size_set(frag, data_len);
-		__skb_frag_set_page(frag, page);
+		xdp_set_frag_offset(frag, pp->rx_offset_correction);
+		xdp_set_frag_size(frag, data_len);
+		xdp_set_frag_page(frag, page);
 

why three separate setters ? why not just one
xdp_set_frag(page, offset, size) ?

Quoted 13 lines

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

 		/* last fragment */
 		if (len == *size) {
-			struct skb_shared_info *sinfo;
+			struct xdp_shared_info *sinfo;
 

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

 			sinfo = xdp_get_shared_info_from_buff(xdp);
 			sinfo->nr_frags = xdp_sinfo->nr_frags;

@@ -2324,10 +2327,13 @@ static struct sk_buff *
 mvneta_swbm_build_skb(struct mvneta_port *pp, struct mvneta_rx_queue
 *rxq,
 		      struct xdp_buff *xdp, u32 desc_status)
 {

drivers/net/ethernet/marvell/mvneta.c

Quoted 3 lines

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

-static inline struct skb_shared_info *
+struct xdp_shared_info {

xdp_shared_info is a bad name, we need this to have a specific purpose xdp_frags should the proper name, so people will think twice before adding weird bits to this so called shared_info.

drivers/net/ethernet/marvell/mvneta.c

Quoted 4 lines

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

+	u16 nr_frags;
+	u16 data_length; /* paged area length */
+	skb_frag_t frags[MAX_SKB_FRAGS];

why MAX_SKB_FRAGS ? just use a flexible array member skb_frag_t frags[];

and enforce size via the n_frags and on the construction of the tailroom preserved buffer, which is already being done.

this is waste of unnecessary space, at lease by definition of the struct, in your use case you do:
memcpy(frag_list, xdp_sinfo->frags, sizeof(skb_frag_t) * num_frags); And the tailroom space was already preserved for a full skb_shinfo. so i don't see why you need this array to be of a fixed MAX_SKB_FRAGS size.

drivers/net/ethernet/marvell/mvneta.c

Quoted 12 lines

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

+};
+
+static inline struct xdp_shared_info *
 xdp_get_shared_info_from_buff(struct xdp_buff *xdp)
 {
-	return (struct skb_shared_info *)xdp_data_hard_end(xdp);
+	BUILD_BUG_ON(sizeof(struct xdp_shared_info) >
+		     sizeof(struct skb_shared_info));
+	return (struct xdp_shared_info *)xdp_data_hard_end(xdp);
+}
+

Back to my first comment, do we have plans to use this tail room buffer for other than frag_list use cases ? what will be the buffer format then ? should we push all new fields to the end of the xdp_shared_info struct ? or deal with this tailroom buffer as a stack ?
my main concern is that for drivers that don't support frag list and still want to utilize the tailroom buffer for other usecases they will have to skip the first sizeof(xdp_shared_info) so they won't break the stack.

Quoted 55 lines

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

+static inline struct page *xdp_get_frag_page(const skb_frag_t *frag)
+{
+	return frag->bv_page;
+}
+
+static inline unsigned int xdp_get_frag_offset(const skb_frag_t

*frag)

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

+{
+	return frag->bv_offset;
+}
+
+static inline unsigned int xdp_get_frag_size(const skb_frag_t *frag)
+{
+	return frag->bv_len;
+}
+
+static inline void *xdp_get_frag_address(const skb_frag_t *frag)
+{
+	return page_address(xdp_get_frag_page(frag)) +
+	       xdp_get_frag_offset(frag);
+}
+
+static inline void xdp_set_frag_page(skb_frag_t *frag, struct page

*page)

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

+{
+	frag->bv_page = page;
+}
+
+static inline void xdp_set_frag_offset(skb_frag_t *frag, u32 offset)
+{
+	frag->bv_offset = offset;
+}
+
+static inline void xdp_set_frag_size(skb_frag_t *frag, u32 size)
+{
+	frag->bv_len = size;
 }
 
 struct xdp_frame {

@@ -120,12 +164,12 @@ static __always_inline void
 xdp_frame_bulk_init(struct xdp_frame_bulk *bq)
 	bq->xa = NULL;
 }
 
-static inline struct skb_shared_info *
+static inline struct xdp_shared_info *
 xdp_get_shared_info_from_frame(struct xdp_frame *frame)
 {
 	void *data_hard_start = frame->data - frame->headroom -
 sizeof(*frame);
 
-	return (struct skb_shared_info *)(data_hard_start + frame-

Quoted 1 line

frame_sz -

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

+	return (struct xdp_shared_info *)(data_hard_start + frame-

Quoted 5 lines

Quoted 1 line

frame_sz -

SKB_DATA_ALIGN(sizeof(struct
skb_shared_info)));
}

need a comment here why we preserve the size of skb_shared_info, yet the usable buffer is of type xdp_shared_info.

Quoted 4 lines · Lorenzo Bianconi

Quoted 3 lines

Introduce xdp_shared_info data structure to contain info about "non-linear" xdp frame. xdp_shared_info will alias skb_shared_info allowing to keep most of the frags in the same cache-line.

Quoted 10 lines

include/net/xdp.h struct xdp_buff { · line 93

Quoted 4 lines

include/net/xdp.h +3 -0

+	u16 nr_frags;
+	u16 data_length; /* paged area length */
+	skb_frag_t frags[MAX_SKB_FRAGS];

why MAX_SKB_FRAGS ? just use a flexible array member skb_frag_t frags[];

and enforce size via the n_frags and on the construction of the tailroom preserved buffer, which is already being done.

Quoted 6 lines

this is waste of unnecessary space, at lease by definition of the struct, in your use case you do:
memcpy(frag_list, xdp_sinfo->frags, sizeof(skb_frag_t) * num_frags); And the tailroom space was already preserved for a full skb_shinfo. so i don't see why you need this array to be of a fixed MAX_SKB_FRAGS size.

In order to avoid cache-misses, xdp_shared info is built as a variable on mvneta_rx_swbm() stack and it is written to "shared_info" area only on the last fragment in mvneta_swbm_add_rx_fragment(). I used MAX_SKB_FRAGS to be aligned with skb_shared_info struct but probably we can use even a smaller value. Another approach would be to define two different struct, e.g.

stuct xdp_frag_metadata {
	u16 nr_frags;
	u16 data_length; /* paged area length */
};

struct xdp_frags {
	skb_frag_t frags[MAX_SKB_FRAGS];
};

and then define xdp_shared_info as

struct xdp_shared_info {
	stuct xdp_frag_metadata meta;
	skb_frag_t frags[];
};

In this way we can probably optimize the space. What do you think?

Quoted 13 lines

include/net/xdp.h struct xdp_buff { · line 103

Quoted 12 lines

include/net/xdp.h +8 -1

+};
+
+static inline struct xdp_shared_info *
 xdp_get_shared_info_from_buff(struct xdp_buff *xdp)
 {
-	return (struct skb_shared_info *)xdp_data_hard_end(xdp);
+	BUILD_BUG_ON(sizeof(struct xdp_shared_info) >
+		     sizeof(struct skb_shared_info));
+	return (struct xdp_shared_info *)xdp_data_hard_end(xdp);
+}
+

Quoted 7 lines

Back to my first comment, do we have plans to use this tail room buffer
for other than frag_list use cases ? what will be the buffer format
then ? should we push all new fields to the end of the xdp_shared_info
struct ? or deal with this tailroom buffer as a stack ? my main concern is that for drivers that don't support frag list and still want to utilize the tailroom buffer for other usecases they will
have to skip the first sizeof(xdp_shared_info) so they won't break the
stack.

for the moment I do not know if this area is used for other purposes. Do you think there are other use-cases for it?

Quoted 1 line

Quoted 2 lines

Quoted 2 lines · Lorenzo Bianconi

for the moment I do not know if this area is used for other purposes. Do you think there are other use-cases for it?

Quoted 6 lines · Shay Agroskin

Quoted 2 lines · Lorenzo Bianconi

for the moment I do not know if this area is used for other purposes. Do you think there are other use-cases for it?

Sorry to interject:
Does it make sense to use it to store arbitrary metadata or a scratchpad in this space? Something equivalent to skb->cb which is lacking in XDP.

On Sat, 19 Dec 2020 10:30:57 -0500

Quoted 4 lines

Sorry to interject:
Does it make sense to use it to store arbitrary metadata or a scratchpad in this space? Something equivalent to skb->cb which is lacking in XDP.

Well, XDP have the data_meta area. But difficult to rely on because a lot of driver don't implement it. And Saeed and I plan to use this area and populate it with driver info from RX-descriptor.

Quoted 6 lines · Lorenzo Bianconi

Quoted 6 lines · Lorenzo Bianconi

Quoted 5 lines

Introduce xdp_shared_info data structure to contain info about
"non-linear" xdp frame. xdp_shared_info will alias skb_shared_info
allowing to keep most of the frags in the same cache-line.

[...]

Quoted 48 lines

Quoted 11 lines

include/net/xdp.h struct xdp_buff { · line 93

Quoted 4 lines

include/net/xdp.h +3 -0

+  u16 nr_frags;
+  u16 data_length; /* paged area length */
+  skb_frag_t frags[MAX_SKB_FRAGS];

why MAX_SKB_FRAGS ? just use a flexible array member skb_frag_t frags[];

and enforce size via the n_frags and on the construction of the tailroom preserved buffer, which is already being done.

Quoted 10 lines

this is waste of unnecessary space, at lease by definition of the
struct, in your use case you do:
memcpy(frag_list, xdp_sinfo->frags, sizeof(skb_frag_t) * num_frags);
And the tailroom space was already preserved for a full skb_shinfo.
so i don't see why you need this array to be of a fixed MAX_SKB_FRAGS
size.

In order to avoid cache-misses, xdp_shared info is built as a variable
on mvneta_rx_swbm() stack and it is written to "shared_info" area only on the
last fragment in mvneta_swbm_add_rx_fragment(). I used MAX_SKB_FRAGS to be
aligned with skb_shared_info struct but probably we can use even a smaller value.
Another approach would be to define two different struct, e.g.

stuct xdp_frag_metadata {
u16 nr_frags;
u16 data_length; /* paged area length */
};

struct xdp_frags {
      skb_frag_t frags[MAX_SKB_FRAGS];
};

and then define xdp_shared_info as

struct xdp_shared_info {
      stuct xdp_frag_metadata meta;
      skb_frag_t frags[];
};

Quoted 2 lines

In this way we can probably optimize the space. What do you think?

We're still reserving ~sizeof(skb_shared_info) bytes at the end of the first buffer and it seems like in mvneta code you keep updating all three fields (frags, nr_frags and data_length). Can you explain how the space is optimized by splitting the structs please?

Quoted 29 lines

Quoted 20 lines

include/net/xdp.h struct xdp_buff { · line 103

Quoted 13 lines

include/net/xdp.h +8 -1

+};
+
+static inline struct xdp_shared_info *
 xdp_get_shared_info_from_buff(struct xdp_buff *xdp)
 {
-  return (struct skb_shared_info *)xdp_data_hard_end(xdp);
+  BUILD_BUG_ON(sizeof(struct xdp_shared_info) >
+               sizeof(struct skb_shared_info));
+  return (struct xdp_shared_info *)xdp_data_hard_end(xdp);
+}
+

Back to my first comment, do we have plans to use this tail room buffer
for other than frag_list use cases ? what will be the buffer format
then ? should we push all new fields to the end of the xdp_shared_info

Quoted 8 lines

struct ? or deal with this tailroom buffer as a stack ? my main concern is that for drivers that don't support frag list and
still want to utilize the tailroom buffer for other usecases they will
have to skip the first sizeof(xdp_shared_info) so they won't break the
stack.

Quoted 4 lines

for the moment I do not know if this area is used for other purposes.
Do you think there are other use-cases for it?

Saeed, the stack receives skb_shared_info when the frames are passed to the stack (skb_add_rx_frag is used to add the whole information to skb's shared info), and for XDP_REDIRECT use case, it doesn't seem like all drivers check page's tailroom for more information anyway (ena doesn't at least).
Can you please explain what do you mean by "break the stack"?

Thanks, Shay

Quoted 1 line

Quoted 1 line

[...]

Quoted 2 lines

Quoted 2 lines

Quoted 59 lines

Quoted 6 lines · Lorenzo Bianconi

Quoted 6 lines · Lorenzo Bianconi

Quoted 5 lines

Introduce xdp_shared_info data structure to contain info about
"non-linear" xdp frame. xdp_shared_info will alias skb_shared_info
allowing to keep most of the frags in the same cache-line.

[...]

Quoted 48 lines

Quoted 11 lines

include/net/xdp.h struct xdp_buff { · line 93

Quoted 4 lines

include/net/xdp.h +3 -0

+  u16 nr_frags;
+  u16 data_length; /* paged area length */
+  skb_frag_t frags[MAX_SKB_FRAGS];

why MAX_SKB_FRAGS ? just use a flexible array member skb_frag_t frags[];

and enforce size via the n_frags and on the construction of the tailroom preserved buffer, which is already being done.

Quoted 10 lines

this is waste of unnecessary space, at lease by definition of the
struct, in your use case you do:
memcpy(frag_list, xdp_sinfo->frags, sizeof(skb_frag_t) * num_frags);
And the tailroom space was already preserved for a full skb_shinfo.
so i don't see why you need this array to be of a fixed MAX_SKB_FRAGS
size.

In order to avoid cache-misses, xdp_shared info is built as a variable
on mvneta_rx_swbm() stack and it is written to "shared_info" area only on the
last fragment in mvneta_swbm_add_rx_fragment(). I used MAX_SKB_FRAGS to be
aligned with skb_shared_info struct but probably we can use even a smaller value.
Another approach would be to define two different struct, e.g.

stuct xdp_frag_metadata {
u16 nr_frags;
u16 data_length; /* paged area length */
};

struct xdp_frags {
      skb_frag_t frags[MAX_SKB_FRAGS];
};

and then define xdp_shared_info as

struct xdp_shared_info {
      stuct xdp_frag_metadata meta;
      skb_frag_t frags[];
};

Quoted 8 lines

Quoted 2 lines

In this way we can probably optimize the space. What do you think?

We're still reserving ~sizeof(skb_shared_info) bytes at the end of the first buffer and it seems like in mvneta code you keep updating all three fields (frags, nr_frags and data_length). Can you explain how the space is optimized by splitting the structs please?

using xdp_shared_info struct we will have the first 3 fragments in the same cacheline of nr_frags while using skb_shared_info struct only the first fragment will be in the same cacheline of nr_frags. Moreover skb_shared_info has multiple fields unused by xdp.

Regards,
Lorenzo

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

(quoted diff) +2 -0

+};
+

Quoted 15 lines

Saeed, the stack receives skb_shared_info when the frames are passed to the stack (skb_add_rx_frag is used to add the whole information to skb's shared info), and for XDP_REDIRECT use case, it doesn't seem like all drivers check page's tailroom for more information anyway (ena doesn't at least).
Can you please explain what do you mean by "break the stack"?

Thanks, Shay

Quoted 1 line

Quoted 1 line

[...]

Quoted 2 lines

Quoted 2 lines

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 | 94 
 ++++++++++++++++-----------
 1 file changed, 56 insertions(+), 38 deletions(-)

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

 			if (napi && buf->type == 
 MVNETA_TYPE_XDP_TX)
 				xdp_return_frame_rx_napi(buf->xdpf);
 			else

@@ -2054,45 +2054,64 @@ mvneta_xdp_put_buff(struct mvneta_port 
 *pp, struct mvneta_rx_queue *rxq,
 
 static int
 mvneta_xdp_submit_frame(struct mvneta_port *pp, struct 
 mvneta_tx_queue *txq,
-			struct xdp_frame *xdpf, bool dma_map)
+			struct xdp_frame *xdpf, int *nxmit_byte, 
 bool dma_map)
 {
-	struct mvneta_tx_desc *tx_desc;
-	struct mvneta_tx_buf *buf;
-	dma_addr_t dma_addr;
+	struct xdp_shared_info *xdp_sinfo = 
 xdp_get_shared_info_from_frame(xdpf);
+	int i, num_frames = xdpf->mb ? xdp_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->size)

Quoted 8 lines · Lorenzo Bianconi

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 | 94
 ++++++++++++++++-----------
 1 file changed, 56 insertions(+), 38 deletions(-)

[...]

Quoted 26 lines

                      if (napi && buf->type ==
 MVNETA_TYPE_XDP_TX)
                              xdp_return_frame_rx_napi(buf->xdpf);
                      else

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

@@ -2054,45 +2054,64 @@ mvneta_xdp_put_buff(struct mvneta_port
 *pp, struct mvneta_rx_queue *rxq,
 
 static int
 mvneta_xdp_submit_frame(struct mvneta_port *pp, struct
 mvneta_tx_queue *txq,
-                     struct xdp_frame *xdpf, bool dma_map)
+                     struct xdp_frame *xdpf, int *nxmit_byte,
 bool dma_map)
 {
-     struct mvneta_tx_desc *tx_desc;
-     struct mvneta_tx_buf *buf;
-     dma_addr_t dma_addr;
+     struct xdp_shared_info *xdp_sinfo =
 xdp_get_shared_info_from_frame(xdpf);
+     int i, num_frames = xdpf->mb ? xdp_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->size)

drivers/net/ethernet/marvell/mvneta.c mvneta_xdp_put_buff(struct mvneta_port · line 2078

Quoted 10 lines

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

              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 ? &xdp_sinfo->frags[i - 1] :
 NULL;
+             int len = frag ? xdp_get_frag_size(frag) :
 xdpf->len;

nit, from branch prediction point of view, maybe it would be better to write

     int len = i ? xdp_get_frag_size(frag) : xdpf->len;

since the value of i is checked one line above
Disclaimer: I'm far from a compiler expert, and don't know whether the compiler would know to group these two assignments together into a single branch prediction decision, but it feels like using 'i' would make this decision easier for it.

Thanks,
Shay

[...]

Introduce __xdp_build_skb_from_frame and xdp_build_skb_from_frame utility routines to build the skb from xdp_frame.
Add xdp multi-buff support to cpumap

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

---
 include/net/xdp.h   |  5 ++++
 kernel/bpf/cpumap.c | 45 +---------------------------
 net/core/xdp.c      | 73 
 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 44 deletions(-)

net/core/xdp.c +2 -0

index 6c8e743ad03a..55f3e9c69427 100644
@@ -597,3 +597,76 @@ void xdp_warn(const char *msg, const char 
 *func, const int line)
 	WARN(1, "XDP_WARN: %s(line:%d): %s\n", func, line, msg);
 };
 EXPORT_SYMBOL_GPL(xdp_warn);
+
+struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame 

*xdpf,

net/core/xdp.c +5 -0

+					   struct sk_buff *skb,
+					   struct net_device *dev)
+{
+	unsigned int headroom = sizeof(*xdpf) + xdpf->headroom;
+	void *hard_start = xdpf->data - headroom;

net/core/xdp.c +4 -0

+
+	skb = build_skb_around(skb, hard_start, xdpf->frame_sz);
+	if (unlikely(!skb))
+		return NULL;

Quoted 13 lines · Lorenzo Bianconi

Introduce __xdp_build_skb_from_frame and
xdp_build_skb_from_frame
utility routines to build the skb from xdp_frame.
Add xdp multi-buff support to cpumap

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

---
 include/net/xdp.h   |  5 ++++
 kernel/bpf/cpumap.c | 45 +---------------------------
 net/core/xdp.c      | 73
 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 44 deletions(-)

[...]

Quoted 17 lines

net/core/xdp.c +2 -0

index 6c8e743ad03a..55f3e9c69427 100644
@@ -597,3 +597,76 @@ void xdp_warn(const char *msg, const char
 *func, const int line)
      WARN(1, "XDP_WARN: %s(line:%d): %s\n", func, line, msg);
 };
 EXPORT_SYMBOL_GPL(xdp_warn);
+
+struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame

*xdpf,

net/core/xdp.c +5 -0

+                                        struct sk_buff *skb,
+                                        struct net_device *dev)
+{
+     unsigned int headroom = sizeof(*xdpf) + xdpf->headroom;
+     void *hard_start = xdpf->data - headroom;

net/core/xdp.c void xdp_warn(const char *msg, const char *func, const int line) · line 616

Quoted 10 lines

net/core/xdp.c +10 -0

+     skb_frag_t frag_list[MAX_SKB_FRAGS];
+     struct xdp_shared_info *xdp_sinfo;
+     int i, num_frags = 0;
+
+     xdp_sinfo = xdp_get_shared_info_from_frame(xdpf);
+     if (unlikely(xdpf->mb)) {
+             num_frags = xdp_sinfo->nr_frags;
+             memcpy(frag_list, xdp_sinfo->frags,
+                    sizeof(skb_frag_t) * num_frags);
+     }

nit, can you please move the xdp_sinfo assignment inside this 'if' ? This would help to emphasize that regarding xdp_frame tailroom as xdp_shared_info struct (rather than skb_shared_info) is correct only when the mb bit is set

thanks,
Shay

Quoted 5 lines

net/core/xdp.c +4 -0

+
+     skb = build_skb_around(skb, hard_start, xdpf->frame_sz);
+     if (unlikely(!skb))
+             return NULL;

[...]

From: Eelco Chaudron <[email protected]>

This patch adds a new field to the XDP context called frame_length, which will hold the full length of the packet, including fragments if existing.

eBPF programs can determine if fragments are present using something like:

  if (ctx->data_end - ctx->data < ctx->frame_length) {
    /* Fragements exists. /*
  }

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

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

---
 include/net/xdp.h              | 22 +++++++++
 include/uapi/linux/bpf.h       |  1 +
 kernel/bpf/verifier.c          |  2 +-
 net/core/filter.c              | 83 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h |  1 +
 5 files changed, 108 insertions(+), 1 deletion(-)

include/net/xdp.h +8 -0

index 09078ab6644c..e54d733c90ed 100644
@@ -73,8 +73,30 @@ struct xdp_buff {
 	void *data_hard_start;
 	struct xdp_rxq_info *rxq;
 	struct xdp_txq_info *txq;
+	/* If any of the bitfield lengths for frame_sz or mb below change,
+	 * make sure the defines here are also updated!
+	 */
+#ifdef __BIG_ENDIAN_BITFIELD
+#define MB_SHIFT	  0
+#define MB_MASK		  0x00000001
+#define FRAME_SZ_SHIFT	  1
+#define FRAME_SZ_MASK	  0xfffffffe

include/net/xdp.h +0 -0

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

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

index 30b477a26482..62c50ab28ea9 100644
@@ -4380,6 +4380,7 @@ struct xdp_md {
 	__u32 rx_queue_index;  /* rxq->queue_index  */
 
 	__u32 egress_ifindex;  /* txq->dev->ifindex */
+	__u32 frame_length;
 };
 
 /* DEVMAP map-value layout

kernel/bpf/verifier.c +1 -1

index 93def76cf32b..c50caea29fa2 100644
@@ -10526,7 +10526,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 	const struct bpf_verifier_ops *ops = env->ops;
 	int i, cnt, size, ctx_field_size, delta = 0;
 	const int insn_cnt = env->prog->len;
-	struct bpf_insn insn_buf[16], *insn;
+	struct bpf_insn insn_buf[32], *insn;
 	u32 target_size, size_default, off;
 	struct bpf_prog *new_prog;
 	enum bpf_access_type type;

net/core/filter.c +15 -0

index 4c4882d4d92c..278640db9e0a 100644
@@ -8908,6 +8908,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
 				  struct bpf_insn *insn_buf,
 				  struct bpf_prog *prog, u32 *target_size)
 {
+	int ctx_reg, dst_reg, scratch_reg;
 	struct bpf_insn *insn = insn_buf;
 
 	switch (si->off) {

@@ -8954,6 +8955,88 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
 		*insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg,
 				      offsetof(struct net_device, ifindex));
 		break;
+	case offsetof(struct xdp_md, frame_length):
+		/* Need tmp storage for src_reg in case src_reg == dst_reg,
+		 * and a scratch reg */
+		scratch_reg = BPF_REG_9;
+		dst_reg = si->dst_reg;
+
+		if (dst_reg == scratch_reg)
+			scratch_reg--;
+
+		ctx_reg = (si->src_reg == si->dst_reg) ? scratch_reg - 1 : si->src_reg;
+		while (dst_reg == ctx_reg || scratch_reg == ctx_reg)
+			ctx_reg--;
+
+		/* Save scratch registers */

net/core/filter.c +58 -0

+
+		/* What does this code do?
+		 *   dst_reg = 0
+		 *
+		 *   if (!ctx_reg->mb)
+		 *      goto no_mb:
+		 *
+		 *   dst_reg = (struct xdp_shared_info *)xdp_data_hard_end(xdp)
+		 *   dst_reg = dst_reg->data_length
+		 *
+		 * NOTE: xdp_data_hard_end() is xdp->hard_start +
+		 *       xdp->frame_sz - sizeof(shared_info)
+		 *
+		 * no_mb:
+		 *   dst_reg += ctx_reg->data_end - ctx_reg->data
+		 */
+		*insn++ = BPF_MOV64_IMM(dst_reg, 0);
+
+		*insn++ = BPF_LDX_MEM(BPF_W, scratch_reg, ctx_reg, MB_OFFSET());
+		*insn++ = BPF_ALU32_IMM(BPF_AND, scratch_reg, MB_MASK);
+		*insn++ = BPF_JMP_IMM(BPF_JEQ, scratch_reg, 0, 7); /*goto no_mb; */
+
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_buff,
+						       data_hard_start),
+				      dst_reg, ctx_reg,
+				      offsetof(struct xdp_buff, data_hard_start));
+		*insn++ = BPF_LDX_MEM(BPF_W, scratch_reg, ctx_reg,
+				      FRAME_SZ_OFFSET());
+		*insn++ = BPF_ALU32_IMM(BPF_AND, scratch_reg, FRAME_SZ_MASK);
+		*insn++ = BPF_ALU32_IMM(BPF_RSH, scratch_reg, FRAME_SZ_SHIFT);
+		*insn++ = BPF_ALU64_REG(BPF_ADD, dst_reg, scratch_reg);
+		*insn++ = BPF_ALU64_IMM(BPF_SUB, dst_reg,
+					SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_shared_info,
+						       data_length),
+				      dst_reg, dst_reg,
+				      offsetof(struct xdp_shared_info,
+					       data_length));
+
+		/* no_mb: */
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_buff, data_end),
+				      scratch_reg, ctx_reg,
+				      offsetof(struct xdp_buff, data_end));
+		*insn++ = BPF_ALU64_REG(BPF_ADD, dst_reg, scratch_reg);
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_buff, data),
+				      scratch_reg, ctx_reg,
+				      offsetof(struct xdp_buff, data));
+		*insn++ = BPF_ALU64_REG(BPF_SUB, dst_reg, scratch_reg);
+
+		/* Restore scratch registers */
+		*insn++ = BPF_LDX_MEM(BPF_DW, scratch_reg, ctx_reg,
+				      offsetof(struct xdp_buff, tmp_reg[0]));
+
+		if (ctx_reg != si->src_reg)
+			*insn++ = BPF_LDX_MEM(BPF_DW, ctx_reg, ctx_reg,
+					      offsetof(struct xdp_buff,
+						       tmp_reg[1]));
+		break;
 	}
 
 	return insn - insn_buf;

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

index 30b477a26482..62c50ab28ea9 100644
@@ -4380,6 +4380,7 @@ struct xdp_md {
 	__u32 rx_queue_index;  /* rxq->queue_index  */
 
 	__u32 egress_ifindex;  /* txq->dev->ifindex */
+	__u32 frame_length;
 };
 
 /* DEVMAP map-value layout

Signature · 1 line

2.28.0

Quoted 5 lines · Lorenzo Bianconi

From: Eelco Chaudron <[email protected]>

This patch adds a new field to the XDP context called frame_length, which will hold the full length of the packet, including fragments if existing.

The approach you took for ctx access conversion is barely described :/

Quoted 41 lines

eBPF programs can determine if fragments are present using something like:

  if (ctx->data_end - ctx->data < ctx->frame_length) {
    /* Fragements exists. /*
  }

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

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

---
 include/net/xdp.h              | 22 +++++++++
 include/uapi/linux/bpf.h       |  1 +
 kernel/bpf/verifier.c          |  2 +-
 net/core/filter.c              | 83 
++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h |  1 +
 5 files changed, 108 insertions(+), 1 deletion(-)

include/net/xdp.h +14 -0

index 09078ab6644c..e54d733c90ed 100644
@@ -73,8 +73,30 @@ struct xdp_buff {
 	void *data_hard_start;
 	struct xdp_rxq_info *rxq;
 	struct xdp_txq_info *txq;
+	/* If any of the bitfield lengths for frame_sz or mb below change,
+	 * make sure the defines here are also updated!
+	 */
+#ifdef __BIG_ENDIAN_BITFIELD
+#define MB_SHIFT	  0
+#define MB_MASK		  0x00000001
+#define FRAME_SZ_SHIFT	  1
+#define FRAME_SZ_MASK	  0xfffffffe
+#else
+#define MB_SHIFT	  31
+#define MB_MASK		  0x80000000
+#define FRAME_SZ_SHIFT	  0
+#define FRAME_SZ_MASK	  0x7fffffff
+#endif

include/net/xdp.h struct xdp_buff { · line 95

Quoted 6 lines

include/net/xdp.h +4 -0

+#define FRAME_SZ_OFFSET() offsetof(struct xdp_buff, 
 __u32_bit_fields_offset)
+#define MB_OFFSET()	  offsetof(struct xdp_buff, 
 __u32_bit_fields_offset)
+	/* private: */
+	u32 __u32_bit_fields_offset[0];

Why? I don't get that. Please explain.

Also, looking at all the need for masking/shifting, I wonder if it would be better to have u32 frame_sz and u8 mb...

include/net/xdp.h struct xdp_buff { · line 99

Quoted 8 lines

include/net/xdp.h +1 -0

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

tailroom*/

include/net/xdp.h +2 -0

 	u32 mb:1; /* xdp non-linear buffer */
+
+	/* Temporary registers to make conditional access/stores possible. 

*/

include/net/xdp.h +1 -0

+	u64 tmp_reg[2];

IMHO this kills the bitfield approach we have for vars above.

Quoted 67 lines

};

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

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

index 30b477a26482..62c50ab28ea9 100644
@@ -4380,6 +4380,7 @@ struct xdp_md {
 	__u32 rx_queue_index;  /* rxq->queue_index  */
 
 	__u32 egress_ifindex;  /* txq->dev->ifindex */
+	__u32 frame_length;
 };
 
 /* DEVMAP map-value layout

kernel/bpf/verifier.c +1 -1

index 93def76cf32b..c50caea29fa2 100644
@@ -10526,7 +10526,7 @@ static int convert_ctx_accesses(struct 
 bpf_verifier_env *env)
 	const struct bpf_verifier_ops *ops = env->ops;
 	int i, cnt, size, ctx_field_size, delta = 0;
 	const int insn_cnt = env->prog->len;
-	struct bpf_insn insn_buf[16], *insn;
+	struct bpf_insn insn_buf[32], *insn;
 	u32 target_size, size_default, off;
 	struct bpf_prog *new_prog;

kernel/bpf/verifier.c +0 -0

 	enum bpf_access_type type;

net/core/filter.c +1 -0

index 4c4882d4d92c..278640db9e0a 100644
@@ -8908,6 +8908,7 @@ static u32 xdp_convert_ctx_access(enum 
 bpf_access_type type,
 				  struct bpf_insn *insn_buf,
 				  struct bpf_prog *prog, u32 *target_size)
 {
+	int ctx_reg, dst_reg, scratch_reg;
 	struct bpf_insn *insn = insn_buf;
 

net/core/filter.c +19 -0

 	switch (si->off) {

@@ -8954,6 +8955,88 @@ static u32 xdp_convert_ctx_access(enum 
 bpf_access_type type,
 		*insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg,
 				      offsetof(struct net_device, ifindex));
 		break;
+	case offsetof(struct xdp_md, frame_length):
+		/* Need tmp storage for src_reg in case src_reg == dst_reg,
+		 * and a scratch reg */
+		scratch_reg = BPF_REG_9;
+		dst_reg = si->dst_reg;
+
+		if (dst_reg == scratch_reg)
+			scratch_reg--;
+
+		ctx_reg = (si->src_reg == si->dst_reg) ? scratch_reg - 1 : 
 si->src_reg;
+		while (dst_reg == ctx_reg || scratch_reg == ctx_reg)
+			ctx_reg--;
+
+		/* Save scratch registers */
+		if (ctx_reg != si->src_reg) {
+			*insn++ = BPF_STX_MEM(BPF_DW, si->src_reg, ctx_reg,
+					      offsetof(struct xdp_buff,
+						       tmp_reg[1]));
+

net/core/filter.c static u32 xdp_convert_ctx_access(enum bpf_access_type type, · line 8981

Quoted 5 lines

net/core/filter.c +5 -0

+			*insn++ = BPF_MOV64_REG(ctx_reg, si->src_reg);
+		}
+
+		*insn++ = BPF_STX_MEM(BPF_DW, ctx_reg, scratch_reg,
+				      offsetof(struct xdp_buff, tmp_reg[0]));

Why don't you push regs to stack, use it and then pop it back? That way I suppose you could avoid polluting xdp_buff with tmp_reg[2].

Quoted 77 lines

net/core/filter.c +58 -0

+
+		/* What does this code do?
+		 *   dst_reg = 0
+		 *
+		 *   if (!ctx_reg->mb)
+		 *      goto no_mb:
+		 *
+		 *   dst_reg = (struct xdp_shared_info *)xdp_data_hard_end(xdp)
+		 *   dst_reg = dst_reg->data_length
+		 *
+		 * NOTE: xdp_data_hard_end() is xdp->hard_start +
+		 *       xdp->frame_sz - sizeof(shared_info)
+		 *
+		 * no_mb:
+		 *   dst_reg += ctx_reg->data_end - ctx_reg->data
+		 */
+		*insn++ = BPF_MOV64_IMM(dst_reg, 0);
+
+		*insn++ = BPF_LDX_MEM(BPF_W, scratch_reg, ctx_reg, MB_OFFSET());
+		*insn++ = BPF_ALU32_IMM(BPF_AND, scratch_reg, MB_MASK);
+		*insn++ = BPF_JMP_IMM(BPF_JEQ, scratch_reg, 0, 7); /*goto no_mb; 
 */
+
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_buff,
+						       data_hard_start),
+				      dst_reg, ctx_reg,
+				      offsetof(struct xdp_buff, data_hard_start));
+		*insn++ = BPF_LDX_MEM(BPF_W, scratch_reg, ctx_reg,
+				      FRAME_SZ_OFFSET());
+		*insn++ = BPF_ALU32_IMM(BPF_AND, scratch_reg, FRAME_SZ_MASK);
+		*insn++ = BPF_ALU32_IMM(BPF_RSH, scratch_reg, FRAME_SZ_SHIFT);
+		*insn++ = BPF_ALU64_REG(BPF_ADD, dst_reg, scratch_reg);
+		*insn++ = BPF_ALU64_IMM(BPF_SUB, dst_reg,
+					SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_shared_info,
+						       data_length),
+				      dst_reg, dst_reg,
+				      offsetof(struct xdp_shared_info,
+					       data_length));
+
+		/* no_mb: */
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_buff, data_end),
+				      scratch_reg, ctx_reg,
+				      offsetof(struct xdp_buff, data_end));
+		*insn++ = BPF_ALU64_REG(BPF_ADD, dst_reg, scratch_reg);
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_buff, data),
+				      scratch_reg, ctx_reg,
+				      offsetof(struct xdp_buff, data));
+		*insn++ = BPF_ALU64_REG(BPF_SUB, dst_reg, scratch_reg);
+
+		/* Restore scratch registers */
+		*insn++ = BPF_LDX_MEM(BPF_DW, scratch_reg, ctx_reg,
+				      offsetof(struct xdp_buff, tmp_reg[0]));
+
+		if (ctx_reg != si->src_reg)
+			*insn++ = BPF_LDX_MEM(BPF_DW, ctx_reg, ctx_reg,
+					      offsetof(struct xdp_buff,
+						       tmp_reg[1]));
+		break;

net/core/filter.c +0 -0

 	}
 
 	return insn - insn_buf;

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

index 30b477a26482..62c50ab28ea9 100644
@@ -4380,6 +4380,7 @@ struct xdp_md {
 	__u32 rx_queue_index;  /* rxq->queue_index  */
 
 	__u32 egress_ifindex;  /* txq->dev->ifindex */
+	__u32 frame_length;
 };
 
 /* DEVMAP map-value layout

Signature · 1 line

2.28.0

Quoted 8 lines · Maciej Fijalkowski

Quoted 5 lines · Lorenzo Bianconi

From: Eelco Chaudron <[email protected]>

This patch adds a new field to the XDP context called frame_length, which will hold the full length of the packet, including fragments if existing.

The approach you took for ctx access conversion is barely described :/

You are right, I should have added some details on why I have chosen to take this approach. The reason is, to avoid a dedicated entry in the xdp_frame structure and maintaining it in the various eBPF helpers.

I'll update the commit message in the next revision to include this.

Quoted 41 lines

Quoted 41 lines

eBPF programs can determine if fragments are present using something like:

  if (ctx->data_end - ctx->data < ctx->frame_length) {
    /* Fragements exists. /*
  }

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

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

---
 include/net/xdp.h              | 22 +++++++++
 include/uapi/linux/bpf.h       |  1 +
 kernel/bpf/verifier.c          |  2 +-
 net/core/filter.c              | 83
++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h |  1 +
 5 files changed, 108 insertions(+), 1 deletion(-)

include/net/xdp.h +14 -0

index 09078ab6644c..e54d733c90ed 100644
@@ -73,8 +73,30 @@ struct xdp_buff {
 	void *data_hard_start;
 	struct xdp_rxq_info *rxq;
 	struct xdp_txq_info *txq;
+	/* If any of the bitfield lengths for frame_sz or mb below change,
+	 * make sure the defines here are also updated!
+	 */
+#ifdef __BIG_ENDIAN_BITFIELD
+#define MB_SHIFT	  0
+#define MB_MASK		  0x00000001
+#define FRAME_SZ_SHIFT	  1
+#define FRAME_SZ_MASK	  0xfffffffe
+#else
+#define MB_SHIFT	  31
+#define MB_MASK		  0x80000000
+#define FRAME_SZ_SHIFT	  0
+#define FRAME_SZ_MASK	  0x7fffffff
+#endif

Quoted 8 lines

include/net/xdp.h struct xdp_buff { · line 95

Quoted 6 lines

include/net/xdp.h +4 -0

+#define FRAME_SZ_OFFSET() offsetof(struct xdp_buff,
 __u32_bit_fields_offset)
+#define MB_OFFSET()	  offsetof(struct xdp_buff,
 __u32_bit_fields_offset)
+	/* private: */
+	u32 __u32_bit_fields_offset[0];

Why? I don't get that. Please explain.

I was trying to find an easy way to extract the data/fields, maybe using BTF but had no luck.
So I resorted back to an existing approach in sk_buff, see
https://elixir.bootlin.com/linux/v5.10-rc7/source/include/linux/skbuff.h#L780

Quoted 2 lines

Also, looking at all the need for masking/shifting, I wonder if it would be better to have u32 frame_sz and u8 mb...

Yes, I agree having u32 would be way better, even for u32 for the mb field. I’ve seen other code converting flags to u32 for easy access in the eBPF context structures.

I’ll see there are some comments in general on the bit definitions for mb, but I’ll try to convince them to use u32 for both in the next revision, as I think for the xdp_buff structure size is not a real problem ;)

Quoted 11 lines

include/net/xdp.h struct xdp_buff { · line 99

Quoted 9 lines

include/net/xdp.h +1 -0

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

tailroom*/

include/net/xdp.h +2 -0

 	u32 mb:1; /* xdp non-linear buffer */
+
+	/* Temporary registers to make conditional access/stores possible.

*/

include/net/xdp.h +1 -0

+	u64 tmp_reg[2];

IMHO this kills the bitfield approach we have for vars above.

See above…

Quoted 68 lines

Quoted 68 lines

};

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

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

index 30b477a26482..62c50ab28ea9 100644
@@ -4380,6 +4380,7 @@ struct xdp_md {
 	__u32 rx_queue_index;  /* rxq->queue_index  */
 
 	__u32 egress_ifindex;  /* txq->dev->ifindex */
+	__u32 frame_length;
 };
 
 /* DEVMAP map-value layout

kernel/bpf/verifier.c +1 -1

index 93def76cf32b..c50caea29fa2 100644
@@ -10526,7 +10526,7 @@ static int convert_ctx_accesses(struct
 bpf_verifier_env *env)
 	const struct bpf_verifier_ops *ops = env->ops;
 	int i, cnt, size, ctx_field_size, delta = 0;
 	const int insn_cnt = env->prog->len;
-	struct bpf_insn insn_buf[16], *insn;
+	struct bpf_insn insn_buf[32], *insn;
 	u32 target_size, size_default, off;
 	struct bpf_prog *new_prog;

kernel/bpf/verifier.c +0 -0

 	enum bpf_access_type type;

net/core/filter.c +1 -0

index 4c4882d4d92c..278640db9e0a 100644
@@ -8908,6 +8908,7 @@ static u32 xdp_convert_ctx_access(enum
 bpf_access_type type,
 				  struct bpf_insn *insn_buf,
 				  struct bpf_prog *prog, u32 *target_size)
 {
+	int ctx_reg, dst_reg, scratch_reg;
 	struct bpf_insn *insn = insn_buf;
 

net/core/filter.c +20 -0

 	switch (si->off) {

@@ -8954,6 +8955,88 @@ static u32 xdp_convert_ctx_access(enum
 bpf_access_type type,
 		*insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg,
 				      offsetof(struct net_device, ifindex));
 		break;
+	case offsetof(struct xdp_md, frame_length):
+		/* Need tmp storage for src_reg in case src_reg == dst_reg,
+		 * and a scratch reg */
+		scratch_reg = BPF_REG_9;
+		dst_reg = si->dst_reg;
+
+		if (dst_reg == scratch_reg)
+			scratch_reg--;
+
+		ctx_reg = (si->src_reg == si->dst_reg) ? scratch_reg - 1 :
 si->src_reg;
+		while (dst_reg == ctx_reg || scratch_reg == ctx_reg)
+			ctx_reg--;
+
+		/* Save scratch registers */
+		if (ctx_reg != si->src_reg) {
+			*insn++ = BPF_STX_MEM(BPF_DW, si->src_reg, ctx_reg,
+					      offsetof(struct xdp_buff,
+						       tmp_reg[1]));
+
+			*insn++ = BPF_MOV64_REG(ctx_reg, si->src_reg);

Quoted 8 lines

net/core/filter.c static u32 xdp_convert_ctx_access(enum bpf_access_type type, · line 8981

Quoted 4 lines

net/core/filter.c +4 -0

+		}
+
+		*insn++ = BPF_STX_MEM(BPF_DW, ctx_reg, scratch_reg,
+				      offsetof(struct xdp_buff, tmp_reg[0]));

Why don't you push regs to stack, use it and then pop it back? That way I
suppose you could avoid polluting xdp_buff with tmp_reg[2].

There is no “real” stack in eBPF, only a read-only frame pointer, and as we are replacing a single instruction, we have no info on what we can use as scratch space.

Quoted 79 lines

Quoted 79 lines

net/core/filter.c +58 -0

+
+		/* What does this code do?
+		 *   dst_reg = 0
+		 *
+		 *   if (!ctx_reg->mb)
+		 *      goto no_mb:
+		 *
+		 *   dst_reg = (struct xdp_shared_info *)xdp_data_hard_end(xdp)
+		 *   dst_reg = dst_reg->data_length
+		 *
+		 * NOTE: xdp_data_hard_end() is xdp->hard_start +
+		 *       xdp->frame_sz - sizeof(shared_info)
+		 *
+		 * no_mb:
+		 *   dst_reg += ctx_reg->data_end - ctx_reg->data
+		 */
+		*insn++ = BPF_MOV64_IMM(dst_reg, 0);
+
+		*insn++ = BPF_LDX_MEM(BPF_W, scratch_reg, ctx_reg, MB_OFFSET());
+		*insn++ = BPF_ALU32_IMM(BPF_AND, scratch_reg, MB_MASK);
+		*insn++ = BPF_JMP_IMM(BPF_JEQ, scratch_reg, 0, 7); /*goto no_mb;
 */
+
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_buff,
+						       data_hard_start),
+				      dst_reg, ctx_reg,
+				      offsetof(struct xdp_buff, data_hard_start));
+		*insn++ = BPF_LDX_MEM(BPF_W, scratch_reg, ctx_reg,
+				      FRAME_SZ_OFFSET());
+		*insn++ = BPF_ALU32_IMM(BPF_AND, scratch_reg, FRAME_SZ_MASK);
+		*insn++ = BPF_ALU32_IMM(BPF_RSH, scratch_reg, FRAME_SZ_SHIFT);
+		*insn++ = BPF_ALU64_REG(BPF_ADD, dst_reg, scratch_reg);
+		*insn++ = BPF_ALU64_IMM(BPF_SUB, dst_reg,
+					SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_shared_info,
+						       data_length),
+				      dst_reg, dst_reg,
+				      offsetof(struct xdp_shared_info,
+					       data_length));
+
+		/* no_mb: */
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_buff, data_end),
+				      scratch_reg, ctx_reg,
+				      offsetof(struct xdp_buff, data_end));
+		*insn++ = BPF_ALU64_REG(BPF_ADD, dst_reg, scratch_reg);
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_buff, data),
+				      scratch_reg, ctx_reg,
+				      offsetof(struct xdp_buff, data));
+		*insn++ = BPF_ALU64_REG(BPF_SUB, dst_reg, scratch_reg);
+
+		/* Restore scratch registers */
+		*insn++ = BPF_LDX_MEM(BPF_DW, scratch_reg, ctx_reg,
+				      offsetof(struct xdp_buff, tmp_reg[0]));
+
+		if (ctx_reg != si->src_reg)
+			*insn++ = BPF_LDX_MEM(BPF_DW, ctx_reg, ctx_reg,
+					      offsetof(struct xdp_buff,
+						       tmp_reg[1]));
+		break;

net/core/filter.c +0 -0

 	}
 
 	return insn - insn_buf;

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

index 30b477a26482..62c50ab28ea9 100644
@@ -4380,6 +4380,7 @@ struct xdp_md {
 	__u32 rx_queue_index;  /* rxq->queue_index  */
 
 	__u32 egress_ifindex;  /* txq->dev->ifindex */
+	__u32 frame_length;
 };
 
 /* DEVMAP map-value layout

Signature · 1 line

2.28.0

Quoted 82 lines · Eelco Chaudron

Quoted 8 lines · Maciej Fijalkowski

Quoted 5 lines · Lorenzo Bianconi

From: Eelco Chaudron <[email protected]>

This patch adds a new field to the XDP context called frame_length, which will hold the full length of the packet, including fragments if existing.

The approach you took for ctx access conversion is barely described :/

You are right, I should have added some details on why I have chosen to take this approach. The reason is, to avoid a dedicated entry in the xdp_frame structure and maintaining it in the various eBPF helpers.

I'll update the commit message in the next revision to include this.

Quoted 42 lines

Quoted 42 lines

eBPF programs can determine if fragments are present using something like:

  if (ctx->data_end - ctx->data < ctx->frame_length) {
    /* Fragements exists. /*
  }

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

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

---
 include/net/xdp.h              | 22 +++++++++
 include/uapi/linux/bpf.h       |  1 +
 kernel/bpf/verifier.c          |  2 +-
 net/core/filter.c              | 83
++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h |  1 +
 5 files changed, 108 insertions(+), 1 deletion(-)

include/net/xdp.h +14 -0

index 09078ab6644c..e54d733c90ed 100644
@@ -73,8 +73,30 @@ struct xdp_buff {
 	void *data_hard_start;
 	struct xdp_rxq_info *rxq;
 	struct xdp_txq_info *txq;
+	/* If any of the bitfield lengths for frame_sz or mb below 
 change,
+	 * make sure the defines here are also updated!
+	 */
+#ifdef __BIG_ENDIAN_BITFIELD
+#define MB_SHIFT	  0
+#define MB_MASK		  0x00000001
+#define FRAME_SZ_SHIFT	  1
+#define FRAME_SZ_MASK	  0xfffffffe
+#else
+#define MB_SHIFT	  31
+#define MB_MASK		  0x80000000
+#define FRAME_SZ_SHIFT	  0
+#define FRAME_SZ_MASK	  0x7fffffff
+#endif

Quoted 8 lines

include/net/xdp.h struct xdp_buff { · line 96

Quoted 6 lines

include/net/xdp.h +4 -0

+#define FRAME_SZ_OFFSET() offsetof(struct xdp_buff,
 __u32_bit_fields_offset)
+#define MB_OFFSET()	  offsetof(struct xdp_buff,
 __u32_bit_fields_offset)
+	/* private: */
+	u32 __u32_bit_fields_offset[0];

Why? I don't get that. Please explain.

I was trying to find an easy way to extract the data/fields, maybe using BTF but had no luck.
So I resorted back to an existing approach in sk_buff, see
https://elixir.bootlin.com/linux/v5.10-rc7/source/include/linux/skbuff.h#L780

Quoted 2 lines

Also, looking at all the need for masking/shifting, I wonder if it would be better to have u32 frame_sz and u8 mb...

Yes, I agree having u32 would be way better, even for u32 for the mb field. I’ve seen other code converting flags to u32 for easy access in the eBPF context structures.

Quoted 3 lines

I’ll see there are some comments in general on the bit definitions for mb, but I’ll try to convince them to use u32 for both in the next revision, as I think for the xdp_buff structure size is not a real problem ;)

Generally people were really strict on xdp_buff extensions as we didn't want to end up with another skb-like monster. I think Jesper somewhere said that one cacheline is max for that. With your tmp_reg[2] you exceed that from what I see, but I might be short on coffee.

Quoted 92 lines

Quoted 12 lines

include/net/xdp.h struct xdp_buff { · line 99

Quoted 10 lines

include/net/xdp.h +1 -0

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

tailroom*/

include/net/xdp.h +2 -0

 	u32 mb:1; /* xdp non-linear buffer */
+
+	/* Temporary registers to make conditional access/stores 

possible.
*/

include/net/xdp.h +1 -0

+	u64 tmp_reg[2];

IMHO this kills the bitfield approach we have for vars above.

See above…

Quoted 68 lines

Quoted 68 lines

};

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

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

index 30b477a26482..62c50ab28ea9 100644
@@ -4380,6 +4380,7 @@ struct xdp_md {
 	__u32 rx_queue_index;  /* rxq->queue_index  */
 
 	__u32 egress_ifindex;  /* txq->dev->ifindex */
+	__u32 frame_length;
 };
 
 /* DEVMAP map-value layout

kernel/bpf/verifier.c +1 -1

index 93def76cf32b..c50caea29fa2 100644
@@ -10526,7 +10526,7 @@ static int convert_ctx_accesses(struct
 bpf_verifier_env *env)
 	const struct bpf_verifier_ops *ops = env->ops;
 	int i, cnt, size, ctx_field_size, delta = 0;
 	const int insn_cnt = env->prog->len;
-	struct bpf_insn insn_buf[16], *insn;
+	struct bpf_insn insn_buf[32], *insn;
 	u32 target_size, size_default, off;
 	struct bpf_prog *new_prog;

kernel/bpf/verifier.c +0 -0

 	enum bpf_access_type type;

net/core/filter.c +1 -0

index 4c4882d4d92c..278640db9e0a 100644
@@ -8908,6 +8908,7 @@ static u32 xdp_convert_ctx_access(enum
 bpf_access_type type,
 				  struct bpf_insn *insn_buf,
 				  struct bpf_prog *prog, u32 *target_size)
 {
+	int ctx_reg, dst_reg, scratch_reg;
 	struct bpf_insn *insn = insn_buf;
 

net/core/filter.c +20 -0

 	switch (si->off) {

@@ -8954,6 +8955,88 @@ static u32 xdp_convert_ctx_access(enum
 bpf_access_type type,
 		*insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg,
 				      offsetof(struct net_device, ifindex));
 		break;
+	case offsetof(struct xdp_md, frame_length):
+		/* Need tmp storage for src_reg in case src_reg == dst_reg,
+		 * and a scratch reg */
+		scratch_reg = BPF_REG_9;
+		dst_reg = si->dst_reg;
+
+		if (dst_reg == scratch_reg)
+			scratch_reg--;
+
+		ctx_reg = (si->src_reg == si->dst_reg) ? scratch_reg - 1 :
 si->src_reg;
+		while (dst_reg == ctx_reg || scratch_reg == ctx_reg)
+			ctx_reg--;
+
+		/* Save scratch registers */
+		if (ctx_reg != si->src_reg) {
+			*insn++ = BPF_STX_MEM(BPF_DW, si->src_reg, ctx_reg,
+					      offsetof(struct xdp_buff,
+						       tmp_reg[1]));
+
+			*insn++ = BPF_MOV64_REG(ctx_reg, si->src_reg);

Quoted 8 lines

net/core/filter.c static u32 xdp_convert_ctx_access(enum bpf_access_type type, · line 8981

Quoted 4 lines

net/core/filter.c +4 -0

+		}
+
+		*insn++ = BPF_STX_MEM(BPF_DW, ctx_reg, scratch_reg,
+				      offsetof(struct xdp_buff, tmp_reg[0]));

Why don't you push regs to stack, use it and then pop it back? That way I
suppose you could avoid polluting xdp_buff with tmp_reg[2].

Quoted 3 lines

There is no “real” stack in eBPF, only a read-only frame pointer, and as we are replacing a single instruction, we have no info on what we can use as scratch space.