All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6 bpf-next] xsk: reuseq cleanup
@ 2019-06-27 22:08 Jonathan Lemon
  2019-06-27 22:08 ` [PATCH 1/6 bpf-next] Have xsk_umem_peek_addr_rq() return chunk-aligned handles Jonathan Lemon
                   ` (5 more replies)
  0 siblings, 6 replies; 16+ messages in thread
From: Jonathan Lemon @ 2019-06-27 22:08 UTC (permalink / raw)
  To: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer; +Cc: kernel-team

Clean up and normalize usage of the recycle queue in order to
support upcoming TX from RX queue functionality.

Jonathan Lemon (6):
  Have xsk_umem_peek_addr_rq() return chunk-aligned handles.
  Clean up xsk reuseq API
  Always check the recycle stack when using the umem fq.
  Simplify AF_XDP umem allocation path for Intel drivers.
  Remove use of umem _rq variants from Mellanox driver.
  Remove the umem _rq variants now that the last consumer is gone.

 drivers/net/ethernet/intel/i40e/i40e_xsk.c    | 86 +++----------------
 .../ethernet/intel/ixgbe/ixgbe_txrx_common.h  |  2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  | 59 ++-----------
 .../ethernet/mellanox/mlx5/core/en/xsk/rx.c   |  8 +-
 .../ethernet/mellanox/mlx5/core/en/xsk/umem.c |  7 +-
 include/net/xdp_sock.h                        | 69 ++-------------
 net/xdp/xdp_umem.c                            |  2 +-
 net/xdp/xsk.c                                 | 22 ++++-
 net/xdp/xsk_queue.c                           | 56 +++++-------
 net/xdp/xsk_queue.h                           |  2 +-
 10 files changed, 68 insertions(+), 245 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/6 bpf-next] Have xsk_umem_peek_addr_rq() return chunk-aligned handles.
  2019-06-27 22:08 [PATCH 0/6 bpf-next] xsk: reuseq cleanup Jonathan Lemon
@ 2019-06-27 22:08 ` Jonathan Lemon
  2019-06-27 22:08 ` [PATCH 2/6 bpf-next] Clean up xsk reuseq API Jonathan Lemon
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Jonathan Lemon @ 2019-06-27 22:08 UTC (permalink / raw)
  To: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer; +Cc: kernel-team

xkq_peek_addr() returns chunk-aligned handles, so have the rq behave
the same way.  Clean up callsites.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c          | 2 --
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c        | 2 --
 drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c | 2 +-
 include/net/xdp_sock.h                              | 2 +-
 4 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 32bad014d76c..3b84fca1c11d 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -289,8 +289,6 @@ static bool i40e_alloc_buffer_slow_zc(struct i40e_ring *rx_ring,
 		return false;
 	}
 
-	handle &= rx_ring->xsk_umem->chunk_mask;
-
 	hr = umem->headroom + XDP_PACKET_HEADROOM;
 
 	bi->dma = xdp_umem_get_dma(umem, handle);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
index 6b609553329f..a9cab3271ac9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
@@ -293,8 +293,6 @@ static bool ixgbe_alloc_buffer_slow_zc(struct ixgbe_ring *rx_ring,
 		return false;
 	}
 
-	handle &= rx_ring->xsk_umem->chunk_mask;
-
 	hr = umem->headroom + XDP_PACKET_HEADROOM;
 
 	bi->dma = xdp_umem_get_dma(umem, handle);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c
index 6a55573ec8f2..85440b1c1c3f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c
@@ -44,7 +44,7 @@ int mlx5e_xsk_page_alloc_umem(struct mlx5e_rq *rq,
 
 static inline void mlx5e_xsk_recycle_frame(struct mlx5e_rq *rq, u64 handle)
 {
-	xsk_umem_fq_reuse(rq->umem, handle & rq->umem->chunk_mask);
+	xsk_umem_fq_reuse(rq->umem, handle);
 }
 
 /* XSKRQ uses pages from UMEM, they must not be released. They are returned to
diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index 057b159ff8b9..39516d960636 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -117,7 +117,7 @@ static inline u64 *xsk_umem_peek_addr_rq(struct xdp_umem *umem, u64 *addr)
 	if (!rq->length)
 		return xsk_umem_peek_addr(umem, addr);
 
-	*addr = rq->handles[rq->length - 1];
+	*addr = rq->handles[rq->length - 1] & umem->chunk_mask;
 	return addr;
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/6 bpf-next] Clean up xsk reuseq API
  2019-06-27 22:08 [PATCH 0/6 bpf-next] xsk: reuseq cleanup Jonathan Lemon
  2019-06-27 22:08 ` [PATCH 1/6 bpf-next] Have xsk_umem_peek_addr_rq() return chunk-aligned handles Jonathan Lemon
@ 2019-06-27 22:08 ` Jonathan Lemon
  2019-06-27 22:38   ` Jakub Kicinski
  2019-06-27 22:08 ` [PATCH 3/6 bpf-next] Always check the recycle stack when using the umem fq Jonathan Lemon
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Jonathan Lemon @ 2019-06-27 22:08 UTC (permalink / raw)
  To: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer; +Cc: kernel-team

The reuseq is actually a recycle stack, only accessed from the kernel side.
Also, the implementation details of the stack should belong to the umem
object, and not exposed to the caller.

Clean up and rename for consistency in preparation for the next patch.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c    |  8 +--
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  |  8 +--
 .../ethernet/mellanox/mlx5/core/en/xsk/rx.c   |  2 +-
 .../ethernet/mellanox/mlx5/core/en/xsk/umem.c |  7 +--
 include/net/xdp_sock.h                        | 23 ++------
 net/xdp/xdp_umem.c                            |  2 +-
 net/xdp/xsk_queue.c                           | 56 +++++++------------
 net/xdp/xsk_queue.h                           |  2 +-
 8 files changed, 33 insertions(+), 75 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 3b84fca1c11d..7efe5905b0af 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -78,7 +78,6 @@ static int i40e_xsk_umem_enable(struct i40e_vsi *vsi, struct xdp_umem *umem,
 				u16 qid)
 {
 	struct net_device *netdev = vsi->netdev;
-	struct xdp_umem_fq_reuse *reuseq;
 	bool if_running;
 	int err;
 
@@ -92,12 +91,9 @@ static int i40e_xsk_umem_enable(struct i40e_vsi *vsi, struct xdp_umem *umem,
 	    qid >= netdev->real_num_tx_queues)
 		return -EINVAL;
 
-	reuseq = xsk_reuseq_prepare(vsi->rx_rings[0]->count);
-	if (!reuseq)
+	if (!xsk_umem_recycle_alloc(umem, vsi->rx_rings[0]->count))
 		return -ENOMEM;
 
-	xsk_reuseq_free(xsk_reuseq_swap(umem, reuseq));
-
 	err = i40e_xsk_umem_dma_map(vsi, umem);
 	if (err)
 		return err;
@@ -811,7 +807,7 @@ void i40e_xsk_clean_rx_ring(struct i40e_ring *rx_ring)
 		if (!rx_bi->addr)
 			continue;
 
-		xsk_umem_fq_reuse(rx_ring->xsk_umem, rx_bi->handle);
+		xsk_umem_recycle_addr(rx_ring->xsk_umem, rx_bi->handle);
 		rx_bi->addr = NULL;
 	}
 }
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
index a9cab3271ac9..1e09fa7ffb90 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
@@ -67,7 +67,6 @@ static int ixgbe_xsk_umem_enable(struct ixgbe_adapter *adapter,
 				 u16 qid)
 {
 	struct net_device *netdev = adapter->netdev;
-	struct xdp_umem_fq_reuse *reuseq;
 	bool if_running;
 	int err;
 
@@ -78,12 +77,9 @@ static int ixgbe_xsk_umem_enable(struct ixgbe_adapter *adapter,
 	    qid >= netdev->real_num_tx_queues)
 		return -EINVAL;
 
-	reuseq = xsk_reuseq_prepare(adapter->rx_ring[0]->count);
-	if (!reuseq)
+	if (!xsk_umem_recycle_alloc(umem, adapter->rx_ring[0]->count))
 		return -ENOMEM;
 
-	xsk_reuseq_free(xsk_reuseq_swap(umem, reuseq));
-
 	err = ixgbe_xsk_umem_dma_map(adapter, umem);
 	if (err)
 		return err;
@@ -554,7 +550,7 @@ void ixgbe_xsk_clean_rx_ring(struct ixgbe_ring *rx_ring)
 	struct ixgbe_rx_buffer *bi = &rx_ring->rx_buffer_info[i];
 
 	while (i != rx_ring->next_to_alloc) {
-		xsk_umem_fq_reuse(rx_ring->xsk_umem, bi->handle);
+		xsk_umem_recycle_addr(rx_ring->xsk_umem, bi->handle);
 		i++;
 		bi++;
 		if (i == rx_ring->count) {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c
index 85440b1c1c3f..8d24eaa660a8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c
@@ -44,7 +44,7 @@ int mlx5e_xsk_page_alloc_umem(struct mlx5e_rq *rq,
 
 static inline void mlx5e_xsk_recycle_frame(struct mlx5e_rq *rq, u64 handle)
 {
-	xsk_umem_fq_reuse(rq->umem, handle);
+	xsk_umem_recycle_addr(rq->umem, handle);
 }
 
 /* XSKRQ uses pages from UMEM, they must not be released. They are returned to
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/umem.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/umem.c
index 4baaa5788320..6f75ac653697 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/umem.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/umem.c
@@ -243,13 +243,8 @@ int mlx5e_xsk_setup_umem(struct net_device *dev, struct xdp_umem *umem, u16 qid)
 
 int mlx5e_xsk_resize_reuseq(struct xdp_umem *umem, u32 nentries)
 {
-	struct xdp_umem_fq_reuse *reuseq;
-
-	reuseq = xsk_reuseq_prepare(nentries);
-	if (unlikely(!reuseq))
+	if (!xsk_umem_recycle_alloc(umem, nentries))
 		return -ENOMEM;
-	xsk_reuseq_free(xsk_reuseq_swap(umem, reuseq));
-
 	return 0;
 }
 
diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index 39516d960636..7df7b417ac53 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -83,10 +83,7 @@ void xsk_umem_discard_addr(struct xdp_umem *umem);
 void xsk_umem_complete_tx(struct xdp_umem *umem, u32 nb_entries);
 bool xsk_umem_consume_tx(struct xdp_umem *umem, struct xdp_desc *desc);
 void xsk_umem_consume_tx_done(struct xdp_umem *umem);
-struct xdp_umem_fq_reuse *xsk_reuseq_prepare(u32 nentries);
-struct xdp_umem_fq_reuse *xsk_reuseq_swap(struct xdp_umem *umem,
-					  struct xdp_umem_fq_reuse *newq);
-void xsk_reuseq_free(struct xdp_umem_fq_reuse *rq);
+bool xsk_umem_recycle_alloc(struct xdp_umem *umem, u32 nentries);
 struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev, u16 queue_id);
 
 static inline char *xdp_umem_get_data(struct xdp_umem *umem, u64 addr)
@@ -131,7 +128,7 @@ static inline void xsk_umem_discard_addr_rq(struct xdp_umem *umem)
 		rq->length--;
 }
 
-static inline void xsk_umem_fq_reuse(struct xdp_umem *umem, u64 addr)
+static inline void xsk_umem_recycle_addr(struct xdp_umem *umem, u64 addr)
 {
 	struct xdp_umem_fq_reuse *rq = umem->fq_reuse;
 
@@ -185,19 +182,9 @@ static inline void xsk_umem_consume_tx_done(struct xdp_umem *umem)
 {
 }
 
-static inline struct xdp_umem_fq_reuse *xsk_reuseq_prepare(u32 nentries)
-{
-	return NULL;
-}
-
-static inline struct xdp_umem_fq_reuse *xsk_reuseq_swap(
-	struct xdp_umem *umem,
-	struct xdp_umem_fq_reuse *newq)
-{
-	return NULL;
-}
-static inline void xsk_reuseq_free(struct xdp_umem_fq_reuse *rq)
+static inline bool xsk_umem_recycle_alloc(struct xdp_umem *umem, u32 nentries)
 {
+	return false;
 }
 
 static inline struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev,
@@ -230,7 +217,7 @@ static inline void xsk_umem_discard_addr_rq(struct xdp_umem *umem)
 {
 }
 
-static inline void xsk_umem_fq_reuse(struct xdp_umem *umem, u64 addr)
+static inline void xsk_umem_recycle_addr(struct xdp_umem *umem, u64 addr)
 {
 }
 
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index 9c6de4f114f8..2150ba3fc744 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -208,7 +208,7 @@ static void xdp_umem_release(struct xdp_umem *umem)
 		umem->cq = NULL;
 	}
 
-	xsk_reuseq_destroy(umem);
+	xsk_umem_recycle_destroy(umem);
 
 	xdp_umem_unpin_pages(umem);
 
diff --git a/net/xdp/xsk_queue.c b/net/xdp/xsk_queue.c
index b66504592d9b..116b28622297 100644
--- a/net/xdp/xsk_queue.c
+++ b/net/xdp/xsk_queue.c
@@ -65,55 +65,39 @@ void xskq_destroy(struct xsk_queue *q)
 	kfree(q);
 }
 
-struct xdp_umem_fq_reuse *xsk_reuseq_prepare(u32 nentries)
+bool xsk_umem_recycle_alloc(struct xdp_umem *umem, u32 nentries)
 {
-	struct xdp_umem_fq_reuse *newq;
+	struct xdp_umem_fq_reuse *newq, *oldq;
 
 	/* Check for overflow */
 	if (nentries > (u32)roundup_pow_of_two(nentries))
-		return NULL;
+		return false;
 	nentries = roundup_pow_of_two(nentries);
+	oldq = umem->fq_reuse;
+	if (oldq && (oldq->length >= nentries ||
+		     oldq->nentries == nentries))
+		return true;
 
 	newq = kvmalloc(struct_size(newq, handles, nentries), GFP_KERNEL);
 	if (!newq)
-		return NULL;
-	memset(newq, 0, offsetof(typeof(*newq), handles));
-
-	newq->nentries = nentries;
-	return newq;
-}
-EXPORT_SYMBOL_GPL(xsk_reuseq_prepare);
-
-struct xdp_umem_fq_reuse *xsk_reuseq_swap(struct xdp_umem *umem,
-					  struct xdp_umem_fq_reuse *newq)
-{
-	struct xdp_umem_fq_reuse *oldq = umem->fq_reuse;
-
-	if (!oldq) {
-		umem->fq_reuse = newq;
-		return NULL;
+		return false;
+	if (oldq) {
+		memcpy(newq->handles, oldq->handles,
+		       array_size(oldq->length, sizeof(u64)));
+		newq->length = oldq->length;
+		kvfree(oldq);
+	} else {
+		newq->length = 0;
 	}
-
-	if (newq->nentries < oldq->length)
-		return newq;
-
-	memcpy(newq->handles, oldq->handles,
-	       array_size(oldq->length, sizeof(u64)));
-	newq->length = oldq->length;
-
+	newq->nentries = nentries;
 	umem->fq_reuse = newq;
-	return oldq;
-}
-EXPORT_SYMBOL_GPL(xsk_reuseq_swap);
 
-void xsk_reuseq_free(struct xdp_umem_fq_reuse *rq)
-{
-	kvfree(rq);
+	return true;
 }
-EXPORT_SYMBOL_GPL(xsk_reuseq_free);
+EXPORT_SYMBOL_GPL(xsk_umem_recycle_alloc);
 
-void xsk_reuseq_destroy(struct xdp_umem *umem)
+void xsk_umem_recycle_destroy(struct xdp_umem *umem)
 {
-	xsk_reuseq_free(umem->fq_reuse);
+	kvfree(umem->fq_reuse);
 	umem->fq_reuse = NULL;
 }
diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index 12b49784a6d5..c9db4e8ae80e 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -321,6 +321,6 @@ struct xsk_queue *xskq_create(u32 nentries, bool umem_queue);
 void xskq_destroy(struct xsk_queue *q_ops);
 
 /* Executed by the core when the entire UMEM gets freed */
-void xsk_reuseq_destroy(struct xdp_umem *umem);
+void xsk_umem_recycle_destroy(struct xdp_umem *umem);
 
 #endif /* _LINUX_XSK_QUEUE_H */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/6 bpf-next] Always check the recycle stack when using the umem fq.
  2019-06-27 22:08 [PATCH 0/6 bpf-next] xsk: reuseq cleanup Jonathan Lemon
  2019-06-27 22:08 ` [PATCH 1/6 bpf-next] Have xsk_umem_peek_addr_rq() return chunk-aligned handles Jonathan Lemon
  2019-06-27 22:08 ` [PATCH 2/6 bpf-next] Clean up xsk reuseq API Jonathan Lemon
@ 2019-06-27 22:08 ` Jonathan Lemon
  2019-06-27 22:08 ` [PATCH 4/6 bfp-next] Simplify AF_XDP umem allocation path for Intel drivers Jonathan Lemon
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 16+ messages in thread
From: Jonathan Lemon @ 2019-06-27 22:08 UTC (permalink / raw)
  To: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer; +Cc: kernel-team

The specific _rq variants are deprecated, and will be removed next.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 include/net/xdp_sock.h | 22 +++-------------------
 net/xdp/xsk.c          | 22 +++++++++++++++++++---
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index 7df7b417ac53..55f5f27ef22a 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -99,33 +99,17 @@ static inline dma_addr_t xdp_umem_get_dma(struct xdp_umem *umem, u64 addr)
 /* Reuse-queue aware version of FILL queue helpers */
 static inline bool xsk_umem_has_addrs_rq(struct xdp_umem *umem, u32 cnt)
 {
-	struct xdp_umem_fq_reuse *rq = umem->fq_reuse;
-
-	if (rq->length >= cnt)
-		return true;
-
-	return xsk_umem_has_addrs(umem, cnt - rq->length);
+	return xsk_umem_has_addrs(umem, cnt);
 }
 
 static inline u64 *xsk_umem_peek_addr_rq(struct xdp_umem *umem, u64 *addr)
 {
-	struct xdp_umem_fq_reuse *rq = umem->fq_reuse;
-
-	if (!rq->length)
-		return xsk_umem_peek_addr(umem, addr);
-
-	*addr = rq->handles[rq->length - 1] & umem->chunk_mask;
-	return addr;
+	return xsk_umem_peek_addr(umem, addr);
 }
 
 static inline void xsk_umem_discard_addr_rq(struct xdp_umem *umem)
 {
-	struct xdp_umem_fq_reuse *rq = umem->fq_reuse;
-
-	if (!rq->length)
-		xsk_umem_discard_addr(umem);
-	else
-		rq->length--;
+	return xsk_umem_discard_addr(umem);
 }
 
 static inline void xsk_umem_recycle_addr(struct xdp_umem *umem, u64 addr)
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 74417a851ed5..fc33070b1821 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -39,19 +39,35 @@ bool xsk_is_setup_for_bpf_map(struct xdp_sock *xs)
 
 bool xsk_umem_has_addrs(struct xdp_umem *umem, u32 cnt)
 {
-	return xskq_has_addrs(umem->fq, cnt);
+	struct xdp_umem_fq_reuse *rq = umem->fq_reuse;
+
+        if (rq->length >= cnt)
+                return true;
+
+	return xskq_has_addrs(umem->fq, cnt - rq->length);
 }
 EXPORT_SYMBOL(xsk_umem_has_addrs);
 
 u64 *xsk_umem_peek_addr(struct xdp_umem *umem, u64 *addr)
 {
-	return xskq_peek_addr(umem->fq, addr);
+	struct xdp_umem_fq_reuse *rq = umem->fq_reuse;
+
+	if (!rq->length)
+		return xskq_peek_addr(umem->fq, addr);
+
+	*addr = rq->handles[rq->length - 1] & umem->chunk_mask;
+	return addr;
 }
 EXPORT_SYMBOL(xsk_umem_peek_addr);
 
 void xsk_umem_discard_addr(struct xdp_umem *umem)
 {
-	xskq_discard_addr(umem->fq);
+	struct xdp_umem_fq_reuse *rq = umem->fq_reuse;
+
+	if (!rq->length)
+		xskq_discard_addr(umem->fq);
+	else
+		rq->length--;
 }
 EXPORT_SYMBOL(xsk_umem_discard_addr);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/6 bfp-next] Simplify AF_XDP umem allocation path for Intel drivers.
  2019-06-27 22:08 [PATCH 0/6 bpf-next] xsk: reuseq cleanup Jonathan Lemon
                   ` (2 preceding siblings ...)
  2019-06-27 22:08 ` [PATCH 3/6 bpf-next] Always check the recycle stack when using the umem fq Jonathan Lemon
@ 2019-06-27 22:08 ` Jonathan Lemon
  2019-06-27 22:42   ` Jakub Kicinski
  2019-06-27 22:08 ` [PATCH 5/6 bpf-next] Remove use of umem _rq variants from Mellanox driver Jonathan Lemon
  2019-07-01 10:04 ` [PATCH 0/6 bpf-next] xsk: reuseq cleanup Magnus Karlsson
  5 siblings, 1 reply; 16+ messages in thread
From: Jonathan Lemon @ 2019-06-27 22:08 UTC (permalink / raw)
  To: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer; +Cc: kernel-team

Now that the recycle stack is always used for the driver umem path, the
driver code path can be simplified.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c    | 76 ++-----------------
 .../ethernet/intel/ixgbe/ixgbe_txrx_common.h  |  2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  | 49 +-----------
 3 files changed, 13 insertions(+), 114 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 7efe5905b0af..ce8650d06962 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -265,44 +265,16 @@ static bool i40e_alloc_buffer_zc(struct i40e_ring *rx_ring,
 }
 
 /**
- * i40e_alloc_buffer_slow_zc - Allocates an i40e_rx_buffer
+ * i40e_alloc_rx_buffers_zc - Allocates a number of Rx buffers
  * @rx_ring: Rx ring
- * @bi: Rx buffer to populate
+ * @count: The number of buffers to allocate
  *
- * This function allocates an Rx buffer. The buffer can come from fill
- * queue, or via the reuse queue.
+ * This function allocates a number of Rx buffers from the fill ring
+ * or the internal recycle mechanism and places them on the Rx ring.
  *
  * Returns true for a successful allocation, false otherwise
  **/
-static bool i40e_alloc_buffer_slow_zc(struct i40e_ring *rx_ring,
-				      struct i40e_rx_buffer *bi)
-{
-	struct xdp_umem *umem = rx_ring->xsk_umem;
-	u64 handle, hr;
-
-	if (!xsk_umem_peek_addr_rq(umem, &handle)) {
-		rx_ring->rx_stats.alloc_page_failed++;
-		return false;
-	}
-
-	hr = umem->headroom + XDP_PACKET_HEADROOM;
-
-	bi->dma = xdp_umem_get_dma(umem, handle);
-	bi->dma += hr;
-
-	bi->addr = xdp_umem_get_data(umem, handle);
-	bi->addr += hr;
-
-	bi->handle = handle + umem->headroom;
-
-	xsk_umem_discard_addr_rq(umem);
-	return true;
-}
-
-static __always_inline bool
-__i40e_alloc_rx_buffers_zc(struct i40e_ring *rx_ring, u16 count,
-			   bool alloc(struct i40e_ring *rx_ring,
-				      struct i40e_rx_buffer *bi))
+bool i40e_alloc_rx_buffers_zc(struct i40e_ring *rx_ring, u16 count)
 {
 	u16 ntu = rx_ring->next_to_use;
 	union i40e_rx_desc *rx_desc;
@@ -312,7 +284,7 @@ __i40e_alloc_rx_buffers_zc(struct i40e_ring *rx_ring, u16 count,
 	rx_desc = I40E_RX_DESC(rx_ring, ntu);
 	bi = &rx_ring->rx_bi[ntu];
 	do {
-		if (!alloc(rx_ring, bi)) {
+		if (!i40e_alloc_buffer_zc(rx_ring, bi)) {
 			ok = false;
 			goto no_buffers;
 		}
@@ -344,38 +316,6 @@ __i40e_alloc_rx_buffers_zc(struct i40e_ring *rx_ring, u16 count,
 	return ok;
 }
 
-/**
- * i40e_alloc_rx_buffers_zc - Allocates a number of Rx buffers
- * @rx_ring: Rx ring
- * @count: The number of buffers to allocate
- *
- * This function allocates a number of Rx buffers from the reuse queue
- * or fill ring and places them on the Rx ring.
- *
- * Returns true for a successful allocation, false otherwise
- **/
-bool i40e_alloc_rx_buffers_zc(struct i40e_ring *rx_ring, u16 count)
-{
-	return __i40e_alloc_rx_buffers_zc(rx_ring, count,
-					  i40e_alloc_buffer_slow_zc);
-}
-
-/**
- * i40e_alloc_rx_buffers_fast_zc - Allocates a number of Rx buffers
- * @rx_ring: Rx ring
- * @count: The number of buffers to allocate
- *
- * This function allocates a number of Rx buffers from the fill ring
- * or the internal recycle mechanism and places them on the Rx ring.
- *
- * Returns true for a successful allocation, false otherwise
- **/
-static bool i40e_alloc_rx_buffers_fast_zc(struct i40e_ring *rx_ring, u16 count)
-{
-	return __i40e_alloc_rx_buffers_zc(rx_ring, count,
-					  i40e_alloc_buffer_zc);
-}
-
 /**
  * i40e_get_rx_buffer_zc - Return the current Rx buffer
  * @rx_ring: Rx ring
@@ -541,8 +481,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 
 		if (cleaned_count >= I40E_RX_BUFFER_WRITE) {
 			failure = failure ||
-				  !i40e_alloc_rx_buffers_fast_zc(rx_ring,
-								 cleaned_count);
+				  !i40e_alloc_rx_buffers_zc(rx_ring,
+							    cleaned_count);
 			cleaned_count = 0;
 		}
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h
index d93a690aff74..7408fbc2e1e1 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h
@@ -35,7 +35,7 @@ int ixgbe_xsk_umem_setup(struct ixgbe_adapter *adapter, struct xdp_umem *umem,
 
 void ixgbe_zca_free(struct zero_copy_allocator *alloc, unsigned long handle);
 
-void ixgbe_alloc_rx_buffers_zc(struct ixgbe_ring *rx_ring, u16 cleaned_count);
+bool ixgbe_alloc_rx_buffers_zc(struct ixgbe_ring *rx_ring, u16 cleaned_count);
 int ixgbe_clean_rx_irq_zc(struct ixgbe_q_vector *q_vector,
 			  struct ixgbe_ring *rx_ring,
 			  const int budget);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
index 1e09fa7ffb90..65feb16200ea 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
@@ -278,35 +278,7 @@ static bool ixgbe_alloc_buffer_zc(struct ixgbe_ring *rx_ring,
 	return true;
 }
 
-static bool ixgbe_alloc_buffer_slow_zc(struct ixgbe_ring *rx_ring,
-				       struct ixgbe_rx_buffer *bi)
-{
-	struct xdp_umem *umem = rx_ring->xsk_umem;
-	u64 handle, hr;
-
-	if (!xsk_umem_peek_addr_rq(umem, &handle)) {
-		rx_ring->rx_stats.alloc_rx_page_failed++;
-		return false;
-	}
-
-	hr = umem->headroom + XDP_PACKET_HEADROOM;
-
-	bi->dma = xdp_umem_get_dma(umem, handle);
-	bi->dma += hr;
-
-	bi->addr = xdp_umem_get_data(umem, handle);
-	bi->addr += hr;
-
-	bi->handle = handle + umem->headroom;
-
-	xsk_umem_discard_addr_rq(umem);
-	return true;
-}
-
-static __always_inline bool
-__ixgbe_alloc_rx_buffers_zc(struct ixgbe_ring *rx_ring, u16 cleaned_count,
-			    bool alloc(struct ixgbe_ring *rx_ring,
-				       struct ixgbe_rx_buffer *bi))
+bool ixgbe_alloc_rx_buffers_zc(struct ixgbe_ring *rx_ring, u16 cleaned_count)
 {
 	union ixgbe_adv_rx_desc *rx_desc;
 	struct ixgbe_rx_buffer *bi;
@@ -322,7 +294,7 @@ __ixgbe_alloc_rx_buffers_zc(struct ixgbe_ring *rx_ring, u16 cleaned_count,
 	i -= rx_ring->count;
 
 	do {
-		if (!alloc(rx_ring, bi)) {
+		if (!ixgbe_alloc_buffer_zc(rx_ring, bi)) {
 			ok = false;
 			break;
 		}
@@ -373,19 +345,6 @@ __ixgbe_alloc_rx_buffers_zc(struct ixgbe_ring *rx_ring, u16 cleaned_count,
 	return ok;
 }
 
-void ixgbe_alloc_rx_buffers_zc(struct ixgbe_ring *rx_ring, u16 count)
-{
-	__ixgbe_alloc_rx_buffers_zc(rx_ring, count,
-				    ixgbe_alloc_buffer_slow_zc);
-}
-
-static bool ixgbe_alloc_rx_buffers_fast_zc(struct ixgbe_ring *rx_ring,
-					   u16 count)
-{
-	return __ixgbe_alloc_rx_buffers_zc(rx_ring, count,
-					   ixgbe_alloc_buffer_zc);
-}
-
 static struct sk_buff *ixgbe_construct_skb_zc(struct ixgbe_ring *rx_ring,
 					      struct ixgbe_rx_buffer *bi,
 					      struct xdp_buff *xdp)
@@ -441,8 +400,8 @@ int ixgbe_clean_rx_irq_zc(struct ixgbe_q_vector *q_vector,
 		/* return some buffers to hardware, one at a time is too slow */
 		if (cleaned_count >= IXGBE_RX_BUFFER_WRITE) {
 			failure = failure ||
-				  !ixgbe_alloc_rx_buffers_fast_zc(rx_ring,
-								 cleaned_count);
+				  !ixgbe_alloc_rx_buffers_zc(rx_ring,
+							     cleaned_count);
 			cleaned_count = 0;
 		}
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 5/6 bpf-next] Remove use of umem _rq variants from Mellanox driver.
  2019-06-27 22:08 [PATCH 0/6 bpf-next] xsk: reuseq cleanup Jonathan Lemon
                   ` (3 preceding siblings ...)
  2019-06-27 22:08 ` [PATCH 4/6 bfp-next] Simplify AF_XDP umem allocation path for Intel drivers Jonathan Lemon
@ 2019-06-27 22:08 ` Jonathan Lemon
  2019-07-01 10:04 ` [PATCH 0/6 bpf-next] xsk: reuseq cleanup Magnus Karlsson
  5 siblings, 0 replies; 16+ messages in thread
From: Jonathan Lemon @ 2019-06-27 22:08 UTC (permalink / raw)
  To: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer; +Cc: kernel-team

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c
index 8d24eaa660a8..e116b1fde797 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/rx.c
@@ -12,7 +12,7 @@ bool mlx5e_xsk_pages_enough_umem(struct mlx5e_rq *rq, int count)
 	/* Check in advance that we have enough frames, instead of allocating
 	 * one-by-one, failing and moving frames to the Reuse Ring.
 	 */
-	return xsk_umem_has_addrs_rq(rq->umem, count);
+	return xsk_umem_has_addrs(rq->umem, count);
 }
 
 int mlx5e_xsk_page_alloc_umem(struct mlx5e_rq *rq,
@@ -21,7 +21,7 @@ int mlx5e_xsk_page_alloc_umem(struct mlx5e_rq *rq,
 	struct xdp_umem *umem = rq->umem;
 	u64 handle;
 
-	if (!xsk_umem_peek_addr_rq(umem, &handle))
+	if (!xsk_umem_peek_addr(umem, &handle))
 		return -ENOMEM;
 
 	dma_info->xsk.handle = handle + rq->buff.umem_headroom;
@@ -34,7 +34,7 @@ int mlx5e_xsk_page_alloc_umem(struct mlx5e_rq *rq,
 	 */
 	dma_info->addr = xdp_umem_get_dma(umem, handle);
 
-	xsk_umem_discard_addr_rq(umem);
+	xsk_umem_discard_addr(umem);
 
 	dma_sync_single_for_device(rq->pdev, dma_info->addr, PAGE_SIZE,
 				   DMA_BIDIRECTIONAL);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6 bpf-next] Clean up xsk reuseq API
  2019-06-27 22:08 ` [PATCH 2/6 bpf-next] Clean up xsk reuseq API Jonathan Lemon
@ 2019-06-27 22:38   ` Jakub Kicinski
  2019-06-28  2:31     ` Jonathan Lemon
  0 siblings, 1 reply; 16+ messages in thread
From: Jakub Kicinski @ 2019-06-27 22:38 UTC (permalink / raw)
  To: Jonathan Lemon
  Cc: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer,
	kernel-team

On Thu, 27 Jun 2019 15:08:32 -0700, Jonathan Lemon wrote:
> The reuseq is actually a recycle stack, only accessed from the kernel side.
> Also, the implementation details of the stack should belong to the umem
> object, and not exposed to the caller.
> 
> Clean up and rename for consistency in preparation for the next patch.
> 
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>

Prepare/swap is to cater to how drivers should be written - being able
to allocate resources independently of those currently used.  Allowing
for changing ring sizes and counts on the fly.  This patch makes it
harder to write drivers in the way we are encouraging people to.

IOW no, please don't do this.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/6 bfp-next] Simplify AF_XDP umem allocation path for Intel drivers.
  2019-06-27 22:08 ` [PATCH 4/6 bfp-next] Simplify AF_XDP umem allocation path for Intel drivers Jonathan Lemon
@ 2019-06-27 22:42   ` Jakub Kicinski
  2019-06-28  2:36     ` Jonathan Lemon
  0 siblings, 1 reply; 16+ messages in thread
From: Jakub Kicinski @ 2019-06-27 22:42 UTC (permalink / raw)
  To: Jonathan Lemon
  Cc: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer,
	kernel-team

On Thu, 27 Jun 2019 15:08:34 -0700, Jonathan Lemon wrote:
> Now that the recycle stack is always used for the driver umem path, the
> driver code path can be simplified.
> 
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>

I guess it's a question to Bjorn and Magnus whether they want Intel
drivers to always go through the reuse queue..

Could you be more explicit on the motivation?  I'd call this patch set
"make all drivers use reuse queue" rather than "clean up".

Also when you're changing code please make sure you CC the author.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6 bpf-next] Clean up xsk reuseq API
  2019-06-27 22:38   ` Jakub Kicinski
@ 2019-06-28  2:31     ` Jonathan Lemon
  2019-06-28 20:41       ` Jakub Kicinski
  0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Lemon @ 2019-06-28  2:31 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer,
	kernel-team



On 27 Jun 2019, at 15:38, Jakub Kicinski wrote:

> On Thu, 27 Jun 2019 15:08:32 -0700, Jonathan Lemon wrote:
>> The reuseq is actually a recycle stack, only accessed from the kernel 
>> side.
>> Also, the implementation details of the stack should belong to the 
>> umem
>> object, and not exposed to the caller.
>>
>> Clean up and rename for consistency in preparation for the next 
>> patch.
>>
>> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
>
> Prepare/swap is to cater to how drivers should be written - being able
> to allocate resources independently of those currently used.  Allowing
> for changing ring sizes and counts on the fly.  This patch makes it
> harder to write drivers in the way we are encouraging people to.
>
> IOW no, please don't do this.

The main reason I rewrote this was to provide the same type
of functionality as realloc() - no need to allocate/initialize a new
array if the old one would still end up being used.  This would seem
to be a win for the typical case of having the interface go up/down.

Perhaps I should have named the function differently?
-- 
Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/6 bfp-next] Simplify AF_XDP umem allocation path for Intel drivers.
  2019-06-27 22:42   ` Jakub Kicinski
@ 2019-06-28  2:36     ` Jonathan Lemon
  2019-06-28 20:48       ` Jakub Kicinski
  0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Lemon @ 2019-06-28  2:36 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer,
	kernel-team



On 27 Jun 2019, at 15:42, Jakub Kicinski wrote:

> On Thu, 27 Jun 2019 15:08:34 -0700, Jonathan Lemon wrote:
>> Now that the recycle stack is always used for the driver umem path, the
>> driver code path can be simplified.
>>
>> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
>
> I guess it's a question to Bjorn and Magnus whether they want Intel
> drivers to always go through the reuse queue..

I did pass this by them earlier.


> Could you be more explicit on the motivation?  I'd call this patch set
> "make all drivers use reuse queue" rather than "clean up".

The motivation is to have packets which were received on a zero-copy
AF_XDP socket, and which returned a TX verdict from the bpf program,
queued directly on the TX ring (if they're in the same napi context).

When these TX packets are completed, they are placed back onto the
reuse queue, as there isn't really any other place to handle them.

It also addresses Maxim's concern about having buffers end up sitting
on the rq after a ring resize.

I was going to send the TX change out as part of this patch, but
figured it would be better split unto its own series.

> Also when you're changing code please make sure you CC the author.

Who did I miss?
-- 
Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6 bpf-next] Clean up xsk reuseq API
  2019-06-28  2:31     ` Jonathan Lemon
@ 2019-06-28 20:41       ` Jakub Kicinski
  2019-06-28 21:08         ` Jonathan Lemon
  0 siblings, 1 reply; 16+ messages in thread
From: Jakub Kicinski @ 2019-06-28 20:41 UTC (permalink / raw)
  To: Jonathan Lemon
  Cc: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer,
	kernel-team

On Thu, 27 Jun 2019 19:31:26 -0700, Jonathan Lemon wrote:
> On 27 Jun 2019, at 15:38, Jakub Kicinski wrote:
> 
> > On Thu, 27 Jun 2019 15:08:32 -0700, Jonathan Lemon wrote:  
> >> The reuseq is actually a recycle stack, only accessed from the kernel 
> >> side.
> >> Also, the implementation details of the stack should belong to the 
> >> umem
> >> object, and not exposed to the caller.
> >>
> >> Clean up and rename for consistency in preparation for the next 
> >> patch.
> >>
> >> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>  
> >
> > Prepare/swap is to cater to how drivers should be written - being able
> > to allocate resources independently of those currently used.  Allowing
> > for changing ring sizes and counts on the fly.  This patch makes it
> > harder to write drivers in the way we are encouraging people to.
> >
> > IOW no, please don't do this.  
> 
> The main reason I rewrote this was to provide the same type
> of functionality as realloc() - no need to allocate/initialize a new
> array if the old one would still end up being used.  This would seem
> to be a win for the typical case of having the interface go up/down.
> 
> Perhaps I should have named the function differently?

Perhaps add a helper which calls both parts to help poorly architected
drivers? 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/6 bfp-next] Simplify AF_XDP umem allocation path for Intel drivers.
  2019-06-28  2:36     ` Jonathan Lemon
@ 2019-06-28 20:48       ` Jakub Kicinski
  2019-06-28 21:06         ` Jonathan Lemon
  0 siblings, 1 reply; 16+ messages in thread
From: Jakub Kicinski @ 2019-06-28 20:48 UTC (permalink / raw)
  To: Jonathan Lemon
  Cc: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer,
	kernel-team

On Thu, 27 Jun 2019 19:36:42 -0700, Jonathan Lemon wrote:
> On 27 Jun 2019, at 15:42, Jakub Kicinski wrote:
> > Could you be more explicit on the motivation?  I'd call this patch set
> > "make all drivers use reuse queue" rather than "clean up".  
> 
> The motivation is to have packets which were received on a zero-copy
> AF_XDP socket, and which returned a TX verdict from the bpf program,
> queued directly on the TX ring (if they're in the same napi context).
> 
> When these TX packets are completed, they are placed back onto the
> reuse queue, as there isn't really any other place to handle them.
> 
> It also addresses Maxim's concern about having buffers end up sitting
> on the rq after a ring resize.
> 
> I was going to send the TX change out as part of this patch, but
> figured it would be better split unto its own series.

Makes sense, please put that info in the cover letter.  (in other nit
picks please prefix the patches which the name of the subsystem they
are touching - net: xsk: ?, and make sure all changes have a commit
message).

Looking at this yesterday I think I've seen you didn't change the RQ
sizing -  for the TX side to work the rq size must be RX ring size
+ XDP TX ring size (IIRC that was the worst case number of packets
which we may need to park during reconfiguration).  

Maybe you did do that and I missed it.

> > Also when you're changing code please make sure you CC the author.  
> 
> Who did I miss?

commit f5bd91388e26557f64ca999e0006038c7a919308
Author: Jakub Kicinski <jakub.kicinski@netronome.com>
Date:   Fri Sep 7 10:18:46 2018 +0200

    net: xsk: add a simple buffer reuse queue

    XSK UMEM is strongly single producer single consumer so reuse of
    frames is challenging.  Add a simple "stash" of FILL packets to
    reuse for drivers to optionally make use of.  This is useful
    when driver has to free (ndo_stop) or resize a ring with an active
    AF_XDP ZC socket.
    
    Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
    Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
    Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>


Sorry to get upset, I missed a bpf patch recently and had to do 
a revert..

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/6 bfp-next] Simplify AF_XDP umem allocation path for Intel drivers.
  2019-06-28 20:48       ` Jakub Kicinski
@ 2019-06-28 21:06         ` Jonathan Lemon
  0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Lemon @ 2019-06-28 21:06 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer,
	kernel-team

On 28 Jun 2019, at 13:48, Jakub Kicinski wrote:

> On Thu, 27 Jun 2019 19:36:42 -0700, Jonathan Lemon wrote:
>> On 27 Jun 2019, at 15:42, Jakub Kicinski wrote:
>>> Could you be more explicit on the motivation?  I'd call this patch set
>>> "make all drivers use reuse queue" rather than "clean up".
>>
>> The motivation is to have packets which were received on a zero-copy
>> AF_XDP socket, and which returned a TX verdict from the bpf program,
>> queued directly on the TX ring (if they're in the same napi context).
>>
>> When these TX packets are completed, they are placed back onto the
>> reuse queue, as there isn't really any other place to handle them.
>>
>> It also addresses Maxim's concern about having buffers end up sitting
>> on the rq after a ring resize.
>>
>> I was going to send the TX change out as part of this patch, but
>> figured it would be better split unto its own series.
>
> Makes sense, please put that info in the cover letter.  (in other nit
> picks please prefix the patches which the name of the subsystem they
> are touching - net: xsk: ?, and make sure all changes have a commit
> message).
>
> Looking at this yesterday I think I've seen you didn't change the RQ
> sizing -  for the TX side to work the rq size must be RX ring size
> + XDP TX ring size (IIRC that was the worst case number of packets
> which we may need to park during reconfiguration).
>
> Maybe you did do that and I missed it.

Not yet - that's in the TX section of the patches, I'll post the
Intel versions shortly.



>>> Also when you're changing code please make sure you CC the author.
>>
>> Who did I miss?
>
> commit f5bd91388e26557f64ca999e0006038c7a919308
> Author: Jakub Kicinski <jakub.kicinski@netronome.com>
> Date:   Fri Sep 7 10:18:46 2018 +0200
>
>     net: xsk: add a simple buffer reuse queue
>
>     XSK UMEM is strongly single producer single consumer so reuse of
>     frames is challenging.  Add a simple "stash" of FILL packets to
>     reuse for drivers to optionally make use of.  This is useful
>     when driver has to free (ndo_stop) or resize a ring with an active
>     AF_XDP ZC socket.
>
>     Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>     Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
>     Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>
>
> Sorry to get upset, I missed a bpf patch recently and had to do
> a revert..

Ack.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6 bpf-next] Clean up xsk reuseq API
  2019-06-28 20:41       ` Jakub Kicinski
@ 2019-06-28 21:08         ` Jonathan Lemon
  2019-07-01  9:58           ` Magnus Karlsson
  0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Lemon @ 2019-06-28 21:08 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, bjorn.topel, magnus.karlsson, saeedm, maximmi, brouer,
	kernel-team



On 28 Jun 2019, at 13:41, Jakub Kicinski wrote:

> On Thu, 27 Jun 2019 19:31:26 -0700, Jonathan Lemon wrote:
>> On 27 Jun 2019, at 15:38, Jakub Kicinski wrote:
>>
>>> On Thu, 27 Jun 2019 15:08:32 -0700, Jonathan Lemon wrote:
>>>> The reuseq is actually a recycle stack, only accessed from the kernel
>>>> side.
>>>> Also, the implementation details of the stack should belong to the
>>>> umem
>>>> object, and not exposed to the caller.
>>>>
>>>> Clean up and rename for consistency in preparation for the next
>>>> patch.
>>>>
>>>> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
>>>
>>> Prepare/swap is to cater to how drivers should be written - being able
>>> to allocate resources independently of those currently used.  Allowing
>>> for changing ring sizes and counts on the fly.  This patch makes it
>>> harder to write drivers in the way we are encouraging people to.
>>>
>>> IOW no, please don't do this.
>>
>> The main reason I rewrote this was to provide the same type
>> of functionality as realloc() - no need to allocate/initialize a new
>> array if the old one would still end up being used.  This would seem
>> to be a win for the typical case of having the interface go up/down.
>>
>> Perhaps I should have named the function differently?
>
> Perhaps add a helper which calls both parts to help poorly architected
> drivers?

Still ends up taking more memory.

There are only 3 drivers in the tree which do AF_XDP: i40e, ixgbe, and mlx5.

All of these do the same thing:
    reuseq = xsk_reuseq_prepare(n)
    if (!reuseq)
       error
    xsk_reuseq_free(xsk_reuseq_swap(umem, reuseq));

I figured simplifying was a good thing.

But I do take your point that some future driver might want to allocate
everything up front before performing a commit of the resources.
-- 
Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6 bpf-next] Clean up xsk reuseq API
  2019-06-28 21:08         ` Jonathan Lemon
@ 2019-07-01  9:58           ` Magnus Karlsson
  0 siblings, 0 replies; 16+ messages in thread
From: Magnus Karlsson @ 2019-07-01  9:58 UTC (permalink / raw)
  To: Jonathan Lemon
  Cc: Jakub Kicinski, Network Development, Björn Töpel,
	Karlsson, Magnus, Saeed Mahameed, Maxim Mikityanskiy,
	Jesper Dangaard Brouer, kernel-team

On Fri, Jun 28, 2019 at 11:09 PM Jonathan Lemon
<jonathan.lemon@gmail.com> wrote:
>
>
>
> On 28 Jun 2019, at 13:41, Jakub Kicinski wrote:
>
> > On Thu, 27 Jun 2019 19:31:26 -0700, Jonathan Lemon wrote:
> >> On 27 Jun 2019, at 15:38, Jakub Kicinski wrote:
> >>
> >>> On Thu, 27 Jun 2019 15:08:32 -0700, Jonathan Lemon wrote:
> >>>> The reuseq is actually a recycle stack, only accessed from the kernel
> >>>> side.
> >>>> Also, the implementation details of the stack should belong to the
> >>>> umem
> >>>> object, and not exposed to the caller.
> >>>>
> >>>> Clean up and rename for consistency in preparation for the next
> >>>> patch.
> >>>>
> >>>> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> >>>
> >>> Prepare/swap is to cater to how drivers should be written - being able
> >>> to allocate resources independently of those currently used.  Allowing
> >>> for changing ring sizes and counts on the fly.  This patch makes it
> >>> harder to write drivers in the way we are encouraging people to.
> >>>
> >>> IOW no, please don't do this.
> >>
> >> The main reason I rewrote this was to provide the same type
> >> of functionality as realloc() - no need to allocate/initialize a new
> >> array if the old one would still end up being used.  This would seem
> >> to be a win for the typical case of having the interface go up/down.
> >>
> >> Perhaps I should have named the function differently?
> >
> > Perhaps add a helper which calls both parts to help poorly architected
> > drivers?
>
> Still ends up taking more memory.
>
> There are only 3 drivers in the tree which do AF_XDP: i40e, ixgbe, and mlx5.
>
> All of these do the same thing:
>     reuseq = xsk_reuseq_prepare(n)
>     if (!reuseq)
>        error
>     xsk_reuseq_free(xsk_reuseq_swap(umem, reuseq));
>
> I figured simplifying was a good thing.
>
> But I do take your point that some future driver might want to allocate
> everything up front before performing a commit of the resources.

Jonathan, can you come up with a solution that satisfies both these
goals: providing a lower level API that Jakub can and would like to
use for his driver and a higher level helper that can be used by
today's driver to make the AF_XDP part smaller and easier to
implement? I like the fact that you are simplifying the AF_XDP enabled
drivers that are out there today, but at the same time I do not want
to hinder Jakub from, hopefully, in the future upstreaming his
support.

Thanks: Magnus

> --
> Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/6 bpf-next] xsk: reuseq cleanup
  2019-06-27 22:08 [PATCH 0/6 bpf-next] xsk: reuseq cleanup Jonathan Lemon
                   ` (4 preceding siblings ...)
  2019-06-27 22:08 ` [PATCH 5/6 bpf-next] Remove use of umem _rq variants from Mellanox driver Jonathan Lemon
@ 2019-07-01 10:04 ` Magnus Karlsson
  5 siblings, 0 replies; 16+ messages in thread
From: Magnus Karlsson @ 2019-07-01 10:04 UTC (permalink / raw)
  To: Jonathan Lemon
  Cc: Network Development, Björn Töpel, Karlsson, Magnus,
	Saeed Mahameed, Maxim Mikityanskiy, Jesper Dangaard Brouer,
	kernel-team

On Fri, Jun 28, 2019 at 12:10 AM Jonathan Lemon
<jonathan.lemon@gmail.com> wrote:
>
> Clean up and normalize usage of the recycle queue in order to
> support upcoming TX from RX queue functionality.
>
> Jonathan Lemon (6):
>   Have xsk_umem_peek_addr_rq() return chunk-aligned handles.
>   Clean up xsk reuseq API
>   Always check the recycle stack when using the umem fq.
>   Simplify AF_XDP umem allocation path for Intel drivers.
>   Remove use of umem _rq variants from Mellanox driver.
>   Remove the umem _rq variants now that the last consumer is gone.

Maybe it is just me, but I cannot find patch 6/6. Not in my gmail
account and not in my Intel account. Am I just going insane :-)?

/Magnus

>  drivers/net/ethernet/intel/i40e/i40e_xsk.c    | 86 +++----------------
>  .../ethernet/intel/ixgbe/ixgbe_txrx_common.h  |  2 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c  | 59 ++-----------
>  .../ethernet/mellanox/mlx5/core/en/xsk/rx.c   |  8 +-
>  .../ethernet/mellanox/mlx5/core/en/xsk/umem.c |  7 +-
>  include/net/xdp_sock.h                        | 69 ++-------------
>  net/xdp/xdp_umem.c                            |  2 +-
>  net/xdp/xsk.c                                 | 22 ++++-
>  net/xdp/xsk_queue.c                           | 56 +++++-------
>  net/xdp/xsk_queue.h                           |  2 +-
>  10 files changed, 68 insertions(+), 245 deletions(-)
>
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2019-07-01 10:05 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-27 22:08 [PATCH 0/6 bpf-next] xsk: reuseq cleanup Jonathan Lemon
2019-06-27 22:08 ` [PATCH 1/6 bpf-next] Have xsk_umem_peek_addr_rq() return chunk-aligned handles Jonathan Lemon
2019-06-27 22:08 ` [PATCH 2/6 bpf-next] Clean up xsk reuseq API Jonathan Lemon
2019-06-27 22:38   ` Jakub Kicinski
2019-06-28  2:31     ` Jonathan Lemon
2019-06-28 20:41       ` Jakub Kicinski
2019-06-28 21:08         ` Jonathan Lemon
2019-07-01  9:58           ` Magnus Karlsson
2019-06-27 22:08 ` [PATCH 3/6 bpf-next] Always check the recycle stack when using the umem fq Jonathan Lemon
2019-06-27 22:08 ` [PATCH 4/6 bfp-next] Simplify AF_XDP umem allocation path for Intel drivers Jonathan Lemon
2019-06-27 22:42   ` Jakub Kicinski
2019-06-28  2:36     ` Jonathan Lemon
2019-06-28 20:48       ` Jakub Kicinski
2019-06-28 21:06         ` Jonathan Lemon
2019-06-27 22:08 ` [PATCH 5/6 bpf-next] Remove use of umem _rq variants from Mellanox driver Jonathan Lemon
2019-07-01 10:04 ` [PATCH 0/6 bpf-next] xsk: reuseq cleanup Magnus Karlsson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.