netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings
@ 2019-06-13  7:37 Magnus Karlsson
  2019-06-13  7:37 ` [PATCH bpf-next 1/6] xsk: replace ndo_xsk_async_xmit with ndo_xsk_wakeup Magnus Karlsson
                   ` (7 more replies)
  0 siblings, 8 replies; 13+ messages in thread
From: Magnus Karlsson @ 2019-06-13  7:37 UTC (permalink / raw)
  To: magnus.karlsson, bjorn.topel, ast, daniel, netdev, brouer
  Cc: bpf, bruce.richardson, ciara.loftus, jakub.kicinski, xiaolong.ye,
	qi.z.zhang, maximmi, sridhar.samudrala, kevin.laatz,
	ilias.apalodimas, kiran.patil, axboe, maciej.fijalkowski,
	maciejromanfijalkowski, intel-wired-lan


This patch set adds support for a new flag called need_wakeup in the
AF_XDP Tx and fill rings. When this flag is set by the driver, it
means that the application has to explicitly wake up the kernel Rx
(for the bit in the fill ring) or kernel Tx (for bit in the Tx ring)
processing by issuing a syscall. Poll() can wake up both and sendto()
will wake up Tx processing only.

The main reason for introducing this new flag is to be able to
efficiently support the case when application and driver is executing
on the same core. Previously, the driver was just busy-spinning on the
fill ring if it ran out of buffers in the HW and there were none to
get from the fill ring. This approach works when the application and
driver is running on different cores as the application can replenish
the fill ring while the driver is busy-spinning. Though, this is a
lousy approach if both of them are running on the same core as the
probability of the fill ring getting more entries when the driver is
busy-spinning is zero. With this new feature the driver now sets the
need_wakeup flag and returns to the application. The application can
then replenish the fill queue and then explicitly wake up the Rx
processing in the kernel using the syscall poll(). For Tx, the flag is
only set to one if the driver has no outstanding Tx completion
interrupts. If it has some, the flag is zero as it will be woken up by
a completion interrupt anyway. This flag can also be used in other
situations where the driver needs to be woken up explicitly.

As a nice side effect, this new flag also improves the Tx performance
of the case where application and driver are running on two different
cores as it reduces the number of syscalls to the kernel. The kernel
tells user space if it needs to be woken up by a syscall, and this
eliminates many of the syscalls. The Rx performance of the 2-core case
is on the other hand slightly worse, since there is a need to use a
syscall now to wake up the driver, instead of the driver
busy-spinning. It does waste less CPU cycles though, which might lead
to better overall system performance.

This new flag needs some simple driver support. If the driver does not
support it, the Rx flag is always zero and the Tx flag is always
one. This makes any application relying on this feature default to the
old behavior of not requiring any syscalls in the Rx path and always
having to call sendto() in the Tx path.

For backwards compatibility reasons, this feature has to be explicitly
turned on using a new bind flag (XDP_USE_NEED_WAKEUP). I recommend
that you always turn it on as it has a large positive performance
impact for the one core case and does not degrade 2 core performance
and actually improves it for Tx heavy workloads.

Here are some performance numbers measured on my local,
non-performance optimized development system. That is why you are
seeing numbers lower than the ones from Björn and Jesper. 64 byte
packets at 40Gbit/s line rate. All results in Mpps. Cores == 1 means
that both application and driver is executing on the same core. Cores
== 2 that they are on different cores.

                              Applications
need_wakeup  cores    txpush    rxdrop      l2fwd
---------------------------------------------------------------
     n         1       0.07      0.06        0.03
     y         1       21.6      8.2         6.5
     n         2       32.3      11.7        8.7
     y         2       33.1      11.7        8.7

Overall, the need_wakeup flag provides the same or better performance
in all the micro-benchmarks. The reduction of sendto() calls in txpush
is large. Only a few per second is needed. For l2fwd, the drop is 50%
for the 1 core case and more than 99.9% for the 2 core case. Do not
know why I am not seeing the same drop for the 1 core case yet.

The name and inspiration of the flag has been taken from io_uring by
Jens Axboe. Details about this feature in io_uring can be found in
http://kernel.dk/io_uring.pdf, section 8.3. It also addresses most of
the denial of service and sendto() concerns raised by Maxim
Mikityanskiy in https://www.spinics.net/lists/netdev/msg554657.html.

The typical Tx part of an application will have to change from:

ret = sendto(fd,....)

to:

if (xsk_ring_prod__needs_wakeup(&xsk->tx))
       ret = sendto(fd,....)

and th Rx part from:

rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
if (!rcvd)
       return;

to:

rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
if (!rcvd) {
       if (xsk_ring_prod__needs_wakeup(&xsk->umem->fq))
              ret = poll(fd,.....);
       return;
}

This patch has been applied against commit aee450cbe482 ("bpf: silence warning messages in core")

Structure of the patch set:

Patch 1: Replaces the ndo_xsk_async_xmit with ndo_xsk_wakeup to
         support waking up both Rx and Tx processing
Patch 2: Implements the need_wakeup functionality in common code
Patch 3-4: Add need_wakeup support to the i40e and ixgbe drivers
Patch 5: Add need_wakeup support to libbpf
Patch 6: Add need_wakeup support to the xdpsock sample application

Thanks: Magnus

Magnus Karlsson (6):
  xsk: replace ndo_xsk_async_xmit with ndo_xsk_wakeup
  xsk: add support for need_wakeup flag in AF_XDP rings
  i40e: add support for AF_XDP need_wakup feature
  ixgbe: add support for AF_XDP need_wakup feature
  libbpf: add support for need_wakeup flag in AF_XDP part
  samples/bpf: add use of need_sleep flag in xdpsock

 drivers/net/ethernet/intel/i40e/i40e_main.c        |   5 +-
 drivers/net/ethernet/intel/i40e/i40e_xsk.c         |  23 ++-
 drivers/net/ethernet/intel/i40e/i40e_xsk.h         |   2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c      |   5 +-
 .../net/ethernet/intel/ixgbe/ixgbe_txrx_common.h   |   2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c       |  20 ++-
 include/linux/netdevice.h                          |  18 +-
 include/net/xdp_sock.h                             |  33 +++-
 include/uapi/linux/if_xdp.h                        |  13 ++
 net/xdp/xdp_umem.c                                 |   6 +-
 net/xdp/xsk.c                                      |  93 +++++++++-
 net/xdp/xsk_queue.h                                |   1 +
 samples/bpf/xdpsock_user.c                         | 191 +++++++++++++--------
 tools/include/uapi/linux/if_xdp.h                  |  13 ++
 tools/lib/bpf/xsk.c                                |   4 +
 tools/lib/bpf/xsk.h                                |   6 +
 16 files changed, 343 insertions(+), 92 deletions(-)

--
2.7.4

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

* [PATCH bpf-next 1/6] xsk: replace ndo_xsk_async_xmit with ndo_xsk_wakeup
  2019-06-13  7:37 [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings Magnus Karlsson
@ 2019-06-13  7:37 ` Magnus Karlsson
  2019-06-13  7:37 ` [PATCH bpf-next 2/6] xsk: add support for need_wakeup flag in AF_XDP rings Magnus Karlsson
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Magnus Karlsson @ 2019-06-13  7:37 UTC (permalink / raw)
  To: magnus.karlsson, bjorn.topel, ast, daniel, netdev, brouer
  Cc: bpf, bruce.richardson, ciara.loftus, jakub.kicinski, xiaolong.ye,
	qi.z.zhang, maximmi, sridhar.samudrala, kevin.laatz,
	ilias.apalodimas, kiran.patil, axboe, maciej.fijalkowski,
	maciejromanfijalkowski, intel-wired-lan

This commit replaces ndo_xsk_async_xmit with ndo_xsk_wakeup. This new
ndo provides the same functionality as before but with the addition of
a new flags field that is used to specifiy if Rx, Tx or both should be
woken up. The previous ndo only woke up Tx, as implied by the
name. The i40e and ixgbe drivers (which are all the supported ones)
are updated with this new interface.

This new ndo will be used by the new need_wakeup functionality of XDP
sockets that need to be able to wake up both Rx and Tx driver
processing.

Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c          |  5 +++--
 drivers/net/ethernet/intel/i40e/i40e_xsk.c           |  7 ++++---
 drivers/net/ethernet/intel/i40e/i40e_xsk.h           |  2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c        |  5 +++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h |  2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c         |  4 ++--
 include/linux/netdevice.h                            | 14 ++++++++++++--
 7 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 320562b..327182e 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -12025,7 +12025,8 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi,
 	if (need_reset && prog)
 		for (i = 0; i < vsi->num_queue_pairs; i++)
 			if (vsi->xdp_rings[i]->xsk_umem)
-				(void)i40e_xsk_async_xmit(vsi->netdev, i);
+				(void)i40e_xsk_wakeup(vsi->netdev, i,
+						      XDP_WAKEUP_RX);
 
 	return 0;
 }
@@ -12347,7 +12348,7 @@ static const struct net_device_ops i40e_netdev_ops = {
 	.ndo_bridge_setlink	= i40e_ndo_bridge_setlink,
 	.ndo_bpf		= i40e_xdp,
 	.ndo_xdp_xmit		= i40e_xdp_xmit,
-	.ndo_xsk_async_xmit	= i40e_xsk_async_xmit,
+	.ndo_xsk_wakeup	        = i40e_xsk_wakeup,
 };
 
 /**
diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 1b17486..32be3dd 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -116,7 +116,7 @@ static int i40e_xsk_umem_enable(struct i40e_vsi *vsi, struct xdp_umem *umem,
 			return err;
 
 		/* Kick start the NAPI context so that receiving will start */
-		err = i40e_xsk_async_xmit(vsi->netdev, qid);
+		err = i40e_xsk_wakeup(vsi->netdev, qid, XDP_WAKEUP_RX);
 		if (err)
 			return err;
 	}
@@ -762,13 +762,14 @@ bool i40e_clean_xdp_tx_irq(struct i40e_vsi *vsi,
 }
 
 /**
- * i40e_xsk_async_xmit - Implements the ndo_xsk_async_xmit
+ * i40e_xsk_wakeup - Implements the ndo_xsk_wakeup
  * @dev: the netdevice
  * @queue_id: queue id to wake up
+ * @flags: ignored in our case since we have Rx and Tx in the same NAPI.
  *
  * Returns <0 for errors, 0 otherwise.
  **/
-int i40e_xsk_async_xmit(struct net_device *dev, u32 queue_id)
+int i40e_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags)
 {
 	struct i40e_netdev_priv *np = netdev_priv(dev);
 	struct i40e_vsi *vsi = np->vsi;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.h b/drivers/net/ethernet/intel/i40e/i40e_xsk.h
index 8cc0a2e..9ed59c1 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.h
@@ -18,6 +18,6 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget);
 
 bool i40e_clean_xdp_tx_irq(struct i40e_vsi *vsi,
 			   struct i40e_ring *tx_ring, int napi_budget);
-int i40e_xsk_async_xmit(struct net_device *dev, u32 queue_id);
+int i40e_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags);
 
 #endif /* _I40E_XSK_H_ */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 57fd9ee..5812f84 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -10272,7 +10272,8 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
 	if (need_reset && prog)
 		for (i = 0; i < adapter->num_rx_queues; i++)
 			if (adapter->xdp_ring[i]->xsk_umem)
-				(void)ixgbe_xsk_async_xmit(adapter->netdev, i);
+				(void)ixgbe_xsk_wakeup(adapter->netdev, i,
+						       XDP_WAKEUP_RX);
 
 	return 0;
 }
@@ -10391,7 +10392,7 @@ static const struct net_device_ops ixgbe_netdev_ops = {
 	.ndo_features_check	= ixgbe_features_check,
 	.ndo_bpf		= ixgbe_xdp,
 	.ndo_xdp_xmit		= ixgbe_xdp_xmit,
-	.ndo_xsk_async_xmit	= ixgbe_xsk_async_xmit,
+	.ndo_xsk_wakeup         = ixgbe_xsk_wakeup,
 };
 
 static void ixgbe_disable_txr_hw(struct ixgbe_adapter *adapter,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h
index d93a690..6d01700 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h
@@ -42,7 +42,7 @@ int ixgbe_clean_rx_irq_zc(struct ixgbe_q_vector *q_vector,
 void ixgbe_xsk_clean_rx_ring(struct ixgbe_ring *rx_ring);
 bool ixgbe_clean_xdp_tx_irq(struct ixgbe_q_vector *q_vector,
 			    struct ixgbe_ring *tx_ring, int napi_budget);
-int ixgbe_xsk_async_xmit(struct net_device *dev, u32 queue_id);
+int ixgbe_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags);
 void ixgbe_xsk_clean_tx_ring(struct ixgbe_ring *tx_ring);
 
 #endif /* #define _IXGBE_TXRX_COMMON_H_ */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
index bfe95ce..1a1add8 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
@@ -151,7 +151,7 @@ static int ixgbe_xsk_umem_enable(struct ixgbe_adapter *adapter,
 		ixgbe_txrx_ring_enable(adapter, qid);
 
 		/* Kick start the NAPI context so that receiving will start */
-		err = ixgbe_xsk_async_xmit(adapter->netdev, qid);
+		err = ixgbe_xsk_wakeup(adapter->netdev, qid, XDP_WAKEUP_RX);
 		if (err)
 			return err;
 	}
@@ -739,7 +739,7 @@ bool ixgbe_clean_xdp_tx_irq(struct ixgbe_q_vector *q_vector,
 	return budget > 0 && xmit_done;
 }
 
-int ixgbe_xsk_async_xmit(struct net_device *dev, u32 qid)
+int ixgbe_xsk_wakeup(struct net_device *dev, u32 qid, u32 flags)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(dev);
 	struct ixgbe_ring *ring;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 44b47e9..fd29d39 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -903,6 +903,10 @@ struct netdev_bpf {
 	};
 };
 
+/* Flags for ndo_xsk_wakeup. */
+#define XDP_WAKEUP_RX (1 << 0)
+#define XDP_WAKEUP_TX (1 << 1)
+
 #ifdef CONFIG_XFRM_OFFLOAD
 struct xfrmdev_ops {
 	int	(*xdo_dev_state_add) (struct xfrm_state *x);
@@ -1229,6 +1233,12 @@ struct tlsdev_ops;
  *	that got dropped are freed/returned via xdp_return_frame().
  *	Returns negative number, means general error invoking ndo, meaning
  *	no frames were xmit'ed and core-caller will free all frames.
+ * int (*ndo_xsk_wakeup)(struct net_device *dev, u32 queue_id, u32 flags);
+ *      This function is used to wake up the softirq, ksoftirqd or kthread
+ *	responsible for sending and/or receiving packets on a specific
+ *	queue id bound to an AF_XDP socket. The flags field specifies if
+ *	only RX, only Tx, or both should be woken up using the flags
+ *	XDP_WAKEUP_RX and XDP_WAKEUP_TX.
  * struct devlink_port *(*ndo_get_devlink_port)(struct net_device *dev);
  *	Get devlink port instance associated with a given netdev.
  *	Called with a reference on the netdevice and devlink locks only,
@@ -1428,8 +1438,8 @@ struct net_device_ops {
 	int			(*ndo_xdp_xmit)(struct net_device *dev, int n,
 						struct xdp_frame **xdp,
 						u32 flags);
-	int			(*ndo_xsk_async_xmit)(struct net_device *dev,
-						      u32 queue_id);
+	int			(*ndo_xsk_wakeup)(struct net_device *dev,
+						  u32 queue_id, u32 flags);
 	struct devlink_port *	(*ndo_get_devlink_port)(struct net_device *dev);
 };
 
-- 
2.7.4


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

* [PATCH bpf-next 2/6] xsk: add support for need_wakeup flag in AF_XDP rings
  2019-06-13  7:37 [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings Magnus Karlsson
  2019-06-13  7:37 ` [PATCH bpf-next 1/6] xsk: replace ndo_xsk_async_xmit with ndo_xsk_wakeup Magnus Karlsson
@ 2019-06-13  7:37 ` Magnus Karlsson
  2019-06-13 19:04   ` Jakub Kicinski
  2019-06-13  7:37 ` [PATCH bpf-next 3/6] i40e: add support for AF_XDP need_wakup feature Magnus Karlsson
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Magnus Karlsson @ 2019-06-13  7:37 UTC (permalink / raw)
  To: magnus.karlsson, bjorn.topel, ast, daniel, netdev, brouer
  Cc: bpf, bruce.richardson, ciara.loftus, jakub.kicinski, xiaolong.ye,
	qi.z.zhang, maximmi, sridhar.samudrala, kevin.laatz,
	ilias.apalodimas, kiran.patil, axboe, maciej.fijalkowski,
	maciejromanfijalkowski, intel-wired-lan

This commit adds support for a new flag called need_wakeup in the
AF_XDP Tx and fill rings. When this flag is set, it means that the
application has to explicitly wake up the kernel Rx (for the bit in
the fill ring) or kernel Tx (for bit in the Tx ring) processing by
issuing a syscall. Poll() can wake up both depending on the flags
submitted and sendto() will wake up tx processing only.

The main reason for introducing this new flag is to be able to
efficiently support the case when application and driver is executing
on the same core. Previously, the driver was just busy-spinning on the
fill ring if it ran out of buffers in the HW and there were none on
the fill ring. This approach works when the application is running on
another core as it can replenish the fill ring while the driver is
busy-spinning. Though, this is a lousy approach if both of them are
running on the same core as the probability of the fill ring getting
more entries when the driver is busy-spinning is zero. With this new
feature the driver now sets the need_wakeup flag and returns to the
application. The application can then replenish the fill queue and
then explicitly wake up the Rx processing in the kernel using the
syscall poll(). For Tx, the flag is only set to one if the driver has
no outstanding Tx completion interrupts. If it has some, the flag is
zero as it will be woken up by a completion interrupt anyway.

As a nice side effect, this new flag also improves the performance of
the case where application and driver are running on two different
cores as it reduces the number of syscalls to the kernel. The kernel
tells user space if it needs to be woken up by a syscall, and this
eliminates many of the syscalls.

This flag needs some simple driver support. If the driver does not
support this, the Rx flag is always zero and the Tx flag is always
one. This makes any application relying on this feature default to the
old behaviour of not requiring any syscalls in the Rx path and always
having to call sendto() in the Tx path.

For backwards compatibility reasons, this feature has to be explicitly
turned on using a new bind flag (XDP_USE_NEED_WAKEUP). I recommend
that you always turn it on as it so far always have had a positive
performance impact.

The name and inspiration of the flag has been taken from io_uring by
Jens Axboe. Details about this feature in io_uring can be found in
http://kernel.dk/io_uring.pdf, section 8.3.

Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 include/linux/netdevice.h   |  4 ++
 include/net/xdp_sock.h      | 33 +++++++++++++++-
 include/uapi/linux/if_xdp.h | 13 +++++++
 net/xdp/xdp_umem.c          |  6 ++-
 net/xdp/xsk.c               | 93 +++++++++++++++++++++++++++++++++++++++++++--
 net/xdp/xsk_queue.h         |  1 +
 6 files changed, 143 insertions(+), 7 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index fd29d39..8c4320b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -876,6 +876,9 @@ struct bpf_prog_offload_ops;
 struct netlink_ext_ack;
 struct xdp_umem;
 
+/* Flags for the flags field in XDP_SETUP_XSK_UMEM */
+#define XSK_DRV_CAN_SLEEP (1 << 0) /* The driver is alowed to sleep. */
+
 struct netdev_bpf {
 	enum bpf_netdev_command command;
 	union {
@@ -898,6 +901,7 @@ struct netdev_bpf {
 		/* XDP_SETUP_XSK_UMEM */
 		struct {
 			struct xdp_umem *umem;
+			u32 flags;
 			u16 queue_id;
 		} xsk;
 	};
diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index ae0f368..4d894a5 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -27,6 +27,9 @@ struct xdp_umem_fq_reuse {
 	u64 handles[];
 };
 
+/* Flags for the umem flags field. */
+#define XDP_UMEM_MIGHT_SLEEP (1 << 0)
+
 struct xdp_umem {
 	struct xsk_queue *fq;
 	struct xsk_queue *cq;
@@ -41,10 +44,12 @@ struct xdp_umem {
 	struct work_struct work;
 	struct page **pgs;
 	u32 npgs;
+	u16 queue_id;
+	u8 need_wakeup;
+	u8 flags;
 	int id;
 	struct net_device *dev;
 	struct xdp_umem_fq_reuse *fq_reuse;
-	u16 queue_id;
 	bool zc;
 	spinlock_t xsk_list_lock;
 	struct list_head xsk_list;
@@ -87,6 +92,11 @@ 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);
 struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev, u16 queue_id);
+void xsk_set_rx_need_wakeup(struct xdp_umem *umem);
+void xsk_set_tx_need_wakeup(struct xdp_umem *umem);
+void xsk_clear_rx_need_wakeup(struct xdp_umem *umem);
+void xsk_clear_tx_need_wakeup(struct xdp_umem *umem);
+bool xsk_umem_uses_might_sleep(struct xdp_umem *umem);
 
 static inline char *xdp_umem_get_data(struct xdp_umem *umem, u64 addr)
 {
@@ -213,6 +223,27 @@ static inline void xsk_umem_fq_reuse(struct xdp_umem *umem, u64 addr)
 {
 }
 
+static inline void xsk_set_rx_need_wakeup(struct xdp_umem *umem)
+{
+}
+
+static inline void xsk_set_tx_need_wakeup(struct xdp_umem *umem)
+{
+}
+
+static inline void xsk_clear_rx_need_wakeup(struct xdp_umem *umem)
+{
+}
+
+static inline void xsk_clear_tx_need_wakeup(struct xdp_umem *umem)
+{
+}
+
+static inline bool xsk_umem_uses_might_sleep(struct xdp_umem *umem)
+{
+	return false;
+}
+
 #endif /* CONFIG_XDP_SOCKETS */
 
 #endif /* _LINUX_XDP_SOCK_H */
diff --git a/include/uapi/linux/if_xdp.h b/include/uapi/linux/if_xdp.h
index caed8b1..3aa958e 100644
--- a/include/uapi/linux/if_xdp.h
+++ b/include/uapi/linux/if_xdp.h
@@ -16,6 +16,15 @@
 #define XDP_SHARED_UMEM	(1 << 0)
 #define XDP_COPY	(1 << 1) /* Force copy-mode */
 #define XDP_ZEROCOPY	(1 << 2) /* Force zero-copy mode */
+/* If this option is set, the driver might go sleep and in that case
+ * the XDP_RING_NEED_WAKEUP flag in the fill and/or Tx rings will be
+ * set. If it is set, the application need to explicitly wake up the
+ * driver with a poll() (Rx and Tx) or sendto() (Tx only). If you are
+ * running the driver and the application on the same core, you should
+ * use this option so that the kernel will yield to the user space
+ * application.
+ */
+#define XDP_USE_NEED_WAKEUP (1 << 3)
 
 struct sockaddr_xdp {
 	__u16 sxdp_family;
@@ -25,10 +34,14 @@ struct sockaddr_xdp {
 	__u32 sxdp_shared_umem_fd;
 };
 
+/* XDP_RING flags */
+#define XDP_RING_NEED_WAKEUP (1 << 0)
+
 struct xdp_ring_offset {
 	__u64 producer;
 	__u64 consumer;
 	__u64 desc;
+	__u64 flags;
 };
 
 struct xdp_mmap_offsets {
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index 2b18223..15d7c53 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -105,12 +105,14 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
 
 	umem->dev = dev;
 	umem->queue_id = queue_id;
+	if (flags & XDP_USE_NEED_WAKEUP)
+		umem->flags |= XDP_UMEM_MIGHT_SLEEP;
+
 	if (force_copy)
 		/* For copy-mode, we are done. */
 		goto out_rtnl_unlock;
 
-	if (!dev->netdev_ops->ndo_bpf ||
-	    !dev->netdev_ops->ndo_xsk_async_xmit) {
+	if (!dev->netdev_ops->ndo_bpf || !dev->netdev_ops->ndo_xsk_wakeup) {
 		err = -EOPNOTSUPP;
 		goto err_unreg_umem;
 	}
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index a14e886..e9b9c98 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -49,6 +49,66 @@ void xsk_umem_discard_addr(struct xdp_umem *umem)
 }
 EXPORT_SYMBOL(xsk_umem_discard_addr);
 
+void xsk_set_rx_need_wakeup(struct xdp_umem *umem)
+{
+	if (umem->need_wakeup & XDP_WAKEUP_RX)
+		return;
+
+	umem->fq->ring->flags |= XDP_RING_NEED_WAKEUP;
+	umem->need_wakeup |= XDP_WAKEUP_RX;
+}
+EXPORT_SYMBOL(xsk_set_rx_need_wakeup);
+
+void xsk_set_tx_need_wakeup(struct xdp_umem *umem)
+{
+	struct xdp_sock *xs;
+
+	if (umem->need_wakeup & XDP_WAKEUP_TX)
+		return;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(xs, &umem->xsk_list, list) {
+		xs->tx->ring->flags |= XDP_RING_NEED_WAKEUP;
+	}
+	rcu_read_unlock();
+
+	umem->need_wakeup |= XDP_WAKEUP_TX;
+}
+EXPORT_SYMBOL(xsk_set_tx_need_wakeup);
+
+void xsk_clear_rx_need_wakeup(struct xdp_umem *umem)
+{
+	if (!(umem->need_wakeup & XDP_WAKEUP_RX))
+		return;
+
+	umem->fq->ring->flags &= ~XDP_RING_NEED_WAKEUP;
+	umem->need_wakeup &= ~XDP_WAKEUP_RX;
+}
+EXPORT_SYMBOL(xsk_clear_rx_need_wakeup);
+
+void xsk_clear_tx_need_wakeup(struct xdp_umem *umem)
+{
+	struct xdp_sock *xs;
+
+	if (!(umem->need_wakeup & XDP_WAKEUP_TX))
+		return;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(xs, &umem->xsk_list, list) {
+		xs->tx->ring->flags &= ~XDP_RING_NEED_WAKEUP;
+	}
+	rcu_read_unlock();
+
+	umem->need_wakeup &= ~XDP_WAKEUP_TX;
+}
+EXPORT_SYMBOL(xsk_clear_tx_need_wakeup);
+
+bool xsk_umem_uses_might_sleep(struct xdp_umem *umem)
+{
+	return umem->flags & XDP_UMEM_MIGHT_SLEEP;
+}
+EXPORT_SYMBOL(xsk_umem_uses_might_sleep);
+
 static int __xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
 {
 	void *to_buf, *from_buf;
@@ -198,7 +258,8 @@ static int xsk_zc_xmit(struct sock *sk)
 	struct xdp_sock *xs = xdp_sk(sk);
 	struct net_device *dev = xs->dev;
 
-	return dev->netdev_ops->ndo_xsk_async_xmit(dev, xs->queue_id);
+	return dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id,
+					       XDP_WAKEUP_TX);
 }
 
 static void xsk_destruct_skb(struct sk_buff *skb)
@@ -308,6 +369,12 @@ static unsigned int xsk_poll(struct file *file, struct socket *sock,
 	unsigned int mask = datagram_poll(file, sock, wait);
 	struct sock *sk = sock->sk;
 	struct xdp_sock *xs = xdp_sk(sk);
+	struct net_device *dev = xs->dev;
+	struct xdp_umem *umem = xs->umem;
+
+	if (umem->need_wakeup)
+		dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id,
+						umem->need_wakeup);
 
 	if (xs->rx && !xskq_empty_desc(xs->rx))
 		mask |= POLLIN | POLLRDNORM;
@@ -408,7 +475,8 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
 		return -EINVAL;
 
 	flags = sxdp->sxdp_flags;
-	if (flags & ~(XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY))
+	if (flags & ~(XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY |
+		      XDP_USE_NEED_WAKEUP))
 		return -EINVAL;
 
 	mutex_lock(&xs->mutex);
@@ -434,7 +502,8 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
 		struct xdp_sock *umem_xs;
 		struct socket *sock;
 
-		if ((flags & XDP_COPY) || (flags & XDP_ZEROCOPY)) {
+		if ((flags & XDP_COPY) || (flags & XDP_ZEROCOPY) ||
+		    (flags & XDP_USE_NEED_WAKEUP)) {
 			/* Cannot specify flags for shared sockets. */
 			err = -EINVAL;
 			goto out_unlock;
@@ -522,6 +591,9 @@ static int xsk_setsockopt(struct socket *sock, int level, int optname,
 		mutex_lock(&xs->mutex);
 		q = (optname == XDP_TX_RING) ? &xs->tx : &xs->rx;
 		err = xsk_init_queue(entries, q, false);
+		if (!err && optname == XDP_TX_RING)
+			/* Tx needs to be explicitly woken up the first time */
+			xs->tx->ring->flags |= XDP_RING_NEED_WAKEUP;
 		mutex_unlock(&xs->mutex);
 		return err;
 	}
@@ -618,9 +690,12 @@ static int xsk_getsockopt(struct socket *sock, int level, int optname,
 	case XDP_MMAP_OFFSETS:
 	{
 		struct xdp_mmap_offsets off;
+		bool flags_supported = true;
 
-		if (len < sizeof(off))
+		if (len < sizeof(off) - sizeof(off.rx.flags))
 			return -EINVAL;
+		else if (len < sizeof(off))
+			flags_supported = false;
 
 		off.rx.producer = offsetof(struct xdp_rxtx_ring, ptrs.producer);
 		off.rx.consumer = offsetof(struct xdp_rxtx_ring, ptrs.consumer);
@@ -635,6 +710,16 @@ static int xsk_getsockopt(struct socket *sock, int level, int optname,
 		off.cr.producer = offsetof(struct xdp_umem_ring, ptrs.producer);
 		off.cr.consumer = offsetof(struct xdp_umem_ring, ptrs.consumer);
 		off.cr.desc	= offsetof(struct xdp_umem_ring, desc);
+		if (flags_supported) {
+			off.rx.flags = offsetof(struct xdp_rxtx_ring,
+						ptrs.flags);
+			off.tx.flags = offsetof(struct xdp_rxtx_ring,
+						ptrs.flags);
+			off.fr.flags = offsetof(struct xdp_umem_ring,
+						ptrs.flags);
+			off.cr.flags = offsetof(struct xdp_umem_ring,
+						ptrs.flags);
+		}
 
 		len = sizeof(off);
 		if (copy_to_user(optval, &off, len))
diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index 88b9ae2..e438eeb 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -16,6 +16,7 @@
 struct xdp_ring {
 	u32 producer ____cacheline_aligned_in_smp;
 	u32 consumer ____cacheline_aligned_in_smp;
+	u32 flags;
 };
 
 /* Used for the RX and TX queues for packets */
-- 
2.7.4


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

* [PATCH bpf-next 3/6] i40e: add support for AF_XDP need_wakup feature
  2019-06-13  7:37 [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings Magnus Karlsson
  2019-06-13  7:37 ` [PATCH bpf-next 1/6] xsk: replace ndo_xsk_async_xmit with ndo_xsk_wakeup Magnus Karlsson
  2019-06-13  7:37 ` [PATCH bpf-next 2/6] xsk: add support for need_wakeup flag in AF_XDP rings Magnus Karlsson
@ 2019-06-13  7:37 ` Magnus Karlsson
  2019-06-13  7:37 ` [PATCH bpf-next 4/6] ixgbe: " Magnus Karlsson
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Magnus Karlsson @ 2019-06-13  7:37 UTC (permalink / raw)
  To: magnus.karlsson, bjorn.topel, ast, daniel, netdev, brouer
  Cc: bpf, bruce.richardson, ciara.loftus, jakub.kicinski, xiaolong.ye,
	qi.z.zhang, maximmi, sridhar.samudrala, kevin.laatz,
	ilias.apalodimas, kiran.patil, axboe, maciej.fijalkowski,
	maciejromanfijalkowski, intel-wired-lan

This patch adds support for the need_wakeup feature of AF_XDP. If the
application has told the kernel that it might sleep using the new bind
flag XDP_USE_NEED_WAKEUP, the driver will then set this flag if it has
no more buffers on the NIC Rx ring and yield to the application. For
Tx, it will set the flag if it has no outstanding Tx completion
interrupts and return to the application.

Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 32be3dd..1863974 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -625,6 +625,15 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 
 	i40e_finalize_xdp_rx(rx_ring, xdp_xmit);
 	i40e_update_rx_stats(rx_ring, total_rx_bytes, total_rx_packets);
+
+	if (xsk_umem_uses_might_sleep(rx_ring->xsk_umem)) {
+		if (failure || rx_ring->next_to_clean == rx_ring->next_to_use)
+			xsk_set_rx_need_wakeup(rx_ring->xsk_umem);
+		else
+			xsk_clear_rx_need_wakeup(rx_ring->xsk_umem);
+
+		return (int)total_rx_packets;
+	}
 	return failure ? budget : (int)total_rx_packets;
 }
 
@@ -758,6 +767,13 @@ bool i40e_clean_xdp_tx_irq(struct i40e_vsi *vsi,
 out_xmit:
 	xmit_done = i40e_xmit_zc(tx_ring, budget);
 
+	if (xsk_umem_uses_might_sleep(tx_ring->xsk_umem)) {
+		if (tx_ring->next_to_clean == tx_ring->next_to_use)
+			xsk_set_tx_need_wakeup(tx_ring->xsk_umem);
+		else
+			xsk_clear_tx_need_wakeup(tx_ring->xsk_umem);
+	}
+
 	return work_done && xmit_done;
 }
 
-- 
2.7.4


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

* [PATCH bpf-next 4/6] ixgbe: add support for AF_XDP need_wakup feature
  2019-06-13  7:37 [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings Magnus Karlsson
                   ` (2 preceding siblings ...)
  2019-06-13  7:37 ` [PATCH bpf-next 3/6] i40e: add support for AF_XDP need_wakup feature Magnus Karlsson
@ 2019-06-13  7:37 ` Magnus Karlsson
  2019-06-13  7:37 ` [PATCH bpf-next 5/6] libbpf: add support for need_wakeup flag in AF_XDP part Magnus Karlsson
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Magnus Karlsson @ 2019-06-13  7:37 UTC (permalink / raw)
  To: magnus.karlsson, bjorn.topel, ast, daniel, netdev, brouer
  Cc: bpf, bruce.richardson, ciara.loftus, jakub.kicinski, xiaolong.ye,
	qi.z.zhang, maximmi, sridhar.samudrala, kevin.laatz,
	ilias.apalodimas, kiran.patil, axboe, maciej.fijalkowski,
	maciejromanfijalkowski, intel-wired-lan

This patch adds support for the need_wakeup feature of AF_XDP. If the
application has told the kernel that it might sleep using the new bind
flag XDP_USE_NEED_WAKEUP, the driver will then set this flag if it has
no more buffers on the NIC Rx ring and yield to the application. For
Tx, it will set the flag if it has no outstanding Tx completion
interrupts and return to the application.

Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
index 1a1add8..a4bb50d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c
@@ -597,6 +597,14 @@ int ixgbe_clean_rx_irq_zc(struct ixgbe_q_vector *q_vector,
 	q_vector->rx.total_packets += total_rx_packets;
 	q_vector->rx.total_bytes += total_rx_bytes;
 
+	if (xsk_umem_uses_might_sleep(rx_ring->xsk_umem)) {
+		if (failure || rx_ring->next_to_clean == rx_ring->next_to_use)
+			xsk_set_rx_need_wakeup(rx_ring->xsk_umem);
+		else
+			xsk_clear_rx_need_wakeup(rx_ring->xsk_umem);
+
+		return (int)total_rx_packets;
+	}
 	return failure ? budget : (int)total_rx_packets;
 }
 
@@ -736,6 +744,14 @@ bool ixgbe_clean_xdp_tx_irq(struct ixgbe_q_vector *q_vector,
 		xsk_umem_complete_tx(umem, xsk_frames);
 
 	xmit_done = ixgbe_xmit_zc(tx_ring, q_vector->tx.work_limit);
+
+	if (xsk_umem_uses_might_sleep(tx_ring->xsk_umem)) {
+		if (tx_ring->next_to_clean == tx_ring->next_to_use)
+			xsk_set_tx_need_wakeup(tx_ring->xsk_umem);
+		else
+			xsk_clear_tx_need_wakeup(tx_ring->xsk_umem);
+	}
+
 	return budget > 0 && xmit_done;
 }
 
-- 
2.7.4


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

* [PATCH bpf-next 5/6] libbpf: add support for need_wakeup flag in AF_XDP part
  2019-06-13  7:37 [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings Magnus Karlsson
                   ` (3 preceding siblings ...)
  2019-06-13  7:37 ` [PATCH bpf-next 4/6] ixgbe: " Magnus Karlsson
@ 2019-06-13  7:37 ` Magnus Karlsson
  2019-06-13  7:37 ` [PATCH bpf-next 6/6] samples/bpf: add use of need_sleep flag in xdpsock Magnus Karlsson
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Magnus Karlsson @ 2019-06-13  7:37 UTC (permalink / raw)
  To: magnus.karlsson, bjorn.topel, ast, daniel, netdev, brouer
  Cc: bpf, bruce.richardson, ciara.loftus, jakub.kicinski, xiaolong.ye,
	qi.z.zhang, maximmi, sridhar.samudrala, kevin.laatz,
	ilias.apalodimas, kiran.patil, axboe, maciej.fijalkowski,
	maciejromanfijalkowski, intel-wired-lan

This commit adds support for the new need_wakeup flag in AF_XDP. The
xsk_socket__create function is updated to handle this and a new
function is introduced called xsk_ring_prod__needs_wakeup(). This
function can be used by the application to check if Rx and/or Tx
processing needs to be explicitly woken up.

Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 tools/include/uapi/linux/if_xdp.h | 13 +++++++++++++
 tools/lib/bpf/xsk.c               |  4 ++++
 tools/lib/bpf/xsk.h               |  6 ++++++
 3 files changed, 23 insertions(+)

diff --git a/tools/include/uapi/linux/if_xdp.h b/tools/include/uapi/linux/if_xdp.h
index caed8b1..3aa958e 100644
--- a/tools/include/uapi/linux/if_xdp.h
+++ b/tools/include/uapi/linux/if_xdp.h
@@ -16,6 +16,15 @@
 #define XDP_SHARED_UMEM	(1 << 0)
 #define XDP_COPY	(1 << 1) /* Force copy-mode */
 #define XDP_ZEROCOPY	(1 << 2) /* Force zero-copy mode */
+/* If this option is set, the driver might go sleep and in that case
+ * the XDP_RING_NEED_WAKEUP flag in the fill and/or Tx rings will be
+ * set. If it is set, the application need to explicitly wake up the
+ * driver with a poll() (Rx and Tx) or sendto() (Tx only). If you are
+ * running the driver and the application on the same core, you should
+ * use this option so that the kernel will yield to the user space
+ * application.
+ */
+#define XDP_USE_NEED_WAKEUP (1 << 3)
 
 struct sockaddr_xdp {
 	__u16 sxdp_family;
@@ -25,10 +34,14 @@ struct sockaddr_xdp {
 	__u32 sxdp_shared_umem_fd;
 };
 
+/* XDP_RING flags */
+#define XDP_RING_NEED_WAKEUP (1 << 0)
+
 struct xdp_ring_offset {
 	__u64 producer;
 	__u64 consumer;
 	__u64 desc;
+	__u64 flags;
 };
 
 struct xdp_mmap_offsets {
diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index 7ef6293..02d57fa 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -223,6 +223,7 @@ int xsk_umem__create(struct xsk_umem **umem_ptr, void *umem_area, __u64 size,
 	fill->size = umem->config.fill_size;
 	fill->producer = map + off.fr.producer;
 	fill->consumer = map + off.fr.consumer;
+	fill->flags = map + off.fr.flags;
 	fill->ring = map + off.fr.desc;
 	fill->cached_cons = umem->config.fill_size;
 
@@ -240,6 +241,7 @@ int xsk_umem__create(struct xsk_umem **umem_ptr, void *umem_area, __u64 size,
 	comp->size = umem->config.comp_size;
 	comp->producer = map + off.cr.producer;
 	comp->consumer = map + off.cr.consumer;
+	comp->flags = map + off.cr.flags;
 	comp->ring = map + off.cr.desc;
 
 	*umem_ptr = umem;
@@ -561,6 +563,7 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
 		rx->size = xsk->config.rx_size;
 		rx->producer = rx_map + off.rx.producer;
 		rx->consumer = rx_map + off.rx.consumer;
+		rx->flags = rx_map + off.rx.flags;
 		rx->ring = rx_map + off.rx.desc;
 	}
 	xsk->rx = rx;
@@ -580,6 +583,7 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
 		tx->size = xsk->config.tx_size;
 		tx->producer = tx_map + off.tx.producer;
 		tx->consumer = tx_map + off.tx.consumer;
+		tx->flags = tx_map + off.tx.flags;
 		tx->ring = tx_map + off.tx.desc;
 		tx->cached_cons = xsk->config.tx_size;
 	}
diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
index 82ea71a..2500fca 100644
--- a/tools/lib/bpf/xsk.h
+++ b/tools/lib/bpf/xsk.h
@@ -32,6 +32,7 @@ struct name { \
 	__u32 *producer; \
 	__u32 *consumer; \
 	void *ring; \
+	__u32 *flags; \
 }
 
 DEFINE_XSK_RING(xsk_ring_prod);
@@ -76,6 +77,11 @@ xsk_ring_cons__rx_desc(const struct xsk_ring_cons *rx, __u32 idx)
 	return &descs[idx & rx->mask];
 }
 
+static inline int xsk_ring_prod__needs_wakeup(const struct xsk_ring_prod *r)
+{
+	return *r->flags & XDP_RING_NEED_WAKEUP;
+}
+
 static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb)
 {
 	__u32 free_entries = r->cached_cons - r->cached_prod;
-- 
2.7.4


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

* [PATCH bpf-next 6/6] samples/bpf: add use of need_sleep flag in xdpsock
  2019-06-13  7:37 [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings Magnus Karlsson
                   ` (4 preceding siblings ...)
  2019-06-13  7:37 ` [PATCH bpf-next 5/6] libbpf: add support for need_wakeup flag in AF_XDP part Magnus Karlsson
@ 2019-06-13  7:37 ` Magnus Karlsson
  2019-06-13 23:34 ` [Intel-wired-lan] [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings Jeff Kirsher
  2019-06-14 13:38 ` Maxim Mikityanskiy
  7 siblings, 0 replies; 13+ messages in thread
From: Magnus Karlsson @ 2019-06-13  7:37 UTC (permalink / raw)
  To: magnus.karlsson, bjorn.topel, ast, daniel, netdev, brouer
  Cc: bpf, bruce.richardson, ciara.loftus, jakub.kicinski, xiaolong.ye,
	qi.z.zhang, maximmi, sridhar.samudrala, kevin.laatz,
	ilias.apalodimas, kiran.patil, axboe, maciej.fijalkowski,
	maciejromanfijalkowski, intel-wired-lan

This commit adds using the need_sleep flag to the xdpsock sample
application. It is turned on by default as we think it is a feature
that seems to always produce a performance benefit, if the application
has been written taking advantage of it. It can be turned off in the
sample app by using the '-m' command line option.

The txpush and l2fwd sub applications have also been updated to
support poll() with multiple sockets.

Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 samples/bpf/xdpsock_user.c | 191 ++++++++++++++++++++++++++++-----------------
 1 file changed, 119 insertions(+), 72 deletions(-)

diff --git a/samples/bpf/xdpsock_user.c b/samples/bpf/xdpsock_user.c
index d08ee1a..4b760b8 100644
--- a/samples/bpf/xdpsock_user.c
+++ b/samples/bpf/xdpsock_user.c
@@ -67,7 +67,9 @@ static int opt_ifindex;
 static int opt_queue;
 static int opt_poll;
 static int opt_interval = 1;
-static u32 opt_xdp_bind_flags;
+static u32 opt_xdp_bind_flags = XDP_USE_NEED_WAKEUP;
+static int opt_timeout = 1000;
+static bool opt_might_sleep = true;
 static __u32 prog_id;
 
 struct xsk_umem_info {
@@ -346,6 +348,7 @@ static struct option long_options[] = {
 	{"interval", required_argument, 0, 'n'},
 	{"zero-copy", no_argument, 0, 'z'},
 	{"copy", no_argument, 0, 'c'},
+	{"no-might-sleep", no_argument, 0, 'm'},
 	{0, 0, 0, 0}
 };
 
@@ -365,6 +368,7 @@ static void usage(const char *prog)
 		"  -n, --interval=n	Specify statistics update interval (default 1 sec).\n"
 		"  -z, --zero-copy      Force zero-copy mode.\n"
 		"  -c, --copy           Force copy mode.\n"
+		"  -m, --no-might-sleep Turn off use of driver might sleep flag.\n"
 		"\n";
 	fprintf(stderr, str, prog);
 	exit(EXIT_FAILURE);
@@ -377,7 +381,7 @@ static void parse_command_line(int argc, char **argv)
 	opterr = 0;
 
 	for (;;) {
-		c = getopt_long(argc, argv, "Frtli:q:psSNn:cz", long_options,
+		c = getopt_long(argc, argv, "Frtli:q:psSNn:czm", long_options,
 				&option_index);
 		if (c == -1)
 			break;
@@ -420,6 +424,10 @@ static void parse_command_line(int argc, char **argv)
 		case 'F':
 			opt_xdp_flags &= ~XDP_FLAGS_UPDATE_IF_NOEXIST;
 			break;
+		case 'm':
+			opt_might_sleep = false;
+			opt_xdp_bind_flags &= ~XDP_USE_NEED_WAKEUP;
+			break;
 		default:
 			usage(basename(argv[0]));
 		}
@@ -444,7 +452,8 @@ static void kick_tx(struct xsk_socket_info *xsk)
 	exit_with_error(errno);
 }
 
-static inline void complete_tx_l2fwd(struct xsk_socket_info *xsk)
+static inline void complete_tx_l2fwd(struct xsk_socket_info *xsk,
+				     struct pollfd *fds)
 {
 	u32 idx_cq = 0, idx_fq = 0;
 	unsigned int rcvd;
@@ -453,7 +462,9 @@ static inline void complete_tx_l2fwd(struct xsk_socket_info *xsk)
 	if (!xsk->outstanding_tx)
 		return;
 
-	kick_tx(xsk);
+	if (!opt_might_sleep || xsk_ring_prod__needs_wakeup(&xsk->tx))
+		kick_tx(xsk);
+
 	ndescs = (xsk->outstanding_tx > BATCH_SIZE) ? BATCH_SIZE :
 		xsk->outstanding_tx;
 
@@ -467,6 +478,8 @@ static inline void complete_tx_l2fwd(struct xsk_socket_info *xsk)
 		while (ret != rcvd) {
 			if (ret < 0)
 				exit_with_error(-ret);
+			if (xsk_ring_prod__needs_wakeup(&xsk->umem->fq))
+				ret = poll(fds, num_socks, opt_timeout);
 			ret = xsk_ring_prod__reserve(&xsk->umem->fq, rcvd,
 						     &idx_fq);
 		}
@@ -490,7 +503,8 @@ static inline void complete_tx_only(struct xsk_socket_info *xsk)
 	if (!xsk->outstanding_tx)
 		return;
 
-	kick_tx(xsk);
+	if (!opt_might_sleep || xsk_ring_prod__needs_wakeup(&xsk->tx))
+		kick_tx(xsk);
 
 	rcvd = xsk_ring_cons__peek(&xsk->umem->cq, BATCH_SIZE, &idx);
 	if (rcvd > 0) {
@@ -500,20 +514,25 @@ static inline void complete_tx_only(struct xsk_socket_info *xsk)
 	}
 }
 
-static void rx_drop(struct xsk_socket_info *xsk)
+static void rx_drop(struct xsk_socket_info *xsk, struct pollfd *fds)
 {
 	unsigned int rcvd, i;
 	u32 idx_rx = 0, idx_fq = 0;
 	int ret;
 
 	rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
-	if (!rcvd)
+	if (!rcvd) {
+		if (xsk_ring_prod__needs_wakeup(&xsk->umem->fq))
+			ret = poll(fds, num_socks, opt_timeout);
 		return;
+	}
 
 	ret = xsk_ring_prod__reserve(&xsk->umem->fq, rcvd, &idx_fq);
 	while (ret != rcvd) {
 		if (ret < 0)
 			exit_with_error(-ret);
+		if (xsk_ring_prod__needs_wakeup(&xsk->umem->fq))
+			ret = poll(fds, num_socks, opt_timeout);
 		ret = xsk_ring_prod__reserve(&xsk->umem->fq, rcvd, &idx_fq);
 	}
 
@@ -534,42 +553,65 @@ static void rx_drop(struct xsk_socket_info *xsk)
 static void rx_drop_all(void)
 {
 	struct pollfd fds[MAX_SOCKS + 1];
-	int i, ret, timeout, nfds = 1;
+	int i, ret;
 
 	memset(fds, 0, sizeof(fds));
 
 	for (i = 0; i < num_socks; i++) {
 		fds[i].fd = xsk_socket__fd(xsks[i]->xsk);
 		fds[i].events = POLLIN;
-		timeout = 1000; /* 1sn */
 	}
 
 	for (;;) {
 		if (opt_poll) {
-			ret = poll(fds, nfds, timeout);
+			ret = poll(fds, num_socks, opt_timeout);
 			if (ret <= 0)
 				continue;
 		}
 
 		for (i = 0; i < num_socks; i++)
-			rx_drop(xsks[i]);
+			rx_drop(xsks[i], fds);
+	}
+}
+
+static void tx_only(struct xsk_socket_info *xsk, u32 frame_nb)
+{
+	u32 idx;
+
+	if (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) == BATCH_SIZE) {
+		unsigned int i;
+
+		for (i = 0; i < BATCH_SIZE; i++) {
+			xsk_ring_prod__tx_desc(&xsk->tx, idx + i)->addr	=
+				(frame_nb + i) << XSK_UMEM__DEFAULT_FRAME_SHIFT;
+			xsk_ring_prod__tx_desc(&xsk->tx, idx + i)->len =
+				sizeof(pkt_data) - 1;
+		}
+
+		xsk_ring_prod__submit(&xsk->tx, BATCH_SIZE);
+		xsk->outstanding_tx += BATCH_SIZE;
+		frame_nb += BATCH_SIZE;
+		frame_nb %= NUM_FRAMES;
 	}
+
+	complete_tx_only(xsk);
 }
 
-static void tx_only(struct xsk_socket_info *xsk)
+static void tx_only_all(void)
 {
-	int timeout, ret, nfds = 1;
-	struct pollfd fds[nfds + 1];
-	u32 idx, frame_nb = 0;
+	struct pollfd fds[MAX_SOCKS];
+	u32 frame_nb[MAX_SOCKS] = {};
+	int i, ret;
 
 	memset(fds, 0, sizeof(fds));
-	fds[0].fd = xsk_socket__fd(xsk->xsk);
-	fds[0].events = POLLOUT;
-	timeout = 1000; /* 1sn */
+	for (i = 0; i < num_socks; i++) {
+		fds[0].fd = xsk_socket__fd(xsks[i]->xsk);
+		fds[0].events = POLLOUT;
+	}
 
 	for (;;) {
 		if (opt_poll) {
-			ret = poll(fds, nfds, timeout);
+			ret = poll(fds, num_socks, opt_timeout);
 			if (ret <= 0)
 				continue;
 
@@ -577,70 +619,75 @@ static void tx_only(struct xsk_socket_info *xsk)
 				continue;
 		}
 
-		if (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) ==
-		    BATCH_SIZE) {
-			unsigned int i;
-
-			for (i = 0; i < BATCH_SIZE; i++) {
-				xsk_ring_prod__tx_desc(&xsk->tx, idx + i)->addr
-					= (frame_nb + i) <<
-					XSK_UMEM__DEFAULT_FRAME_SHIFT;
-				xsk_ring_prod__tx_desc(&xsk->tx, idx + i)->len =
-					sizeof(pkt_data) - 1;
-			}
-
-			xsk_ring_prod__submit(&xsk->tx, BATCH_SIZE);
-			xsk->outstanding_tx += BATCH_SIZE;
-			frame_nb += BATCH_SIZE;
-			frame_nb %= NUM_FRAMES;
-		}
-
-		complete_tx_only(xsk);
+		for (i = 0; i < num_socks; i++)
+			tx_only(xsks[i], frame_nb[i]);
 	}
 }
 
-static void l2fwd(struct xsk_socket_info *xsk)
+static void l2fwd(struct xsk_socket_info *xsk, struct pollfd *fds)
 {
-	for (;;) {
-		unsigned int rcvd, i;
-		u32 idx_rx = 0, idx_tx = 0;
-		int ret;
+	unsigned int rcvd, i;
+	u32 idx_rx = 0, idx_tx = 0;
+	int ret;
 
-		for (;;) {
-			complete_tx_l2fwd(xsk);
+	complete_tx_l2fwd(xsk, fds);
 
-			rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE,
-						   &idx_rx);
-			if (rcvd > 0)
-				break;
-		}
+	rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
+	if (!rcvd) {
+		if (xsk_ring_prod__needs_wakeup(&xsk->umem->fq))
+			ret = poll(fds, num_socks, opt_timeout);
+		return;
+	}
 
+	ret = xsk_ring_prod__reserve(&xsk->tx, rcvd, &idx_tx);
+	while (ret != rcvd) {
+		if (ret < 0)
+			exit_with_error(-ret);
+		if (xsk_ring_prod__needs_wakeup(&xsk->tx))
+			kick_tx(xsk);
 		ret = xsk_ring_prod__reserve(&xsk->tx, rcvd, &idx_tx);
-		while (ret != rcvd) {
-			if (ret < 0)
-				exit_with_error(-ret);
-			ret = xsk_ring_prod__reserve(&xsk->tx, rcvd, &idx_tx);
-		}
+	}
+
+	for (i = 0; i < rcvd; i++) {
+		u64 addr = xsk_ring_cons__rx_desc(&xsk->rx, idx_rx)->addr;
+		u32 len = xsk_ring_cons__rx_desc(&xsk->rx, idx_rx++)->len;
+		char *pkt = xsk_umem__get_data(xsk->umem->buffer, addr);
+
+		swap_mac_addresses(pkt);
 
-		for (i = 0; i < rcvd; i++) {
-			u64 addr = xsk_ring_cons__rx_desc(&xsk->rx,
-							  idx_rx)->addr;
-			u32 len = xsk_ring_cons__rx_desc(&xsk->rx,
-							 idx_rx++)->len;
-			char *pkt = xsk_umem__get_data(xsk->umem->buffer, addr);
+		hex_dump(pkt, len, addr);
+		xsk_ring_prod__tx_desc(&xsk->tx, idx_tx)->addr = addr;
+		xsk_ring_prod__tx_desc(&xsk->tx, idx_tx++)->len = len;
+	}
 
-			swap_mac_addresses(pkt);
+	xsk_ring_prod__submit(&xsk->tx, rcvd);
+	xsk_ring_cons__release(&xsk->rx, rcvd);
 
-			hex_dump(pkt, len, addr);
-			xsk_ring_prod__tx_desc(&xsk->tx, idx_tx)->addr = addr;
-			xsk_ring_prod__tx_desc(&xsk->tx, idx_tx++)->len = len;
-		}
+	xsk->rx_npkts += rcvd;
+	xsk->outstanding_tx += rcvd;
+}
+
+static void l2fwd_all(void)
+{
+	struct pollfd fds[MAX_SOCKS];
+	int i, ret;
+
+	memset(fds, 0, sizeof(fds));
+
+	for (i = 0; i < num_socks; i++) {
+		fds[i].fd = xsk_socket__fd(xsks[i]->xsk);
+		fds[i].events = POLLOUT | POLLIN;
+	}
 
-		xsk_ring_prod__submit(&xsk->tx, rcvd);
-		xsk_ring_cons__release(&xsk->rx, rcvd);
+	for (;;) {
+		if (opt_poll) {
+			ret = poll(fds, num_socks, opt_timeout);
+			if (ret <= 0)
+				continue;
+		}
 
-		xsk->rx_npkts += rcvd;
-		xsk->outstanding_tx += rcvd;
+		for (i = 0; i < num_socks; i++)
+			l2fwd(xsks[i], fds);
 	}
 }
 
@@ -693,9 +740,9 @@ int main(int argc, char **argv)
 	if (opt_bench == BENCH_RXDROP)
 		rx_drop_all();
 	else if (opt_bench == BENCH_TXONLY)
-		tx_only(xsks[0]);
+		tx_only_all();
 	else
-		l2fwd(xsks[0]);
+		l2fwd_all();
 
 	return 0;
 }
-- 
2.7.4


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

* Re: [PATCH bpf-next 2/6] xsk: add support for need_wakeup flag in AF_XDP rings
  2019-06-13  7:37 ` [PATCH bpf-next 2/6] xsk: add support for need_wakeup flag in AF_XDP rings Magnus Karlsson
@ 2019-06-13 19:04   ` Jakub Kicinski
  2019-06-13 19:07     ` Magnus Karlsson
  0 siblings, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2019-06-13 19:04 UTC (permalink / raw)
  To: Magnus Karlsson
  Cc: bjorn.topel, ast, daniel, netdev, brouer, bpf, bruce.richardson,
	ciara.loftus, xiaolong.ye, qi.z.zhang, maximmi,
	sridhar.samudrala, kevin.laatz, ilias.apalodimas, kiran.patil,
	axboe, maciej.fijalkowski, maciejromanfijalkowski,
	intel-wired-lan

On Thu, 13 Jun 2019 09:37:26 +0200, Magnus Karlsson wrote:
>  
> -	if (!dev->netdev_ops->ndo_bpf ||
> -	    !dev->netdev_ops->ndo_xsk_async_xmit) {
> +	if (!dev->netdev_ops->ndo_bpf || !dev->netdev_ops->ndo_xsk_wakeup) {
>  		err = -EOPNOTSUPP;
>  		goto err_unreg_umem;
>  	}

> @@ -198,7 +258,8 @@ static int xsk_zc_xmit(struct sock *sk)
>  	struct xdp_sock *xs = xdp_sk(sk);
>  	struct net_device *dev = xs->dev;
>  
> -	return dev->netdev_ops->ndo_xsk_async_xmit(dev, xs->queue_id);
> +	return dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id,
> +					       XDP_WAKEUP_TX);
>  }
>  
>  static void xsk_destruct_skb(struct sk_buff *skb)

Those two look like they should be in the previous patch?  Won't it
break build?

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

* Re: [PATCH bpf-next 2/6] xsk: add support for need_wakeup flag in AF_XDP rings
  2019-06-13 19:04   ` Jakub Kicinski
@ 2019-06-13 19:07     ` Magnus Karlsson
  0 siblings, 0 replies; 13+ messages in thread
From: Magnus Karlsson @ 2019-06-13 19:07 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Magnus Karlsson, Björn Töpel, Alexei Starovoitov,
	Daniel Borkmann, Network Development, Jesper Dangaard Brouer,
	bpf, bruce.richardson, ciara.loftus, Ye Xiaolong, Zhang, Qi Z,
	Maxim Mikityanskiy, Samudrala, Sridhar, kevin.laatz,
	ilias.apalodimas, Kiran, axboe, Fijalkowski, Maciej,
	Maciej Fijalkowski, intel-wired-lan

On Thu, Jun 13, 2019 at 9:05 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Thu, 13 Jun 2019 09:37:26 +0200, Magnus Karlsson wrote:
> >
> > -     if (!dev->netdev_ops->ndo_bpf ||
> > -         !dev->netdev_ops->ndo_xsk_async_xmit) {
> > +     if (!dev->netdev_ops->ndo_bpf || !dev->netdev_ops->ndo_xsk_wakeup) {
> >               err = -EOPNOTSUPP;
> >               goto err_unreg_umem;
> >       }
>
> > @@ -198,7 +258,8 @@ static int xsk_zc_xmit(struct sock *sk)
> >       struct xdp_sock *xs = xdp_sk(sk);
> >       struct net_device *dev = xs->dev;
> >
> > -     return dev->netdev_ops->ndo_xsk_async_xmit(dev, xs->queue_id);
> > +     return dev->netdev_ops->ndo_xsk_wakeup(dev, xs->queue_id,
> > +                                            XDP_WAKEUP_TX);
> >  }
> >
> >  static void xsk_destruct_skb(struct sk_buff *skb)
>
> Those two look like they should be in the previous patch?  Won't it
> break build?

You are correct. That should have been in patch 1. Will fix that in the v2.

Thanks: Magnus

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

* Re: [Intel-wired-lan] [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings
  2019-06-13  7:37 [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings Magnus Karlsson
                   ` (5 preceding siblings ...)
  2019-06-13  7:37 ` [PATCH bpf-next 6/6] samples/bpf: add use of need_sleep flag in xdpsock Magnus Karlsson
@ 2019-06-13 23:34 ` Jeff Kirsher
  2019-06-14 13:38 ` Maxim Mikityanskiy
  7 siblings, 0 replies; 13+ messages in thread
From: Jeff Kirsher @ 2019-06-13 23:34 UTC (permalink / raw)
  To: Magnus Karlsson, bjorn.topel, ast, daniel, netdev, brouer
  Cc: axboe, maximmi, kevin.laatz, jakub.kicinski,
	maciejromanfijalkowski, bruce.richardson, ciara.loftus,
	ilias.apalodimas, xiaolong.ye, intel-wired-lan, qi.z.zhang,
	maciej.fijalkowski, bpf

[-- Attachment #1: Type: text/plain, Size: 7512 bytes --]

On Thu, 2019-06-13 at 09:37 +0200, Magnus Karlsson wrote:
> This patch set adds support for a new flag called need_wakeup in the
> AF_XDP Tx and fill rings. When this flag is set by the driver, it
> means that the application has to explicitly wake up the kernel Rx
> (for the bit in the fill ring) or kernel Tx (for bit in the Tx ring)
> processing by issuing a syscall. Poll() can wake up both and sendto()
> will wake up Tx processing only.
> 
> The main reason for introducing this new flag is to be able to
> efficiently support the case when application and driver is executing
> on the same core. Previously, the driver was just busy-spinning on
> the
> fill ring if it ran out of buffers in the HW and there were none to
> get from the fill ring. This approach works when the application and
> driver is running on different cores as the application can replenish
> the fill ring while the driver is busy-spinning. Though, this is a
> lousy approach if both of them are running on the same core as the
> probability of the fill ring getting more entries when the driver is
> busy-spinning is zero. With this new feature the driver now sets the
> need_wakeup flag and returns to the application. The application can
> then replenish the fill queue and then explicitly wake up the Rx
> processing in the kernel using the syscall poll(). For Tx, the flag
> is
> only set to one if the driver has no outstanding Tx completion
> interrupts. If it has some, the flag is zero as it will be woken up
> by
> a completion interrupt anyway. This flag can also be used in other
> situations where the driver needs to be woken up explicitly.
> 
> As a nice side effect, this new flag also improves the Tx performance
> of the case where application and driver are running on two different
> cores as it reduces the number of syscalls to the kernel. The kernel
> tells user space if it needs to be woken up by a syscall, and this
> eliminates many of the syscalls. The Rx performance of the 2-core
> case
> is on the other hand slightly worse, since there is a need to use a
> syscall now to wake up the driver, instead of the driver
> busy-spinning. It does waste less CPU cycles though, which might lead
> to better overall system performance.
> 
> This new flag needs some simple driver support. If the driver does
> not
> support it, the Rx flag is always zero and the Tx flag is always
> one. This makes any application relying on this feature default to
> the
> old behavior of not requiring any syscalls in the Rx path and always
> having to call sendto() in the Tx path.
> 
> For backwards compatibility reasons, this feature has to be
> explicitly
> turned on using a new bind flag (XDP_USE_NEED_WAKEUP). I recommend
> that you always turn it on as it has a large positive performance
> impact for the one core case and does not degrade 2 core performance
> and actually improves it for Tx heavy workloads.
> 
> Here are some performance numbers measured on my local,
> non-performance optimized development system. That is why you are
> seeing numbers lower than the ones from Björn and Jesper. 64 byte
> packets at 40Gbit/s line rate. All results in Mpps. Cores == 1 means
> that both application and driver is executing on the same core. Cores
> == 2 that they are on different cores.
> 
>                               Applications
> need_wakeup  cores    txpush    rxdrop      l2fwd
> ---------------------------------------------------------------
>      n         1       0.07      0.06        0.03
>      y         1       21.6      8.2         6.5
>      n         2       32.3      11.7        8.7
>      y         2       33.1      11.7        8.7
> 
> Overall, the need_wakeup flag provides the same or better performance
> in all the micro-benchmarks. The reduction of sendto() calls in
> txpush
> is large. Only a few per second is needed. For l2fwd, the drop is 50%
> for the 1 core case and more than 99.9% for the 2 core case. Do not
> know why I am not seeing the same drop for the 1 core case yet.
> 
> The name and inspiration of the flag has been taken from io_uring by
> Jens Axboe. Details about this feature in io_uring can be found in
> http://kernel.dk/io_uring.pdf, section 8.3. It also addresses most of
> the denial of service and sendto() concerns raised by Maxim
> Mikityanskiy in https://www.spinics.net/lists/netdev/msg554657.html.
> 
> The typical Tx part of an application will have to change from:
> 
> ret = sendto(fd,....)
> 
> to:
> 
> if (xsk_ring_prod__needs_wakeup(&xsk->tx))
>        ret = sendto(fd,....)
> 
> and th Rx part from:
> 
> rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
> if (!rcvd)
>        return;
> 
> to:
> 
> rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
> if (!rcvd) {
>        if (xsk_ring_prod__needs_wakeup(&xsk->umem->fq))
>               ret = poll(fd,.....);
>        return;
> }
> 
> This patch has been applied against commit aee450cbe482 ("bpf:
> silence warning messages in core")
> 
> Structure of the patch set:
> 
> Patch 1: Replaces the ndo_xsk_async_xmit with ndo_xsk_wakeup to
>          support waking up both Rx and Tx processing
> Patch 2: Implements the need_wakeup functionality in common code
> Patch 3-4: Add need_wakeup support to the i40e and ixgbe drivers
> Patch 5: Add need_wakeup support to libbpf
> Patch 6: Add need_wakeup support to the xdpsock sample application
> 
> Thanks: Magnus

Since the i40e and ixgbe changes will not apply against my dev-queue
branch (with the current queue of i40e and ixgbe changes), can you
please rebase against my next-queue tree (dev-queue branch) when you
submit v2?  It will make it easier for me to apply and have validation
verify the changes.

> 
> Magnus Karlsson (6):
>   xsk: replace ndo_xsk_async_xmit with ndo_xsk_wakeup
>   xsk: add support for need_wakeup flag in AF_XDP rings
>   i40e: add support for AF_XDP need_wakup feature
>   ixgbe: add support for AF_XDP need_wakup feature
>   libbpf: add support for need_wakeup flag in AF_XDP part
>   samples/bpf: add use of need_sleep flag in xdpsock
> 
>  drivers/net/ethernet/intel/i40e/i40e_main.c        |   5 +-
>  drivers/net/ethernet/intel/i40e/i40e_xsk.c         |  23 ++-
>  drivers/net/ethernet/intel/i40e/i40e_xsk.h         |   2 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c      |   5 +-
>  .../net/ethernet/intel/ixgbe/ixgbe_txrx_common.h   |   2 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c       |  20 ++-
>  include/linux/netdevice.h                          |  18 +-
>  include/net/xdp_sock.h                             |  33 +++-
>  include/uapi/linux/if_xdp.h                        |  13 ++
>  net/xdp/xdp_umem.c                                 |   6 +-
>  net/xdp/xsk.c                                      |  93 +++++++++-
>  net/xdp/xsk_queue.h                                |   1 +
>  samples/bpf/xdpsock_user.c                         | 191
> +++++++++++++--------
>  tools/include/uapi/linux/if_xdp.h                  |  13 ++
>  tools/lib/bpf/xsk.c                                |   4 +
>  tools/lib/bpf/xsk.h                                |   6 +
>  16 files changed, 343 insertions(+), 92 deletions(-)
> 
> --
> 2.7.4
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings
  2019-06-13  7:37 [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings Magnus Karlsson
                   ` (6 preceding siblings ...)
  2019-06-13 23:34 ` [Intel-wired-lan] [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings Jeff Kirsher
@ 2019-06-14 13:38 ` Maxim Mikityanskiy
  2019-06-14 14:17   ` Maciej Fijalkowski
  7 siblings, 1 reply; 13+ messages in thread
From: Maxim Mikityanskiy @ 2019-06-14 13:38 UTC (permalink / raw)
  To: Magnus Karlsson, bjorn.topel, ast, daniel, netdev, brouer
  Cc: bpf, bruce.richardson, ciara.loftus, jakub.kicinski, xiaolong.ye,
	qi.z.zhang, sridhar.samudrala, kevin.laatz, ilias.apalodimas,
	kiran.patil, axboe, maciej.fijalkowski, maciejromanfijalkowski,
	intel-wired-lan

On 2019-06-13 10:37, Magnus Karlsson wrote:
> 
> This patch set adds support for a new flag called need_wakeup in the
> AF_XDP Tx and fill rings. When this flag is set by the driver, it
> means that the application has to explicitly wake up the kernel Rx
> (for the bit in the fill ring) or kernel Tx (for bit in the Tx ring)
> processing by issuing a syscall. Poll() can wake up both and sendto()
> will wake up Tx processing only.

At first sight, sounds useful! (I didn't have time to have a deeper look 
at the series yet.)

I see you are replacing ndo_xsk_async_xmit with another function to 
support your extension, and some driver changes are made. Does it mean 
that every driver must support the new extension? How about making it 
optional? I.e. the kernel can check whether the new NDO is implemented 
or not, and use the new feature with drivers that support it.

Thanks,
Max

> The main reason for introducing this new flag is to be able to
> efficiently support the case when application and driver is executing
> on the same core. Previously, the driver was just busy-spinning on the
> fill ring if it ran out of buffers in the HW and there were none to
> get from the fill ring. This approach works when the application and
> driver is running on different cores as the application can replenish
> the fill ring while the driver is busy-spinning. Though, this is a
> lousy approach if both of them are running on the same core as the
> probability of the fill ring getting more entries when the driver is
> busy-spinning is zero. With this new feature the driver now sets the
> need_wakeup flag and returns to the application. The application can
> then replenish the fill queue and then explicitly wake up the Rx
> processing in the kernel using the syscall poll(). For Tx, the flag is
> only set to one if the driver has no outstanding Tx completion
> interrupts. If it has some, the flag is zero as it will be woken up by
> a completion interrupt anyway. This flag can also be used in other
> situations where the driver needs to be woken up explicitly.
> 
> As a nice side effect, this new flag also improves the Tx performance
> of the case where application and driver are running on two different
> cores as it reduces the number of syscalls to the kernel. The kernel
> tells user space if it needs to be woken up by a syscall, and this
> eliminates many of the syscalls. The Rx performance of the 2-core case
> is on the other hand slightly worse, since there is a need to use a
> syscall now to wake up the driver, instead of the driver
> busy-spinning. It does waste less CPU cycles though, which might lead
> to better overall system performance.
> 
> This new flag needs some simple driver support. If the driver does not
> support it, the Rx flag is always zero and the Tx flag is always
> one. This makes any application relying on this feature default to the
> old behavior of not requiring any syscalls in the Rx path and always
> having to call sendto() in the Tx path.
> 
> For backwards compatibility reasons, this feature has to be explicitly
> turned on using a new bind flag (XDP_USE_NEED_WAKEUP). I recommend
> that you always turn it on as it has a large positive performance
> impact for the one core case and does not degrade 2 core performance
> and actually improves it for Tx heavy workloads.
> 
> Here are some performance numbers measured on my local,
> non-performance optimized development system. That is why you are
> seeing numbers lower than the ones from Björn and Jesper. 64 byte
> packets at 40Gbit/s line rate. All results in Mpps. Cores == 1 means
> that both application and driver is executing on the same core. Cores
> == 2 that they are on different cores.
> 
>                                Applications
> need_wakeup  cores    txpush    rxdrop      l2fwd
> ---------------------------------------------------------------
>       n         1       0.07      0.06        0.03
>       y         1       21.6      8.2         6.5
>       n         2       32.3      11.7        8.7
>       y         2       33.1      11.7        8.7
> 
> Overall, the need_wakeup flag provides the same or better performance
> in all the micro-benchmarks. The reduction of sendto() calls in txpush
> is large. Only a few per second is needed. For l2fwd, the drop is 50%
> for the 1 core case and more than 99.9% for the 2 core case. Do not
> know why I am not seeing the same drop for the 1 core case yet.
> 
> The name and inspiration of the flag has been taken from io_uring by
> Jens Axboe. Details about this feature in io_uring can be found in
> http://kernel.dk/io_uring.pdf, section 8.3. It also addresses most of
> the denial of service and sendto() concerns raised by Maxim
> Mikityanskiy in https://www.spinics.net/lists/netdev/msg554657.html.
> 
> The typical Tx part of an application will have to change from:
> 
> ret = sendto(fd,....)
> 
> to:
> 
> if (xsk_ring_prod__needs_wakeup(&xsk->tx))
>         ret = sendto(fd,....)
> 
> and th Rx part from:
> 
> rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
> if (!rcvd)
>         return;
> 
> to:
> 
> rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
> if (!rcvd) {
>         if (xsk_ring_prod__needs_wakeup(&xsk->umem->fq))
>                ret = poll(fd,.....);
>         return;
> }
> 
> This patch has been applied against commit aee450cbe482 ("bpf: silence warning messages in core")
> 
> Structure of the patch set:
> 
> Patch 1: Replaces the ndo_xsk_async_xmit with ndo_xsk_wakeup to
>           support waking up both Rx and Tx processing
> Patch 2: Implements the need_wakeup functionality in common code
> Patch 3-4: Add need_wakeup support to the i40e and ixgbe drivers
> Patch 5: Add need_wakeup support to libbpf
> Patch 6: Add need_wakeup support to the xdpsock sample application
> 
> Thanks: Magnus
> 
> Magnus Karlsson (6):
>    xsk: replace ndo_xsk_async_xmit with ndo_xsk_wakeup
>    xsk: add support for need_wakeup flag in AF_XDP rings
>    i40e: add support for AF_XDP need_wakup feature
>    ixgbe: add support for AF_XDP need_wakup feature
>    libbpf: add support for need_wakeup flag in AF_XDP part
>    samples/bpf: add use of need_sleep flag in xdpsock
> 
>   drivers/net/ethernet/intel/i40e/i40e_main.c        |   5 +-
>   drivers/net/ethernet/intel/i40e/i40e_xsk.c         |  23 ++-
>   drivers/net/ethernet/intel/i40e/i40e_xsk.h         |   2 +-
>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c      |   5 +-
>   .../net/ethernet/intel/ixgbe/ixgbe_txrx_common.h   |   2 +-
>   drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c       |  20 ++-
>   include/linux/netdevice.h                          |  18 +-
>   include/net/xdp_sock.h                             |  33 +++-
>   include/uapi/linux/if_xdp.h                        |  13 ++
>   net/xdp/xdp_umem.c                                 |   6 +-
>   net/xdp/xsk.c                                      |  93 +++++++++-
>   net/xdp/xsk_queue.h                                |   1 +
>   samples/bpf/xdpsock_user.c                         | 191 +++++++++++++--------
>   tools/include/uapi/linux/if_xdp.h                  |  13 ++
>   tools/lib/bpf/xsk.c                                |   4 +
>   tools/lib/bpf/xsk.h                                |   6 +
>   16 files changed, 343 insertions(+), 92 deletions(-)
> 
> --
> 2.7.4
> 


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

* Re: [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings
  2019-06-14 13:38 ` Maxim Mikityanskiy
@ 2019-06-14 14:17   ` Maciej Fijalkowski
  2019-06-15 19:13     ` Magnus Karlsson
  0 siblings, 1 reply; 13+ messages in thread
From: Maciej Fijalkowski @ 2019-06-14 14:17 UTC (permalink / raw)
  To: Maxim Mikityanskiy
  Cc: Magnus Karlsson, bjorn.topel, ast, daniel, netdev, brouer, bpf,
	bruce.richardson, ciara.loftus, jakub.kicinski, xiaolong.ye,
	qi.z.zhang, sridhar.samudrala, kevin.laatz, ilias.apalodimas,
	kiran.patil, axboe, maciej.fijalkowski, intel-wired-lan

On Fri, 14 Jun 2019 13:38:04 +0000
Maxim Mikityanskiy <maximmi@mellanox.com> wrote:

> On 2019-06-13 10:37, Magnus Karlsson wrote:
> > 
> > This patch set adds support for a new flag called need_wakeup in the
> > AF_XDP Tx and fill rings. When this flag is set by the driver, it
> > means that the application has to explicitly wake up the kernel Rx
> > (for the bit in the fill ring) or kernel Tx (for bit in the Tx ring)
> > processing by issuing a syscall. Poll() can wake up both and sendto()
> > will wake up Tx processing only.  
> 
> At first sight, sounds useful! (I didn't have time to have a deeper look 
> at the series yet.)
> 
> I see you are replacing ndo_xsk_async_xmit with another function to 
> support your extension, and some driver changes are made. Does it mean 
> that every driver must support the new extension? How about making it 
> optional? I.e. the kernel can check whether the new NDO is implemented 
> or not, and use the new feature with drivers that support it.

I think I can speak up for Magnus.
That NDO was just renamed in order to better reflect cases where it is
currently being used, e.g. having ndo_xsk_async_xmit() called in order to get
into NAPI and take the buffers from fill queue was misleading a bit, as you
were waking up the Rx side.

The functionality of that NDO stays the same. Magnus also provided explanations
in commit messages, which I suppose will clarify it more once you go through
the series.

> 
> Thanks,
> Max
> 
> > The main reason for introducing this new flag is to be able to
> > efficiently support the case when application and driver is executing
> > on the same core. Previously, the driver was just busy-spinning on the
> > fill ring if it ran out of buffers in the HW and there were none to
> > get from the fill ring. This approach works when the application and
> > driver is running on different cores as the application can replenish
> > the fill ring while the driver is busy-spinning. Though, this is a
> > lousy approach if both of them are running on the same core as the
> > probability of the fill ring getting more entries when the driver is
> > busy-spinning is zero. With this new feature the driver now sets the
> > need_wakeup flag and returns to the application. The application can
> > then replenish the fill queue and then explicitly wake up the Rx
> > processing in the kernel using the syscall poll(). For Tx, the flag is
> > only set to one if the driver has no outstanding Tx completion
> > interrupts. If it has some, the flag is zero as it will be woken up by
> > a completion interrupt anyway. This flag can also be used in other
> > situations where the driver needs to be woken up explicitly.
> > 
> > As a nice side effect, this new flag also improves the Tx performance
> > of the case where application and driver are running on two different
> > cores as it reduces the number of syscalls to the kernel. The kernel
> > tells user space if it needs to be woken up by a syscall, and this
> > eliminates many of the syscalls. The Rx performance of the 2-core case
> > is on the other hand slightly worse, since there is a need to use a
> > syscall now to wake up the driver, instead of the driver
> > busy-spinning. It does waste less CPU cycles though, which might lead
> > to better overall system performance.
> > 
> > This new flag needs some simple driver support. If the driver does not
> > support it, the Rx flag is always zero and the Tx flag is always
> > one. This makes any application relying on this feature default to the
> > old behavior of not requiring any syscalls in the Rx path and always
> > having to call sendto() in the Tx path.
> > 
> > For backwards compatibility reasons, this feature has to be explicitly
> > turned on using a new bind flag (XDP_USE_NEED_WAKEUP). I recommend
> > that you always turn it on as it has a large positive performance
> > impact for the one core case and does not degrade 2 core performance
> > and actually improves it for Tx heavy workloads.
> > 
> > Here are some performance numbers measured on my local,
> > non-performance optimized development system. That is why you are
> > seeing numbers lower than the ones from Björn and Jesper. 64 byte
> > packets at 40Gbit/s line rate. All results in Mpps. Cores == 1 means
> > that both application and driver is executing on the same core. Cores
> > == 2 that they are on different cores.
> > 
> >                                Applications
> > need_wakeup  cores    txpush    rxdrop      l2fwd
> > ---------------------------------------------------------------
> >       n         1       0.07      0.06        0.03
> >       y         1       21.6      8.2         6.5
> >       n         2       32.3      11.7        8.7
> >       y         2       33.1      11.7        8.7
> > 
> > Overall, the need_wakeup flag provides the same or better performance
> > in all the micro-benchmarks. The reduction of sendto() calls in txpush
> > is large. Only a few per second is needed. For l2fwd, the drop is 50%
> > for the 1 core case and more than 99.9% for the 2 core case. Do not
> > know why I am not seeing the same drop for the 1 core case yet.
> > 
> > The name and inspiration of the flag has been taken from io_uring by
> > Jens Axboe. Details about this feature in io_uring can be found in
> > http://kernel.dk/io_uring.pdf, section 8.3. It also addresses most of
> > the denial of service and sendto() concerns raised by Maxim
> > Mikityanskiy in https://www.spinics.net/lists/netdev/msg554657.html.
> > 
> > The typical Tx part of an application will have to change from:
> > 
> > ret = sendto(fd,....)
> > 
> > to:
> > 
> > if (xsk_ring_prod__needs_wakeup(&xsk->tx))
> >         ret = sendto(fd,....)
> > 
> > and th Rx part from:
> > 
> > rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
> > if (!rcvd)
> >         return;
> > 
> > to:
> > 
> > rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
> > if (!rcvd) {
> >         if (xsk_ring_prod__needs_wakeup(&xsk->umem->fq))
> >                ret = poll(fd,.....);
> >         return;
> > }
> > 
> > This patch has been applied against commit aee450cbe482 ("bpf: silence warning messages in core")
> > 
> > Structure of the patch set:
> > 
> > Patch 1: Replaces the ndo_xsk_async_xmit with ndo_xsk_wakeup to
> >           support waking up both Rx and Tx processing
> > Patch 2: Implements the need_wakeup functionality in common code
> > Patch 3-4: Add need_wakeup support to the i40e and ixgbe drivers
> > Patch 5: Add need_wakeup support to libbpf
> > Patch 6: Add need_wakeup support to the xdpsock sample application
> > 
> > Thanks: Magnus
> > 
> > Magnus Karlsson (6):
> >    xsk: replace ndo_xsk_async_xmit with ndo_xsk_wakeup
> >    xsk: add support for need_wakeup flag in AF_XDP rings
> >    i40e: add support for AF_XDP need_wakup feature
> >    ixgbe: add support for AF_XDP need_wakup feature
> >    libbpf: add support for need_wakeup flag in AF_XDP part
> >    samples/bpf: add use of need_sleep flag in xdpsock
> > 
> >   drivers/net/ethernet/intel/i40e/i40e_main.c        |   5 +-
> >   drivers/net/ethernet/intel/i40e/i40e_xsk.c         |  23 ++-
> >   drivers/net/ethernet/intel/i40e/i40e_xsk.h         |   2 +-
> >   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c      |   5 +-
> >   .../net/ethernet/intel/ixgbe/ixgbe_txrx_common.h   |   2 +-
> >   drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c       |  20 ++-
> >   include/linux/netdevice.h                          |  18 +-
> >   include/net/xdp_sock.h                             |  33 +++-
> >   include/uapi/linux/if_xdp.h                        |  13 ++
> >   net/xdp/xdp_umem.c                                 |   6 +-
> >   net/xdp/xsk.c                                      |  93 +++++++++-
> >   net/xdp/xsk_queue.h                                |   1 +
> >   samples/bpf/xdpsock_user.c                         | 191 +++++++++++++--------
> >   tools/include/uapi/linux/if_xdp.h                  |  13 ++
> >   tools/lib/bpf/xsk.c                                |   4 +
> >   tools/lib/bpf/xsk.h                                |   6 +
> >   16 files changed, 343 insertions(+), 92 deletions(-)
> > 
> > --
> > 2.7.4
> >   
> 


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

* Re: [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings
  2019-06-14 14:17   ` Maciej Fijalkowski
@ 2019-06-15 19:13     ` Magnus Karlsson
  0 siblings, 0 replies; 13+ messages in thread
From: Magnus Karlsson @ 2019-06-15 19:13 UTC (permalink / raw)
  To: Maciej Fijalkowski
  Cc: Maxim Mikityanskiy, Magnus Karlsson, bjorn.topel, ast, daniel,
	netdev, brouer, bpf, bruce.richardson, ciara.loftus,
	jakub.kicinski, xiaolong.ye, qi.z.zhang, sridhar.samudrala,
	kevin.laatz, ilias.apalodimas, kiran.patil, axboe,
	maciej.fijalkowski, intel-wired-lan

On Fri, Jun 14, 2019 at 4:18 PM Maciej Fijalkowski
<maciejromanfijalkowski@gmail.com> wrote:
>
> On Fri, 14 Jun 2019 13:38:04 +0000
> Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> > On 2019-06-13 10:37, Magnus Karlsson wrote:
> > >
> > > This patch set adds support for a new flag called need_wakeup in the
> > > AF_XDP Tx and fill rings. When this flag is set by the driver, it
> > > means that the application has to explicitly wake up the kernel Rx
> > > (for the bit in the fill ring) or kernel Tx (for bit in the Tx ring)
> > > processing by issuing a syscall. Poll() can wake up both and sendto()
> > > will wake up Tx processing only.
> >
> > At first sight, sounds useful! (I didn't have time to have a deeper look
> > at the series yet.)
> >
> > I see you are replacing ndo_xsk_async_xmit with another function to
> > support your extension, and some driver changes are made. Does it mean
> > that every driver must support the new extension? How about making it
> > optional? I.e. the kernel can check whether the new NDO is implemented
> > or not, and use the new feature with drivers that support it.
>
> I think I can speak up for Magnus.
> That NDO was just renamed in order to better reflect cases where it is
> currently being used, e.g. having ndo_xsk_async_xmit() called in order to get
> into NAPI and take the buffers from fill queue was misleading a bit, as you
> were waking up the Rx side.

This is correct. As a side note, the Rx/Tx flag is not used in the
Intel drivers as Rx and Tx is done in the same napi. If you wake up
one, you wake up the other too. I put the flag there as a courtesy for
drivers that have Rx and Tx in different napi contexts. If there are
no such drivers out there, let me know and I will remove it.

The other small change that is done to the drivers in patch 2 is that
one of the new functions xsk_[set|clear]_[rx|tx]_need_wakeup need to
be called at the end of Rx and Tx processing respectively. These just
indicate if the flag (Rx or Tx) should be set or cleared. Basically,
does the driver need to be woken up or not. To your point about
compatibility, if the driver does not support this but the application
does: the Rx flag is default set to 0 and the Tx flag is set to 1. If
the driver does not support changing the flag (i.e., it has no support
for this), the app will not call any poll() for Rx since the flag is 0
and always call sendto() for Tx as that flag is always one. This
defaults to the same behavior as previously. The driver will busy-poll
the fill queue and the application will slam sendto() at regular
intervals as it has no idea if it is needed or not.

With that said, I noticed that I forgot to put the Tx flag
initialization to one in the code, sigh. Will fix that in v2.

These lines in xdp_assign_dev() in xdp_umem.c

if (flags & XDP_USE_NEED_WAKEUP)
                umem->flags |= XDP_UMEM_MIGHT_SLEEP;

should be:

if (flags & XDP_USE_NEED_WAKEUP) {
                umem->flags |= XDP_UMEM_MIGHT_SLEEP;
                /* Tx needs to be explicitly woken up the first time.
                 * Also for supporting drivers that do not implement this
                 * feature. They will always have to call sendto().
                 */
                xsk_set_tx_need_wakeup(umem);
        }

Thanks: Magnus

> The functionality of that NDO stays the same. Magnus also provided explanations
> in commit messages, which I suppose will clarify it more once you go through
> the series.
>
> >
> > Thanks,
> > Max
> >
> > > The main reason for introducing this new flag is to be able to
> > > efficiently support the case when application and driver is executing
> > > on the same core. Previously, the driver was just busy-spinning on the
> > > fill ring if it ran out of buffers in the HW and there were none to
> > > get from the fill ring. This approach works when the application and
> > > driver is running on different cores as the application can replenish
> > > the fill ring while the driver is busy-spinning. Though, this is a
> > > lousy approach if both of them are running on the same core as the
> > > probability of the fill ring getting more entries when the driver is
> > > busy-spinning is zero. With this new feature the driver now sets the
> > > need_wakeup flag and returns to the application. The application can
> > > then replenish the fill queue and then explicitly wake up the Rx
> > > processing in the kernel using the syscall poll(). For Tx, the flag is
> > > only set to one if the driver has no outstanding Tx completion
> > > interrupts. If it has some, the flag is zero as it will be woken up by
> > > a completion interrupt anyway. This flag can also be used in other
> > > situations where the driver needs to be woken up explicitly.
> > >
> > > As a nice side effect, this new flag also improves the Tx performance
> > > of the case where application and driver are running on two different
> > > cores as it reduces the number of syscalls to the kernel. The kernel
> > > tells user space if it needs to be woken up by a syscall, and this
> > > eliminates many of the syscalls. The Rx performance of the 2-core case
> > > is on the other hand slightly worse, since there is a need to use a
> > > syscall now to wake up the driver, instead of the driver
> > > busy-spinning. It does waste less CPU cycles though, which might lead
> > > to better overall system performance.
> > >
> > > This new flag needs some simple driver support. If the driver does not
> > > support it, the Rx flag is always zero and the Tx flag is always
> > > one. This makes any application relying on this feature default to the
> > > old behavior of not requiring any syscalls in the Rx path and always
> > > having to call sendto() in the Tx path.
> > >
> > > For backwards compatibility reasons, this feature has to be explicitly
> > > turned on using a new bind flag (XDP_USE_NEED_WAKEUP). I recommend
> > > that you always turn it on as it has a large positive performance
> > > impact for the one core case and does not degrade 2 core performance
> > > and actually improves it for Tx heavy workloads.
> > >
> > > Here are some performance numbers measured on my local,
> > > non-performance optimized development system. That is why you are
> > > seeing numbers lower than the ones from Björn and Jesper. 64 byte
> > > packets at 40Gbit/s line rate. All results in Mpps. Cores == 1 means
> > > that both application and driver is executing on the same core. Cores
> > > == 2 that they are on different cores.
> > >
> > >                                Applications
> > > need_wakeup  cores    txpush    rxdrop      l2fwd
> > > ---------------------------------------------------------------
> > >       n         1       0.07      0.06        0.03
> > >       y         1       21.6      8.2         6.5
> > >       n         2       32.3      11.7        8.7
> > >       y         2       33.1      11.7        8.7
> > >
> > > Overall, the need_wakeup flag provides the same or better performance
> > > in all the micro-benchmarks. The reduction of sendto() calls in txpush
> > > is large. Only a few per second is needed. For l2fwd, the drop is 50%
> > > for the 1 core case and more than 99.9% for the 2 core case. Do not
> > > know why I am not seeing the same drop for the 1 core case yet.
> > >
> > > The name and inspiration of the flag has been taken from io_uring by
> > > Jens Axboe. Details about this feature in io_uring can be found in
> > > http://kernel.dk/io_uring.pdf, section 8.3. It also addresses most of
> > > the denial of service and sendto() concerns raised by Maxim
> > > Mikityanskiy in https://www.spinics.net/lists/netdev/msg554657.html.
> > >
> > > The typical Tx part of an application will have to change from:
> > >
> > > ret = sendto(fd,....)
> > >
> > > to:
> > >
> > > if (xsk_ring_prod__needs_wakeup(&xsk->tx))
> > >         ret = sendto(fd,....)
> > >
> > > and th Rx part from:
> > >
> > > rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
> > > if (!rcvd)
> > >         return;
> > >
> > > to:
> > >
> > > rcvd = xsk_ring_cons__peek(&xsk->rx, BATCH_SIZE, &idx_rx);
> > > if (!rcvd) {
> > >         if (xsk_ring_prod__needs_wakeup(&xsk->umem->fq))
> > >                ret = poll(fd,.....);
> > >         return;
> > > }
> > >
> > > This patch has been applied against commit aee450cbe482 ("bpf: silence warning messages in core")
> > >
> > > Structure of the patch set:
> > >
> > > Patch 1: Replaces the ndo_xsk_async_xmit with ndo_xsk_wakeup to
> > >           support waking up both Rx and Tx processing
> > > Patch 2: Implements the need_wakeup functionality in common code
> > > Patch 3-4: Add need_wakeup support to the i40e and ixgbe drivers
> > > Patch 5: Add need_wakeup support to libbpf
> > > Patch 6: Add need_wakeup support to the xdpsock sample application
> > >
> > > Thanks: Magnus
> > >
> > > Magnus Karlsson (6):
> > >    xsk: replace ndo_xsk_async_xmit with ndo_xsk_wakeup
> > >    xsk: add support for need_wakeup flag in AF_XDP rings
> > >    i40e: add support for AF_XDP need_wakup feature
> > >    ixgbe: add support for AF_XDP need_wakup feature
> > >    libbpf: add support for need_wakeup flag in AF_XDP part
> > >    samples/bpf: add use of need_sleep flag in xdpsock
> > >
> > >   drivers/net/ethernet/intel/i40e/i40e_main.c        |   5 +-
> > >   drivers/net/ethernet/intel/i40e/i40e_xsk.c         |  23 ++-
> > >   drivers/net/ethernet/intel/i40e/i40e_xsk.h         |   2 +-
> > >   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c      |   5 +-
> > >   .../net/ethernet/intel/ixgbe/ixgbe_txrx_common.h   |   2 +-
> > >   drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c       |  20 ++-
> > >   include/linux/netdevice.h                          |  18 +-
> > >   include/net/xdp_sock.h                             |  33 +++-
> > >   include/uapi/linux/if_xdp.h                        |  13 ++
> > >   net/xdp/xdp_umem.c                                 |   6 +-
> > >   net/xdp/xsk.c                                      |  93 +++++++++-
> > >   net/xdp/xsk_queue.h                                |   1 +
> > >   samples/bpf/xdpsock_user.c                         | 191 +++++++++++++--------
> > >   tools/include/uapi/linux/if_xdp.h                  |  13 ++
> > >   tools/lib/bpf/xsk.c                                |   4 +
> > >   tools/lib/bpf/xsk.h                                |   6 +
> > >   16 files changed, 343 insertions(+), 92 deletions(-)
> > >
> > > --
> > > 2.7.4
> > >
> >
>

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

end of thread, other threads:[~2019-06-15 19:13 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-13  7:37 [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings Magnus Karlsson
2019-06-13  7:37 ` [PATCH bpf-next 1/6] xsk: replace ndo_xsk_async_xmit with ndo_xsk_wakeup Magnus Karlsson
2019-06-13  7:37 ` [PATCH bpf-next 2/6] xsk: add support for need_wakeup flag in AF_XDP rings Magnus Karlsson
2019-06-13 19:04   ` Jakub Kicinski
2019-06-13 19:07     ` Magnus Karlsson
2019-06-13  7:37 ` [PATCH bpf-next 3/6] i40e: add support for AF_XDP need_wakup feature Magnus Karlsson
2019-06-13  7:37 ` [PATCH bpf-next 4/6] ixgbe: " Magnus Karlsson
2019-06-13  7:37 ` [PATCH bpf-next 5/6] libbpf: add support for need_wakeup flag in AF_XDP part Magnus Karlsson
2019-06-13  7:37 ` [PATCH bpf-next 6/6] samples/bpf: add use of need_sleep flag in xdpsock Magnus Karlsson
2019-06-13 23:34 ` [Intel-wired-lan] [PATCH bpf-next 0/6] add need_wakeup flag to the AF_XDP rings Jeff Kirsher
2019-06-14 13:38 ` Maxim Mikityanskiy
2019-06-14 14:17   ` Maciej Fijalkowski
2019-06-15 19:13     ` Magnus Karlsson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).