[bpf-next V1 PATCH 0/8] bpf/xdp: add flags argument to ndo_xdp_xmit and flag flush operation

[bpf-next V1 PATCH 0/8] bpf/xdp: add flags argument to ndo_xdp_xmit and flag flush operation

[Jesper Dangaard Brouer]

As I mentioned in merge commit 10f678683e4 ("Merge branch 'xdp_xmit-bulking'")
I plan to change the API for ndo_xdp_xmit once more, by adding a flags
argument, which is done in this patchset.

I know it is late in the cycle (currently at rc7), but it would be
nice to avoid changing NDOs over several kernel releases, as it is
annoying to vendors and distro backporters, but it is not strictly
UAPI so it is allowed (according to Alexei).

The end-goal is getting rid of the ndo_xdp_flush operation, as it will
make it possible for drivers to implement a TXQ synchronization mechanism
that is not necessarily derived from the CPU id (smp_processor_id).

This patchset removes all callers of the ndo_xdp_flush operation, but
it doesn't take the last step of removing it from all drivers.  This
can be done later, or I can update the patchset on request.

Micro-benchmarks only show a very small performance improvement, for
map-redirect around ~2 ns, and for non-map redirect ~7 ns.  I've not
benchmarked this with CONFIG_RETPOLINE, but the performance benefit
should be more visible given we end-up removing an indirect call.

---

Jesper Dangaard Brouer (8):
      xdp: add flags argument to ndo_xdp_xmit API
      i40e: implement flush flag for ndo_xdp_xmit
      ixgbe: implement flush flag for ndo_xdp_xmit
      tun: implement flush flag for ndo_xdp_xmit
      virtio_net: implement flush flag for ndo_xdp_xmit
      xdp: done implementing ndo_xdp_xmit flush flag for all drivers
      bpf/xdp: non-map redirect can avoid calling ndo_xdp_flush
      bpf/xdp: devmap can avoid calling ndo_xdp_flush


 drivers/net/ethernet/intel/i40e/i40e_txrx.c   |    9 ++++++++-
 drivers/net/ethernet/intel/i40e/i40e_txrx.h   |    3 ++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   23 +++++++++++++++++------
 drivers/net/tun.c                             |   25 ++++++++++++++++++-------
 drivers/net/virtio_net.c                      |    9 ++++++++-
 include/linux/netdevice.h                     |    7 ++++---
 include/net/xdp.h                             |    4 ++++
 kernel/bpf/devmap.c                           |   20 +++++++-------------
 net/core/filter.c                             |    3 +--
 9 files changed, 69 insertions(+), 34 deletions(-)

[bpf-next V1 PATCH 1/8] xdp: add flags argument to ndo_xdp_xmit API

[Jesper Dangaard Brouer]

This patch only change the API and reject any use of flags. This is an
intermediate step that allows us to implement the flush flag operation
later, for each individual driver in a separate patch.

The plan is to implement flush operation via XDP_XMIT_FLUSH flag
and then remove XDP_XMIT_FLAGS_NONE when done.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   |    6 +++++-
 drivers/net/ethernet/intel/i40e/i40e_txrx.h   |    3 ++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    5 ++++-
 drivers/net/tun.c                             |    8 ++++++--
 drivers/net/virtio_net.c                      |    5 ++++-
 include/linux/netdevice.h                     |    7 ++++---
 include/net/xdp.h                             |    5 +++++
 kernel/bpf/devmap.c                           |    2 +-
 net/core/filter.c                             |    2 +-
 9 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 9b698c5acd05..c0451d6e0790 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -3670,7 +3670,8 @@ netdev_tx_t i40e_lan_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
  * For error cases, a negative errno code is returned and no-frames
  * are transmitted (caller must handle freeing frames).
  **/
-int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames)
+int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
+		  u32 flags)
 {
 	struct i40e_netdev_priv *np = netdev_priv(dev);
 	unsigned int queue_index = smp_processor_id();
@@ -3684,6 +3685,9 @@ int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames)
 	if (!i40e_enabled_xdp_vsi(vsi) || queue_index >= vsi->num_queue_pairs)
 		return -ENXIO;
 
+	if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
+		return -EINVAL;
+
 	for (i = 0; i < n; i++) {
 		struct xdp_frame *xdpf = frames[i];
 		int err;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
index eb8804b3d7b6..820f76db251b 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
@@ -487,7 +487,8 @@ u32 i40e_get_tx_pending(struct i40e_ring *ring, bool in_sw);
 void i40e_detect_recover_hung(struct i40e_vsi *vsi);
 int __i40e_maybe_stop_tx(struct i40e_ring *tx_ring, int size);
 bool __i40e_chk_linearize(struct sk_buff *skb);
-int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames);
+int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
+		  u32 flags);
 void i40e_xdp_flush(struct net_device *dev);
 
 /**
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 031d65c4178d..87f088f4af52 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -10023,7 +10023,7 @@ static int ixgbe_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 }
 
 static int ixgbe_xdp_xmit(struct net_device *dev, int n,
-			  struct xdp_frame **frames)
+			  struct xdp_frame **frames, u32 flags)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(dev);
 	struct ixgbe_ring *ring;
@@ -10033,6 +10033,9 @@ static int ixgbe_xdp_xmit(struct net_device *dev, int n,
 	if (unlikely(test_bit(__IXGBE_DOWN, &adapter->state)))
 		return -ENETDOWN;
 
+	if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
+		return -EINVAL;
+
 	/* During program transitions its possible adapter->xdp_prog is assigned
 	 * but ring has not been configured yet. In this case simply abort xmit.
 	 */
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 2265d2ccea47..b182b8cdd219 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1285,7 +1285,8 @@ static const struct net_device_ops tun_netdev_ops = {
 	.ndo_get_stats64	= tun_net_get_stats64,
 };
 
-static int tun_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames)
+static int tun_xdp_xmit(struct net_device *dev, int n,
+			struct xdp_frame **frames, u32 flags)
 {
 	struct tun_struct *tun = netdev_priv(dev);
 	struct tun_file *tfile;
@@ -1294,6 +1295,9 @@ static int tun_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames
 	int cnt = n;
 	int i;
 
+	if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
+		return -EINVAL;
+
 	rcu_read_lock();
 
 	numqueues = READ_ONCE(tun->numqueues);
@@ -1332,7 +1336,7 @@ static int tun_xdp_tx(struct net_device *dev, struct xdp_buff *xdp)
 	if (unlikely(!frame))
 		return -EOVERFLOW;
 
-	return tun_xdp_xmit(dev, 1, &frame);
+	return tun_xdp_xmit(dev, 1, &frame, 0);
 }
 
 static void tun_xdp_flush(struct net_device *dev)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b2647dd5d302..4ed823625953 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -468,7 +468,7 @@ static int __virtnet_xdp_tx_xmit(struct virtnet_info *vi,
 }
 
 static int virtnet_xdp_xmit(struct net_device *dev,
-			    int n, struct xdp_frame **frames)
+			    int n, struct xdp_frame **frames, u32 flags)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct receive_queue *rq = vi->rq;
@@ -481,6 +481,9 @@ static int virtnet_xdp_xmit(struct net_device *dev,
 	int err;
 	int i;
 
+	if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
+		return -EINVAL;
+
 	qp = vi->curr_queue_pairs - vi->xdp_queue_pairs + smp_processor_id();
 	sq = &vi->sq[qp];
 
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8452f72087ef..7f17785a59d7 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1185,13 +1185,13 @@ struct dev_ifalias {
  *	This function is used to set or query state related to XDP on the
  *	netdevice and manage BPF offload. See definition of
  *	enum bpf_netdev_command for details.
- * int (*ndo_xdp_xmit)(struct net_device *dev, int n, struct xdp_frame **xdp);
+ * int (*ndo_xdp_xmit)(struct net_device *dev, int n, struct xdp_frame **xdp,
+ *			u32 flags);
  *	This function is used to submit @n XDP packets for transmit on a
  *	netdevice. Returns number of frames successfully transmitted, frames
  *	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.
- *	TODO: Consider add flag to allow sending flush operation.
  * void (*ndo_xdp_flush)(struct net_device *dev);
  *	This function is used to inform the driver to flush a particular
  *	xdp tx queue. Must be called on same CPU as xdp_xmit.
@@ -1380,7 +1380,8 @@ struct net_device_ops {
 	int			(*ndo_bpf)(struct net_device *dev,
 					   struct netdev_bpf *bpf);
 	int			(*ndo_xdp_xmit)(struct net_device *dev, int n,
-						struct xdp_frame **xdp);
+						struct xdp_frame **xdp,
+						u32 flags);
 	void			(*ndo_xdp_flush)(struct net_device *dev);
 };
 
diff --git a/include/net/xdp.h b/include/net/xdp.h
index 7ad779237ae8..308a4b30b484 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -40,6 +40,11 @@ enum xdp_mem_type {
 	MEM_TYPE_MAX,
 };
 
+/* XDP flags for ndo_xdp_xmit */
+#define XDP_XMIT_FLAGS_NONE	0U
+#define XDP_XMIT_FLUSH		(1U << 0)
+#define XDP_XMIT_FLAGS_MASK	XDP_XMIT_FLUSH
+
 struct xdp_mem_info {
 	u32 type; /* enum xdp_mem_type, but known size type */
 	u32 id;
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index ae16d0c373ef..04fbd75a5274 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -232,7 +232,7 @@ static int bq_xmit_all(struct bpf_dtab_netdev *obj,
 		prefetch(xdpf);
 	}
 
-	sent = dev->netdev_ops->ndo_xdp_xmit(dev, bq->count, bq->q);
+	sent = dev->netdev_ops->ndo_xdp_xmit(dev, bq->count, bq->q, 0);
 	if (sent < 0) {
 		err = sent;
 		sent = 0;
diff --git a/net/core/filter.c b/net/core/filter.c
index 81bd2e9fe8fc..6a21dbcad350 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3056,7 +3056,7 @@ static int __bpf_tx_xdp(struct net_device *dev,
 	if (unlikely(!xdpf))
 		return -EOVERFLOW;
 
-	sent = dev->netdev_ops->ndo_xdp_xmit(dev, 1, &xdpf);
+	sent = dev->netdev_ops->ndo_xdp_xmit(dev, 1, &xdpf, 0);
 	if (sent <= 0)
 		return sent;
 	dev->netdev_ops->ndo_xdp_flush(dev);

[bpf-next V1 PATCH 2/8] i40e: implement flush flag for ndo_xdp_xmit

[Jesper Dangaard Brouer]

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index c0451d6e0790..03c1446f0465 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -3685,7 +3685,7 @@ int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
 	if (!i40e_enabled_xdp_vsi(vsi) || queue_index >= vsi->num_queue_pairs)
 		return -ENXIO;
 
-	if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
+	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
 		return -EINVAL;
 
 	for (i = 0; i < n; i++) {
@@ -3699,6 +3699,9 @@ int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
 		}
 	}
 
+	if (unlikely(flags & XDP_XMIT_FLUSH))
+		i40e_xdp_ring_update_tail(vsi->xdp_rings[queue_index]);
+
 	return n - drops;
 }
 

[bpf-next V1 PATCH 3/8] ixgbe: implement flush flag for ndo_xdp_xmit

[Jesper Dangaard Brouer]

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 87f088f4af52..4fd77c9067f2 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -10022,6 +10022,15 @@ static int ixgbe_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 	}
 }
 
+static void ixgbe_xdp_ring_update_tail(struct ixgbe_ring *ring)
+{
+	/* Force memory writes to complete before letting h/w know there
+	 * are new descriptors to fetch.
+	 */
+	wmb();
+	writel(ring->next_to_use, ring->tail);
+}
+
 static int ixgbe_xdp_xmit(struct net_device *dev, int n,
 			  struct xdp_frame **frames, u32 flags)
 {
@@ -10033,7 +10042,7 @@ static int ixgbe_xdp_xmit(struct net_device *dev, int n,
 	if (unlikely(test_bit(__IXGBE_DOWN, &adapter->state)))
 		return -ENETDOWN;
 
-	if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
+	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
 		return -EINVAL;
 
 	/* During program transitions its possible adapter->xdp_prog is assigned
@@ -10054,6 +10063,9 @@ static int ixgbe_xdp_xmit(struct net_device *dev, int n,
 		}
 	}
 
+	if (unlikely(flags & XDP_XMIT_FLUSH))
+		ixgbe_xdp_ring_update_tail(ring);
+
 	return n - drops;
 }
 
@@ -10072,11 +10084,7 @@ static void ixgbe_xdp_flush(struct net_device *dev)
 	if (unlikely(!ring))
 		return;
 
-	/* Force memory writes to complete before letting h/w know there
-	 * are new descriptors to fetch.
-	 */
-	wmb();
-	writel(ring->next_to_use, ring->tail);
+	ixgbe_xdp_ring_update_tail(ring);
 
 	return;
 }

[bpf-next V1 PATCH 4/8] tun: implement flush flag for ndo_xdp_xmit

[Jesper Dangaard Brouer]

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 drivers/net/tun.c |   19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index b182b8cdd219..d82a05fb0594 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1285,6 +1285,14 @@ static const struct net_device_ops tun_netdev_ops = {
 	.ndo_get_stats64	= tun_net_get_stats64,
 };
 
+static void __tun_xdp_flush_tfile(struct tun_file *tfile)
+{
+	/* Notify and wake up reader process */
+	if (tfile->flags & TUN_FASYNC)
+		kill_fasync(&tfile->fasync, SIGIO, POLL_IN);
+	tfile->socket.sk->sk_data_ready(tfile->socket.sk);
+}
+
 static int tun_xdp_xmit(struct net_device *dev, int n,
 			struct xdp_frame **frames, u32 flags)
 {
@@ -1295,7 +1303,7 @@ static int tun_xdp_xmit(struct net_device *dev, int n,
 	int cnt = n;
 	int i;
 
-	if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
+	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
 		return -EINVAL;
 
 	rcu_read_lock();
@@ -1325,6 +1333,9 @@ static int tun_xdp_xmit(struct net_device *dev, int n,
 	}
 	spin_unlock(&tfile->tx_ring.producer_lock);
 
+	if (flags & XDP_XMIT_FLUSH)
+		__tun_xdp_flush_tfile(tfile);
+
 	rcu_read_unlock();
 	return cnt - drops;
 }
@@ -1353,11 +1364,7 @@ static void tun_xdp_flush(struct net_device *dev)
 
 	tfile = rcu_dereference(tun->tfiles[smp_processor_id() %
 					    numqueues]);
-	/* Notify and wake up reader process */
-	if (tfile->flags & TUN_FASYNC)
-		kill_fasync(&tfile->fasync, SIGIO, POLL_IN);
-	tfile->socket.sk->sk_data_ready(tfile->socket.sk);
-
+	__tun_xdp_flush_tfile(tfile);
 out:
 	rcu_read_unlock();
 }

[bpf-next V1 PATCH 5/8] virtio_net: implement flush flag for ndo_xdp_xmit

[Jesper Dangaard Brouer]

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 drivers/net/virtio_net.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4ed823625953..62ba8aadd8e6 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -481,7 +481,7 @@ static int virtnet_xdp_xmit(struct net_device *dev,
 	int err;
 	int i;
 
-	if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
+	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
 		return -EINVAL;
 
 	qp = vi->curr_queue_pairs - vi->xdp_queue_pairs + smp_processor_id();
@@ -507,6 +507,10 @@ static int virtnet_xdp_xmit(struct net_device *dev,
 			drops++;
 		}
 	}
+
+	if (flags & XDP_XMIT_FLUSH)
+		virtqueue_kick(sq->vq);
+
 	return n - drops;
 }
 

[bpf-next V1 PATCH 6/8] xdp: done implementing ndo_xdp_xmit flush flag for all drivers

[Jesper Dangaard Brouer]

Removing XDP_XMIT_FLAGS_NONE as all driver now implement
a flush operation in their ndo_xdp_xmit call.  The compiler
will catch if any users of XDP_XMIT_FLAGS_NONE remains.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 include/net/xdp.h |    1 -
 1 file changed, 1 deletion(-)

diff --git a/include/net/xdp.h b/include/net/xdp.h
index 308a4b30b484..0bc304b80cdf 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -41,7 +41,6 @@ enum xdp_mem_type {
 };
 
 /* XDP flags for ndo_xdp_xmit */
-#define XDP_XMIT_FLAGS_NONE	0U
 #define XDP_XMIT_FLUSH		(1U << 0)
 #define XDP_XMIT_FLAGS_MASK	XDP_XMIT_FLUSH
 

[bpf-next V1 PATCH 7/8] bpf/xdp: non-map redirect can avoid calling ndo_xdp_flush

[Jesper Dangaard Brouer]

This is the first real user of the XDP_XMIT_FLUSH flag.

As pointed out many times, XDP_REDIRECT without using BPF maps is
significant slower than the map variant.  This is primary due to the
lack of bulking, as the ndo_xdp_flush operation is required after each
frame (to avoid frames hanging on the egress device).

It is still possible to optimize this case.  Instead of invoking two
NDO indirect calls, which are very expensive with CONFIG_RETPOLINE,
instead instruct ndo_xdp_xmit to flush via XDP_XMIT_FLUSH flag.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 net/core/filter.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 6a21dbcad350..6981b4608979 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3056,10 +3056,9 @@ static int __bpf_tx_xdp(struct net_device *dev,
 	if (unlikely(!xdpf))
 		return -EOVERFLOW;
 
-	sent = dev->netdev_ops->ndo_xdp_xmit(dev, 1, &xdpf, 0);
+	sent = dev->netdev_ops->ndo_xdp_xmit(dev, 1, &xdpf, XDP_XMIT_FLUSH);
 	if (sent <= 0)
 		return sent;
-	dev->netdev_ops->ndo_xdp_flush(dev);
 	return 0;
 }
 

[bpf-next V1 PATCH 8/8] bpf/xdp: devmap can avoid calling ndo_xdp_flush

[Jesper Dangaard Brouer]

The XDP_REDIRECT map devmap can avoid using ndo_xdp_flush, by instead
instructing ndo_xdp_xmit to flush via XDP_XMIT_FLUSH flag in
appropriate places.

Notice after this patch it is possible to remove ndo_xdp_flush
completely, as this is the last user of ndo_xdp_flush. This is left
for later patches, to keep driver changes separate.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 kernel/bpf/devmap.c |   20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index 04fbd75a5274..9c846a7a8cff 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -217,7 +217,7 @@ void __dev_map_insert_ctx(struct bpf_map *map, u32 bit)
 }
 
 static int bq_xmit_all(struct bpf_dtab_netdev *obj,
-			 struct xdp_bulk_queue *bq)
+		       struct xdp_bulk_queue *bq, bool flush)
 {
 	struct net_device *dev = obj->dev;
 	int sent = 0, drops = 0, err = 0;
@@ -232,7 +232,8 @@ static int bq_xmit_all(struct bpf_dtab_netdev *obj,
 		prefetch(xdpf);
 	}
 
-	sent = dev->netdev_ops->ndo_xdp_xmit(dev, bq->count, bq->q, 0);
+	sent = dev->netdev_ops->ndo_xdp_xmit(dev, bq->count, bq->q,
+					     flush ? XDP_XMIT_FLUSH : 0);
 	if (sent < 0) {
 		err = sent;
 		sent = 0;
@@ -276,7 +277,6 @@ void __dev_map_flush(struct bpf_map *map)
 	for_each_set_bit(bit, bitmap, map->max_entries) {
 		struct bpf_dtab_netdev *dev = READ_ONCE(dtab->netdev_map[bit]);
 		struct xdp_bulk_queue *bq;
-		struct net_device *netdev;
 
 		/* This is possible if the dev entry is removed by user space
 		 * between xdp redirect and flush op.
@@ -287,10 +287,7 @@ void __dev_map_flush(struct bpf_map *map)
 		__clear_bit(bit, bitmap);
 
 		bq = this_cpu_ptr(dev->bulkq);
-		bq_xmit_all(dev, bq);
-		netdev = dev->dev;
-		if (likely(netdev->netdev_ops->ndo_xdp_flush))
-			netdev->netdev_ops->ndo_xdp_flush(netdev);
+		bq_xmit_all(dev, bq, true);
 	}
 }
 
@@ -320,7 +317,7 @@ static int bq_enqueue(struct bpf_dtab_netdev *obj, struct xdp_frame *xdpf,
 	struct xdp_bulk_queue *bq = this_cpu_ptr(obj->bulkq);
 
 	if (unlikely(bq->count == DEV_MAP_BULK_SIZE))
-		bq_xmit_all(obj, bq);
+		bq_xmit_all(obj, bq, false);
 
 	/* Ingress dev_rx will be the same for all xdp_frame's in
 	 * bulk_queue, because bq stored per-CPU and must be flushed
@@ -359,8 +356,7 @@ static void *dev_map_lookup_elem(struct bpf_map *map, void *key)
 
 static void dev_map_flush_old(struct bpf_dtab_netdev *dev)
 {
-	if (dev->dev->netdev_ops->ndo_xdp_flush) {
-		struct net_device *fl = dev->dev;
+	if (dev->dev->netdev_ops->ndo_xdp_xmit) {
 		struct xdp_bulk_queue *bq;
 		unsigned long *bitmap;
 
@@ -371,9 +367,7 @@ static void dev_map_flush_old(struct bpf_dtab_netdev *dev)
 			__clear_bit(dev->bit, bitmap);
 
 			bq = per_cpu_ptr(dev->bulkq, cpu);
-			bq_xmit_all(dev, bq);
-
-			fl->netdev_ops->ndo_xdp_flush(dev->dev);
+			bq_xmit_all(dev, bq, true);
 		}
 	}
 }

Re: [bpf-next V1 PATCH 1/8] xdp: add flags argument to ndo_xdp_xmit API

[Song Liu]

On Wed, May 30, 2018 at 11:00 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> This patch only change the API and reject any use of flags. This is an
> intermediate step that allows us to implement the flush flag operation
> later, for each individual driver in a separate patch.
>
> The plan is to implement flush operation via XDP_XMIT_FLUSH flag
> and then remove XDP_XMIT_FLAGS_NONE when done.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c   |    6 +++++-
>  drivers/net/ethernet/intel/i40e/i40e_txrx.h   |    3 ++-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    5 ++++-
>  drivers/net/tun.c                             |    8 ++++++--
>  drivers/net/virtio_net.c                      |    5 ++++-
>  include/linux/netdevice.h                     |    7 ++++---
>  include/net/xdp.h                             |    5 +++++
>  kernel/bpf/devmap.c                           |    2 +-
>  net/core/filter.c                             |    2 +-
>  9 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index 9b698c5acd05..c0451d6e0790 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -3670,7 +3670,8 @@ netdev_tx_t i40e_lan_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
>   * For error cases, a negative errno code is returned and no-frames
>   * are transmitted (caller must handle freeing frames).
>   **/
> -int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames)
> +int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
> +                 u32 flags)
>  {
>         struct i40e_netdev_priv *np = netdev_priv(dev);
>         unsigned int queue_index = smp_processor_id();
> @@ -3684,6 +3685,9 @@ int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames)
>         if (!i40e_enabled_xdp_vsi(vsi) || queue_index >= vsi->num_queue_pairs)
>                 return -ENXIO;
>
> +       if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
> +               return -EINVAL;
> +
>         for (i = 0; i < n; i++) {
>                 struct xdp_frame *xdpf = frames[i];
>                 int err;
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> index eb8804b3d7b6..820f76db251b 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> @@ -487,7 +487,8 @@ u32 i40e_get_tx_pending(struct i40e_ring *ring, bool in_sw);
>  void i40e_detect_recover_hung(struct i40e_vsi *vsi);
>  int __i40e_maybe_stop_tx(struct i40e_ring *tx_ring, int size);
>  bool __i40e_chk_linearize(struct sk_buff *skb);
> -int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames);
> +int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
> +                 u32 flags);
>  void i40e_xdp_flush(struct net_device *dev);
>
>  /**
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 031d65c4178d..87f088f4af52 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -10023,7 +10023,7 @@ static int ixgbe_xdp(struct net_device *dev, struct netdev_bpf *xdp)
>  }
>
>  static int ixgbe_xdp_xmit(struct net_device *dev, int n,
> -                         struct xdp_frame **frames)
> +                         struct xdp_frame **frames, u32 flags)
>  {
>         struct ixgbe_adapter *adapter = netdev_priv(dev);
>         struct ixgbe_ring *ring;
> @@ -10033,6 +10033,9 @@ static int ixgbe_xdp_xmit(struct net_device *dev, int n,
>         if (unlikely(test_bit(__IXGBE_DOWN, &adapter->state)))
>                 return -ENETDOWN;
>
> +       if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
> +               return -EINVAL;
> +
>         /* During program transitions its possible adapter->xdp_prog is assigned
>          * but ring has not been configured yet. In this case simply abort xmit.
>          */
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 2265d2ccea47..b182b8cdd219 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1285,7 +1285,8 @@ static const struct net_device_ops tun_netdev_ops = {
>         .ndo_get_stats64        = tun_net_get_stats64,
>  };
>
> -static int tun_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames)
> +static int tun_xdp_xmit(struct net_device *dev, int n,
> +                       struct xdp_frame **frames, u32 flags)
>  {
>         struct tun_struct *tun = netdev_priv(dev);
>         struct tun_file *tfile;
> @@ -1294,6 +1295,9 @@ static int tun_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames
>         int cnt = n;
>         int i;
>
> +       if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
> +               return -EINVAL;
> +
>         rcu_read_lock();
>
>         numqueues = READ_ONCE(tun->numqueues);
> @@ -1332,7 +1336,7 @@ static int tun_xdp_tx(struct net_device *dev, struct xdp_buff *xdp)
>         if (unlikely(!frame))
>                 return -EOVERFLOW;
>
> -       return tun_xdp_xmit(dev, 1, &frame);
> +       return tun_xdp_xmit(dev, 1, &frame, 0);
>  }
>
>  static void tun_xdp_flush(struct net_device *dev)
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index b2647dd5d302..4ed823625953 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -468,7 +468,7 @@ static int __virtnet_xdp_tx_xmit(struct virtnet_info *vi,
>  }
>
>  static int virtnet_xdp_xmit(struct net_device *dev,
> -                           int n, struct xdp_frame **frames)
> +                           int n, struct xdp_frame **frames, u32 flags)
>  {
>         struct virtnet_info *vi = netdev_priv(dev);
>         struct receive_queue *rq = vi->rq;
> @@ -481,6 +481,9 @@ static int virtnet_xdp_xmit(struct net_device *dev,
>         int err;
>         int i;
>
> +       if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
> +               return -EINVAL;
> +
>         qp = vi->curr_queue_pairs - vi->xdp_queue_pairs + smp_processor_id();
>         sq = &vi->sq[qp];
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 8452f72087ef..7f17785a59d7 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1185,13 +1185,13 @@ struct dev_ifalias {
>   *     This function is used to set or query state related to XDP on the
>   *     netdevice and manage BPF offload. See definition of
>   *     enum bpf_netdev_command for details.
> - * int (*ndo_xdp_xmit)(struct net_device *dev, int n, struct xdp_frame **xdp);
> + * int (*ndo_xdp_xmit)(struct net_device *dev, int n, struct xdp_frame **xdp,
> + *                     u32 flags);
>   *     This function is used to submit @n XDP packets for transmit on a
>   *     netdevice. Returns number of frames successfully transmitted, frames
>   *     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.
> - *     TODO: Consider add flag to allow sending flush operation.
>   * void (*ndo_xdp_flush)(struct net_device *dev);
>   *     This function is used to inform the driver to flush a particular
>   *     xdp tx queue. Must be called on same CPU as xdp_xmit.
> @@ -1380,7 +1380,8 @@ struct net_device_ops {
>         int                     (*ndo_bpf)(struct net_device *dev,
>                                            struct netdev_bpf *bpf);
>         int                     (*ndo_xdp_xmit)(struct net_device *dev, int n,
> -                                               struct xdp_frame **xdp);
> +                                               struct xdp_frame **xdp,
> +                                               u32 flags);
>         void                    (*ndo_xdp_flush)(struct net_device *dev);
>  };
>
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index 7ad779237ae8..308a4b30b484 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -40,6 +40,11 @@ enum xdp_mem_type {
>         MEM_TYPE_MAX,
>  };
>
> +/* XDP flags for ndo_xdp_xmit */
> +#define XDP_XMIT_FLAGS_NONE    0U
> +#define XDP_XMIT_FLUSH         (1U << 0)
> +#define XDP_XMIT_FLAGS_MASK    XDP_XMIT_FLUSH
> +

I guess we need more documentation here on what XDP_XMIT_FLUSH does.

Other than this, it looks good to me.

Acked-by: Song Liu <songliubraving@fb.com>


>  struct xdp_mem_info {
>         u32 type; /* enum xdp_mem_type, but known size type */
>         u32 id;
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index ae16d0c373ef..04fbd75a5274 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -232,7 +232,7 @@ static int bq_xmit_all(struct bpf_dtab_netdev *obj,
>                 prefetch(xdpf);
>         }
>
> -       sent = dev->netdev_ops->ndo_xdp_xmit(dev, bq->count, bq->q);
> +       sent = dev->netdev_ops->ndo_xdp_xmit(dev, bq->count, bq->q, 0);
>         if (sent < 0) {
>                 err = sent;
>                 sent = 0;
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 81bd2e9fe8fc..6a21dbcad350 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3056,7 +3056,7 @@ static int __bpf_tx_xdp(struct net_device *dev,
>         if (unlikely(!xdpf))
>                 return -EOVERFLOW;
>
> -       sent = dev->netdev_ops->ndo_xdp_xmit(dev, 1, &xdpf);
> +       sent = dev->netdev_ops->ndo_xdp_xmit(dev, 1, &xdpf, 0);
>         if (sent <= 0)
>                 return sent;
>         dev->netdev_ops->ndo_xdp_flush(dev);
>

Re: [bpf-next V1 PATCH 2/8] i40e: implement flush flag for ndo_xdp_xmit

[Song Liu]

On Wed, May 30, 2018 at 11:00 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

I guess we still need to say something in the commit message?

> ---
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index c0451d6e0790..03c1446f0465 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -3685,7 +3685,7 @@ int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
>         if (!i40e_enabled_xdp_vsi(vsi) || queue_index >= vsi->num_queue_pairs)
>                 return -ENXIO;
>
> -       if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
> +       if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
>                 return -EINVAL;
>
>         for (i = 0; i < n; i++) {
> @@ -3699,6 +3699,9 @@ int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
>                 }
>         }
>
> +       if (unlikely(flags & XDP_XMIT_FLUSH))
> +               i40e_xdp_ring_update_tail(vsi->xdp_rings[queue_index]);
> +
>         return n - drops;

Do we still flush when drops > 0?

Thanks,
Song

>  }
>
>

Re: [bpf-next V1 PATCH 8/8] bpf/xdp: devmap can avoid calling ndo_xdp_flush

[Song Liu]

On Wed, May 30, 2018 at 11:01 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> The XDP_REDIRECT map devmap can avoid using ndo_xdp_flush, by instead
> instructing ndo_xdp_xmit to flush via XDP_XMIT_FLUSH flag in
> appropriate places.
>
> Notice after this patch it is possible to remove ndo_xdp_flush
> completely, as this is the last user of ndo_xdp_flush. This is left
> for later patches, to keep driver changes separate.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
>  kernel/bpf/devmap.c |   20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index 04fbd75a5274..9c846a7a8cff 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -217,7 +217,7 @@ void __dev_map_insert_ctx(struct bpf_map *map, u32 bit)
>  }
>
>  static int bq_xmit_all(struct bpf_dtab_netdev *obj,
> -                        struct xdp_bulk_queue *bq)
> +                      struct xdp_bulk_queue *bq, bool flush)

How about we use "int flags" instead of "bool flush" for easier extension?

Thanks,
Song

>  {
>         struct net_device *dev = obj->dev;
>         int sent = 0, drops = 0, err = 0;
> @@ -232,7 +232,8 @@ static int bq_xmit_all(struct bpf_dtab_netdev *obj,
>                 prefetch(xdpf);
>         }
>
> -       sent = dev->netdev_ops->ndo_xdp_xmit(dev, bq->count, bq->q, 0);
> +       sent = dev->netdev_ops->ndo_xdp_xmit(dev, bq->count, bq->q,
> +                                            flush ? XDP_XMIT_FLUSH : 0);
>         if (sent < 0) {
>                 err = sent;
>                 sent = 0;
> @@ -276,7 +277,6 @@ void __dev_map_flush(struct bpf_map *map)
>         for_each_set_bit(bit, bitmap, map->max_entries) {
>                 struct bpf_dtab_netdev *dev = READ_ONCE(dtab->netdev_map[bit]);
>                 struct xdp_bulk_queue *bq;
> -               struct net_device *netdev;
>
>                 /* This is possible if the dev entry is removed by user space
>                  * between xdp redirect and flush op.
> @@ -287,10 +287,7 @@ void __dev_map_flush(struct bpf_map *map)
>                 __clear_bit(bit, bitmap);
>
>                 bq = this_cpu_ptr(dev->bulkq);
> -               bq_xmit_all(dev, bq);
> -               netdev = dev->dev;
> -               if (likely(netdev->netdev_ops->ndo_xdp_flush))
> -                       netdev->netdev_ops->ndo_xdp_flush(netdev);
> +               bq_xmit_all(dev, bq, true);
>         }
>  }
>
> @@ -320,7 +317,7 @@ static int bq_enqueue(struct bpf_dtab_netdev *obj, struct xdp_frame *xdpf,
>         struct xdp_bulk_queue *bq = this_cpu_ptr(obj->bulkq);
>
>         if (unlikely(bq->count == DEV_MAP_BULK_SIZE))
> -               bq_xmit_all(obj, bq);
> +               bq_xmit_all(obj, bq, false);
>
>         /* Ingress dev_rx will be the same for all xdp_frame's in
>          * bulk_queue, because bq stored per-CPU and must be flushed
> @@ -359,8 +356,7 @@ static void *dev_map_lookup_elem(struct bpf_map *map, void *key)
>
>  static void dev_map_flush_old(struct bpf_dtab_netdev *dev)
>  {
> -       if (dev->dev->netdev_ops->ndo_xdp_flush) {
> -               struct net_device *fl = dev->dev;
> +       if (dev->dev->netdev_ops->ndo_xdp_xmit) {
>                 struct xdp_bulk_queue *bq;
>                 unsigned long *bitmap;
>
> @@ -371,9 +367,7 @@ static void dev_map_flush_old(struct bpf_dtab_netdev *dev)
>                         __clear_bit(dev->bit, bitmap);
>
>                         bq = per_cpu_ptr(dev->bulkq, cpu);
> -                       bq_xmit_all(dev, bq);
> -
> -                       fl->netdev_ops->ndo_xdp_flush(dev->dev);
> +                       bq_xmit_all(dev, bq, true);
>                 }
>         }
>  }
>

Re: [bpf-next V1 PATCH 0/8] bpf/xdp: add flags argument to ndo_xdp_xmit and flag flush operation

[Song Liu]

Overall, this set looks good to me. The only suggestion I have is to add more
documentation on the expected behavior of XDP_XMIT_FLUSH in netdevice.h
(as part of 01/08).

Thanks,
Song


On Wed, May 30, 2018 at 11:00 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> As I mentioned in merge commit 10f678683e4 ("Merge branch 'xdp_xmit-bulking'")
> I plan to change the API for ndo_xdp_xmit once more, by adding a flags
> argument, which is done in this patchset.
>
> I know it is late in the cycle (currently at rc7), but it would be
> nice to avoid changing NDOs over several kernel releases, as it is
> annoying to vendors and distro backporters, but it is not strictly
> UAPI so it is allowed (according to Alexei).
>
> The end-goal is getting rid of the ndo_xdp_flush operation, as it will
> make it possible for drivers to implement a TXQ synchronization mechanism
> that is not necessarily derived from the CPU id (smp_processor_id).
>
> This patchset removes all callers of the ndo_xdp_flush operation, but
> it doesn't take the last step of removing it from all drivers.  This
> can be done later, or I can update the patchset on request.
>
> Micro-benchmarks only show a very small performance improvement, for
> map-redirect around ~2 ns, and for non-map redirect ~7 ns.  I've not
> benchmarked this with CONFIG_RETPOLINE, but the performance benefit
> should be more visible given we end-up removing an indirect call.
>
> ---
>
> Jesper Dangaard Brouer (8):
>       xdp: add flags argument to ndo_xdp_xmit API
>       i40e: implement flush flag for ndo_xdp_xmit
>       ixgbe: implement flush flag for ndo_xdp_xmit
>       tun: implement flush flag for ndo_xdp_xmit
>       virtio_net: implement flush flag for ndo_xdp_xmit
>       xdp: done implementing ndo_xdp_xmit flush flag for all drivers
>       bpf/xdp: non-map redirect can avoid calling ndo_xdp_flush
>       bpf/xdp: devmap can avoid calling ndo_xdp_flush
>
>
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c   |    9 ++++++++-
>  drivers/net/ethernet/intel/i40e/i40e_txrx.h   |    3 ++-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   23 +++++++++++++++++------
>  drivers/net/tun.c                             |   25 ++++++++++++++++++-------
>  drivers/net/virtio_net.c                      |    9 ++++++++-
>  include/linux/netdevice.h                     |    7 ++++---
>  include/net/xdp.h                             |    4 ++++
>  kernel/bpf/devmap.c                           |   20 +++++++-------------
>  net/core/filter.c                             |    3 +--
>  9 files changed, 69 insertions(+), 34 deletions(-)
>
> --

Re: [bpf-next V1 PATCH 0/8] bpf/xdp: add flags argument to ndo_xdp_xmit and flag flush operation

[Jesper Dangaard Brouer]

On Wed, 30 May 2018 15:18:02 -0700
Song Liu <liu.song.a23@gmail.com> wrote:

> Overall, this set looks good to me. The only suggestion I have is to add more
> documentation on the expected behavior of XDP_XMIT_FLUSH in netdevice.h
> (as part of 01/08).

I do see your point, as the behavior of XDP_XMIT_FLUSH is actually more
a "doorbell" functionality.  I still choose to call it "flush", because
it is replacing a function called ndo_xdp_flush, and providing the
exact same code-function as ndo_xdp_flush.  (IMHO it should have been
called ndo_xdo_doorbell).

Any opinions about renaming XDP_XMIT_FLUSH to XDP_XMIT_DOORBELL?

If you look at virtio_net and tun usage, the effect is a wakeup
(virtqueue_kick and sk_data_ready).   Still I like the name "doorbell"
better than "wakeup", as it also maps to NIC usage which often call
this "doorbell" or "tail" pointer update.

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

Re: [bpf-next V1 PATCH 8/8] bpf/xdp: devmap can avoid calling ndo_xdp_flush

[Jesper Dangaard Brouer]

On Wed, 30 May 2018 15:06:27 -0700
Song Liu <liu.song.a23@gmail.com> wrote:

> > diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> > index 04fbd75a5274..9c846a7a8cff 100644
> > --- a/kernel/bpf/devmap.c
> > +++ b/kernel/bpf/devmap.c
> > @@ -217,7 +217,7 @@ void __dev_map_insert_ctx(struct bpf_map *map, u32 bit)
> >  }
> >
> >  static int bq_xmit_all(struct bpf_dtab_netdev *obj,
> > -                        struct xdp_bulk_queue *bq)
> > +                      struct xdp_bulk_queue *bq, bool flush)  
> 
> How about we use "int flags" instead of "bool flush" for easier extension?

This is an internal function in devmap.c, so we can change it
completely as we like.   But I do see your point, and will change this
to match the flags type of ndo_xdp_xmit, because that allows us to pass
this directly to that function without any conditional statements
(although I was expecting to compiler to optimized this out anyway).

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

Re: [bpf-next V1 PATCH 2/8] i40e: implement flush flag for ndo_xdp_xmit

[Jesper Dangaard Brouer]

On Wed, 30 May 2018 14:58:16 -0700
Song Liu <liu.song.a23@gmail.com> wrote:

> On Wed, May 30, 2018 at 11:00 AM, Jesper Dangaard Brouer
> <brouer@redhat.com> wrote:
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>  
> 
> I guess we still need to say something in the commit message?

Okay, I'll go through all of the driver changes and add some text.

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

Re: [bpf-next V1 PATCH 2/8] i40e: implement flush flag for ndo_xdp_xmit

[Jesper Dangaard Brouer]

On Wed, 30 May 2018 14:58:16 -0700 Song Liu <liu.song.a23@gmail.com> wrote:

> > @@ -3699,6 +3699,9 @@ int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
> >                 }
> >         }
> >
> > +       if (unlikely(flags & XDP_XMIT_FLUSH))
> > +               i40e_xdp_ring_update_tail(vsi->xdp_rings[queue_index]);
> > +
> >         return n - drops;  
> 
> Do we still flush when drops > 0?

Yes, just as before. Even if all frames are dropped, then it's allowed
to update the tail.

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