mvneta: introduce XDP multi-buffer support

Acked-by Jesper Dangaard Brouer <[email protected]>

---
 include/linux/skbuff.h | 1 +
 1 file changed, 1 insertion(+)

include/linux/skbuff.h +1 -0

index 642acb0d1646..d6d2ca936ddf 100644
@@ -539,6 +539,7 @@ struct skb_shared_info {
 	 * Warning : all fields before dataref are cleared in __alloc_skb()
 	 */
 	atomic_t	dataref;
+	unsigned int	xdp_frags_size;
 
 	/* Intermediate layers must ensure that destructor_arg
 	 * remains valid until skb destructor */

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:54 UTC #2 lore raw

PATCH v21 2/23 xdp: introduce flags field in xdp_buff/xdp_frame

Introduce flags field in xdp_frame and xdp_buffer data structures to define additional buffer features. At the moment the only supported buffer feature is multi-buffer bit (mb). Multi-buffer bit is used to specify if this is a linear buffer (mb = 0) or a multi-buffer frame (mb = 1). In the latter case the driver is expected to initialize the skb_shared_info structure at the end of the first buffer to link together subsequent buffers belonging to the same frame.

Acked-by Toke Hoiland-Jorgensen <[email protected]>

Acked-by Jesper Dangaard Brouer <[email protected]>

---
 include/net/xdp.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

include/net/xdp.h +29 -0

index 8f0812e4996d..f73ef0e66a09 100644
@@ -66,6 +66,10 @@ struct xdp_txq_info {
 	struct net_device *dev;
 };
 
+enum xdp_buff_flags {
+	XDP_FLAGS_MULTI_BUFF	= BIT(0), /* non-linear xdp buff */
+};
+
 struct xdp_buff {
 	void *data;
 	void *data_end;

@@ -74,13 +78,30 @@ struct xdp_buff {
 	struct xdp_rxq_info *rxq;
 	struct xdp_txq_info *txq;
 	u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/
+	u32 flags; /* supported values defined in xdp_buff_flags */
 };
 
+static __always_inline bool xdp_buff_is_mb(struct xdp_buff *xdp)
+{
+	return !!(xdp->flags & XDP_FLAGS_MULTI_BUFF);
+}
+
+static __always_inline void xdp_buff_set_mb(struct xdp_buff *xdp)
+{
+	xdp->flags |= XDP_FLAGS_MULTI_BUFF;
+}
+
+static __always_inline void xdp_buff_clear_mb(struct xdp_buff *xdp)
+{
+	xdp->flags &= ~XDP_FLAGS_MULTI_BUFF;
+}
+
 static __always_inline void
 xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq)
 {
 	xdp->frame_sz = frame_sz;
 	xdp->rxq = rxq;
+	xdp->flags = 0;
 }
 
 static __always_inline void

@@ -122,8 +143,14 @@ struct xdp_frame {
 	 */
 	struct xdp_mem_info mem;
 	struct net_device *dev_rx; /* used by cpumap */
+	u32 flags; /* supported values defined in xdp_buff_flags */
 };
 
+static __always_inline bool xdp_frame_is_mb(struct xdp_frame *frame)
+{
+	return !!(frame->flags & XDP_FLAGS_MULTI_BUFF);
+}
+
 #define XDP_BULK_QUEUE_SIZE	16
 struct xdp_frame_bulk {
 	int count;

@@ -180,6 +207,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->flags = frame->flags;
 }
 
 static inline

@@ -206,6 +234,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->flags = xdp->flags;
 
 	return 0;
 }

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:54 UTC #3 lore raw

PATCH v21 3/23 net: mvneta: update mb bit before passing the xdp buffer to eBPF layer

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 in order to avoid possible cache-misses.

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

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

index 83c8908f0cc7..f97e764ec5b3 100644
@@ -2065,9 +2065,14 @@ mvneta_xdp_put_buff(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
 {
 	int i;
 
+	if (likely(!xdp_buff_is_mb(xdp)))
+		goto out;
+
 	for (i = 0; i < sinfo->nr_frags; i++)
 		page_pool_put_full_page(rxq->page_pool,
 					skb_frag_page(&sinfo->frags[i]), true);
+
+out:
 	page_pool_put_page(rxq->page_pool, virt_to_head_page(xdp->data),
 			   sync_len, true);
 }

@@ -2269,7 +2274,6 @@ mvneta_swbm_rx_frame(struct mvneta_port *pp,
 	int data_len = -MVNETA_MH_SIZE, len;
 	struct net_device *dev = pp->dev;
 	enum dma_data_direction dma_dir;
-	struct skb_shared_info *sinfo;
 
 	if (*size > MVNETA_MAX_RX_BUF_SIZE) {
 		len = MVNETA_MAX_RX_BUF_SIZE;

@@ -2289,11 +2293,9 @@ mvneta_swbm_rx_frame(struct mvneta_port *pp,
 
 	/* Prefetch header */
 	prefetch(data);
+	xdp_buff_clear_mb(xdp);
 	xdp_prepare_buff(xdp, data, pp->rx_offset_correction + MVNETA_MH_SIZE,
 			 data_len, false);
-
-	sinfo = xdp_get_shared_info_from_buff(xdp);
-	sinfo->nr_frags = 0;
 }
 
 static void

@@ -2327,6 +2329,9 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp,
 		skb_frag_off_set(frag, pp->rx_offset_correction);
 		skb_frag_size_set(frag, data_len);
 		__skb_frag_set_page(frag, page);
+
+		if (!xdp_buff_is_mb(xdp))
+			xdp_buff_set_mb(xdp);
 	} else {
 		page_pool_put_full_page(rxq->page_pool, page, true);
 	}

@@ -2348,8 +2353,12 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
 		      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 sk_buff *skb;
+	u8 num_frags;
+	int i;
+
+	if (unlikely(xdp_buff_is_mb(xdp)))
+		num_frags = sinfo->nr_frags;
 
 	skb = build_skb(xdp->data_hard_start, PAGE_SIZE);
 	if (!skb)

@@ -2361,6 +2370,9 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
 	skb_put(skb, xdp->data_end - xdp->data);
 	skb->ip_summed = mvneta_rx_csum(pp, desc_status);
 
+	if (likely(!xdp_buff_is_mb(xdp)))
+		goto out;
+
 	for (i = 0; i < num_frags; i++) {
 		skb_frag_t *frag = &sinfo->frags[i];

@@ -2369,6 +2381,7 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
 				skb_frag_size(frag), PAGE_SIZE);
 	}
 
+out:
 	return skb;
 }

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:54 UTC #4 lore raw

PATCH v21 4/23 net: mvneta: simplify mvneta_swbm_add_rx_fragment management

Relying on xdp mb bit, remove skb_shared_info structure allocated on the stack in mvneta_rx_swbm routine and simplify mvneta_swbm_add_rx_fragment accessing skb_shared_info in the xdp_buff structure directly. There is no performance penalty in this approach since mvneta_swbm_add_rx_fragment is run just for multi-buff use-case.

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 42 ++++++++++-----------------
 1 file changed, 15 insertions(+), 27 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +15 ~~-27~~

index f97e764ec5b3..775ffd91b741 100644
@@ -2060,9 +2060,9 @@ int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq)
 
 static void
 mvneta_xdp_put_buff(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
-		    struct xdp_buff *xdp, struct skb_shared_info *sinfo,
-		    int sync_len)
+		    struct xdp_buff *xdp, int sync_len)
 {
+	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
 	int i;
 
 	if (likely(!xdp_buff_is_mb(xdp)))

@@ -2210,7 +2210,6 @@ 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;

@@ -2231,7 +2230,7 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
 
 		err = xdp_do_redirect(pp->dev, xdp, prog);
 		if (unlikely(err)) {
-			mvneta_xdp_put_buff(pp, rxq, xdp, sinfo, sync);
+			mvneta_xdp_put_buff(pp, rxq, xdp, sync);
 			ret = MVNETA_XDP_DROPPED;
 		} else {
 			ret = MVNETA_XDP_REDIR;

@@ -2242,7 +2241,7 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
 	case XDP_TX:
 		ret = mvneta_xdp_xmit_back(pp, xdp);
 		if (ret != MVNETA_XDP_TX)
-			mvneta_xdp_put_buff(pp, rxq, xdp, sinfo, sync);
+			mvneta_xdp_put_buff(pp, rxq, xdp, sync);
 		break;
 	default:
 		bpf_warn_invalid_xdp_action(pp->dev, prog, act);

@@ -2251,7 +2250,7 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
 		trace_xdp_exception(pp->dev, prog, act);
 		fallthrough;
 	case XDP_DROP:
-		mvneta_xdp_put_buff(pp, rxq, xdp, sinfo, sync);
+		mvneta_xdp_put_buff(pp, rxq, xdp, sync);
 		ret = MVNETA_XDP_DROPPED;
 		stats->xdp_drop++;
 		break;

@@ -2303,9 +2302,9 @@ 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 page *page)
 {
+	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
 	struct net_device *dev = pp->dev;
 	enum dma_data_direction dma_dir;
 	int data_len, len;

@@ -2323,8 +2322,11 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp,
 				len, dma_dir);
 	rx_desc->buf_phys_addr = 0;
 
-	if (data_len > 0 && xdp_sinfo->nr_frags < MAX_SKB_FRAGS) {
-		skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo->nr_frags++];
+	if (!xdp_buff_is_mb(xdp))
+		sinfo->nr_frags = 0;
+
+	if (data_len > 0 && sinfo->nr_frags < MAX_SKB_FRAGS) {
+		skb_frag_t *frag = &sinfo->frags[sinfo->nr_frags++];
 
 		skb_frag_off_set(frag, pp->rx_offset_correction);
 		skb_frag_size_set(frag, data_len);

@@ -2335,16 +2337,6 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp,
 	} else {
 		page_pool_put_full_page(rxq->page_pool, page, true);
 	}
-
-	/* last fragment */
-	if (len == *size) {
-		struct skb_shared_info *sinfo;
-
-		sinfo = xdp_get_shared_info_from_buff(xdp);
-		sinfo->nr_frags = xdp_sinfo->nr_frags;
-		memcpy(sinfo->frags, xdp_sinfo->frags,
-		       sinfo->nr_frags * sizeof(skb_frag_t));
-	}
 	*size -= len;
 }

@@ -2392,7 +2384,6 @@ 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 mvneta_stats ps = {};
 	struct bpf_prog *xdp_prog;
 	u32 desc_status, frame_sz;

@@ -2401,8 +2392,6 @@ static int mvneta_rx_swbm(struct napi_struct *napi,
 	xdp_init_buff(&xdp_buf, PAGE_SIZE, &rxq->xdp_rxq);
 	xdp_buf.data_hard_start = NULL;
 
-	sinfo.nr_frags = 0;
-
 	/* Get number of received packets */
 	rx_todo = mvneta_rxq_busy_desc_num_get(pp, rxq);

@@ -2444,7 +2433,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, page);
 		} /* Middle or Last descriptor */
 
 		if (!(rx_status & MVNETA_RXD_LAST_DESC))

@@ -2452,7 +2441,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, -1);
 			goto next;
 		}

@@ -2464,7 +2453,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, -1);
 
 			u64_stats_update_begin(&stats->syncp);
 			stats->es.skb_alloc_error++;

@@ -2481,11 +2470,10 @@ 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;
 	}
 
 	if (xdp_buf.data_hard_start)
-		mvneta_xdp_put_buff(pp, rxq, &xdp_buf, &sinfo, -1);
+		mvneta_xdp_put_buff(pp, rxq, &xdp_buf, -1);
 
 	if (ps.xdp_redirect)
 		xdp_do_flush_map();

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:54 UTC #5 lore raw

PATCH v21 5/23 net: xdp: add xdp_update_skb_shared_info utility routine

Introduce xdp_update_skb_shared_info routine to update frags array metadata in skb_shared_info data structure converting to a skb from a xdp_buff or xdp_frame.
According to the current skb_shared_info architecture in xdp_frame/xdp_buff and to the xdp multi-buff support, there is
no need to run skb_add_rx_frag() and reset frags array converting the buffer to a skb since the frag array will be in the same position for xdp_buff/xdp_frame and for the skb, we just need to update memory metadata.
Introduce XDP_FLAGS_PF_MEMALLOC flag in xdp_buff_flags in order to mark the xdp_buff or xdp_frame as under memory-pressure if pages of the frags array are under memory pressure. Doing so we can avoid looping over all fragments in xdp_update_skb_shared_info routine. The driver is expected to set the flag constructing the xdp_buffer using xdp_buff_set_frag_pfmemalloc utility routine.
Rely on xdp_update_skb_shared_info in __xdp_build_skb_from_frame routine converting the multi-buff xdp_frame to a skb after performing a XDP_REDIRECT.

Acked-by Toke Hoiland-Jorgensen <[email protected]>

Acked-by Jesper Dangaard Brouer <[email protected]>

---
 include/net/xdp.h | 33 ++++++++++++++++++++++++++++++++-
 net/core/xdp.c    | 12 ++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

include/net/xdp.h +32 -1

index f73ef0e66a09..6cd9437e52c6 100644
@@ -67,7 +67,10 @@ struct xdp_txq_info {
 };
 
 enum xdp_buff_flags {
-	XDP_FLAGS_MULTI_BUFF	= BIT(0), /* non-linear xdp buff */
+	XDP_FLAGS_MULTI_BUFF		= BIT(0), /* non-linear xdp buff */
+	XDP_FLAGS_FRAGS_PF_MEMALLOC	= BIT(1), /* xdp multi-buff paged memory
+						   * is under pressure
+						   */
 };
 
 struct xdp_buff {

@@ -96,6 +99,16 @@ static __always_inline void xdp_buff_clear_mb(struct xdp_buff *xdp)
 	xdp->flags &= ~XDP_FLAGS_MULTI_BUFF;
 }
 
+static __always_inline bool xdp_buff_is_frag_pfmemalloc(struct xdp_buff *xdp)
+{
+	return !!(xdp->flags & XDP_FLAGS_FRAGS_PF_MEMALLOC);
+}
+
+static __always_inline void xdp_buff_set_frag_pfmemalloc(struct xdp_buff *xdp)
+{
+	xdp->flags |= XDP_FLAGS_FRAGS_PF_MEMALLOC;
+}
+
 static __always_inline void
 xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq)
 {

@@ -151,6 +164,11 @@ static __always_inline bool xdp_frame_is_mb(struct xdp_frame *frame)
 	return !!(frame->flags & XDP_FLAGS_MULTI_BUFF);
 }
 
+static __always_inline bool xdp_frame_is_frag_pfmemalloc(struct xdp_frame *frame)
+{
+	return !!(frame->flags & XDP_FLAGS_FRAGS_PF_MEMALLOC);
+}
+
 #define XDP_BULK_QUEUE_SIZE	16
 struct xdp_frame_bulk {
 	int count;

@@ -186,6 +204,19 @@ static inline void xdp_scrub_frame(struct xdp_frame *frame)
 	frame->dev_rx = NULL;
 }
 
+static inline void
+xdp_update_skb_shared_info(struct sk_buff *skb, u8 nr_frags,
+			   unsigned int size, unsigned int truesize,
+			   bool pfmemalloc)
+{
+	skb_shinfo(skb)->nr_frags = nr_frags;
+
+	skb->len += size;
+	skb->data_len += size;
+	skb->truesize += truesize;
+	skb->pfmemalloc |= pfmemalloc;
+}
+
 /* Avoids inlining WARN macro in fast-path */
 void xdp_warn(const char *msg, const char *func, const int line);
 #define XDP_WARN(msg) xdp_warn(msg, __func__, __LINE__)

net/core/xdp.c +12 -0

index 7aba35504986..3262cfed7055 100644
@@ -561,8 +561,14 @@ struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf,
 					   struct sk_buff *skb,
 					   struct net_device *dev)
 {
+	struct skb_shared_info *sinfo = xdp_get_shared_info_from_frame(xdpf);
 	unsigned int headroom, frame_size;
 	void *hard_start;
+	u8 nr_frags;
+
+	/* xdp multi-buff frame */
+	if (unlikely(xdp_frame_is_mb(xdpf)))
+		nr_frags = sinfo->nr_frags;
 
 	/* Part of headroom was reserved to xdpf */
 	headroom = sizeof(*xdpf) + xdpf->headroom;

@@ -582,6 +588,12 @@ struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf,
 	if (xdpf->metasize)
 		skb_metadata_set(skb, xdpf->metasize);
 
+	if (unlikely(xdp_frame_is_mb(xdpf)))
+		xdp_update_skb_shared_info(skb, nr_frags,
+					   sinfo->xdp_frags_size,
+					   nr_frags * xdpf->frame_sz,
+					   xdp_frame_is_frag_pfmemalloc(xdpf));
+
 	/* Essential SKB info: protocol and skb->dev */
 	skb->protocol = eth_type_trans(skb, dev);

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:54 UTC #6 lore raw

PATCH v21 6/23 net: marvell: rely on xdp_update_skb_shared_info utility routine

Rely on xdp_update_skb_shared_info routine in order to avoid resetting frags array in skb_shared_info structure building the skb in mvneta_swbm_build_skb(). Frags array is expected to be initialized by the receiving driver building the xdp_buff and here we just need to update memory metadata.

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +10 ~~-13~~

index 775ffd91b741..267a306d9c75 100644
@@ -2332,8 +2332,12 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp,
 		skb_frag_size_set(frag, data_len);
 		__skb_frag_set_page(frag, page);
 
-		if (!xdp_buff_is_mb(xdp))
+		if (!xdp_buff_is_mb(xdp)) {
+			sinfo->xdp_frags_size = *size;
 			xdp_buff_set_mb(xdp);
+		}
+		if (page_is_pfmemalloc(page))
+			xdp_buff_set_frag_pfmemalloc(xdp);
 	} else {
 		page_pool_put_full_page(rxq->page_pool, page, true);
 	}

@@ -2347,7 +2351,6 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
 	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
 	struct sk_buff *skb;
 	u8 num_frags;
-	int i;
 
 	if (unlikely(xdp_buff_is_mb(xdp)))
 		num_frags = sinfo->nr_frags;

@@ -2362,18 +2365,12 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
 	skb_put(skb, xdp->data_end - xdp->data);
 	skb->ip_summed = mvneta_rx_csum(pp, desc_status);
 
-	if (likely(!xdp_buff_is_mb(xdp)))
-		goto out;
-
-	for (i = 0; i < num_frags; i++) {
-		skb_frag_t *frag = &sinfo->frags[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);
-	}
+	if (unlikely(xdp_buff_is_mb(xdp)))
+		xdp_update_skb_shared_info(skb, num_frags,
+					   sinfo->xdp_frags_size,
+					   num_frags * xdp->frame_sz,
+					   xdp_buff_is_frag_pfmemalloc(xdp));
 
-out:
 	return skb;
 }

Signature · 1 line

2.33.1

Andy Gospodarek

2022-01-10 16:37 UTC #7 lore raw

Re: [PATCH v21 bpf-next 06/23] net: marvell: rely on xdp_update_skb_shared_info utility routine

Quoted 42 lines · Lorenzo Bianconi

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

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

index 775ffd91b741..267a306d9c75 100644
@@ -2332,8 +2332,12 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp,
 		skb_frag_size_set(frag, data_len);
 		__skb_frag_set_page(frag, page);
 
-		if (!xdp_buff_is_mb(xdp))
+		if (!xdp_buff_is_mb(xdp)) {
+			sinfo->xdp_frags_size = *size;
 			xdp_buff_set_mb(xdp);
+		}
+		if (page_is_pfmemalloc(page))
+			xdp_buff_set_frag_pfmemalloc(xdp);
 	} else {
 		page_pool_put_full_page(rxq->page_pool, page, true);
 	}

@@ -2347,7 +2351,6 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
 	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
 	struct sk_buff *skb;
 	u8 num_frags;
-	int i;
 
 	if (unlikely(xdp_buff_is_mb(xdp)))
 		num_frags = sinfo->nr_frags;

@@ -2362,18 +2365,12 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
 	skb_put(skb, xdp->data_end - xdp->data);
 	skb->ip_summed = mvneta_rx_csum(pp, desc_status);

drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool, · line 2373

Quoted 10 lines

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

 
-	if (likely(!xdp_buff_is_mb(xdp)))
-		goto out;
-
-	for (i = 0; i < num_frags; i++) {
-		skb_frag_t *frag = &sinfo->frags[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);

Maybe I'm missing something but I'm not sure you have a suitable replacement for the 3 lines above this in your proposed change.

drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool, · line 2373

Quoted 7 lines

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

-	}
+	if (unlikely(xdp_buff_is_mb(xdp)))
+		xdp_update_skb_shared_info(skb, num_frags,
+					   sinfo->xdp_frags_size,
+					   num_frags * xdp->frame_sz,
+					   xdp_buff_is_frag_pfmemalloc(xdp));

When I did an implementation of this on a different driver I also needed to add:

	for (i = 0; i < num_frags; i++)
		skb_frag_set_page(skb, i, skb_frag_page(&sinfo->frags[i]));

to make sure that frames that were given XDP_PASS were formatted correctly so they could be handled by the stack. Don't you need something similar to make sure frags are properly set?

Thanks,

-andy

P.S. Sorry for noticing this so late in the process; I realize this version was just a rebase of v20 and this would have been useful information earlier if I'm correct.

Quoted 7 lines

drivers/net/ethernet/marvell/mvneta.c +0 -1
-out:
 	return skb;
 }
 
Signature · 1 line
2.33.1

Lorenzo Bianconi

2022-01-11 13:05 UTC #8 lore raw

Re: [PATCH v21 bpf-next 06/23] net: marvell: rely on xdp_update_skb_shared_info utility routine

Quoted 45 lines · Lorenzo Bianconi

Quoted 43 lines

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

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

index 775ffd91b741..267a306d9c75 100644
@@ -2332,8 +2332,12 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp,
              skb_frag_size_set(frag, data_len);
              __skb_frag_set_page(frag, page);
 
-             if (!xdp_buff_is_mb(xdp))
+             if (!xdp_buff_is_mb(xdp)) {
+                     sinfo->xdp_frags_size = *size;
                      xdp_buff_set_mb(xdp);
+             }
+             if (page_is_pfmemalloc(page))
+                     xdp_buff_set_frag_pfmemalloc(xdp);
      } else {
              page_pool_put_full_page(rxq->page_pool, page, true);
      }

@@ -2347,7 +2351,6 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
      struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
      struct sk_buff *skb;
      u8 num_frags;
-     int i;
 
      if (unlikely(xdp_buff_is_mb(xdp)))
              num_frags = sinfo->nr_frags;

@@ -2362,18 +2365,12 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
      skb_put(skb, xdp->data_end - xdp->data);
      skb->ip_summed = mvneta_rx_csum(pp, desc_status);

Quoted 13 lines

drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool, · line 2373
Quoted 9 lines
drivers/net/ethernet/marvell/mvneta.c +0 -9
-     if (likely(!xdp_buff_is_mb(xdp)))
-             goto out;
-
-     for (i = 0; i < num_frags; i++) {
-             skb_frag_t *frag = &sinfo->frags[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);
Maybe I'm missing something but I'm not sure you have a suitable replacement for the 3 lines above this in your proposed change.

Hi Andy,

mvneta_swbm_add_rx_fragment() initializes frags array in skb_shared_info for xdp whenever we receive a multi-descriptors frame. Since frags array is at the same offset for the xdp_buff and for the new skb and build_skb() in mvneta_swbm_build_skb() does not overwrite it, we do not need to initialize it again allocating the skb, just account metadata info running xdp_update_skb_shared_info(). Agree?

Quoted 18 lines

drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool, · line 2373
Quoted 7 lines
drivers/net/ethernet/marvell/mvneta.c +5 -1
-     }
+     if (unlikely(xdp_buff_is_mb(xdp)))
+             xdp_update_skb_shared_info(skb, num_frags,
+                                        sinfo->xdp_frags_size,
+                                        num_frags * xdp->frame_sz,
+                                        xdp_buff_is_frag_pfmemalloc(xdp));
 
When I did an implementation of this on a different driver I also needed to add:
        for (i = 0; i < num_frags; i++)
                skb_frag_set_page(skb, i, skb_frag_page(&sinfo->frags[i]));
to make sure that frames that were given XDP_PASS were formatted correctly so they could be handled by the stack. Don't you need something similar to make sure frags are properly set?

Quoted 8 lines

Thanks,
-andy
P.S. Sorry for noticing this so late in the process; I realize this version was just a rebase of v20 and this would have been useful information earlier if I'm correct.

no worries :)

Regards,
Lorenzo

Quoted 7 lines

Quoted 7 lines
drivers/net/ethernet/marvell/mvneta.c +0 -2
-out:
      return skb;
 }
 
--
2.33.1

Andy Gospodarek

2022-01-11 14:30 UTC #9 lore raw

Re: [PATCH v21 bpf-next 06/23] net: marvell: rely on xdp_update_skb_shared_info utility routine

Quoted 59 lines · Lorenzo Bianconi

Quoted 45 lines · Lorenzo Bianconi

Quoted 43 lines

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

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

index 775ffd91b741..267a306d9c75 100644
@@ -2332,8 +2332,12 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp,
              skb_frag_size_set(frag, data_len);
              __skb_frag_set_page(frag, page);
 
-             if (!xdp_buff_is_mb(xdp))
+             if (!xdp_buff_is_mb(xdp)) {
+                     sinfo->xdp_frags_size = *size;
                      xdp_buff_set_mb(xdp);
+             }
+             if (page_is_pfmemalloc(page))
+                     xdp_buff_set_frag_pfmemalloc(xdp);
      } else {
              page_pool_put_full_page(rxq->page_pool, page, true);
      }

@@ -2347,7 +2351,6 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
      struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
      struct sk_buff *skb;
      u8 num_frags;
-     int i;
 
      if (unlikely(xdp_buff_is_mb(xdp)))
              num_frags = sinfo->nr_frags;

@@ -2362,18 +2365,12 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
      skb_put(skb, xdp->data_end - xdp->data);
      skb->ip_summed = mvneta_rx_csum(pp, desc_status);

Quoted 13 lines

drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool, · line 2373
Quoted 9 lines
drivers/net/ethernet/marvell/mvneta.c +0 -9
-     if (likely(!xdp_buff_is_mb(xdp)))
-             goto out;
-
-     for (i = 0; i < num_frags; i++) {
-             skb_frag_t *frag = &sinfo->frags[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);
Maybe I'm missing something but I'm not sure you have a suitable replacement for the 3 lines above this in your proposed change.

Quoted 9 lines

Hi Andy,
mvneta_swbm_add_rx_fragment() initializes frags array in skb_shared_info for xdp whenever we receive a multi-descriptors frame. Since frags array is at the same offset for the xdp_buff and for the new skb and build_skb() in mvneta_swbm_build_skb() does not overwrite it, we do not need to initialize it again allocating the skb, just account metadata info running xdp_update_skb_shared_info(). Agree?

Lorenzo,

Thanks for the explanation; I do agree. I was thinking about this last night and suspected this was the case. My current implementation doesn't use build_skb to reuse the DMA buffer; it allocates a new skb->data area for passing up the stack. This is why I needed the skb_frag_set_page calls and you did not. That may change, but the first implementation will probably continue on that path.

Overall this set looks pretty solid to me. Thanks to you and other contributors for all the work on this!

-andy

Quoted 40 lines

Quoted 18 lines
drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool, · line 2373
Quoted 7 lines
drivers/net/ethernet/marvell/mvneta.c +5 -1
-     }
+     if (unlikely(xdp_buff_is_mb(xdp)))
+             xdp_update_skb_shared_info(skb, num_frags,
+                                        sinfo->xdp_frags_size,
+                                        num_frags * xdp->frame_sz,
+                                        xdp_buff_is_frag_pfmemalloc(xdp));
 
When I did an implementation of this on a different driver I also needed to add:
        for (i = 0; i < num_frags; i++)
                skb_frag_set_page(skb, i, skb_frag_page(&sinfo->frags[i]));
to make sure that frames that were given XDP_PASS were formatted correctly so they could be handled by the stack. Don't you need something similar to make sure frags are properly set?
Quoted 8 lines
Thanks,
-andy
P.S. Sorry for noticing this so late in the process; I realize this version was just a rebase of v20 and this would have been useful information earlier if I'm correct.
no worries :)
Regards,
Lorenzo
Quoted 7 lines
Quoted 7 lines
drivers/net/ethernet/marvell/mvneta.c +0 -2
-out:
      return skb;
 }
 
--
2.33.1

Lorenzo Bianconi

author

2022-01-08 11:54 UTC #10 lore raw

PATCH v21 7/23 xdp: add multi-buff support to xdp_return_{buff/frame}

Take into account if the received xdp_buff/xdp_frame is non-linear recycling/returning the frame memory to the allocator or into xdp_frame_bulk.

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 include/net/xdp.h | 18 ++++++++++++++--
 net/core/xdp.c    | 54 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 69 insertions(+), 3 deletions(-)

include/net/xdp.h +16 -2

index 6cd9437e52c6..8409a2fb1c69 100644
@@ -306,10 +306,24 @@ void __xdp_release_frame(void *data, struct xdp_mem_info *mem);
 static inline void xdp_release_frame(struct xdp_frame *xdpf)
 {
 	struct xdp_mem_info *mem = &xdpf->mem;
+	struct skb_shared_info *sinfo;
+	int i;
 
 	/* Curr only page_pool needs this */
-	if (mem->type == MEM_TYPE_PAGE_POOL)
-		__xdp_release_frame(xdpf->data, mem);
+	if (mem->type != MEM_TYPE_PAGE_POOL)
+		return;
+
+	if (likely(!xdp_frame_is_mb(xdpf)))
+		goto out;
+
+	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_release_frame(page_address(page), mem);
+	}
+out:
+	__xdp_release_frame(xdpf->data, mem);
 }
 
 int xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq,

net/core/xdp.c +53 -1

84 lines not on this page · raw message

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:54 UTC #11 lore raw

PATCH v21 8/23 net: mvneta: add multi buffer support to XDP_TX

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

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 112 +++++++++++++++++---------
 1 file changed, 76 insertions(+), 36 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +76 ~~-36~~

index 267a306d9c75..94f22563695d 100644
@@ -1884,8 +1884,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) {
 			if (napi && buf->type == MVNETA_TYPE_XDP_TX)
 				xdp_return_frame_rx_napi(buf->xdpf);
 			else

@@ -2079,47 +2079,87 @@ 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 skb_shared_info *sinfo = xdp_get_shared_info_from_frame(xdpf);
+	struct device *dev = pp->dev->dev.parent;
+	struct mvneta_tx_desc *tx_desc = NULL;
+	int i, num_frames = 1;
+	struct page *page;
+
+	if (unlikely(xdp_frame_is_mb(xdpf)))
+		num_frames += sinfo->nr_frags;
 
-	if (txq->count >= txq->tx_stop_threshold)
+	if (txq->count + num_frames >= txq->size)
 		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 = NULL;
+		int len = 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;
+		if (unlikely(i)) { /* paged area */
+			frag = &sinfo->frags[i - 1];
+			len = skb_frag_size(frag);
 		}
-		buf->type = MVNETA_TYPE_XDP_NDO;
-	} else {
-		struct page *page = virt_to_page(xdpf->data);
 
-		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);
-		buf->type = MVNETA_TYPE_XDP_TX;
+		tx_desc = mvneta_txq_next_desc_get(txq);
+		if (dma_map) {
+			/* ndo_xdp_xmit */
+			void *data;
+
+			data = unlikely(frag) ? skb_frag_address(frag)
+					      : xdpf->data;
+			dma_addr = dma_map_single(dev, data, len,
+						  DMA_TO_DEVICE);
+			if (dma_mapping_error(dev, dma_addr)) {
+				mvneta_txq_desc_put(txq);
+				goto unmap;
+			}
+
+			buf->type = MVNETA_TYPE_XDP_NDO;
+		} else {
+			page = unlikely(frag) ? skb_frag_page(frag)
+					      : virt_to_page(xdpf->data);
+			dma_addr = page_pool_get_dma_addr(page);
+			if (unlikely(frag))
+				dma_addr += skb_frag_off(frag);
+			else
+				dma_addr += sizeof(*xdpf) + xdpf->headroom;
+			dma_sync_single_for_device(dev, dma_addr, len,
+						   DMA_BIDIRECTIONAL);
+			buf->type = MVNETA_TYPE_XDP_TX;
+		}
+		buf->xdpf = unlikely(i) ? NULL : xdpf;
+
+		tx_desc->command = unlikely(i) ? 0 : MVNETA_TXD_F_DESC;
+		tx_desc->buf_phys_addr = dma_addr;
+		tx_desc->data_size = len;
+		*nxmit_byte += 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 (likely(tx_desc))
+		tx_desc->command |= MVNETA_TXD_L_DESC | MVNETA_TXD_Z_PAD;
 
-	mvneta_txq_inc_put(txq);
-	txq->pending++;
-	txq->count++;
+	txq->pending += num_frames;
+	txq->count += num_frames;
 
 	return MVNETA_XDP_TX;
+
+unmap:
+	for (i--; i >= 0; i--) {
+		mvneta_txq_desc_put(txq);
+		tx_desc = txq->descs + txq->next_desc_to_proc;
+		dma_unmap_single(dev, tx_desc->buf_phys_addr,
+				 tx_desc->data_size,
+				 DMA_TO_DEVICE);
+	}
+
+	return MVNETA_XDP_DROPPED;
 }
 
 static int

@@ -2128,8 +2168,8 @@ mvneta_xdp_xmit_back(struct mvneta_port *pp, struct xdp_buff *xdp)
 	struct mvneta_pcpu_stats *stats = this_cpu_ptr(pp->stats);
 	struct mvneta_tx_queue *txq;
 	struct netdev_queue *nq;
+	int cpu, nxmit_byte = 0;
 	struct xdp_frame *xdpf;
-	int cpu;
 	u32 ret;
 
 	xdpf = xdp_convert_buff_to_frame(xdp);

@@ -2141,10 +2181,10 @@ mvneta_xdp_xmit_back(struct mvneta_port *pp, struct xdp_buff *xdp)
 	nq = netdev_get_tx_queue(pp->dev, txq->id);
 
 	__netif_tx_lock(nq, cpu);
-	ret = mvneta_xdp_submit_frame(pp, txq, xdpf, false);
+	ret = mvneta_xdp_submit_frame(pp, txq, xdpf, &nxmit_byte, false);
 	if (ret == MVNETA_XDP_TX) {
 		u64_stats_update_begin(&stats->syncp);
-		stats->es.ps.tx_bytes += xdpf->len;
+		stats->es.ps.tx_bytes += nxmit_byte;
 		stats->es.ps.tx_packets++;
 		stats->es.ps.xdp_tx++;
 		u64_stats_update_end(&stats->syncp);

@@ -2183,11 +2223,11 @@ mvneta_xdp_xmit(struct net_device *dev, int num_frame,
 
 	__netif_tx_lock(nq, cpu);
 	for (i = 0; i < num_frame; i++) {
-		ret = mvneta_xdp_submit_frame(pp, txq, frames[i], true);
+		ret = mvneta_xdp_submit_frame(pp, txq, frames[i], &nxmit_byte,
+					      true);
 		if (ret != MVNETA_XDP_TX)
 			break;
 
-		nxmit_byte += frames[i]->len;
 		nxmit++;
 	}

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:54 UTC #12 lore raw

PATCH v21 9/23 bpf: introduce BPF_F_XDP_MB flag in prog_flags loading the ebpf program

Introduce BPF_F_XDP_MB and the related field in bpf_prog_aux in order to notify the driver the loaded program support xdp multi-buffer.

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 include/linux/bpf.h            | 1 +
 include/uapi/linux/bpf.h       | 5 +++++
 kernel/bpf/syscall.c           | 4 +++-
 tools/include/uapi/linux/bpf.h | 5 +++++
 4 files changed, 14 insertions(+), 1 deletion(-)

include/linux/bpf.h +1 -0

index 6e947cd91152..8e5ec0839138 100644
@@ -934,6 +934,7 @@ struct bpf_prog_aux {
 	bool func_proto_unreliable;
 	bool sleepable;
 	bool tail_call_reachable;
+	bool xdp_mb;
 	struct hlist_node tramp_hlist;
 	/* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
 	const struct btf_type *attach_func_proto;

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

index b0383d371b9a..4a4f46ffd31d 100644
@@ -1111,6 +1111,11 @@ enum bpf_link_type {
  */
 #define BPF_F_SLEEPABLE		(1U << 4)
 
+/* If BPF_F_XDP_MB is used in BPF_PROG_LOAD command, the loaded program
+ * fully support xdp multi-buffer
+ */
+#define BPF_F_XDP_MB		(1U << 5)
+
 /* When BPF ldimm64's insn[0].src_reg != 0 then this can have
  * the following extensions:
  *

kernel/bpf/syscall.c +3 -1

index fa4505f9b611..f2d870888ded 100644
@@ -2217,7 +2217,8 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
 				 BPF_F_ANY_ALIGNMENT |
 				 BPF_F_TEST_STATE_FREQ |
 				 BPF_F_SLEEPABLE |
-				 BPF_F_TEST_RND_HI32))
+				 BPF_F_TEST_RND_HI32 |
+				 BPF_F_XDP_MB))
 		return -EINVAL;
 
 	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) &&

@@ -2303,6 +2304,7 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
 	prog->aux->dst_prog = dst_prog;
 	prog->aux->offload_requested = !!attr->prog_ifindex;
 	prog->aux->sleepable = attr->prog_flags & BPF_F_SLEEPABLE;
+	prog->aux->xdp_mb = attr->prog_flags & BPF_F_XDP_MB;
 
 	err = security_bpf_prog_alloc(prog->aux);
 	if (err)

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

index b0383d371b9a..4a4f46ffd31d 100644
@@ -1111,6 +1111,11 @@ enum bpf_link_type {
  */
 #define BPF_F_SLEEPABLE		(1U << 4)
 
+/* If BPF_F_XDP_MB is used in BPF_PROG_LOAD command, the loaded program
+ * fully support xdp multi-buffer
+ */
+#define BPF_F_XDP_MB		(1U << 5)
+
 /* When BPF ldimm64's insn[0].src_reg != 0 then this can have
  * the following extensions:
  *

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:54 UTC #13 lore raw

PATCH v21 10/23 net: mvneta: enable jumbo frames if the loaded XDP program support mb

Enable the capability to receive jumbo frames even if the interface is running in XDP mode if the loaded program declare to properly support xdp multi-buff. At same time reject a xdp program not supporting xdp multi-buffer if the driver is running in xdp multi-buffer mode.

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

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

index 94f22563695d..bec18136d29a 100644
@@ -3778,6 +3778,7 @@ static void mvneta_percpu_disable(void *arg)
 static int mvneta_change_mtu(struct net_device *dev, int mtu)
 {
 	struct mvneta_port *pp = netdev_priv(dev);
+	struct bpf_prog *prog = pp->xdp_prog;
 	int ret;
 
 	if (!IS_ALIGNED(MVNETA_RX_PKT_SIZE(mtu), 8)) {

@@ -3786,8 +3787,11 @@ static int mvneta_change_mtu(struct net_device *dev, int mtu)
 		mtu = ALIGN(MVNETA_RX_PKT_SIZE(mtu), 8);
 	}
 
-	if (pp->xdp_prog && mtu > MVNETA_MAX_RX_BUF_SIZE) {
-		netdev_info(dev, "Illegal MTU value %d for XDP mode\n", mtu);
+	if (prog && !prog->aux->xdp_mb && mtu > MVNETA_MAX_RX_BUF_SIZE) {
+		netdev_info(dev,
+			    "Illegal MTU %d for XDP prog without multi-buf\n",
+			    mtu);
+
 		return -EINVAL;
 	}

@@ -4528,8 +4532,9 @@ static int mvneta_xdp_setup(struct net_device *dev, struct bpf_prog *prog,
 	struct mvneta_port *pp = netdev_priv(dev);
 	struct bpf_prog *old_prog;
 
-	if (prog && dev->mtu > MVNETA_MAX_RX_BUF_SIZE) {
-		NL_SET_ERR_MSG_MOD(extack, "MTU too large for XDP");
+	if (prog && !prog->aux->xdp_mb && dev->mtu > MVNETA_MAX_RX_BUF_SIZE) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "prog does not support XDP multi-buff");
 		return -EOPNOTSUPP;
 	}

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:54 UTC #14 lore raw

PATCH v21 11/23 bpf: introduce bpf_xdp_get_buff_len helper

Introduce bpf_xdp_get_buff_len helper in order to return the xdp buffer total size (linear and paged area)

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 include/net/xdp.h              | 14 ++++++++++++++
 include/uapi/linux/bpf.h       |  7 +++++++
 net/core/filter.c              | 15 +++++++++++++++
 tools/include/uapi/linux/bpf.h |  7 +++++++
 4 files changed, 43 insertions(+)

include/net/xdp.h +14 -0

index 8409a2fb1c69..e75c98e2d573 100644
@@ -145,6 +145,20 @@ xdp_get_shared_info_from_buff(struct xdp_buff *xdp)
 	return (struct skb_shared_info *)xdp_data_hard_end(xdp);
 }
 
+static __always_inline unsigned int xdp_get_buff_len(struct xdp_buff *xdp)
+{
+	unsigned int len = xdp->data_end - xdp->data;
+	struct skb_shared_info *sinfo;
+
+	if (likely(!xdp_buff_is_mb(xdp)))
+		goto out;
+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+	len += sinfo->xdp_frags_size;
+out:
+	return len;
+}
+
 struct xdp_frame {
 	void *data;
 	u16 len;

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

index 4a4f46ffd31d..09f1b750c1e3 100644
@@ -5023,6 +5023,12 @@ union bpf_attr {
  *
  *	Return
  *		The number of arguments of the traced function.
+ *
+ * u64 bpf_xdp_get_buff_len(struct xdp_buff *xdp_md)
+ *	Description
+ *		Get the total size of a given xdp buff (linear and paged area)
+ *	Return
+ *		The total size of a given xdp buffer.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -5211,6 +5217,7 @@ union bpf_attr {
 	FN(get_func_arg),		\
 	FN(get_func_ret),		\
 	FN(get_func_arg_cnt),		\
+	FN(xdp_get_buff_len),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper

net/core/filter.c +15 -0

index 4603b7cd3cd1..332f8e7802bc 100644
@@ -3783,6 +3783,19 @@ static const struct bpf_func_proto sk_skb_change_head_proto = {
 	.arg2_type	= ARG_ANYTHING,
 	.arg3_type	= ARG_ANYTHING,
 };
+
+BPF_CALL_1(bpf_xdp_get_buff_len, struct  xdp_buff*, xdp)
+{
+	return xdp_get_buff_len(xdp);
+}
+
+static const struct bpf_func_proto bpf_xdp_get_buff_len_proto = {
+	.func		= bpf_xdp_get_buff_len,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_CTX,
+};
+
 static unsigned long xdp_get_metalen(const struct xdp_buff *xdp)
 {
 	return xdp_data_meta_unsupported(xdp) ? 0 :

@@ -7533,6 +7546,8 @@ xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_xdp_redirect_map_proto;
 	case BPF_FUNC_xdp_adjust_tail:
 		return &bpf_xdp_adjust_tail_proto;
+	case BPF_FUNC_xdp_get_buff_len:
+		return &bpf_xdp_get_buff_len_proto;
 	case BPF_FUNC_fib_lookup:
 		return &bpf_xdp_fib_lookup_proto;
 	case BPF_FUNC_check_mtu:

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

index 4a4f46ffd31d..09f1b750c1e3 100644
@@ -5023,6 +5023,12 @@ union bpf_attr {
  *
  *	Return
  *		The number of arguments of the traced function.
+ *
+ * u64 bpf_xdp_get_buff_len(struct xdp_buff *xdp_md)
+ *	Description
+ *		Get the total size of a given xdp buff (linear and paged area)
+ *	Return
+ *		The total size of a given xdp buffer.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -5211,6 +5217,7 @@ union bpf_attr {
 	FN(get_func_arg),		\
 	FN(get_func_ret),		\
 	FN(get_func_arg_cnt),		\
+	FN(xdp_get_buff_len),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:54 UTC #15 lore raw

PATCH v21 12/23 bpf: add multi-buff support to the bpf_xdp_adjust_tail() API

From: Eelco Chaudron <[email protected]>

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

When called on a multi-buffer packet with a grow request, it will work on the last fragment of the packet. So the maximum grow size is the last fragments tailroom, i.e. no new buffer will be allocated. A XDP mb capable driver is expected to set frag_size in xdp_rxq_info data structure to notify the XDP core the fragment size. frag_size set to 0 is interpreted by the XDP core as tail growing is not allowed. Introduce __xdp_rxq_info_reg utility routine to initialize frag_size field.

When shrinking, it will work from the last fragment, all the way down to the base buffer depending on the shrinking size. It's important to mention that once you shrink down the fragment(s) are freed, so you can not grow again to the original size.

Acked-by Toke Hoiland-Jorgensen <[email protected]>

Acked-by Jakub Kicinski <[email protected]>

Co-developed-by Lorenzo Bianconi <[email protected]>

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

---
 drivers/net/ethernet/marvell/mvneta.c |  3 +-
 include/net/xdp.h                     | 16 ++++++-
 net/core/filter.c                     | 65 +++++++++++++++++++++++++++
 net/core/xdp.c                        | 12 ++---
 4 files changed, 88 insertions(+), 8 deletions(-)

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

index bec18136d29a..41e42c04b61e 100644
@@ -3298,7 +3298,8 @@ static int mvneta_create_page_pool(struct mvneta_port *pp,
 		return err;
 	}
 
-	err = xdp_rxq_info_reg(&rxq->xdp_rxq, pp->dev, rxq->id, 0);
+	err = __xdp_rxq_info_reg(&rxq->xdp_rxq, pp->dev, rxq->id, 0,
+				 PAGE_SIZE);
 	if (err < 0)
 		goto err_free_pp;

include/net/xdp.h +14 -2

index e75c98e2d573..587fc7d2f4ae 100644
@@ -60,6 +60,7 @@ struct xdp_rxq_info {
 	u32 reg_state;
 	struct xdp_mem_info mem;
 	unsigned int napi_id;
+	u32 frag_size;
 } ____cacheline_aligned; /* perf critical, avoid false-sharing */
 
 struct xdp_txq_info {

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

@@ -340,8 +343,17 @@ static inline void xdp_release_frame(struct xdp_frame *xdpf)
 	__xdp_release_frame(xdpf->data, mem);
 }
 
-int xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq,
-		     struct net_device *dev, u32 queue_index, unsigned int napi_id);
+int __xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq,
+		       struct net_device *dev, u32 queue_index,
+		       unsigned int napi_id, u32 frag_size);
+static inline int
+xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq,
+		 struct net_device *dev, u32 queue_index,
+		 unsigned int napi_id)
+{
+	return __xdp_rxq_info_reg(xdp_rxq, dev, queue_index, napi_id, 0);
+}
+
 void xdp_rxq_info_unreg(struct xdp_rxq_info *xdp_rxq);
 void xdp_rxq_info_unused(struct xdp_rxq_info *xdp_rxq);
 bool xdp_rxq_info_is_reg(struct xdp_rxq_info *xdp_rxq);

net/core/filter.c +65 -0

76 lines not on this page · raw message

net/core/xdp.c +7 -5

index 086c90203545..13ece970abc2 100644
@@ -162,8 +162,9 @@ static void xdp_rxq_info_init(struct xdp_rxq_info *xdp_rxq)
 }
 
 /* Returns 0 on success, negative on failure */
-int xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq,
-		     struct net_device *dev, u32 queue_index, unsigned int napi_id)
+int __xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq,
+		       struct net_device *dev, u32 queue_index,
+		       unsigned int napi_id, u32 frag_size)
 {
 	if (!dev) {
 		WARN(1, "Missing net_device from driver");

@@ -185,11 +186,12 @@ int xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq,
 	xdp_rxq->dev = dev;
 	xdp_rxq->queue_index = queue_index;
 	xdp_rxq->napi_id = napi_id;
+	xdp_rxq->frag_size = frag_size;
 
 	xdp_rxq->reg_state = REG_STATE_REGISTERED;
 	return 0;
 }
-EXPORT_SYMBOL_GPL(xdp_rxq_info_reg);
+EXPORT_SYMBOL_GPL(__xdp_rxq_info_reg);
 
 void xdp_rxq_info_unused(struct xdp_rxq_info *xdp_rxq)
 {

@@ -369,8 +371,8 @@ EXPORT_SYMBOL_GPL(xdp_rxq_info_reg_mem_model);
  * is used for those calls sites.  Thus, allowing for faster recycling
  * of xdp_frames/pages in those cases.
  */
-static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
-			 struct xdp_buff *xdp)
+void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
+		  struct xdp_buff *xdp)
 {
 	struct xdp_mem_allocator *xa;
 	struct page *page;

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:54 UTC #16 lore raw

PATCH v21 13/23 bpf: add multi-buffer support to xdp copy helpers

From: Eelco Chaudron <[email protected]>

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

Acked-by Toke Hoiland-Jorgensen <[email protected]>

Acked-by Jakub Kicinski <[email protected]>

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

---
 kernel/trace/bpf_trace.c                      |   3 +
 net/core/filter.c                             |  57 ++++++-
 .../selftests/bpf/prog_tests/xdp_bpf2bpf.c    | 151 +++++++++++++-----
 .../selftests/bpf/progs/test_xdp_bpf2bpf.c    |   2 +-
 4 files changed, 168 insertions(+), 45 deletions(-)

kernel/trace/bpf_trace.c +3 -0

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

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

net/core/filter.c +52 -5

79 lines not on this page · raw message

tools/testing/selftests/bpf/prog_tests/xdp_bpf2bpf.c +112 ~~-39~~

187 lines not on this page · raw message

tools/testing/selftests/bpf/progs/test_xdp_bpf2bpf.c +1 -1

index 58cf4345f5cc..3379d303f41a 100644
@@ -49,7 +49,7 @@ int BPF_PROG(trace_on_entry, struct xdp_buff *xdp)
 	void *data = (void *)(long)xdp->data;
 
 	meta.ifindex = xdp->rxq->dev->ifindex;
-	meta.pkt_len = data_end - data;
+	meta.pkt_len = bpf_xdp_get_buff_len((struct xdp_md *)xdp);
 	bpf_xdp_output(xdp, &perf_buf_map,
 		       ((__u64) meta.pkt_len << 32) |
 		       BPF_F_CURRENT_CPU,

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:54 UTC #17 lore raw

PATCH v21 14/23 bpf: move user_size out of bpf_test_init

Rely on data_size_in in bpf_test_init routine signature. This is a preliminary patch to introduce xdp multi-buff selftest

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 net/bpf/test_run.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

net/bpf/test_run.c +7 -6

index 46dd95755967..dbf1227f437c 100644
@@ -249,11 +249,10 @@ bool bpf_prog_test_check_kfunc_call(u32 kfunc_id, struct module *owner)
 	return bpf_check_mod_kfunc_call(&prog_test_kfunc_list, kfunc_id, owner);
 }
 
-static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
-			   u32 headroom, u32 tailroom)
+static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
+			   u32 size, u32 headroom, u32 tailroom)
 {
 	void __user *data_in = u64_to_user_ptr(kattr->test.data_in);
-	u32 user_size = kattr->test.data_size_in;
 	void *data;
 
 	if (size < ETH_HLEN || size > PAGE_SIZE - headroom - tailroom)

@@ -581,7 +580,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 	if (kattr->test.flags || kattr->test.cpu)
 		return -EINVAL;
 
-	data = bpf_test_init(kattr, size, NET_SKB_PAD + NET_IP_ALIGN,
+	data = bpf_test_init(kattr, kattr->test.data_size_in,
+			     size, NET_SKB_PAD + NET_IP_ALIGN,
 			     SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
 	if (IS_ERR(data))
 		return PTR_ERR(data);

@@ -790,7 +790,8 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
 	/* XDP have extra tailroom as (most) drivers use full page */
 	max_data_sz = 4096 - headroom - tailroom;
 
-	data = bpf_test_init(kattr, max_data_sz, headroom, tailroom);
+	data = bpf_test_init(kattr, kattr->test.data_size_in,
+			     max_data_sz, headroom, tailroom);
 	if (IS_ERR(data)) {
 		ret = PTR_ERR(data);
 		goto free_ctx;

@@ -876,7 +877,7 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
 	if (size < ETH_HLEN)
 		return -EINVAL;
 
-	data = bpf_test_init(kattr, size, 0, 0);
+	data = bpf_test_init(kattr, kattr->test.data_size_in, size, 0, 0);
 	if (IS_ERR(data))
 		return PTR_ERR(data);

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:54 UTC #18 lore raw

PATCH v21 15/23 bpf: introduce multibuff support to bpf_prog_test_run_xdp()

Introduce the capability to allocate a xdp multi-buff in bpf_prog_test_run_xdp routine. This is a preliminary patch to introduce the selftests for new xdp multi-buff ebpf helpers

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 net/bpf/test_run.c | 58 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 13 deletions(-)

net/bpf/test_run.c +45 ~~-13~~

index dbf1227f437c..9cb5d5eced9a 100644
@@ -758,16 +758,16 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
 			  union bpf_attr __user *uattr)
 {
 	u32 tailroom = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
-	u32 headroom = XDP_PACKET_HEADROOM;
 	u32 size = kattr->test.data_size_in;
+	u32 headroom = XDP_PACKET_HEADROOM;
+	u32 retval, duration, max_data_sz;
 	u32 repeat = kattr->test.repeat;
 	struct netdev_rx_queue *rxqueue;
+	struct skb_shared_info *sinfo;
 	struct xdp_buff xdp = {};
-	u32 retval, duration;
+	int i, ret = -EINVAL;
 	struct xdp_md *ctx;
-	u32 max_data_sz;
 	void *data;
-	int ret = -EINVAL;
 
 	if (prog->expected_attach_type == BPF_XDP_DEVMAP ||
 	    prog->expected_attach_type == BPF_XDP_CPUMAP)

@@ -787,27 +787,60 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
 		headroom -= ctx->data;
 	}
 
-	/* XDP have extra tailroom as (most) drivers use full page */
 	max_data_sz = 4096 - headroom - tailroom;
+	size = min_t(u32, size, max_data_sz);
 
-	data = bpf_test_init(kattr, kattr->test.data_size_in,
-			     max_data_sz, headroom, tailroom);
+	data = bpf_test_init(kattr, size, max_data_sz, headroom, tailroom);
 	if (IS_ERR(data)) {
 		ret = PTR_ERR(data);
 		goto free_ctx;
 	}
 
 	rxqueue = __netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev, 0);
-	xdp_init_buff(&xdp, headroom + max_data_sz + tailroom,
-		      &rxqueue->xdp_rxq);
+	rxqueue->xdp_rxq.frag_size = headroom + max_data_sz + tailroom;
+	xdp_init_buff(&xdp, rxqueue->xdp_rxq.frag_size, &rxqueue->xdp_rxq);
 	xdp_prepare_buff(&xdp, data, headroom, size, true);
+	sinfo = xdp_get_shared_info_from_buff(&xdp);
 
 	ret = xdp_convert_md_to_buff(ctx, &xdp);
 	if (ret)
 		goto free_data;
 
+	if (unlikely(kattr->test.data_size_in > size)) {
+		void __user *data_in = u64_to_user_ptr(kattr->test.data_in);
+
+		while (size < kattr->test.data_size_in) {
+			struct page *page;
+			skb_frag_t *frag;
+			int data_len;
+
+			page = alloc_page(GFP_KERNEL);
+			if (!page) {
+				ret = -ENOMEM;
+				goto out;
+			}
+
+			frag = &sinfo->frags[sinfo->nr_frags++];
+			__skb_frag_set_page(frag, page);
+
+			data_len = min_t(int, kattr->test.data_size_in - size,
+					 PAGE_SIZE);
+			skb_frag_size_set(frag, data_len);
+
+			if (copy_from_user(page_address(page), data_in + size,
+					   data_len)) {
+				ret = -EFAULT;
+				goto out;
+			}
+			sinfo->xdp_frags_size += data_len;
+			size += data_len;
+		}
+		xdp_buff_set_mb(&xdp);
+	}
+
 	if (repeat > 1)
 		bpf_prog_change_xdp(NULL, prog);
+
 	ret = bpf_test_run(prog, &xdp, repeat, &retval, &duration, true);
 	/* We convert the xdp_buff back to an xdp_md before checking the return
 	 * code so the reference count of any held netdevice will be decremented

@@ -817,10 +850,7 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
 	if (ret)
 		goto out;
 
-	if (xdp.data_meta != data + headroom ||
-	    xdp.data_end != xdp.data_meta + size)
-		size = xdp.data_end - xdp.data_meta;
-
+	size = xdp.data_end - xdp.data_meta + sinfo->xdp_frags_size;
 	ret = bpf_test_finish(kattr, uattr, xdp.data_meta, size, retval,
 			      duration);
 	if (!ret)

@@ -831,6 +861,8 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
 	if (repeat > 1)
 		bpf_prog_change_xdp(prog, NULL);
 free_data:
+	for (i = 0; i < sinfo->nr_frags; i++)
+		__free_page(skb_frag_page(&sinfo->frags[i]));
 	kfree(data);
 free_ctx:
 	kfree(ctx);

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:55 UTC #19 lore raw

PATCH v21 16/23 bpf: test_run: add xdp_shared_info pointer in bpf_test_finish signature

introduce xdp_shared_info pointer in bpf_test_finish signature in order to copy back paged data from a xdp multi-buff frame to userspace buffer

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 net/bpf/test_run.c | 48 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

net/bpf/test_run.c +39 -9

index 9cb5d5eced9a..e57c71085dd5 100644
@@ -130,7 +130,8 @@ static int bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat,
 
 static int bpf_test_finish(const union bpf_attr *kattr,
 			   union bpf_attr __user *uattr, const void *data,
-			   u32 size, u32 retval, u32 duration)
+			   struct skb_shared_info *sinfo, u32 size,
+			   u32 retval, u32 duration)
 {
 	void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
 	int err = -EFAULT;

@@ -145,8 +146,36 @@ static int bpf_test_finish(const union bpf_attr *kattr,
 		err = -ENOSPC;
 	}
 
-	if (data_out && copy_to_user(data_out, data, copy_size))
-		goto out;
+	if (data_out) {
+		int len = sinfo ? copy_size - sinfo->xdp_frags_size : copy_size;
+
+		if (copy_to_user(data_out, data, len))
+			goto out;
+
+		if (sinfo) {
+			int i, offset = len, data_len;
+
+			for (i = 0; i < sinfo->nr_frags; i++) {
+				skb_frag_t *frag = &sinfo->frags[i];
+
+				if (offset >= copy_size) {
+					err = -ENOSPC;
+					break;
+				}
+
+				data_len = min_t(int, copy_size - offset,
+						 skb_frag_size(frag));
+
+				if (copy_to_user(data_out + offset,
+						 skb_frag_address(frag),
+						 data_len))
+					goto out;
+
+				offset += data_len;
+			}
+		}
+	}
+
 	if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
 		goto out;
 	if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval)))

@@ -683,7 +712,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
 	/* bpf program can never convert linear skb to non-linear */
 	if (WARN_ON_ONCE(skb_is_nonlinear(skb)))
 		size = skb_headlen(skb);
-	ret = bpf_test_finish(kattr, uattr, skb->data, size, retval, duration);
+	ret = bpf_test_finish(kattr, uattr, skb->data, NULL, size, retval,
+			      duration);
 	if (!ret)
 		ret = bpf_ctx_finish(kattr, uattr, ctx,
 				     sizeof(struct __sk_buff));

@@ -851,8 +881,8 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
 		goto out;
 
 	size = xdp.data_end - xdp.data_meta + sinfo->xdp_frags_size;
-	ret = bpf_test_finish(kattr, uattr, xdp.data_meta, size, retval,
-			      duration);
+	ret = bpf_test_finish(kattr, uattr, xdp.data_meta, sinfo, size,
+			      retval, duration);
 	if (!ret)
 		ret = bpf_ctx_finish(kattr, uattr, ctx,
 				     sizeof(struct xdp_md));

@@ -944,8 +974,8 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
 	if (ret < 0)
 		goto out;
 
-	ret = bpf_test_finish(kattr, uattr, &flow_keys, sizeof(flow_keys),
-			      retval, duration);
+	ret = bpf_test_finish(kattr, uattr, &flow_keys, NULL,
+			      sizeof(flow_keys), retval, duration);
 	if (!ret)
 		ret = bpf_ctx_finish(kattr, uattr, user_ctx,
 				     sizeof(struct bpf_flow_keys));

@@ -1049,7 +1079,7 @@ int bpf_prog_test_run_sk_lookup(struct bpf_prog *prog, const union bpf_attr *kat
 		user_ctx->cookie = sock_gen_cookie(ctx.selected_sk);
 	}
 
-	ret = bpf_test_finish(kattr, uattr, NULL, 0, retval, duration);
+	ret = bpf_test_finish(kattr, uattr, NULL, NULL, 0, retval, duration);
 	if (!ret)
 		ret = bpf_ctx_finish(kattr, uattr, user_ctx, sizeof(*user_ctx));

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:55 UTC #20 lore raw

PATCH v21 17/23 bpf: selftests: update xdp_adjust_tail selftest to include multi-buffer

From: Eelco Chaudron <[email protected]>

This change adds test cases for the multi-buffer scenarios when shrinking and growing.

Acked-by Toke Hoiland-Jorgensen <[email protected]>

Co-developed-by Lorenzo Bianconi <[email protected]>

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

---
 .../bpf/prog_tests/xdp_adjust_tail.c          | 131 ++++++++++++++++++
 .../bpf/progs/test_xdp_adjust_tail_grow.c     |  10 +-
 .../bpf/progs/test_xdp_adjust_tail_shrink.c   |  32 ++++-
 3 files changed, 166 insertions(+), 7 deletions(-)

tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c +131 -0

index 3f5a17c38be5..d21e0a597b3c 100644
@@ -130,6 +130,133 @@ static void test_xdp_adjust_tail_grow2(void)
 	bpf_object__close(obj);
 }
 
+void test_xdp_adjust_mb_tail_shrink(void)
+{
+	const char *file = "./test_xdp_adjust_tail_shrink.o";
+	__u32 duration, retval, size, exp_size;
+	struct bpf_program *prog;
+	struct bpf_object *obj;
+	int err, prog_fd;
+	__u8 *buf;
+
+	/* For the individual test cases, the first byte in the packet
+	 * indicates which test will be run.
+	 */
+	obj = bpf_object__open(file);
+	if (libbpf_get_error(obj))
+		return;
+
+	prog = bpf_object__next_program(obj, NULL);
+	if (bpf_object__load(obj))
+		return;
+
+	prog_fd = bpf_program__fd(prog);
+
+	buf = malloc(9000);
+	if (CHECK(!buf, "malloc()", "error:%s\n", strerror(errno)))
+		goto out;
+
+	memset(buf, 0, 9000);
+
+	/* Test case removing 10 bytes from last frag, NOT freeing it */
+	exp_size = 8990; /* 9000 - 10 */
+	err = bpf_prog_test_run(prog_fd, 1, buf, 9000,
+				buf, &size, &retval, &duration);
+
+	CHECK(err || retval != XDP_TX || size != exp_size,
+	      "9k-10b", "err %d errno %d retval %d[%d] size %d[%u]\n",
+	      err, errno, retval, XDP_TX, size, exp_size);
+
+	/* Test case removing one of two pages, assuming 4K pages */
+	buf[0] = 1;
+	exp_size = 4900; /* 9000 - 4100 */
+	err = bpf_prog_test_run(prog_fd, 1, buf, 9000,
+				buf, &size, &retval, &duration);
+
+	CHECK(err || retval != XDP_TX || size != exp_size,
+	      "9k-1p", "err %d errno %d retval %d[%d] size %d[%u]\n",
+	      err, errno, retval, XDP_TX, size, exp_size);
+
+	/* Test case removing two pages resulting in a non mb xdp_buff */
+	buf[0] = 2;
+	exp_size = 800; /* 9000 - 8200 */
+	err = bpf_prog_test_run(prog_fd, 1, buf, 9000,
+				buf, &size, &retval, &duration);
+
+	CHECK(err || retval != XDP_TX || size != exp_size,
+	      "9k-2p", "err %d errno %d retval %d[%d] size %d[%u]\n",
+	      err, errno, retval, XDP_TX, size, exp_size);
+
+	free(buf);
+out:
+	bpf_object__close(obj);
+}
+
+void test_xdp_adjust_mb_tail_grow(void)
+{
+	const char *file = "./test_xdp_adjust_tail_grow.o";
+	__u32 duration, retval, size, exp_size;
+	struct bpf_program *prog;
+	struct bpf_object *obj;
+	int err, i, prog_fd;
+	__u8 *buf;
+
+	obj = bpf_object__open(file);
+	if (libbpf_get_error(obj))
+		return;
+
+	prog = bpf_object__next_program(obj, NULL);
+	if (bpf_object__load(obj))
+		return;
+
+	prog_fd = bpf_program__fd(prog);
+
+	buf = malloc(16384);
+	if (CHECK(!buf, "malloc()", "error:%s\n", strerror(errno)))
+		goto out;
+
+	/* Test case add 10 bytes to last frag */
+	memset(buf, 1, 16384);
+	size = 9000;
+	exp_size = size + 10;
+	err = bpf_prog_test_run(prog_fd, 1, buf, size,
+				buf, &size, &retval, &duration);
+
+	CHECK(err || retval != XDP_TX || size != exp_size,
+	      "9k+10b", "err %d retval %d[%d] size %d[%u]\n",
+	      err, retval, XDP_TX, size, exp_size);
+
+	for (i = 0; i < 9000; i++)
+		CHECK(buf[i] != 1, "9k+10b-old",
+		      "Old data not all ok, offset %i is failing [%u]!\n",
+		      i, buf[i]);
+
+	for (i = 9000; i < 9010; i++)
+		CHECK(buf[i] != 0, "9k+10b-new",
+		      "New data not all ok, offset %i is failing [%u]!\n",
+		      i, buf[i]);
+
+	for (i = 9010; i < 16384; i++)
+		CHECK(buf[i] != 1, "9k+10b-untouched",
+		      "Unused data not all ok, offset %i is failing [%u]!\n",
+		      i, buf[i]);
+
+	/* Test a too large grow */
+	memset(buf, 1, 16384);
+	size = 9001;
+	exp_size = size;
+	err = bpf_prog_test_run(prog_fd, 1, buf, size,
+				buf, &size, &retval, &duration);
+
+	CHECK(err || retval != XDP_DROP || size != exp_size,
+	      "9k+10b", "err %d retval %d[%d] size %d[%u]\n",
+	      err, retval, XDP_TX, size, exp_size);
+
+	free(buf);
+out:
+	bpf_object__close(obj);
+}
+
 void test_xdp_adjust_tail(void)
 {
 	if (test__start_subtest("xdp_adjust_tail_shrink"))

@@ -138,4 +265,8 @@ void test_xdp_adjust_tail(void)
 		test_xdp_adjust_tail_grow();
 	if (test__start_subtest("xdp_adjust_tail_grow2"))
 		test_xdp_adjust_tail_grow2();
+	if (test__start_subtest("xdp_adjust_mb_tail_shrink"))
+		test_xdp_adjust_mb_tail_shrink();
+	if (test__start_subtest("xdp_adjust_mb_tail_grow"))
+		test_xdp_adjust_mb_tail_grow();
 }

tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c +7 -3

index 199c61b7d062..53b64c999450 100644
@@ -7,11 +7,10 @@ int _xdp_adjust_tail_grow(struct xdp_md *xdp)
 {
 	void *data_end = (void *)(long)xdp->data_end;
 	void *data = (void *)(long)xdp->data;
-	unsigned int data_len;
+	int data_len = bpf_xdp_get_buff_len(xdp);
 	int offset = 0;
 
 	/* Data length determine test case */
-	data_len = data_end - data;
 
 	if (data_len == 54) { /* sizeof(pkt_v4) */
 		offset = 4096; /* test too large offset */

@@ -20,7 +19,12 @@ int _xdp_adjust_tail_grow(struct xdp_md *xdp)
 	} else if (data_len == 64) {
 		offset = 128;
 	} else if (data_len == 128) {
-		offset = 4096 - 256 - 320 - data_len; /* Max tail grow 3520 */
+		/* Max tail grow 3520 */
+		offset = 4096 - 256 - 320 - data_len;
+	} else if (data_len == 9000) {
+		offset = 10;
+	} else if (data_len == 9001) {
+		offset = 4096;
 	} else {
 		return XDP_ABORTED; /* No matching test */
 	}

tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_shrink.c +28 -4

index b7448253d135..eeff48997b6e 100644
@@ -12,14 +12,38 @@
 SEC("xdp")
 int _xdp_adjust_tail_shrink(struct xdp_md *xdp)
 {
-	void *data_end = (void *)(long)xdp->data_end;
-	void *data = (void *)(long)xdp->data;
+	__u8 *data_end = (void *)(long)xdp->data_end;
+	__u8 *data = (void *)(long)xdp->data;
 	int offset = 0;
 
-	if (data_end - data == 54) /* sizeof(pkt_v4) */
+	switch (bpf_xdp_get_buff_len(xdp)) {
+	case 54:
+		/* sizeof(pkt_v4) */
 		offset = 256; /* shrink too much */
-	else
+		break;
+	case 9000:
+		/* Multi-buffer test cases */
+		if (data + 1 > data_end)
+			return XDP_DROP;
+
+		switch (data[0]) {
+		case 0:
+			offset = 10;
+			break;
+		case 1:
+			offset = 4100;
+			break;
+		case 2:
+			offset = 8200;
+			break;
+		default:
+			return XDP_DROP;
+		}
+		break;
+	default:
 		offset = 20;
+		break;
+	}
 	if (bpf_xdp_adjust_tail(xdp, 0 - offset))
 		return XDP_DROP;
 	return XDP_TX;

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:55 UTC #21 lore raw

PATCH v21 18/23 libbpf: Add SEC name for xdp_mb programs

Introduce support for the following SEC entries for XDP multi-buff property:
- SEC("xdp_mb/")
- SEC("xdp_devmap_mb/")
- SEC("xdp_cpumap_mb/")

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 tools/lib/bpf/libbpf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

tools/lib/bpf/libbpf.c +8 -0

index 7f10dd501a52..c93f6afef96c 100644
@@ -235,6 +235,8 @@ enum sec_def_flags {
 	SEC_SLEEPABLE = 8,
 	/* allow non-strict prefix matching */
 	SEC_SLOPPY_PFX = 16,
+	/* BPF program support XDP multi-buff */
+	SEC_XDP_MB = 32,
 };
 
 struct bpf_sec_def {

@@ -6562,6 +6564,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
 	if (def & SEC_SLEEPABLE)
 		opts->prog_flags |= BPF_F_SLEEPABLE;
 
+	if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_MB))
+		opts->prog_flags |= BPF_F_XDP_MB;
+
 	if ((prog->type == BPF_PROG_TYPE_TRACING ||
 	     prog->type == BPF_PROG_TYPE_LSM ||
 	     prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {

@@ -8600,8 +8605,11 @@ static const struct bpf_sec_def section_defs[] = {
 	SEC_DEF("lsm.s/",		LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
 	SEC_DEF("iter/",		TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
 	SEC_DEF("syscall",		SYSCALL, 0, SEC_SLEEPABLE),
+	SEC_DEF("xdp_devmap_mb/",	XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE | SEC_XDP_MB),
 	SEC_DEF("xdp_devmap/",		XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+	SEC_DEF("xdp_cpumap_mb/",	XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE | SEC_XDP_MB),
 	SEC_DEF("xdp_cpumap/",		XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+	SEC_DEF("xdp_mb/",		XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX | SEC_XDP_MB),
 	SEC_DEF("xdp",			XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
 	SEC_DEF("perf_event",		PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
 	SEC_DEF("lwt_in",		LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),

Signature · 1 line

2.33.1

Andrii Nakryiko

2022-01-10 02:16 UTC #22 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 6 lines · Lorenzo Bianconi

Introduce support for the following SEC entries for XDP multi-buff property:
- SEC("xdp_mb/")
- SEC("xdp_devmap_mb/")
- SEC("xdp_cpumap_mb/")

Libbpf seemed to went with .<suffix> rule (e.g., fentry.s for sleepable, seems like we'll have kprobe.multi or something along those lines as well), so let's stay consistent and call this "xdp_mb", "xdp_devmap.mb", "xdp_cpumap.mb" (btw, is "mb" really all that recognizable? would ".multibuf" be too verbose?). Also, why the "/" part? Also it shouldn't be "sloppy" either. Neither expected attach type should be optional. Also not sure SEC_ATTACHABLE is needed. So at most it should be SEC_XDP_MB, probably.

Quoted 22 lines

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 tools/lib/bpf/libbpf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

tools/lib/bpf/libbpf.c +2 -0

index 7f10dd501a52..c93f6afef96c 100644
@@ -235,6 +235,8 @@ enum sec_def_flags {
        SEC_SLEEPABLE = 8,
        /* allow non-strict prefix matching */
        SEC_SLOPPY_PFX = 16,
+       /* BPF program support XDP multi-buff */
+       SEC_XDP_MB = 32,
 };
 
 struct bpf_sec_def {

tools/lib/bpf/libbpf.c static int libbpf_preload_prog(struct bpf_program *prog, · line 6568

Quoted 5 lines

tools/lib/bpf/libbpf.c +2 -0

@@ -6562,6 +6564,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
        if (def & SEC_SLEEPABLE)
                opts->prog_flags |= BPF_F_SLEEPABLE;
 
+       if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_MB))
+               opts->prog_flags |= BPF_F_XDP_MB;

I'd say you don't even need SEC_XDP_MB flag at all, you can just check that prog->sec_name is one of "xdp.mb", "xdp_devmap.mb" or "xdp_cpumap.mb" and add the flag. SEC_XDP_MB doesn't seem generic enough to warrant a flag.

Quoted 19 lines

tools/lib/bpf/libbpf.c +4 -1

+
        if ((prog->type == BPF_PROG_TYPE_TRACING ||
             prog->type == BPF_PROG_TYPE_LSM ||
             prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {

@@ -8600,8 +8605,11 @@ static const struct bpf_sec_def section_defs[] = {
        SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
        SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
        SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
+       SEC_DEF("xdp_devmap_mb/",       XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE | SEC_XDP_MB),
        SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap_mb/",       XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE | SEC_XDP_MB),
        SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_mb/",              XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX | SEC_XDP_MB),
        SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
        SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
        SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
--

2.33.1

Lorenzo Bianconi

author

2022-01-12 18:18 UTC #23 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 8 lines · Lorenzo Bianconi

Quoted 6 lines
Introduce support for the following SEC entries for XDP multi-buff property:
- SEC("xdp_mb/")
- SEC("xdp_devmap_mb/")
- SEC("xdp_cpumap_mb/")

Quoted 8 lines

Libbpf seemed to went with .<suffix> rule (e.g., fentry.s for sleepable, seems like we'll have kprobe.multi or something along those lines as well), so let's stay consistent and call this "xdp_mb", "xdp_devmap.mb", "xdp_cpumap.mb" (btw, is "mb" really all that recognizable? would ".multibuf" be too verbose?). Also, why the "/" part? Also it shouldn't be "sloppy" either. Neither expected attach type should be optional. Also not sure SEC_ATTACHABLE is needed. So at most it should be SEC_XDP_MB, probably.

ack, I fine with it. Something like:

SEC_DEF("lsm.s/", LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
SEC_DEF("iter/", TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
SEC_DEF("syscall", SYSCALL, 0, SEC_SLEEPABLE),
+ SEC_DEF("xdp_devmap.multibuf", XDP, BPF_XDP_DEVMAP, 0),
SEC_DEF("xdp_devmap/", XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+ SEC_DEF("xdp_cpumap.multibuf", XDP, BPF_XDP_CPUMAP, 0),
SEC_DEF("xdp_cpumap/", XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+ SEC_DEF("xdp.multibuf", XDP, BPF_XDP, 0),
SEC_DEF("xdp", XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
SEC_DEF("perf_event", PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
SEC_DEF("lwt_in", LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),

Quoted 26 lines

Acked-by Toke Hoiland-Jorgensen <[email protected]>

In reply to Andrii Nakryiko

---
 tools/lib/bpf/libbpf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

tools/lib/bpf/libbpf.c +2 -0

index 7f10dd501a52..c93f6afef96c 100644
@@ -235,6 +235,8 @@ enum sec_def_flags {
        SEC_SLEEPABLE = 8,
        /* allow non-strict prefix matching */
        SEC_SLOPPY_PFX = 16,
+       /* BPF program support XDP multi-buff */
+       SEC_XDP_MB = 32,
 };
 
 struct bpf_sec_def {

@@ -6562,6 +6564,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
        if (def & SEC_SLEEPABLE)
                opts->prog_flags |= BPF_F_SLEEPABLE;

Quoted 7 lines

tools/lib/bpf/libbpf.c static int libbpf_preload_prog(struct bpf_program *prog, · line 6568
Quoted 2 lines
tools/lib/bpf/libbpf.c +2 -0
+       if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_MB))
+               opts->prog_flags |= BPF_F_XDP_MB;
I'd say you don't even need SEC_XDP_MB flag at all, you can just check that prog->sec_name is one of "xdp.mb", "xdp_devmap.mb" or "xdp_cpumap.mb" and add the flag. SEC_XDP_MB doesn't seem generic enough to warrant a flag.

ack, something like:

+ if (prog->type == BPF_PROG_TYPE_XDP &&

+	    (!strcmp(prog->sec_name, "xdp_devmap.multibuf") ||
+	     !strcmp(prog->sec_name, "xdp_cpumap.multibuf") ||

+ !strcmp(prog->sec_name, "xdp.multibuf")))
+ opts->prog_flags |= BPF_F_XDP_MB;

Regards,
Lorenzo

Quoted 20 lines

tools/lib/bpf/libbpf.c +4 -1

+
        if ((prog->type == BPF_PROG_TYPE_TRACING ||
             prog->type == BPF_PROG_TYPE_LSM ||
             prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {

@@ -8600,8 +8605,11 @@ static const struct bpf_sec_def section_defs[] = {
        SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
        SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
        SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
+       SEC_DEF("xdp_devmap_mb/",       XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE | SEC_XDP_MB),
        SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap_mb/",       XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE | SEC_XDP_MB),
        SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_mb/",              XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX | SEC_XDP_MB),
        SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
        SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
        SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
--

2.33.1

Andrii Nakryiko

2022-01-12 18:24 UTC #24 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 23 lines · Lorenzo Bianconi

Quoted 9 lines · Lorenzo Bianconi
Quoted 6 lines
Introduce support for the following SEC entries for XDP multi-buff property:
- SEC("xdp_mb/")
- SEC("xdp_devmap_mb/")
- SEC("xdp_cpumap_mb/")
Quoted 8 lines
Libbpf seemed to went with .<suffix> rule (e.g., fentry.s for sleepable, seems like we'll have kprobe.multi or something along those lines as well), so let's stay consistent and call this "xdp_mb", "xdp_devmap.mb", "xdp_cpumap.mb" (btw, is "mb" really all that recognizable? would ".multibuf" be too verbose?). Also, why the "/" part? Also it shouldn't be "sloppy" either. Neither expected attach type should be optional. Also not sure SEC_ATTACHABLE is needed. So at most it should be SEC_XDP_MB, probably.
ack, I fine with it. Something like:
        SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
        SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
        SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),

Review

Quoted 5 lines

(quoted diff) +3 -0

+       SEC_DEF("xdp_devmap.multibuf",  XDP, BPF_XDP_DEVMAP, 0),
        SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap.multibuf",  XDP, BPF_XDP_CPUMAP, 0),
        SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp.multibuf",         XDP, BPF_XDP, 0),

yep, but please use SEC_NONE instead of zero

Quoted 40 lines

        SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
        SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
        SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
Quoted 26 lines
Quoted 26 lines
Acked-by Toke Hoiland-Jorgensen <[email protected]>
Acked-by John Fastabend <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 tools/lib/bpf/libbpf.c | 8 ++++++++
 1 file changed, 8 insertions(+)
tools/lib/bpf/libbpf.c +2 -0
index 7f10dd501a52..c93f6afef96c 100644
@@ -235,6 +235,8 @@ enum sec_def_flags {
        SEC_SLEEPABLE = 8,
        /* allow non-strict prefix matching */
        SEC_SLOPPY_PFX = 16,
+       /* BPF program support XDP multi-buff */
+       SEC_XDP_MB = 32,
 };
 
 struct bpf_sec_def {
@@ -6562,6 +6564,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
        if (def & SEC_SLEEPABLE)
                opts->prog_flags |= BPF_F_SLEEPABLE;
 
Quoted 7 lines
tools/lib/bpf/libbpf.c static int libbpf_preload_prog(struct bpf_program *prog, · line 6568
Quoted 2 lines
tools/lib/bpf/libbpf.c +2 -0
+       if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_MB))
+               opts->prog_flags |= BPF_F_XDP_MB;
I'd say you don't even need SEC_XDP_MB flag at all, you can just check that prog->sec_name is one of "xdp.mb", "xdp_devmap.mb" or "xdp_cpumap.mb" and add the flag. SEC_XDP_MB doesn't seem generic enough to warrant a flag.
ack, something like:

Review

Quoted 5 lines

(quoted diff) +5 -0

+       if (prog->type == BPF_PROG_TYPE_XDP &&
+           (!strcmp(prog->sec_name, "xdp_devmap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp_cpumap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp.multibuf")))
+               opts->prog_flags |= BPF_F_XDP_MB;

yep, can also simplify it a bit with strstr(prog->sec_name, ".multibuf") instead of three strcmp

Quoted 24 lines

Regards,
Lorenzo

Quoted 20 lines

tools/lib/bpf/libbpf.c +4 -1

+
        if ((prog->type == BPF_PROG_TYPE_TRACING ||
             prog->type == BPF_PROG_TYPE_LSM ||
             prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {

@@ -8600,8 +8605,11 @@ static const struct bpf_sec_def section_defs[] = {
        SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
        SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
        SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
+       SEC_DEF("xdp_devmap_mb/",       XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE | SEC_XDP_MB),
        SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap_mb/",       XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE | SEC_XDP_MB),
        SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_mb/",              XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX | SEC_XDP_MB),
        SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
        SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
        SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
--

2.33.1

Lorenzo Bianconi

author

2022-01-12 18:35 UTC #25 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 30 lines · Lorenzo Bianconi

Quoted 23 lines
Quoted 9 lines · Lorenzo Bianconi
Quoted 6 lines
Introduce support for the following SEC entries for XDP multi-buff property:
- SEC("xdp_mb/")
- SEC("xdp_devmap_mb/")
- SEC("xdp_cpumap_mb/")
Quoted 8 lines
Libbpf seemed to went with .<suffix> rule (e.g., fentry.s for sleepable, seems like we'll have kprobe.multi or something along those lines as well), so let's stay consistent and call this "xdp_mb", "xdp_devmap.mb", "xdp_cpumap.mb" (btw, is "mb" really all that recognizable? would ".multibuf" be too verbose?). Also, why the "/" part? Also it shouldn't be "sloppy" either. Neither expected attach type should be optional. Also not sure SEC_ATTACHABLE is needed. So at most it should be SEC_XDP_MB, probably.
ack, I fine with it. Something like:
        SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
        SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
        SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
Review
Quoted 5 lines
(quoted diff) +3 -0
+       SEC_DEF("xdp_devmap.multibuf",  XDP, BPF_XDP_DEVMAP, 0),
        SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap.multibuf",  XDP, BPF_XDP_CPUMAP, 0),
        SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp.multibuf",         XDP, BPF_XDP, 0),

Quoted 1 line

yep, but please use SEC_NONE instead of zero

ack, I will fix it.

Quoted 41 lines

Quoted 41 lines
        SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
        SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
        SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
Quoted 26 lines
Quoted 26 lines
Acked-by Toke Hoiland-Jorgensen <[email protected]>
Acked-by John Fastabend <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 tools/lib/bpf/libbpf.c | 8 ++++++++
 1 file changed, 8 insertions(+)
tools/lib/bpf/libbpf.c +2 -0
index 7f10dd501a52..c93f6afef96c 100644
@@ -235,6 +235,8 @@ enum sec_def_flags {
        SEC_SLEEPABLE = 8,
        /* allow non-strict prefix matching */
        SEC_SLOPPY_PFX = 16,
+       /* BPF program support XDP multi-buff */
+       SEC_XDP_MB = 32,
 };
 
 struct bpf_sec_def {
@@ -6562,6 +6564,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
        if (def & SEC_SLEEPABLE)
                opts->prog_flags |= BPF_F_SLEEPABLE;
 
Quoted 7 lines
tools/lib/bpf/libbpf.c static int libbpf_preload_prog(struct bpf_program *prog, · line 6568
Quoted 2 lines
tools/lib/bpf/libbpf.c +2 -0
+       if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_MB))
+               opts->prog_flags |= BPF_F_XDP_MB;
I'd say you don't even need SEC_XDP_MB flag at all, you can just check that prog->sec_name is one of "xdp.mb", "xdp_devmap.mb" or "xdp_cpumap.mb" and add the flag. SEC_XDP_MB doesn't seem generic enough to warrant a flag.
ack, something like:

Quoted 8 lines

Review
Quoted 5 lines
(quoted diff) +5 -0
+       if (prog->type == BPF_PROG_TYPE_XDP &&
+           (!strcmp(prog->sec_name, "xdp_devmap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp_cpumap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp.multibuf")))
+               opts->prog_flags |= BPF_F_XDP_MB;
yep, can also simplify it a bit with strstr(prog->sec_name, ".multibuf") instead of three strcmp

ack, I will fix it.

Regards,
Lorenzo

Quoted 25 lines

Regards,
Lorenzo

Quoted 20 lines

tools/lib/bpf/libbpf.c +4 -1

+
        if ((prog->type == BPF_PROG_TYPE_TRACING ||
             prog->type == BPF_PROG_TYPE_LSM ||
             prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {

@@ -8600,8 +8605,11 @@ static const struct bpf_sec_def section_defs[] = {
        SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
        SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
        SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
+       SEC_DEF("xdp_devmap_mb/",       XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE | SEC_XDP_MB),
        SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap_mb/",       XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE | SEC_XDP_MB),
        SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_mb/",              XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX | SEC_XDP_MB),
        SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
        SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
        SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
--

2.33.1

Alexei Starovoitov

2022-01-12 19:17 UTC #26 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 73 lines · Andrii Nakryiko

Quoted 23 lines · Lorenzo Bianconi
Quoted 9 lines · Lorenzo Bianconi
Quoted 6 lines
Introduce support for the following SEC entries for XDP multi-buff property:
- SEC("xdp_mb/")
- SEC("xdp_devmap_mb/")
- SEC("xdp_cpumap_mb/")
Quoted 8 lines
Libbpf seemed to went with .<suffix> rule (e.g., fentry.s for sleepable, seems like we'll have kprobe.multi or something along those lines as well), so let's stay consistent and call this "xdp_mb", "xdp_devmap.mb", "xdp_cpumap.mb" (btw, is "mb" really all that recognizable? would ".multibuf" be too verbose?). Also, why the "/" part? Also it shouldn't be "sloppy" either. Neither expected attach type should be optional. Also not sure SEC_ATTACHABLE is needed. So at most it should be SEC_XDP_MB, probably.
ack, I fine with it. Something like:
        SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
        SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
        SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
Review
Quoted 5 lines
(quoted diff) +3 -0
+       SEC_DEF("xdp_devmap.multibuf",  XDP, BPF_XDP_DEVMAP, 0),
        SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap.multibuf",  XDP, BPF_XDP_CPUMAP, 0),
        SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp.multibuf",         XDP, BPF_XDP, 0),
yep, but please use SEC_NONE instead of zero
Quoted 40 lines
        SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
        SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
        SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
Quoted 26 lines
Quoted 26 lines
Acked-by Toke Hoiland-Jorgensen <[email protected]>
Acked-by John Fastabend <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 tools/lib/bpf/libbpf.c | 8 ++++++++
 1 file changed, 8 insertions(+)
tools/lib/bpf/libbpf.c +2 -0
index 7f10dd501a52..c93f6afef96c 100644
@@ -235,6 +235,8 @@ enum sec_def_flags {
        SEC_SLEEPABLE = 8,
        /* allow non-strict prefix matching */
        SEC_SLOPPY_PFX = 16,
+       /* BPF program support XDP multi-buff */
+       SEC_XDP_MB = 32,
 };
 
 struct bpf_sec_def {
@@ -6562,6 +6564,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
        if (def & SEC_SLEEPABLE)
                opts->prog_flags |= BPF_F_SLEEPABLE;
 
Quoted 7 lines
tools/lib/bpf/libbpf.c static int libbpf_preload_prog(struct bpf_program *prog, · line 6568
Quoted 2 lines
tools/lib/bpf/libbpf.c +2 -0
+       if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_MB))
+               opts->prog_flags |= BPF_F_XDP_MB;
I'd say you don't even need SEC_XDP_MB flag at all, you can just check that prog->sec_name is one of "xdp.mb", "xdp_devmap.mb" or "xdp_cpumap.mb" and add the flag. SEC_XDP_MB doesn't seem generic enough to warrant a flag.
ack, something like:

Quoted 8 lines

Review
Quoted 5 lines
(quoted diff) +5 -0
+       if (prog->type == BPF_PROG_TYPE_XDP &&
+           (!strcmp(prog->sec_name, "xdp_devmap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp_cpumap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp.multibuf")))
+               opts->prog_flags |= BPF_F_XDP_MB;
yep, can also simplify it a bit with strstr(prog->sec_name, ".multibuf") instead of three strcmp

Maybe ".mb" ?
".multibuf" is too verbose.
We're fine with ".s" for sleepable :)

Andrii Nakryiko

2022-01-12 19:21 UTC #27 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 82 lines · Alexei Starovoitov

Quoted 79 lines · Andrii Nakryiko
Quoted 23 lines · Lorenzo Bianconi
Quoted 9 lines · Lorenzo Bianconi
Quoted 6 lines
Introduce support for the following SEC entries for XDP multi-buff property:
- SEC("xdp_mb/")
- SEC("xdp_devmap_mb/")
- SEC("xdp_cpumap_mb/")
Quoted 8 lines
Libbpf seemed to went with .<suffix> rule (e.g., fentry.s for sleepable, seems like we'll have kprobe.multi or something along those lines as well), so let's stay consistent and call this "xdp_mb", "xdp_devmap.mb", "xdp_cpumap.mb" (btw, is "mb" really all that recognizable? would ".multibuf" be too verbose?). Also, why the "/" part? Also it shouldn't be "sloppy" either. Neither expected attach type should be optional. Also not sure SEC_ATTACHABLE is needed. So at most it should be SEC_XDP_MB, probably.
ack, I fine with it. Something like:
        SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
        SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
        SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
Review
Quoted 5 lines
(quoted diff) +3 -0
+       SEC_DEF("xdp_devmap.multibuf",  XDP, BPF_XDP_DEVMAP, 0),
        SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap.multibuf",  XDP, BPF_XDP_CPUMAP, 0),
        SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp.multibuf",         XDP, BPF_XDP, 0),
yep, but please use SEC_NONE instead of zero
Quoted 45 lines
        SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
        SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
        SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
Quoted 26 lines
Quoted 26 lines
Acked-by Toke Hoiland-Jorgensen <[email protected]>
Acked-by John Fastabend <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 tools/lib/bpf/libbpf.c | 8 ++++++++
 1 file changed, 8 insertions(+)
tools/lib/bpf/libbpf.c +2 -0
index 7f10dd501a52..c93f6afef96c 100644
@@ -235,6 +235,8 @@ enum sec_def_flags {
        SEC_SLEEPABLE = 8,
        /* allow non-strict prefix matching */
        SEC_SLOPPY_PFX = 16,
+       /* BPF program support XDP multi-buff */
+       SEC_XDP_MB = 32,
 };
 
 struct bpf_sec_def {
@@ -6562,6 +6564,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
        if (def & SEC_SLEEPABLE)
                opts->prog_flags |= BPF_F_SLEEPABLE;
 
Quoted 7 lines
tools/lib/bpf/libbpf.c static int libbpf_preload_prog(struct bpf_program *prog, · line 6568
Quoted 2 lines
tools/lib/bpf/libbpf.c +2 -0
+       if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_MB))
+               opts->prog_flags |= BPF_F_XDP_MB;
I'd say you don't even need SEC_XDP_MB flag at all, you can just check that prog->sec_name is one of "xdp.mb", "xdp_devmap.mb" or "xdp_cpumap.mb" and add the flag. SEC_XDP_MB doesn't seem generic enough to warrant a flag.
ack, something like:
(quoted diff) +5 -0
+       if (prog->type == BPF_PROG_TYPE_XDP &&
+           (!strcmp(prog->sec_name, "xdp_devmap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp_cpumap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp.multibuf")))
+               opts->prog_flags |= BPF_F_XDP_MB;

Quoted 6 lines

Quoted 2 lines
yep, can also simplify it a bit with strstr(prog->sec_name, ".multibuf") instead of three strcmp
Maybe ".mb" ?
".multibuf" is too verbose.
We're fine with ".s" for sleepable :)

I had reservations about "mb" because the first and strong association is "megabyte", not "multibuf". And it's not like anyone would have tens of those programs in a single file so that ".multibuf" becomes way too verbose. But I don't feel too strongly about this, if the consensus is on ".mb".

Alexei Starovoitov

2022-01-12 19:47 UTC #28 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 93 lines · Andrii Nakryiko

Quoted 82 lines · Alexei Starovoitov
Quoted 79 lines · Andrii Nakryiko
Quoted 23 lines · Lorenzo Bianconi
Quoted 9 lines · Lorenzo Bianconi
Quoted 6 lines
Introduce support for the following SEC entries for XDP multi-buff property:
- SEC("xdp_mb/")
- SEC("xdp_devmap_mb/")
- SEC("xdp_cpumap_mb/")
Quoted 8 lines
Libbpf seemed to went with .<suffix> rule (e.g., fentry.s for sleepable, seems like we'll have kprobe.multi or something along those lines as well), so let's stay consistent and call this "xdp_mb", "xdp_devmap.mb", "xdp_cpumap.mb" (btw, is "mb" really all that recognizable? would ".multibuf" be too verbose?). Also, why the "/" part? Also it shouldn't be "sloppy" either. Neither expected attach type should be optional. Also not sure SEC_ATTACHABLE is needed. So at most it should be SEC_XDP_MB, probably.
ack, I fine with it. Something like:
        SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
        SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
        SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
Review
Quoted 5 lines
(quoted diff) +3 -0
+       SEC_DEF("xdp_devmap.multibuf",  XDP, BPF_XDP_DEVMAP, 0),
        SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap.multibuf",  XDP, BPF_XDP_CPUMAP, 0),
        SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp.multibuf",         XDP, BPF_XDP, 0),
yep, but please use SEC_NONE instead of zero
Quoted 45 lines
        SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
        SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
        SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
Quoted 26 lines
Quoted 26 lines
Acked-by Toke Hoiland-Jorgensen <[email protected]>
Acked-by John Fastabend <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 tools/lib/bpf/libbpf.c | 8 ++++++++
 1 file changed, 8 insertions(+)
tools/lib/bpf/libbpf.c +2 -0
index 7f10dd501a52..c93f6afef96c 100644
@@ -235,6 +235,8 @@ enum sec_def_flags {
        SEC_SLEEPABLE = 8,
        /* allow non-strict prefix matching */
        SEC_SLOPPY_PFX = 16,
+       /* BPF program support XDP multi-buff */
+       SEC_XDP_MB = 32,
 };
 
 struct bpf_sec_def {
@@ -6562,6 +6564,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
        if (def & SEC_SLEEPABLE)
                opts->prog_flags |= BPF_F_SLEEPABLE;
 
Quoted 7 lines
tools/lib/bpf/libbpf.c static int libbpf_preload_prog(struct bpf_program *prog, · line 6568
Quoted 2 lines
tools/lib/bpf/libbpf.c +2 -0
+       if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_MB))
+               opts->prog_flags |= BPF_F_XDP_MB;
I'd say you don't even need SEC_XDP_MB flag at all, you can just check that prog->sec_name is one of "xdp.mb", "xdp_devmap.mb" or "xdp_cpumap.mb" and add the flag. SEC_XDP_MB doesn't seem generic enough to warrant a flag.
ack, something like:
(quoted diff) +5 -0
+       if (prog->type == BPF_PROG_TYPE_XDP &&
+           (!strcmp(prog->sec_name, "xdp_devmap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp_cpumap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp.multibuf")))
+               opts->prog_flags |= BPF_F_XDP_MB;
Quoted 6 lines
Quoted 2 lines
yep, can also simplify it a bit with strstr(prog->sec_name, ".multibuf") instead of three strcmp
Maybe ".mb" ?
".multibuf" is too verbose.
We're fine with ".s" for sleepable :)

Quoted 5 lines

I had reservations about "mb" because the first and strong association is "megabyte", not "multibuf". And it's not like anyone would have tens of those programs in a single file so that ".multibuf" becomes way too verbose. But I don't feel too strongly about this, if the consensus is on ".mb".

The rest of the patches are using _mb everywhere.
I would keep libbpf consistent.

Zvi Effron

2022-01-12 20:05 UTC #29 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 96 lines · Alexei Starovoitov

Quoted 93 lines · Andrii Nakryiko
Quoted 88 lines · Alexei Starovoitov
Quoted 73 lines · Andrii Nakryiko
Quoted 23 lines · Lorenzo Bianconi
Quoted 9 lines · Lorenzo Bianconi
Quoted 6 lines
Introduce support for the following SEC entries for XDP multi-buff property:
- SEC("xdp_mb/")
- SEC("xdp_devmap_mb/")
- SEC("xdp_cpumap_mb/")
Quoted 8 lines
Libbpf seemed to went with .<suffix> rule (e.g., fentry.s for sleepable, seems like we'll have kprobe.multi or something along those lines as well), so let's stay consistent and call this "xdp_mb", "xdp_devmap.mb", "xdp_cpumap.mb" (btw, is "mb" really all that recognizable? would ".multibuf" be too verbose?). Also, why the "/" part? Also it shouldn't be "sloppy" either. Neither expected attach type should be optional. Also not sure SEC_ATTACHABLE is needed. So at most it should be SEC_XDP_MB, probably.
ack, I fine with it. Something like:
        SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
        SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
        SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
Review
Quoted 5 lines
(quoted diff) +3 -0
+       SEC_DEF("xdp_devmap.multibuf",  XDP, BPF_XDP_DEVMAP, 0),
        SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap.multibuf",  XDP, BPF_XDP_CPUMAP, 0),
        SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp.multibuf",         XDP, BPF_XDP, 0),
yep, but please use SEC_NONE instead of zero
Quoted 40 lines
        SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
        SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
        SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
Quoted 26 lines
Quoted 26 lines
Acked-by Toke Hoiland-Jorgensen <[email protected]>
Acked-by John Fastabend <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 tools/lib/bpf/libbpf.c | 8 ++++++++
 1 file changed, 8 insertions(+)
tools/lib/bpf/libbpf.c +2 -0
index 7f10dd501a52..c93f6afef96c 100644
@@ -235,6 +235,8 @@ enum sec_def_flags {
        SEC_SLEEPABLE = 8,
        /* allow non-strict prefix matching */
        SEC_SLOPPY_PFX = 16,
+       /* BPF program support XDP multi-buff */
+       SEC_XDP_MB = 32,
 };
 
 struct bpf_sec_def {
@@ -6562,6 +6564,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
        if (def & SEC_SLEEPABLE)
                opts->prog_flags |= BPF_F_SLEEPABLE;
 
Quoted 7 lines
tools/lib/bpf/libbpf.c static int libbpf_preload_prog(struct bpf_program *prog, · line 6568
Quoted 2 lines
tools/lib/bpf/libbpf.c +2 -0
+       if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_MB))
+               opts->prog_flags |= BPF_F_XDP_MB;
I'd say you don't even need SEC_XDP_MB flag at all, you can just check that prog->sec_name is one of "xdp.mb", "xdp_devmap.mb" or "xdp_cpumap.mb" and add the flag. SEC_XDP_MB doesn't seem generic enough to warrant a flag.
ack, something like:
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5 -0
+       if (prog->type == BPF_PROG_TYPE_XDP &&
+           (!strcmp(prog->sec_name, "xdp_devmap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp_cpumap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp.multibuf")))
+               opts->prog_flags |= BPF_F_XDP_MB;
yep, can also simplify it a bit with strstr(prog->sec_name, ".multibuf") instead of three strcmp
Maybe ".mb" ?
".multibuf" is too verbose.
We're fine with ".s" for sleepable :)

Quoted 8 lines

Quoted 5 lines
I had reservations about "mb" because the first and strong association is "megabyte", not "multibuf". And it's not like anyone would have tens of those programs in a single file so that ".multibuf" becomes way too verbose. But I don't feel too strongly about this, if the consensus is on ".mb".
The rest of the patches are using _mb everywhere.
I would keep libbpf consistent.

Should the rest of the patches maybe use "multibuf" instead of "mb"? I've been following this patch series closely and excitedly, and I keep having to remind myself that "mb" is "multibuff" and not "megabyte". If I'm having to correct myself while following the patch series, I'm wondering if future confusion is inevitable?

But, is it enough confusion to be worth updating many other patches? I'm not sure.

I agree consistency is more important than the specific term we're consistent on.

--Zvi

Lorenzo Bianconi

2022-01-12 20:12 UTC #30 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 113 lines

Quoted 96 lines · Alexei Starovoitov
Quoted 93 lines · Andrii Nakryiko
Quoted 88 lines · Alexei Starovoitov
Quoted 73 lines · Andrii Nakryiko
Quoted 23 lines · Lorenzo Bianconi
Quoted 9 lines · Lorenzo Bianconi
Quoted 6 lines
Introduce support for the following SEC entries for XDP multi-buff property:
- SEC("xdp_mb/")
- SEC("xdp_devmap_mb/")
- SEC("xdp_cpumap_mb/")
Quoted 8 lines
Libbpf seemed to went with .<suffix> rule (e.g., fentry.s for sleepable, seems like we'll have kprobe.multi or something along those lines as well), so let's stay consistent and call this "xdp_mb", "xdp_devmap.mb", "xdp_cpumap.mb" (btw, is "mb" really all that recognizable? would ".multibuf" be too verbose?). Also, why the "/" part? Also it shouldn't be "sloppy" either. Neither expected attach type should be optional. Also not sure SEC_ATTACHABLE is needed. So at most it should be SEC_XDP_MB, probably.
ack, I fine with it. Something like:
        SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
        SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
        SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
Review
Quoted 5 lines
(quoted diff) +3 -0
+       SEC_DEF("xdp_devmap.multibuf",  XDP, BPF_XDP_DEVMAP, 0),
        SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap.multibuf",  XDP, BPF_XDP_CPUMAP, 0),
        SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp.multibuf",         XDP, BPF_XDP, 0),
yep, but please use SEC_NONE instead of zero
Quoted 40 lines
        SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
        SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
        SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
Quoted 26 lines
Quoted 26 lines
Acked-by Toke Hoiland-Jorgensen <[email protected]>
Acked-by John Fastabend <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 tools/lib/bpf/libbpf.c | 8 ++++++++
 1 file changed, 8 insertions(+)
tools/lib/bpf/libbpf.c +2 -0
index 7f10dd501a52..c93f6afef96c 100644
@@ -235,6 +235,8 @@ enum sec_def_flags {
        SEC_SLEEPABLE = 8,
        /* allow non-strict prefix matching */
        SEC_SLOPPY_PFX = 16,
+       /* BPF program support XDP multi-buff */
+       SEC_XDP_MB = 32,
 };
 
 struct bpf_sec_def {
@@ -6562,6 +6564,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
        if (def & SEC_SLEEPABLE)
                opts->prog_flags |= BPF_F_SLEEPABLE;
 
Quoted 7 lines
tools/lib/bpf/libbpf.c static int libbpf_preload_prog(struct bpf_program *prog, · line 6568
Quoted 2 lines
tools/lib/bpf/libbpf.c +2 -0
+       if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_MB))
+               opts->prog_flags |= BPF_F_XDP_MB;
I'd say you don't even need SEC_XDP_MB flag at all, you can just check that prog->sec_name is one of "xdp.mb", "xdp_devmap.mb" or "xdp_cpumap.mb" and add the flag. SEC_XDP_MB doesn't seem generic enough to warrant a flag.
ack, something like:
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5 -0
+       if (prog->type == BPF_PROG_TYPE_XDP &&
+           (!strcmp(prog->sec_name, "xdp_devmap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp_cpumap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp.multibuf")))
+               opts->prog_flags |= BPF_F_XDP_MB;
yep, can also simplify it a bit with strstr(prog->sec_name, ".multibuf") instead of three strcmp
Maybe ".mb" ?
".multibuf" is too verbose.
We're fine with ".s" for sleepable :)
Quoted 8 lines
Quoted 5 lines
I had reservations about "mb" because the first and strong association is "megabyte", not "multibuf". And it's not like anyone would have tens of those programs in a single file so that ".multibuf" becomes way too verbose. But I don't feel too strongly about this, if the consensus is on ".mb".
The rest of the patches are using _mb everywhere.
I would keep libbpf consistent.
Should the rest of the patches maybe use "multibuf" instead of "mb"? I've been following this patch series closely and excitedly, and I keep having to remind myself that "mb" is "multibuff" and not "megabyte". If I'm having to correct myself while following the patch series, I'm wondering if future confusion is inevitable?

Quoted 5 lines

But, is it enough confusion to be worth updating many other patches? I'm not sure.
I agree consistency is more important than the specific term we're consistent on.

I would prefer to keep the "_mb" postfix, but naming is hard and I am polarized :)

Regards,
Lorenzo

Quoted 3 lines

--Zvi

Toke Høiland-Jørgensen

2022-01-12 22:05 UTC #31 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 113 lines · Lorenzo Bianconi

Quoted 113 lines
Quoted 96 lines · Alexei Starovoitov
Quoted 93 lines · Andrii Nakryiko
Quoted 88 lines · Alexei Starovoitov
Quoted 73 lines · Andrii Nakryiko
Quoted 23 lines · Lorenzo Bianconi
Quoted 9 lines · Lorenzo Bianconi
Quoted 6 lines
Introduce support for the following SEC entries for XDP multi-buff property:
- SEC("xdp_mb/")
- SEC("xdp_devmap_mb/")
- SEC("xdp_cpumap_mb/")
Quoted 8 lines
Libbpf seemed to went with .<suffix> rule (e.g., fentry.s for sleepable, seems like we'll have kprobe.multi or something along those lines as well), so let's stay consistent and call this "xdp_mb", "xdp_devmap.mb", "xdp_cpumap.mb" (btw, is "mb" really all that recognizable? would ".multibuf" be too verbose?). Also, why the "/" part? Also it shouldn't be "sloppy" either. Neither expected attach type should be optional. Also not sure SEC_ATTACHABLE is needed. So at most it should be SEC_XDP_MB, probably.
ack, I fine with it. Something like:
        SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
        SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
        SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
Review
Quoted 5 lines
(quoted diff) +3 -0
+       SEC_DEF("xdp_devmap.multibuf",  XDP, BPF_XDP_DEVMAP, 0),
        SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap.multibuf",  XDP, BPF_XDP_CPUMAP, 0),
        SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp.multibuf",         XDP, BPF_XDP, 0),
yep, but please use SEC_NONE instead of zero
Quoted 40 lines
        SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
        SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
        SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
Quoted 26 lines
Quoted 26 lines
Acked-by Toke Hoiland-Jorgensen <[email protected]>
Acked-by John Fastabend <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 tools/lib/bpf/libbpf.c | 8 ++++++++
 1 file changed, 8 insertions(+)
tools/lib/bpf/libbpf.c +2 -0
index 7f10dd501a52..c93f6afef96c 100644
@@ -235,6 +235,8 @@ enum sec_def_flags {
        SEC_SLEEPABLE = 8,
        /* allow non-strict prefix matching */
        SEC_SLOPPY_PFX = 16,
+       /* BPF program support XDP multi-buff */
+       SEC_XDP_MB = 32,
 };
 
 struct bpf_sec_def {
@@ -6562,6 +6564,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
        if (def & SEC_SLEEPABLE)
                opts->prog_flags |= BPF_F_SLEEPABLE;
 
Quoted 7 lines
tools/lib/bpf/libbpf.c static int libbpf_preload_prog(struct bpf_program *prog, · line 6568
Quoted 2 lines
tools/lib/bpf/libbpf.c +2 -0
+       if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_MB))
+               opts->prog_flags |= BPF_F_XDP_MB;
I'd say you don't even need SEC_XDP_MB flag at all, you can just check that prog->sec_name is one of "xdp.mb", "xdp_devmap.mb" or "xdp_cpumap.mb" and add the flag. SEC_XDP_MB doesn't seem generic enough to warrant a flag.
ack, something like:
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5 -0
+       if (prog->type == BPF_PROG_TYPE_XDP &&
+           (!strcmp(prog->sec_name, "xdp_devmap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp_cpumap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp.multibuf")))
+               opts->prog_flags |= BPF_F_XDP_MB;
yep, can also simplify it a bit with strstr(prog->sec_name, ".multibuf") instead of three strcmp
Maybe ".mb" ?
".multibuf" is too verbose.
We're fine with ".s" for sleepable :)
Quoted 8 lines
Quoted 5 lines
I had reservations about "mb" because the first and strong association is "megabyte", not "multibuf". And it's not like anyone would have tens of those programs in a single file so that ".multibuf" becomes way too verbose. But I don't feel too strongly about this, if the consensus is on ".mb".
The rest of the patches are using _mb everywhere.
I would keep libbpf consistent.
Should the rest of the patches maybe use "multibuf" instead of "mb"? I've been following this patch series closely and excitedly, and I keep having to remind myself that "mb" is "multibuff" and not "megabyte". If I'm having to correct myself while following the patch series, I'm wondering if future confusion is inevitable?

Quoted 8 lines

Quoted 5 lines
But, is it enough confusion to be worth updating many other patches? I'm not sure.
I agree consistency is more important than the specific term we're consistent on.
I would prefer to keep the "_mb" postfix, but naming is hard and I am polarized :)

I would lean towards keeping _mb as well, but if it does have to be changed why not _mbuf? At least that's not quite as verbose :)

-Toke

Jesper Dangaard Brouer

2022-01-13 09:38 UTC #32 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 118 lines · Toke Høiland-Jørgensen

Quoted 116 lines · Lorenzo Bianconi
Quoted 116 lines
Quoted 96 lines · Alexei Starovoitov
Quoted 93 lines · Andrii Nakryiko
Quoted 88 lines · Alexei Starovoitov
Quoted 73 lines · Andrii Nakryiko
Quoted 23 lines · Lorenzo Bianconi
Quoted 9 lines · Lorenzo Bianconi
Quoted 6 lines
Introduce support for the following SEC entries for XDP multi-buff property:
- SEC("xdp_mb/")
- SEC("xdp_devmap_mb/")
- SEC("xdp_cpumap_mb/")
Quoted 8 lines
Libbpf seemed to went with .<suffix> rule (e.g., fentry.s for sleepable, seems like we'll have kprobe.multi or something along those lines as well), so let's stay consistent and call this "xdp_mb", "xdp_devmap.mb", "xdp_cpumap.mb" (btw, is "mb" really all that recognizable? would ".multibuf" be too verbose?). Also, why the "/" part? Also it shouldn't be "sloppy" either. Neither expected attach type should be optional. Also not sure SEC_ATTACHABLE is needed. So at most it should be SEC_XDP_MB, probably.
ack, I fine with it. Something like:
         SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
         SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
         SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
Review
Quoted 5 lines
(quoted diff) +3 -0
+       SEC_DEF("xdp_devmap.multibuf",  XDP, BPF_XDP_DEVMAP, 0),
         SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap.multibuf",  XDP, BPF_XDP_CPUMAP, 0),
         SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp.multibuf",         XDP, BPF_XDP, 0),
yep, but please use SEC_NONE instead of zero
Quoted 40 lines
         SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
         SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
         SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
Quoted 26 lines
Quoted 26 lines
Acked-by Toke Hoiland-Jorgensen <[email protected]>
Acked-by John Fastabend <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
  tools/lib/bpf/libbpf.c | 8 ++++++++
  1 file changed, 8 insertions(+)
tools/lib/bpf/libbpf.c +2 -0
index 7f10dd501a52..c93f6afef96c 100644
@@ -235,6 +235,8 @@ enum sec_def_flags {
         SEC_SLEEPABLE = 8,
         /* allow non-strict prefix matching */
         SEC_SLOPPY_PFX = 16,
+       /* BPF program support XDP multi-buff */
+       SEC_XDP_MB = 32,
  };
 
  struct bpf_sec_def {
@@ -6562,6 +6564,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
         if (def & SEC_SLEEPABLE)
                 opts->prog_flags |= BPF_F_SLEEPABLE;
 
Quoted 7 lines
tools/lib/bpf/libbpf.c static int libbpf_preload_prog(struct bpf_program *prog, · line 6568
Quoted 2 lines
tools/lib/bpf/libbpf.c +2 -0
+       if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_MB))
+               opts->prog_flags |= BPF_F_XDP_MB;
I'd say you don't even need SEC_XDP_MB flag at all, you can just check that prog->sec_name is one of "xdp.mb", "xdp_devmap.mb" or "xdp_cpumap.mb" and add the flag. SEC_XDP_MB doesn't seem generic enough to warrant a flag.
ack, something like:
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5 -0
+       if (prog->type == BPF_PROG_TYPE_XDP &&
+           (!strcmp(prog->sec_name, "xdp_devmap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp_cpumap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp.multibuf")))
+               opts->prog_flags |= BPF_F_XDP_MB;
yep, can also simplify it a bit with strstr(prog->sec_name, ".multibuf") instead of three strcmp
Maybe ".mb" ?
".multibuf" is too verbose.
We're fine with ".s" for sleepable :)
Quoted 8 lines
Quoted 5 lines
I had reservations about "mb" because the first and strong association is "megabyte", not "multibuf". And it's not like anyone would have tens of those programs in a single file so that ".multibuf" becomes way too verbose. But I don't feel too strongly about this, if the consensus is on ".mb".
The rest of the patches are using _mb everywhere.
I would keep libbpf consistent.
Should the rest of the patches maybe use "multibuf" instead of "mb"? I've been following this patch series closely and excitedly, and I keep having to remind myself that "mb" is "multibuff" and not "megabyte". If I'm having to correct myself while following the patch series, I'm wondering if future confusion is inevitable?
But, is it enough confusion to be worth updating many other patches? I'm not sure.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
I agree consistency is more important than the specific term we're consistent on.
I would prefer to keep the "_mb" postfix, but naming is hard and I am polarized :)
I would lean towards keeping _mb as well, but if it does have to be changed why not _mbuf? At least that's not quite as verbose :)

I dislike the "mb" abbreviation as I forget it stands for multi-buffer.
I like the "mbuf" suggestion, even-though it conflicts with (Free)BSD mbufs (which is their SKB).

I prefer/support the .<suffix> idea from Andrii.
Which would then be ".mbuf" for my taste.

--Jesper
p.s. I like the bikeshed red, meaning I don't feel too strongly about this.

Lorenzo Bianconi

author

2022-01-13 10:22 UTC #33 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 127 lines · Toke Høiland-Jørgensen

Quoted 124 lines
Quoted 121 lines · Lorenzo Bianconi
Quoted 113 lines
Quoted 96 lines · Alexei Starovoitov
Quoted 93 lines · Andrii Nakryiko
Quoted 88 lines · Alexei Starovoitov
Quoted 73 lines · Andrii Nakryiko
Quoted 23 lines · Lorenzo Bianconi
Quoted 9 lines · Lorenzo Bianconi
Quoted 6 lines
Introduce support for the following SEC entries for XDP multi-buff property:
- SEC("xdp_mb/")
- SEC("xdp_devmap_mb/")
- SEC("xdp_cpumap_mb/")
Quoted 8 lines
Libbpf seemed to went with .<suffix> rule (e.g., fentry.s for sleepable, seems like we'll have kprobe.multi or something along those lines as well), so let's stay consistent and call this "xdp_mb", "xdp_devmap.mb", "xdp_cpumap.mb" (btw, is "mb" really all that recognizable? would ".multibuf" be too verbose?). Also, why the "/" part? Also it shouldn't be "sloppy" either. Neither expected attach type should be optional. Also not sure SEC_ATTACHABLE is needed. So at most it should be SEC_XDP_MB, probably.
ack, I fine with it. Something like:
         SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
         SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
         SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
Review
Quoted 5 lines
(quoted diff) +3 -0
+       SEC_DEF("xdp_devmap.multibuf",  XDP, BPF_XDP_DEVMAP, 0),
         SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap.multibuf",  XDP, BPF_XDP_CPUMAP, 0),
         SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp.multibuf",         XDP, BPF_XDP, 0),
yep, but please use SEC_NONE instead of zero
Quoted 40 lines
         SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
         SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
         SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),
Quoted 26 lines
Quoted 26 lines
Acked-by Toke Hoiland-Jorgensen <[email protected]>
Acked-by John Fastabend <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
  tools/lib/bpf/libbpf.c | 8 ++++++++
  1 file changed, 8 insertions(+)
tools/lib/bpf/libbpf.c +2 -0
index 7f10dd501a52..c93f6afef96c 100644
@@ -235,6 +235,8 @@ enum sec_def_flags {
         SEC_SLEEPABLE = 8,
         /* allow non-strict prefix matching */
         SEC_SLOPPY_PFX = 16,
+       /* BPF program support XDP multi-buff */
+       SEC_XDP_MB = 32,
  };
 
  struct bpf_sec_def {
@@ -6562,6 +6564,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
         if (def & SEC_SLEEPABLE)
                 opts->prog_flags |= BPF_F_SLEEPABLE;
 
Quoted 7 lines
tools/lib/bpf/libbpf.c static int libbpf_preload_prog(struct bpf_program *prog, · line 6568
Quoted 2 lines
tools/lib/bpf/libbpf.c +2 -0
+       if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_MB))
+               opts->prog_flags |= BPF_F_XDP_MB;
I'd say you don't even need SEC_XDP_MB flag at all, you can just check that prog->sec_name is one of "xdp.mb", "xdp_devmap.mb" or "xdp_cpumap.mb" and add the flag. SEC_XDP_MB doesn't seem generic enough to warrant a flag.
ack, something like:
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5 -0
+       if (prog->type == BPF_PROG_TYPE_XDP &&
+           (!strcmp(prog->sec_name, "xdp_devmap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp_cpumap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp.multibuf")))
+               opts->prog_flags |= BPF_F_XDP_MB;
yep, can also simplify it a bit with strstr(prog->sec_name, ".multibuf") instead of three strcmp
Maybe ".mb" ?
".multibuf" is too verbose.
We're fine with ".s" for sleepable :)
Quoted 8 lines
Quoted 5 lines
I had reservations about "mb" because the first and strong association is "megabyte", not "multibuf". And it's not like anyone would have tens of those programs in a single file so that ".multibuf" becomes way too verbose. But I don't feel too strongly about this, if the consensus is on ".mb".
The rest of the patches are using _mb everywhere.
I would keep libbpf consistent.
Should the rest of the patches maybe use "multibuf" instead of "mb"? I've been following this patch series closely and excitedly, and I keep having to remind myself that "mb" is "multibuff" and not "megabyte". If I'm having to correct myself while following the patch series, I'm wondering if future confusion is inevitable?
Quoted 5 lines
But, is it enough confusion to be worth updating many other patches? I'm not sure.
I agree consistency is more important than the specific term we're consistent on.
I would prefer to keep the "_mb" postfix, but naming is hard and I am polarized :)

Quoted 6 lines

Quoted 2 lines
I would lean towards keeping _mb as well, but if it does have to be changed why not _mbuf? At least that's not quite as verbose :)
I dislike the "mb" abbreviation as I forget it stands for multi-buffer. I like the "mbuf" suggestion, even-though it conflicts with (Free)BSD mbufs (which is their SKB).

If we all agree, I can go over the series and substitute mb postfix with mbuf. Any objections?

Quoted 3 lines

I prefer/support the .<suffix> idea from Andrii.
Which would then be ".mbuf" for my taste.

ack, I have already implemented it, we need to define just the naming convention now.

Regards,
Lorenzo

Quoted 4 lines

--Jesper
p.s. I like the bikeshed red, meaning I don't feel too strongly about this.

Alexei Starovoitov

2022-01-13 20:20 UTC #34 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 7 lines · Lorenzo Bianconi

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
I would prefer to keep the "_mb" postfix, but naming is hard and I am polarized :)
I would lean towards keeping _mb as well, but if it does have to be changed why not _mbuf? At least that's not quite as verbose :)

Quoted 6 lines

Quoted 3 lines
I dislike the "mb" abbreviation as I forget it stands for multi-buffer. I like the "mbuf" suggestion, even-though it conflicts with (Free)BSD mbufs (which is their SKB).
If we all agree, I can go over the series and substitute mb postfix with mbuf. Any objections?

mbuf has too much bsd taste.

How about ".frags" instead?
Then xdp_buff_is_mb() will be xdp_buff_has_frags().

I agree that it's not obvious what "mb" suffix stands for, but I don't buy at all that it can be confused with "megabyte". It's the context that matters.
In "100mb" it's obvious that "mb" is likely "megabyte", but in "xdp.mb" it's certainly not "xdp megabyte". Such a sentence has no meaning.
Imagine we used that suffix for "tc"...
it would be "tc.mb"... "Traffic Control Megabyte" ??

Anyway "xdp.frags" ?

Btw "xdp_cpumap" should be cleaned up.
xdp_cpumap is an attach type. It's not prog type. Probably it should be "xdp/cpumap" to align with "cgroup/bind[46]" ?

In patch 22 there is a comment:
/* try to attach BPF_XDP_DEVMAP multi-buff program"

It creates further confusion. There is no XDP_DEVMAP program type. It should probably read
"Attach BPF_XDP program with frags to devmap"

Patch 21 still has "CHECK". Pls replace it with ASSERT.

Lorenzo Bianconi

author

2022-01-13 23:58 UTC #35 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 12 lines · Lorenzo Bianconi

Quoted 11 lines
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
I would prefer to keep the "_mb" postfix, but naming is hard and I am polarized :)
I would lean towards keeping _mb as well, but if it does have to be changed why not _mbuf? At least that's not quite as verbose :)
I dislike the "mb" abbreviation as I forget it stands for multi-buffer. I like the "mbuf" suggestion, even-though it conflicts with (Free)BSD mbufs (which is their SKB).

Quoted 6 lines

Quoted 2 lines
If we all agree, I can go over the series and substitute mb postfix with mbuf. Any objections?
mbuf has too much bsd taste.
How about ".frags" instead?

I am fine with this (for me it is better than mb or mbuf). Do we all agree or do we prefer the "mb" suffix?

Quoted 11 lines

Then xdp_buff_is_mb() will be xdp_buff_has_frags().
I agree that it's not obvious what "mb" suffix stands for, but I don't buy at all that it can be confused with "megabyte". It's the context that matters.
In "100mb" it's obvious that "mb" is likely "megabyte", but in "xdp.mb" it's certainly not "xdp megabyte". Such a sentence has no meaning.
Imagine we used that suffix for "tc"...
it would be "tc.mb"... "Traffic Control Megabyte" ??

Quoted 1 line

Anyway "xdp.frags" ?

agree

Quoted 4 lines

Btw "xdp_cpumap" should be cleaned up.
xdp_cpumap is an attach type. It's not prog type. Probably it should be "xdp/cpumap" to align with "cgroup/bind[46]" ?

so for xdp "mb" or xdp "frags" it will be xdp/cpumap.mb (xdp/devmap.mb) or xdp/cpumap.frags (xdp/devmap.frags), right?

Quoted 7 lines

In patch 22 there is a comment:
/* try to attach BPF_XDP_DEVMAP multi-buff program"
It creates further confusion. There is no XDP_DEVMAP program type. It should probably read
"Attach BPF_XDP program with frags to devmap"

ack, I will fix it.

Quoted 2 lines

Patch 21 still has "CHECK". Pls replace it with ASSERT.

ack, I will fix it.

Regards,
Lorenzo

Alexei Starovoitov

2022-01-14 02:09 UTC #36 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 7 lines · Lorenzo Bianconi

Quoted 4 lines
Btw "xdp_cpumap" should be cleaned up.
xdp_cpumap is an attach type. It's not prog type. Probably it should be "xdp/cpumap" to align with "cgroup/bind[46]" ?
so for xdp "mb" or xdp "frags" it will be xdp/cpumap.mb (xdp/devmap.mb) or xdp/cpumap.frags (xdp/devmap.frags), right?

xdp.frags/cpumap
xdp.frags/devmap

The current de-facto standard for SEC("") in libbpf:
prog_type.prog_flags/attach_place

"attach_place" is either function_name for fentry/, tp/, lsm/, etc. or attach_type/hook/target for cgroup/bind4, cgroup_skb/egress.

lsm.s/socket_bind -> prog_type = LSM, flags = SLEEPABLE lsm/socket_bind -> prog_type = LSM, non sleepable.

Jesper Dangaard Brouer

2022-01-14 16:50 UTC #37 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 6 lines · Alexei Starovoitov

Quoted 5 lines · Lorenzo Bianconi
Quoted 4 lines
Btw "xdp_cpumap" should be cleaned up.
xdp_cpumap is an attach type. It's not prog type. Probably it should be "xdp/cpumap" to align with "cgroup/bind[46]" ?

Quoted 8 lines

Quoted 2 lines
so for xdp "mb" or xdp "frags" it will be xdp/cpumap.mb (xdp/devmap.mb) or xdp/cpumap.frags (xdp/devmap.frags), right?
xdp.frags/cpumap
xdp.frags/devmap
The current de-facto standard for SEC("") in libbpf:
prog_type.prog_flags/attach_place

Ups, did we make a mistake with SEC("xdp_devmap/")

and can we correct without breaking existing programs?

Quoted 6 lines

"attach_place" is either function_name for fentry/, tp/, lsm/, etc. or attach_type/hook/target for cgroup/bind4, cgroup_skb/egress.
lsm.s/socket_bind -> prog_type = LSM, flags = SLEEPABLE lsm/socket_bind -> prog_type = LSM, non sleepable.

Zvi Effron

2022-01-14 18:55 UTC #38 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 16 lines · Jesper Dangaard Brouer

Quoted 12 lines · Alexei Starovoitov
Quoted 7 lines · Lorenzo Bianconi
Quoted 4 lines
Btw "xdp_cpumap" should be cleaned up.
xdp_cpumap is an attach type. It's not prog type. Probably it should be "xdp/cpumap" to align with "cgroup/bind[46]" ?
so for xdp "mb" or xdp "frags" it will be xdp/cpumap.mb (xdp/devmap.mb) or xdp/cpumap.frags (xdp/devmap.frags), right?
xdp.frags/cpumap
xdp.frags/devmap

Quoted 7 lines

Quoted 2 lines
The current de-facto standard for SEC("") in libbpf:
prog_type.prog_flags/attach_place
Ups, did we make a mistake with SEC("xdp_devmap/")
and can we correct without breaking existing programs?

We can (at the very least) add the correct sections, even if we leave the current incorrect ones as well. Ideally we'd mark the incorrect ones deprecated and either remove them before libbpf 1.0 or as part of 2.0?

--Zvi

Quoted 7 lines

Quoted 6 lines
"attach_place" is either function_name for fentry/, tp/, lsm/, etc. or attach_type/hook/target for cgroup/bind4, cgroup_skb/egress.
lsm.s/socket_bind -> prog_type = LSM, flags = SLEEPABLE lsm/socket_bind -> prog_type = LSM, non sleepable.

Andrii Nakryiko

2022-01-14 19:37 UTC #39 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 22 lines · Zvi Effron

Quoted 19 lines · Jesper Dangaard Brouer
Quoted 14 lines · Alexei Starovoitov
Quoted 7 lines · Lorenzo Bianconi
Quoted 4 lines
Btw "xdp_cpumap" should be cleaned up.
xdp_cpumap is an attach type. It's not prog type. Probably it should be "xdp/cpumap" to align with "cgroup/bind[46]" ?
so for xdp "mb" or xdp "frags" it will be xdp/cpumap.mb (xdp/devmap.mb) or xdp/cpumap.frags (xdp/devmap.frags), right?
xdp.frags/cpumap
xdp.frags/devmap
The current de-facto standard for SEC("") in libbpf:
prog_type.prog_flags/attach_place

Quoted 9 lines

Quoted 4 lines
Ups, did we make a mistake with SEC("xdp_devmap/")
and can we correct without breaking existing programs?
We can (at the very least) add the correct sections, even if we leave the current incorrect ones as well. Ideally we'd mark the incorrect ones deprecated and either remove them before libbpf 1.0 or as part of 2.0?

Correct, those would need to be new aliases. We can also deprecate old ones, if we have consensus on that. We can teach libbpf to emit warnings (through logs, of course) for such uses of to-be-removed sections aliases. We still have probably a few months before the final 1.0 release, should hopefully be plenty of time to people to adapt.

Quoted 9 lines

--Zvi
Quoted 7 lines
Quoted 6 lines
"attach_place" is either function_name for fentry/, tp/, lsm/, etc. or attach_type/hook/target for cgroup/bind4, cgroup_skb/egress.
lsm.s/socket_bind -> prog_type = LSM, flags = SLEEPABLE lsm/socket_bind -> prog_type = LSM, non sleepable.

Lorenzo Bianconi

author

2022-01-26 16:11 UTC #40 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 33 lines · Zvi Effron

Quoted 22 lines
Quoted 19 lines · Jesper Dangaard Brouer
Quoted 14 lines · Alexei Starovoitov
Quoted 7 lines · Lorenzo Bianconi
Quoted 4 lines
Btw "xdp_cpumap" should be cleaned up.
xdp_cpumap is an attach type. It's not prog type. Probably it should be "xdp/cpumap" to align with "cgroup/bind[46]" ?
so for xdp "mb" or xdp "frags" it will be xdp/cpumap.mb (xdp/devmap.mb) or xdp/cpumap.frags (xdp/devmap.frags), right?
xdp.frags/cpumap
xdp.frags/devmap
The current de-facto standard for SEC("") in libbpf:
prog_type.prog_flags/attach_place
Quoted 9 lines
Quoted 4 lines
Ups, did we make a mistake with SEC("xdp_devmap/")
and can we correct without breaking existing programs?
We can (at the very least) add the correct sections, even if we leave the current incorrect ones as well. Ideally we'd mark the incorrect ones deprecated and either remove them before libbpf 1.0 or as part of 2.0?

Quoted 5 lines

Correct, those would need to be new aliases. We can also deprecate old ones, if we have consensus on that. We can teach libbpf to emit warnings (through logs, of course) for such uses of to-be-removed sections aliases. We still have probably a few months before the final 1.0 release, should hopefully be plenty of time to people to adapt.

If we all agree on old cpumap/devmap sec deprecation and replace them with xdp/cpumap and xdp/devmap, would it be ok something like the patch below or would be necessary something different?

Regards,
Lorenzo

tools/lib/bpf/libbpf.c +8 -0

@@ -6575,6 +6575,12 @@ static int libbpf_preload_prog(struct bpf_program *prog,
 	if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_FRAGS))
 		opts->prog_flags |= BPF_F_XDP_HAS_FRAGS;
 
+	if (strstr(prog->sec_name, "xdp_devmap") ||
+	    strstr(prog->sec_name, "xdp_cpumap")) {
+		pr_warn("sec_name '%s' is deprecated, use xdp/devmap or xdp_cpumap instead\n",
+			prog->sec_name);
+	}
+
 	if ((prog->type == BPF_PROG_TYPE_TRACING ||
 	     prog->type == BPF_PROG_TYPE_LSM ||
 	     prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {

@@ -8618,8 +8624,10 @@ static const struct bpf_sec_def section_defs[] = {
 	SEC_DEF("iter.s/",		TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_iter),
 	SEC_DEF("syscall",		SYSCALL, 0, SEC_SLEEPABLE),
 	SEC_DEF("xdp.frags/devmap",	XDP, BPF_XDP_DEVMAP, SEC_XDP_FRAGS),
+	SEC_DEF("xdp/devmap",		XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
 	SEC_DEF("xdp_devmap/",		XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
 	SEC_DEF("xdp.frags/cpumap",	XDP, BPF_XDP_CPUMAP, SEC_XDP_FRAGS),
+	SEC_DEF("xdp/cpumap",		XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
 	SEC_DEF("xdp_cpumap/",		XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
 	SEC_DEF("xdp.frags",		XDP, BPF_XDP, SEC_XDP_FRAGS),
 	SEC_DEF("xdp",			XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),

Quoted 10 lines

Quoted 10 lines
--Zvi
Quoted 7 lines
Quoted 6 lines
"attach_place" is either function_name for fentry/, tp/, lsm/, etc. or attach_type/hook/target for cgroup/bind4, cgroup_skb/egress.
lsm.s/socket_bind -> prog_type = LSM, flags = SLEEPABLE lsm/socket_bind -> prog_type = LSM, non sleepable.

Andrii Nakryiko

2022-01-26 20:12 UTC #41 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 50 lines · Lorenzo Bianconi

Quoted 34 lines · Zvi Effron
Quoted 22 lines
Quoted 19 lines · Jesper Dangaard Brouer
Quoted 14 lines · Alexei Starovoitov
Quoted 7 lines · Lorenzo Bianconi
Quoted 4 lines
Btw "xdp_cpumap" should be cleaned up.
xdp_cpumap is an attach type. It's not prog type. Probably it should be "xdp/cpumap" to align with "cgroup/bind[46]" ?
so for xdp "mb" or xdp "frags" it will be xdp/cpumap.mb (xdp/devmap.mb) or xdp/cpumap.frags (xdp/devmap.frags), right?
xdp.frags/cpumap
xdp.frags/devmap
The current de-facto standard for SEC("") in libbpf:
prog_type.prog_flags/attach_place
Quoted 9 lines
Quoted 4 lines
Ups, did we make a mistake with SEC("xdp_devmap/")
and can we correct without breaking existing programs?
We can (at the very least) add the correct sections, even if we leave the current incorrect ones as well. Ideally we'd mark the incorrect ones deprecated and either remove them before libbpf 1.0 or as part of 2.0?
Quoted 5 lines
Correct, those would need to be new aliases. We can also deprecate old ones, if we have consensus on that. We can teach libbpf to emit warnings (through logs, of course) for such uses of to-be-removed sections aliases. We still have probably a few months before the final 1.0 release, should hopefully be plenty of time to people to adapt.
If we all agree on old cpumap/devmap sec deprecation and replace them with xdp/cpumap and xdp/devmap, would it be ok something like the patch below or would be necessary something different?
Regards,
Lorenzo

tools/lib/bpf/libbpf.c static int libbpf_preload_prog(struct bpf_program *prog, · line 6582

Quoted 8 lines

tools/lib/bpf/libbpf.c +5 -0

@@ -6575,6 +6575,12 @@ static int libbpf_preload_prog(struct bpf_program *prog,
        if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_FRAGS))
                opts->prog_flags |= BPF_F_XDP_HAS_FRAGS;
 
+       if (strstr(prog->sec_name, "xdp_devmap") ||
+           strstr(prog->sec_name, "xdp_cpumap")) {
+               pr_warn("sec_name '%s' is deprecated, use xdp/devmap or xdp_cpumap instead\n",
+                       prog->sec_name);
+       }

This time it's actually a clear use case for a flag, SEC_DEPRECATED. :) We can look through other SEC_DEF()s and see if some of them have to go.

Quoted 27 lines

tools/lib/bpf/libbpf.c +3 -0

+
        if ((prog->type == BPF_PROG_TYPE_TRACING ||
             prog->type == BPF_PROG_TYPE_LSM ||
             prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {

@@ -8618,8 +8624,10 @@ static const struct bpf_sec_def section_defs[] = {
        SEC_DEF("iter.s/",              TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_iter),
        SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
        SEC_DEF("xdp.frags/devmap",     XDP, BPF_XDP_DEVMAP, SEC_XDP_FRAGS),
+       SEC_DEF("xdp/devmap",           XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
        SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
        SEC_DEF("xdp.frags/cpumap",     XDP, BPF_XDP_CPUMAP, SEC_XDP_FRAGS),
+       SEC_DEF("xdp/cpumap",           XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
        SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
        SEC_DEF("xdp.frags",            XDP, BPF_XDP, SEC_XDP_FRAGS),
        SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),

Quoted 12 lines

Quoted 12 lines
--Zvi
Quoted 7 lines
Quoted 6 lines
"attach_place" is either function_name for fentry/, tp/, lsm/, etc. or attach_type/hook/target for cgroup/bind4, cgroup_skb/egress.
lsm.s/socket_bind -> prog_type = LSM, flags = SLEEPABLE lsm/socket_bind -> prog_type = LSM, non sleepable.

Lorenzo Bianconi

author

2022-01-14 16:36 UTC #42 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

In reply to Alexei Starovoitov

Quoted 29 lines · Lorenzo Bianconi

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
I would prefer to keep the "_mb" postfix, but naming is hard and I am polarized :)
I would lean towards keeping _mb as well, but if it does have to be changed why not _mbuf? At least that's not quite as verbose :)
Quoted 6 lines
Quoted 3 lines
I dislike the "mb" abbreviation as I forget it stands for multi-buffer. I like the "mbuf" suggestion, even-though it conflicts with (Free)BSD mbufs (which is their SKB).
If we all agree, I can go over the series and substitute mb postfix with mbuf. Any objections?
mbuf has too much bsd taste.
How about ".frags" instead?
Then xdp_buff_is_mb() will be xdp_buff_has_frags().
I agree that it's not obvious what "mb" suffix stands for, but I don't buy at all that it can be confused with "megabyte". It's the context that matters.
In "100mb" it's obvious that "mb" is likely "megabyte", but in "xdp.mb" it's certainly not "xdp megabyte". Such a sentence has no meaning.
Imagine we used that suffix for "tc"...
it would be "tc.mb"... "Traffic Control Megabyte" ??

Quoted 5 lines

Anyway "xdp.frags" ?
Btw "xdp_cpumap" should be cleaned up.
xdp_cpumap is an attach type. It's not prog type. Probably it should be "xdp/cpumap" to align with "cgroup/bind[46]" ?

If we change xdp_devmap/ in xdp/devmap (and xdp_cpumap/ in xdp/cpumap), are we going to break backward compatibility?
Maybe there are programs already deployed using it.
This is not a xdp multi-buff problem since we are not breaking backward compatibility there, we can just use:

xdp.frags/devmap
xdp.frags/cpumap

Moreover in samples/bpf we have something like:

SEC("xdp_devmap/egress")

It seems to me the egress postfix is not really used, right? Can we just drop it?

Regards,
Lorenzo

Quoted 9 lines

In patch 22 there is a comment:
/* try to attach BPF_XDP_DEVMAP multi-buff program"
It creates further confusion. There is no XDP_DEVMAP program type. It should probably read
"Attach BPF_XDP program with frags to devmap"
Patch 21 still has "CHECK". Pls replace it with ASSERT.

Andrii Nakryiko

2022-01-14 19:35 UTC #43 lore raw

Re: [PATCH v21 bpf-next 18/23] libbpf: Add SEC name for xdp_mb programs

Quoted 45 lines · Lorenzo Bianconi

Quoted 30 lines · Lorenzo Bianconi
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
I would prefer to keep the "_mb" postfix, but naming is hard and I am polarized :)
I would lean towards keeping _mb as well, but if it does have to be changed why not _mbuf? At least that's not quite as verbose :)
Quoted 6 lines
Quoted 3 lines
I dislike the "mb" abbreviation as I forget it stands for multi-buffer. I like the "mbuf" suggestion, even-though it conflicts with (Free)BSD mbufs (which is their SKB).
If we all agree, I can go over the series and substitute mb postfix with mbuf. Any objections?
mbuf has too much bsd taste.
How about ".frags" instead?
Then xdp_buff_is_mb() will be xdp_buff_has_frags().
I agree that it's not obvious what "mb" suffix stands for, but I don't buy at all that it can be confused with "megabyte". It's the context that matters.
In "100mb" it's obvious that "mb" is likely "megabyte", but in "xdp.mb" it's certainly not "xdp megabyte". Such a sentence has no meaning.
Imagine we used that suffix for "tc"...
it would be "tc.mb"... "Traffic Control Megabyte" ??
Quoted 5 lines
Anyway "xdp.frags" ?
Btw "xdp_cpumap" should be cleaned up.
xdp_cpumap is an attach type. It's not prog type. Probably it should be "xdp/cpumap" to align with "cgroup/bind[46]" ?
If we change xdp_devmap/ in xdp/devmap (and xdp_cpumap/ in xdp/cpumap), are we going to break backward compatibility?
Maybe there are programs already deployed using it.
This is not a xdp multi-buff problem since we are not breaking backward compatibility there, we can just use:
xdp.frags/devmap
xdp.frags/cpumap

Quoted 6 lines

Moreover in samples/bpf we have something like:
SEC("xdp_devmap/egress")
It seems to me the egress postfix is not really used, right? Can we just drop it?

Yeah, by current rules it should be just SEC("xdp_devmap"). This will break in libbpf 1.0 mode. For anyone who knows how to actually test BPF samples, it would be great to add libbpf_set_strict_mode(LIBBPF_STRICT_ALL); in every sample and make sure everything is still working. We've cleaned up selftests and all other places I knew about, but missed samples (and I can't test them properly).

Quoted 13 lines

Regards,
Lorenzo
Quoted 9 lines
In patch 22 there is a comment:
/* try to attach BPF_XDP_DEVMAP multi-buff program"
It creates further confusion. There is no XDP_DEVMAP program type. It should probably read
"Attach BPF_XDP program with frags to devmap"
Patch 21 still has "CHECK". Pls replace it with ASSERT.

Lorenzo Bianconi

author

2022-01-08 11:55 UTC #44 lore raw

PATCH v21 19/23 bpf: generalise tail call map compatibility check

From: Toke Hoiland-Jorgensen <[email protected]>

The check for tail call map compatibility ensures that tail calls only happen between maps of the same type. To ensure backwards compatibility for XDP multi-buffer we need a similar type of check for cpumap and devmap programs, so move the state from bpf_array_aux into bpf_map, add xdp_mb to the check, and apply the same check to cpumap and devmap.

Co-developed-by Lorenzo Bianconi <[email protected]>

Signed-off-by Toke Hoiland-Jorgensen <[email protected]>

---
 include/linux/bpf.h   | 30 +++++++++++++++++++-----------
 kernel/bpf/arraymap.c |  4 +---
 kernel/bpf/core.c     | 28 ++++++++++++++--------------
 kernel/bpf/cpumap.c   |  8 +++++---
 kernel/bpf/devmap.c   |  3 ++-
 kernel/bpf/syscall.c  | 21 +++++++++++----------
 6 files changed, 52 insertions(+), 42 deletions(-)

include/linux/bpf.h +19 ~~-11~~

index 8e5ec0839138..f9d820bfccc6 100644
@@ -194,6 +194,17 @@ struct bpf_map {
 	struct work_struct work;
 	struct mutex freeze_mutex;
 	atomic64_t writecnt;
+	/* 'Ownership' of program-containing map is claimed by the first program
+	 * that is going to use this map or by the first program which FD is
+	 * stored in the map to make sure that all callers and callees have the
+	 * same prog type, JITed flag and xdp_mb flag.
+	 */
+	struct {
+		spinlock_t lock;
+		enum bpf_prog_type type;
+		bool jited;
+		bool xdp_mb;
+	} owner;
 };
 
 static inline bool map_value_has_spin_lock(const struct bpf_map *map)

@@ -995,16 +1006,6 @@ struct bpf_prog_aux {
 };
 
 struct bpf_array_aux {
-	/* 'Ownership' of prog array is claimed by the first program that
-	 * is going to use this map or by the first program which FD is
-	 * stored in the map to make sure that all callers and callees have
-	 * the same prog type and JITed flag.
-	 */
-	struct {
-		spinlock_t lock;
-		enum bpf_prog_type type;
-		bool jited;
-	} owner;
 	/* Programs with direct jumps into programs part of this array. */
 	struct list_head poke_progs;
 	struct bpf_map *map;

@@ -1179,7 +1180,14 @@ struct bpf_event_entry {
 	struct rcu_head rcu;
 };
 
-bool bpf_prog_array_compatible(struct bpf_array *array, const struct bpf_prog *fp);
+static inline bool map_type_contains_progs(struct bpf_map *map)
+{
+	return map->map_type == BPF_MAP_TYPE_PROG_ARRAY ||
+	       map->map_type == BPF_MAP_TYPE_DEVMAP ||
+	       map->map_type == BPF_MAP_TYPE_CPUMAP;
+}
+
+bool bpf_prog_map_compatible(struct bpf_map *map, const struct bpf_prog *fp);
 int bpf_prog_calc_tag(struct bpf_prog *fp);
 
 const struct bpf_func_proto *bpf_get_trace_printk_proto(void);

kernel/bpf/arraymap.c +1 -3

index c7a5be3bf8be..7f145aefbff8 100644
@@ -837,13 +837,12 @@ static int fd_array_map_delete_elem(struct bpf_map *map, void *key)
 static void *prog_fd_array_get_ptr(struct bpf_map *map,
 				   struct file *map_file, int fd)
 {
-	struct bpf_array *array = container_of(map, struct bpf_array, map);
 	struct bpf_prog *prog = bpf_prog_get(fd);
 
 	if (IS_ERR(prog))
 		return prog;
 
-	if (!bpf_prog_array_compatible(array, prog)) {
+	if (!bpf_prog_map_compatible(map, prog)) {
 		bpf_prog_put(prog);
 		return ERR_PTR(-EINVAL);
 	}

@@ -1071,7 +1070,6 @@ static struct bpf_map *prog_array_map_alloc(union bpf_attr *attr)
 	INIT_WORK(&aux->work, prog_array_map_clear_deferred);
 	INIT_LIST_HEAD(&aux->poke_progs);
 	mutex_init(&aux->poke_mutex);
-	spin_lock_init(&aux->owner.lock);
 
 	map = array_map_alloc(attr);
 	if (IS_ERR(map)) {

kernel/bpf/core.c +14 ~~-14~~

index de3e5bc6781f..4a8beab6071e 100644
@@ -1829,28 +1829,30 @@ static unsigned int __bpf_prog_ret0_warn(const void *ctx,
 }
 #endif
 
-bool bpf_prog_array_compatible(struct bpf_array *array,
-			       const struct bpf_prog *fp)
+bool bpf_prog_map_compatible(struct bpf_map *map,
+			     const struct bpf_prog *fp)
 {
 	bool ret;
 
 	if (fp->kprobe_override)
 		return false;
 
-	spin_lock(&array->aux->owner.lock);
-
-	if (!array->aux->owner.type) {
+	spin_lock(&map->owner.lock);
+	if (!map->owner.type) {
 		/* There's no owner yet where we could check for
 		 * compatibility.
 		 */
-		array->aux->owner.type  = fp->type;
-		array->aux->owner.jited = fp->jited;
+		map->owner.type  = fp->type;
+		map->owner.jited = fp->jited;
+		map->owner.xdp_mb = fp->aux->xdp_mb;
 		ret = true;
 	} else {
-		ret = array->aux->owner.type  == fp->type &&
-		      array->aux->owner.jited == fp->jited;
+		ret = map->owner.type  == fp->type &&
+		      map->owner.jited == fp->jited &&
+		      map->owner.xdp_mb == fp->aux->xdp_mb;
 	}
-	spin_unlock(&array->aux->owner.lock);
+	spin_unlock(&map->owner.lock);
+
 	return ret;
 }

@@ -1862,13 +1864,11 @@ static int bpf_check_tail_call(const struct bpf_prog *fp)
 	mutex_lock(&aux->used_maps_mutex);
 	for (i = 0; i < aux->used_map_cnt; i++) {
 		struct bpf_map *map = aux->used_maps[i];
-		struct bpf_array *array;
 
-		if (map->map_type != BPF_MAP_TYPE_PROG_ARRAY)
+		if (!map_type_contains_progs(map))
 			continue;
 
-		array = container_of(map, struct bpf_array, map);
-		if (!bpf_prog_array_compatible(array, fp)) {
+		if (!bpf_prog_map_compatible(map, fp)) {
 			ret = -EINVAL;
 			goto out;
 		}

kernel/bpf/cpumap.c +5 -3

index b3e6b9422238..650e5d21f90d 100644
@@ -397,7 +397,8 @@ static int cpu_map_kthread_run(void *data)
 	return 0;
 }
 
-static int __cpu_map_load_bpf_program(struct bpf_cpu_map_entry *rcpu, int fd)
+static int __cpu_map_load_bpf_program(struct bpf_cpu_map_entry *rcpu,
+				      struct bpf_map *map, int fd)
 {
 	struct bpf_prog *prog;

@@ -405,7 +406,8 @@ static int __cpu_map_load_bpf_program(struct bpf_cpu_map_entry *rcpu, int fd)
 	if (IS_ERR(prog))
 		return PTR_ERR(prog);
 
-	if (prog->expected_attach_type != BPF_XDP_CPUMAP) {
+	if (prog->expected_attach_type != BPF_XDP_CPUMAP ||
+	    !bpf_prog_map_compatible(map, prog)) {
 		bpf_prog_put(prog);
 		return -EINVAL;
 	}

@@ -457,7 +459,7 @@ __cpu_map_entry_alloc(struct bpf_map *map, struct bpf_cpumap_val *value,
 	rcpu->map_id = map->id;
 	rcpu->value.qsize  = value->qsize;
 
-	if (fd > 0 && __cpu_map_load_bpf_program(rcpu, fd))
+	if (fd > 0 && __cpu_map_load_bpf_program(rcpu, map, fd))
 		goto free_ptr_ring;
 
 	/* Setup kthread */

kernel/bpf/devmap.c +2 -1

index fe019dbdb3f0..038f6d7a83e4 100644
@@ -858,7 +858,8 @@ static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net,
 					     BPF_PROG_TYPE_XDP, false);
 		if (IS_ERR(prog))
 			goto err_put_dev;
-		if (prog->expected_attach_type != BPF_XDP_DEVMAP)
+		if (prog->expected_attach_type != BPF_XDP_DEVMAP ||
+		    !bpf_prog_map_compatible(&dtab->map, prog))
 			goto err_put_prog;
 	}

kernel/bpf/syscall.c +11 ~~-10~~

index f2d870888ded..9d87a632b033 100644
@@ -556,16 +556,15 @@ static unsigned long bpf_map_memory_footprint(const struct bpf_map *map)
 
 static void bpf_map_show_fdinfo(struct seq_file *m, struct file *filp)
 {
-	const struct bpf_map *map = filp->private_data;
-	const struct bpf_array *array;
-	u32 type = 0, jited = 0;
-
-	if (map->map_type == BPF_MAP_TYPE_PROG_ARRAY) {
-		array = container_of(map, struct bpf_array, map);
-		spin_lock(&array->aux->owner.lock);
-		type  = array->aux->owner.type;
-		jited = array->aux->owner.jited;
-		spin_unlock(&array->aux->owner.lock);
+	struct bpf_map *map = filp->private_data;
+	u32 type = 0, jited = 0, xdp_mb = 0;
+
+	if (map_type_contains_progs(map)) {
+		spin_lock(&map->owner.lock);
+		type  = map->owner.type;
+		jited = map->owner.jited;
+		xdp_mb = map->owner.xdp_mb;
+		spin_unlock(&map->owner.lock);
 	}
 
 	seq_printf(m,

@@ -590,6 +589,7 @@ static void bpf_map_show_fdinfo(struct seq_file *m, struct file *filp)
 	if (type) {
 		seq_printf(m, "owner_prog_type:\t%u\n", type);
 		seq_printf(m, "owner_jited:\t%u\n", jited);
+		seq_printf(m, "owner_xdp_mb:\t%u\n", xdp_mb);
 	}
 }
 #endif

@@ -874,6 +874,7 @@ static int map_create(union bpf_attr *attr)
 	atomic64_set(&map->refcnt, 1);
 	atomic64_set(&map->usercnt, 1);
 	mutex_init(&map->freeze_mutex);
+	spin_lock_init(&map->owner.lock);
 
 	map->spin_lock_off = -EINVAL;
 	map->timer_off = -EINVAL;

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:55 UTC #45 lore raw

PATCH v21 20/23 net: xdp: introduce bpf_xdp_pointer utility routine

Similar to skb_header_pointer, introduce bpf_xdp_pointer utility routine to return a pointer to a given position in the xdp_buff if the requested area (offset + len) is contained in a contiguous memory area otherwise it will be copied in a bounce buffer provided by the caller. Similar to the tc counterpart, introduce the two following xdp helpers:
- bpf_xdp_load_bytes
- bpf_xdp_store_bytes

Reviewed-by Eelco Chaudron <[email protected]>

Acked-by Toke Hoiland-Jorgensen <[email protected]>

Acked-by Jakub Kicinski <[email protected]>

---
 include/uapi/linux/bpf.h       |  18 ++++
 net/core/filter.c              | 176 ++++++++++++++++++++++++++-------
 tools/include/uapi/linux/bpf.h |  18 ++++
 3 files changed, 174 insertions(+), 38 deletions(-)

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

index 09f1b750c1e3..3c6e250ebe4d 100644
@@ -5029,6 +5029,22 @@ union bpf_attr {
  *		Get the total size of a given xdp buff (linear and paged area)
  *	Return
  *		The total size of a given xdp buffer.
+ *
+ * long bpf_xdp_load_bytes(struct xdp_buff *xdp_md, u32 offset, void *buf, u32 len)
+ *	Description
+ *		This helper is provided as an easy way to load data from a
+ *		xdp buffer. It can be used to load *len* bytes from *offset* from
+ *		the frame associated to *xdp_md*, into the buffer pointed by
+ *		*buf*.
+ *	Return
+ *		0 on success, or a negative error in case of failure.
+ *
+ * long bpf_xdp_store_bytes(struct xdp_buff *xdp_md, u32 offset, void *buf, u32 len)
+ *	Description
+ *		Store *len* bytes from buffer *buf* into the frame
+ *		associated to *xdp_md*, at *offset*.
+ *	Return
+ *		0 on success, or a negative error in case of failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -5218,6 +5234,8 @@ union bpf_attr {
 	FN(get_func_ret),		\
 	FN(get_func_arg_cnt),		\
 	FN(xdp_get_buff_len),		\
+	FN(xdp_load_bytes),		\
+	FN(xdp_store_bytes),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper

net/core/filter.c +138 ~~-38~~

198 lines not on this page · raw message

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

index 09f1b750c1e3..3c6e250ebe4d 100644
@@ -5029,6 +5029,22 @@ union bpf_attr {
  *		Get the total size of a given xdp buff (linear and paged area)
  *	Return
  *		The total size of a given xdp buffer.
+ *
+ * long bpf_xdp_load_bytes(struct xdp_buff *xdp_md, u32 offset, void *buf, u32 len)
+ *	Description
+ *		This helper is provided as an easy way to load data from a
+ *		xdp buffer. It can be used to load *len* bytes from *offset* from
+ *		the frame associated to *xdp_md*, into the buffer pointed by
+ *		*buf*.
+ *	Return
+ *		0 on success, or a negative error in case of failure.
+ *
+ * long bpf_xdp_store_bytes(struct xdp_buff *xdp_md, u32 offset, void *buf, u32 len)
+ *	Description
+ *		Store *len* bytes from buffer *buf* into the frame
+ *		associated to *xdp_md*, at *offset*.
+ *	Return
+ *		0 on success, or a negative error in case of failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -5218,6 +5234,8 @@ union bpf_attr {
 	FN(get_func_ret),		\
 	FN(get_func_arg_cnt),		\
 	FN(xdp_get_buff_len),		\
+	FN(xdp_load_bytes),		\
+	FN(xdp_store_bytes),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:55 UTC #46 lore raw

PATCH v21 21/23 bpf: selftests: introduce bpf_xdp_{load,store}_bytes selftest

Introduce kernel selftest for new bpf_xdp_{load,store}_bytes helpers. and bpf_xdp_pointer/bpf_xdp_copy_buf utility routines.

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 .../bpf/prog_tests/xdp_adjust_frags.c         | 103 ++++++++++++++++++
 .../bpf/progs/test_xdp_update_frags.c         |  42 +++++++
 2 files changed, 145 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_adjust_frags.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_update_frags.c

tools/testing/selftests/bpf/prog_tests/xdp_adjust_frags.c new file +103 -0

new file mode 100644
index 000000000000..4b0d04fb1041
@@ -0,0 +1,103 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <network_helpers.h>
+
+void test_xdp_update_frags(void)
+{
+	const char *file = "./test_xdp_update_frags.o";
+	__u32 duration, retval, size;
+	struct bpf_program *prog;
+	struct bpf_object *obj;
+	int err, prog_fd;
+	__u32 *offset;
+	__u8 *buf;
+
+	obj = bpf_object__open(file);
+	if (libbpf_get_error(obj))
+		return;
+
+	prog = bpf_object__next_program(obj, NULL);
+	if (bpf_object__load(obj))
+		return;
+
+	prog_fd = bpf_program__fd(prog);
+
+	buf = malloc(128);
+	if (CHECK(!buf, "malloc()", "error:%s\n", strerror(errno)))
+		goto out;
+
+	memset(buf, 0, 128);
+	offset = (__u32 *)buf;
+	*offset = 16;
+	buf[*offset] = 0xaa;		/* marker at offset 16 (head) */
+	buf[*offset + 15] = 0xaa;	/* marker at offset 31 (head) */
+
+	err = bpf_prog_test_run(prog_fd, 1, buf, 128,
+				buf, &size, &retval, &duration);
+
+	/* test_xdp_update_frags: buf[16,31]: 0xaa -> 0xbb */
+	CHECK(err || retval != XDP_PASS || buf[16] != 0xbb || buf[31] != 0xbb,
+	      "128b", "err %d errno %d retval %d size %d\n",
+	      err, errno, retval, size);
+
+	free(buf);
+
+	buf = malloc(9000);
+	if (CHECK(!buf, "malloc()", "error:%s\n", strerror(errno)))
+		goto out;
+
+	memset(buf, 0, 9000);
+	offset = (__u32 *)buf;
+	*offset = 5000;
+	buf[*offset] = 0xaa;		/* marker at offset 5000 (frag0) */
+	buf[*offset + 15] = 0xaa;	/* marker at offset 5015 (frag0) */
+
+	err = bpf_prog_test_run(prog_fd, 1, buf, 9000,
+				buf, &size, &retval, &duration);
+
+	/* test_xdp_update_frags: buf[5000,5015]: 0xaa -> 0xbb */
+	CHECK(err || retval != XDP_PASS ||
+	      buf[5000] != 0xbb || buf[5015] != 0xbb,
+	      "9000b", "err %d errno %d retval %d size %d\n",
+	      err, errno, retval, size);
+
+	memset(buf, 0, 9000);
+	offset = (__u32 *)buf;
+	*offset = 3510;
+	buf[*offset] = 0xaa;		/* marker at offset 3510 (head) */
+	buf[*offset + 15] = 0xaa;	/* marker at offset 3525 (frag0) */
+
+	err = bpf_prog_test_run(prog_fd, 1, buf, 9000,
+				buf, &size, &retval, &duration);
+
+	/* test_xdp_update_frags: buf[3510,3525]: 0xaa -> 0xbb */
+	CHECK(err || retval != XDP_PASS ||
+	      buf[3510] != 0xbb || buf[3525] != 0xbb,
+	      "9000b", "err %d errno %d retval %d size %d\n",
+	      err, errno, retval, size);
+
+	memset(buf, 0, 9000);
+	offset = (__u32 *)buf;
+	*offset = 7606;
+	buf[*offset] = 0xaa;		/* marker at offset 7606 (frag0) */
+	buf[*offset + 15] = 0xaa;	/* marker at offset 7621 (frag1) */
+
+	err = bpf_prog_test_run(prog_fd, 1, buf, 9000,
+				buf, &size, &retval, &duration);
+
+	/* test_xdp_update_frags: buf[7606,7621]: 0xaa -> 0xbb */
+	CHECK(err || retval != XDP_PASS ||
+	      buf[7606] != 0xbb || buf[7621] != 0xbb,
+	      "9000b", "err %d errno %d retval %d size %d\n",
+	      err, errno, retval, size);
+
+	free(buf);
+out:
+	bpf_object__close(obj);
+}
+
+void test_xdp_adjust_frags(void)
+{
+	if (test__start_subtest("xdp_adjust_frags"))
+		test_xdp_update_frags();
+}

tools/testing/selftests/bpf/progs/test_xdp_update_frags.c new file +42 -0

42 lines not on this page · raw message

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:55 UTC #47 lore raw

PATCH v21 22/23 bpf: selftests: add CPUMAP/DEVMAP selftests for xdp multi-buff

Verify compatibility checks attaching a XDP multi-buff program to a CPUMAP/DEVMAP

Acked-by Toke Hoiland-Jorgensen <[email protected]>

---
 .../bpf/prog_tests/xdp_cpumap_attach.c        | 65 ++++++++++++++++++-
 .../bpf/prog_tests/xdp_devmap_attach.c        | 56 ++++++++++++++++
 .../bpf/progs/test_xdp_with_cpumap_helpers.c  |  6 ++
 .../progs/test_xdp_with_cpumap_mb_helpers.c   | 27 ++++++++
 .../bpf/progs/test_xdp_with_devmap_helpers.c  |  7 ++
 .../progs/test_xdp_with_devmap_mb_helpers.c   | 27 ++++++++
 6 files changed, 187 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_with_cpumap_mb_helpers.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_with_devmap_mb_helpers.c

tools/testing/selftests/bpf/prog_tests/xdp_cpumap_attach.c +64 -1

index fd812bd43600..ee580b50a945 100644
@@ -3,11 +3,12 @@
 #include <linux/if_link.h>
 #include <test_progs.h>
 
+#include "test_xdp_with_cpumap_mb_helpers.skel.h"
 #include "test_xdp_with_cpumap_helpers.skel.h"
 
 #define IFINDEX_LO	1
 
-void serial_test_xdp_cpumap_attach(void)
+void test_xdp_with_cpumap_helpers(void)
 {
 	struct test_xdp_with_cpumap_helpers *skel;
 	struct bpf_prog_info info = {};

@@ -54,6 +55,68 @@ void serial_test_xdp_cpumap_attach(void)
 	err = bpf_map_update_elem(map_fd, &idx, &val, 0);
 	ASSERT_NEQ(err, 0, "Add non-BPF_XDP_CPUMAP program to cpumap entry");
 
+	/* try to attach BPF_XDP_CPUMAP multi-buff program when we have already
+	 * loaded a legacy XDP program on the map
+	 */
+	idx = 1;
+	val.qsize = 192;
+	val.bpf_prog.fd = bpf_program__fd(skel->progs.xdp_dummy_cm_mb);
+	err = bpf_map_update_elem(map_fd, &idx, &val, 0);
+	ASSERT_NEQ(err, 0,
+		   "Add BPF_XDP_CPUMAP multi-buff program to cpumap entry");
+
 out_close:
 	test_xdp_with_cpumap_helpers__destroy(skel);
 }
+
+void test_xdp_with_cpumap_mb_helpers(void)
+{
+	struct test_xdp_with_cpumap_mb_helpers *skel;
+	struct bpf_prog_info info = {};
+	__u32 len = sizeof(info);
+	struct bpf_cpumap_val val = {
+		.qsize = 192,
+	};
+	int err, mb_prog_fd, map_fd;
+	__u32 idx = 0;
+
+	skel = test_xdp_with_cpumap_mb_helpers__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "test_xdp_with_cpumap_helpers__open_and_load"))
+		return;
+
+	mb_prog_fd = bpf_program__fd(skel->progs.xdp_dummy_cm_mb);
+	map_fd = bpf_map__fd(skel->maps.cpu_map);
+	err = bpf_obj_get_info_by_fd(mb_prog_fd, &info, &len);
+	if (!ASSERT_OK(err, "bpf_obj_get_info_by_fd"))
+		goto out_close;
+
+	val.bpf_prog.fd = mb_prog_fd;
+	err = bpf_map_update_elem(map_fd, &idx, &val, 0);
+	ASSERT_OK(err, "Add program to cpumap entry");
+
+	err = bpf_map_lookup_elem(map_fd, &idx, &val);
+	ASSERT_OK(err, "Read cpumap entry");
+	ASSERT_EQ(info.id, val.bpf_prog.id,
+		  "Match program id to cpumap entry prog_id");
+
+	/* try to attach BPF_XDP_CPUMAP program when we have already
+	 * loaded a multi-buff XDP program on the map
+	 */
+	idx = 1;
+	val.qsize = 192;
+	val.bpf_prog.fd = bpf_program__fd(skel->progs.xdp_dummy_cm);
+	err = bpf_map_update_elem(map_fd, &idx, &val, 0);
+	ASSERT_NEQ(err, 0, "Add BPF_XDP_CPUMAP program to cpumap entry");
+
+out_close:
+	test_xdp_with_cpumap_mb_helpers__destroy(skel);
+}
+
+void serial_test_xdp_cpumap_attach(void)
+{
+	if (test__start_subtest("CPUMAP with programs in entries"))
+		test_xdp_with_cpumap_helpers();
+
+	if (test__start_subtest("CPUMAP with multi-buff programs in entries"))
+		test_xdp_with_cpumap_mb_helpers();
+}

tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c +56 -0

80 lines not on this page · raw message

tools/testing/selftests/bpf/progs/test_xdp_with_cpumap_helpers.c +6 -0

index 532025057711..f32e4dab1751 100644
@@ -33,4 +33,10 @@ int xdp_dummy_cm(struct xdp_md *ctx)
 	return XDP_PASS;
 }
 
+SEC("xdp_cpumap_mb/mb_dummy_cm")
+int xdp_dummy_cm_mb(struct xdp_md *ctx)
+{
+	return XDP_PASS;
+}
+
 char _license[] SEC("license") = "GPL";

tools/testing/selftests/bpf/progs/test_xdp_with_cpumap_mb_helpers.c new file +27 -0

new file mode 100644
index 000000000000..96eedbaef71b
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+#define IFINDEX_LO	1
+
+struct {
+	__uint(type, BPF_MAP_TYPE_CPUMAP);
+	__uint(key_size, sizeof(__u32));
+	__uint(value_size, sizeof(struct bpf_cpumap_val));
+	__uint(max_entries, 4);
+} cpu_map SEC(".maps");
+
+SEC("xdp_cpumap/dummy_cm")
+int xdp_dummy_cm(struct xdp_md *ctx)
+{
+	return XDP_PASS;
+}
+
+SEC("xdp_cpumap_mb/mb_dummy_cm")
+int xdp_dummy_cm_mb(struct xdp_md *ctx)
+{
+	return XDP_PASS;
+}
+
+char _license[] SEC("license") = "GPL";

tools/testing/selftests/bpf/progs/test_xdp_with_devmap_helpers.c +7 -0

index 1e6b9c38ea6d..691f2d70dedc 100644
@@ -40,4 +40,11 @@ int xdp_dummy_dm(struct xdp_md *ctx)
 
 	return XDP_PASS;
 }
+
+SEC("xdp_devmap_mb/mp_map_prog")
+int xdp_dummy_dm_mb(struct xdp_md *ctx)
+{
+	return XDP_PASS;
+}
+
 char _license[] SEC("license") = "GPL";

tools/testing/selftests/bpf/progs/test_xdp_with_devmap_mb_helpers.c new file +27 -0

new file mode 100644
index 000000000000..05221b1fd9f2
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+struct {
+	__uint(type, BPF_MAP_TYPE_DEVMAP);
+	__uint(key_size, sizeof(__u32));
+	__uint(value_size, sizeof(struct bpf_devmap_val));
+	__uint(max_entries, 4);
+} dm_ports SEC(".maps");
+
+/* valid program on DEVMAP entry via SEC name;
+ * has access to egress and ingress ifindex
+ */
+SEC("xdp_devmap/map_prog")
+int xdp_dummy_dm(struct xdp_md *ctx)
+{
+	return XDP_PASS;
+}
+
+SEC("xdp_devmap_mb/mp_map_prog")
+int xdp_dummy_dm_mb(struct xdp_md *ctx)
+{
+	return XDP_PASS;
+}
+
+char _license[] SEC("license") = "GPL";

Signature · 1 line

2.33.1

Lorenzo Bianconi

author

2022-01-08 11:55 UTC #48 lore raw

PATCH v21 23/23 xdp: disable XDP_REDIRECT for xdp multi-buff

XDP_REDIRECT is not fully supported yet for xdp multi-buff since not all XDP capable drivers can map non-linear xdp_frame in ndo_xdp_xmit so disable it for the moment.

Acked-by Toke Hoiland-Jorgensen <[email protected]>

Acked-by Jesper Dangaard Brouer <[email protected]>

---
 net/core/filter.c | 7 +++++++
 1 file changed, 7 insertions(+)

net/core/filter.c +7 -0

index 2249377b3b1f..e6f6abb54af1 100644
@@ -4266,6 +4266,13 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 	struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
 	enum bpf_map_type map_type = ri->map_type;
 
+	/* XDP_REDIRECT is not fully supported yet for xdp multi-buff since
+	 * not all XDP capable drivers can map non-linear xdp_frame in
+	 * ndo_xdp_xmit.
+	 */
+	if (unlikely(xdp_buff_is_mb(xdp) && map_type != BPF_MAP_TYPE_CPUMAP))
+		return -EOPNOTSUPP;
+
 	if (map_type == BPF_MAP_TYPE_XSKMAP)
 		return __xdp_do_redirect_xsk(ri, dev, xdp, xdp_prog);

Signature · 1 line

2.33.1

Lorenzo Bianconi

2022-01-12 18:56 UTC #49 lore raw