veth: add Byte Queue Limits (BQL) support

From: Jesper Dangaard Brouer <[email protected]>

XDP redirect into a veth device (via bpf_redirect()) calls veth_xdp_xmit(), which enqueues frames into the peer's ptr_ring using
smp_processor_id() % peer->real_num_rx_queues
as the ring index. With an asymmetric veth pair where the peer has fewer TX queues than RX queues, that index can exceed peer->real_num_tx_queues.

veth_poll() then resolves peer_txq for the ring via:

  peer_txq = peer_dev ? netdev_get_tx_queue(peer_dev, queue_idx) : NULL;

where queue_idx = rq->xdp_rxq.queue_index. When queue_idx exceeds peer_dev->real_num_tx_queues this is an out-of-bounds (OOB) access into the peer's netdev_queue array, triggering DEBUG_NET_WARN_ON_ONCE in netdev_get_tx_queue().

The normal ndo_start_xmit path is not affected: the stack clamps skb->queue_mapping via netdev_cap_txqueue() before invoking ndo_start_xmit, so rxq in veth_xmit() never exceeds real_num_tx_queues.

Fix veth_poll() by clamping: only dereference peer_txq when queue_idx is within bounds, otherwise set it to NULL. The out-of-range rings are fed exclusively via XDP redirect (veth_xdp_xmit), never via ndo_start_xmit (veth_xmit), so the peer txq was never stopped and there is nothing to wake; NULL is the correct fallback.

Reported-by Sashiko <[email protected]>

Closes https://lore.kernel.org/all/[email protected]/ open here

Fixes dc82a33297fc ("veth: apply qdisc backpressure on full ptr_ring to reduce TX drops")

Signed-off-by Jesper Dangaard Brouer <[email protected]>

From: Jesper Dangaard Brouer <[email protected]>

Commit dc82a33297fc ("veth: apply qdisc backpressure on full ptr_ring to reduce TX drops") gave qdiscs control over veth by returning NETDEV_TX_BUSY when the ptr_ring is full (DRV_XOFF). That commit noted a known limitation: the 256-entry ptr_ring sits in front of the qdisc as a dark buffer, adding base latency because the qdisc has no visibility into how many bytes are already queued there.

Add BQL support so the qdisc gets feedback and can begin shaping traffic before the ring fills. In testing with fq_codel, BQL reduces ping RTT under UDP load from ~6.61ms to ~0.36ms (18x).

Charge a fixed VETH_BQL_UNIT (1) per packet rather than skb->len, so the DQL limit tracks packets-in-flight. Unlike a physical NIC, veth has no link speed -- the ptr_ring drains at CPU speed and is packet-indexed, not byte-indexed, so bytes are not the natural unit. With byte-based charging, small packets sneak many more entries into the ring before STACK_XOFF fires, deepening the dark buffer under mixed-size workloads. Testing with a concurrent min-size packet flood shows 3.7x ping RTT degradation with skb->len charging versus no change with fixed-unit charging.

Charge BQL inside veth_xdp_rx() under the ptr_ring producer_lock, after confirming the ring is not full. The charge must precede the produce because the NAPI consumer can run on another CPU and complete the SKB the instant it becomes visible in the ring. Doing both under the same lock avoids a pre-charge/undo pattern -- BQL is only charged when produce is guaranteed to succeed.

BQL is enabled only when a real qdisc is attached (guarded by !qdisc_txq_has_no_queue), as HARD_TX_LOCK provides serialization for TXQ modification like dql_queued(). For lltx devices, like veth, this HARD_TX_LOCK serialization isn't provided. The ptr_ring producer_lock provides additional serialization that would allow BQL to work correctly even with noqueue, though that combination is not currently enabled, as the netstack will drop and warn.

Track per-SKB BQL state via a VETH_BQL_FLAG pointer tag in the ptr_ring entry. This is necessary because the qdisc can be replaced live while SKBs are in-flight -- each SKB must carry the charge decision made at enqueue time rather than re-checking the peer's qdisc at completion.

Complete per-SKB in veth_xdp_rcv() rather than in bulk, so STACK_XOFF clears promptly when producer and consumer run on different CPUs.

BQL introduces a second independent queue-stop mechanism (STACK_XOFF) alongside the existing DRV_XOFF (ring full). Both must be clear for the queue to transmit. Reset BQL state in veth_napi_del_range() after synchronize_net() to avoid racing with in-flight veth_poll() calls. Clamp the reset loop to the peer's real_num_tx_queues, since the peer may have fewer TX queues than the local device has RX queues (e.g. when veth is enslaved to a bond with XDP attached).

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---
 drivers/net/veth.c | 86 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 75 insertions(+), 11 deletions(-)

drivers/net/veth.c +49 -9

index 0cfb19b760dd..86b78900c48e 100644
@@ -34,9 +34,13 @@
 #define DRV_VERSION	"1.0"
 
 #define VETH_XDP_FLAG		BIT(0)
+#define VETH_BQL_FLAG		BIT(1)
 #define VETH_RING_SIZE		256
 #define VETH_XDP_HEADROOM	(XDP_PACKET_HEADROOM + NET_IP_ALIGN)
 
+/* Fixed BQL charge: DQL limit tracks packets-in-flight, not bytes */
+#define VETH_BQL_UNIT		1
+
 #define VETH_XDP_TX_BULK_SIZE	16
 #define VETH_XDP_BATCH		16
 

@@ -280,6 +284,21 @@ static bool veth_is_xdp_frame(void *ptr)
 	return (unsigned long)ptr & VETH_XDP_FLAG;
 }
 
+static bool veth_ptr_is_bql(void *ptr)
+{
+	return (unsigned long)ptr & VETH_BQL_FLAG;
+}
+
+static struct sk_buff *veth_ptr_to_skb(void *ptr)
+{
+	return (void *)((unsigned long)ptr & ~VETH_BQL_FLAG);
+}
+
+static void *veth_skb_to_ptr(struct sk_buff *skb, bool bql)
+{
+	return bql ? (void *)((unsigned long)skb | VETH_BQL_FLAG) : skb;
+}
+
 static struct xdp_frame *veth_ptr_to_xdp(void *ptr)
 {
 	return (void *)((unsigned long)ptr & ~VETH_XDP_FLAG);

@@ -295,7 +314,7 @@ static void veth_ptr_free(void *ptr)
 	if (veth_is_xdp_frame(ptr))
 		xdp_return_frame(veth_ptr_to_xdp(ptr));
 	else
-		kfree_skb(ptr);
+		kfree_skb(veth_ptr_to_skb(ptr));
 }
 
 static void __veth_xdp_flush(struct veth_rq *rq)

@@ -309,19 +328,33 @@ static void __veth_xdp_flush(struct veth_rq *rq)
 	}
 }
 
-static int veth_xdp_rx(struct veth_rq *rq, struct sk_buff *skb)
+static int veth_xdp_rx(struct veth_rq *rq, struct sk_buff *skb, bool do_bql,
+		       struct netdev_queue *txq)
 {
-	if (unlikely(ptr_ring_produce(&rq->xdp_ring, skb)))
+	struct ptr_ring *ring = &rq->xdp_ring;
+
+	spin_lock(&ring->producer_lock);
+	if (unlikely(!ring->size) || __ptr_ring_full(ring)) {
+		spin_unlock(&ring->producer_lock);
 		return NETDEV_TX_BUSY; /* signal qdisc layer */
+	}
+
+	/* BQL charge before produce; consumer cannot see entry yet */
+	if (do_bql)
+		netdev_tx_sent_queue(txq, VETH_BQL_UNIT);
+
+	__ptr_ring_produce(ring, veth_skb_to_ptr(skb, do_bql));
+	spin_unlock(&ring->producer_lock);
 
 	return NET_RX_SUCCESS; /* same as NETDEV_TX_OK */
 }
 
 static int veth_forward_skb(struct net_device *dev, struct sk_buff *skb,
-			    struct veth_rq *rq, bool xdp)
+			    struct veth_rq *rq, bool xdp, bool do_bql,
+			    struct netdev_queue *txq)
 {
 	return __dev_forward_skb(dev, skb) ?: xdp ?
-		veth_xdp_rx(rq, skb) :
+		veth_xdp_rx(rq, skb, do_bql, txq) :
 		__netif_rx(skb);
 }
 

@@ -348,10 +381,11 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
 	struct veth_rq *rq = NULL;
-	struct netdev_queue *txq;
+	struct netdev_queue *txq = NULL;
 	struct net_device *rcv;
 	int length = skb->len;
 	bool use_napi = false;
+	bool do_bql = false;
 	int ret, rxq;
 
 	rcu_read_lock();

@@ -375,8 +409,12 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	skb_tx_timestamp(skb);
-
-	ret = veth_forward_skb(rcv, skb, rq, use_napi);
+	if (rxq < dev->real_num_tx_queues) {
+		txq = netdev_get_tx_queue(dev, rxq);
+		/* BQL charge happens inside veth_xdp_rx() under producer_lock */
+		do_bql = use_napi && !qdisc_txq_has_no_queue(txq);
+	}
+	ret = veth_forward_skb(rcv, skb, rq, use_napi, do_bql, txq);
 	switch (ret) {
 	case NET_RX_SUCCESS: /* same as NETDEV_TX_OK */
 		if (!use_napi)

@@ -412,6 +450,7 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
 		net_crit_ratelimited("%s(%s): Invalid return code(%d)",
 				     __func__, dev->name, ret);
 	}
+
 	rcu_read_unlock();
 
 	return ret;

@@ -900,7 +939,8 @@ static struct sk_buff *veth_xdp_rcv_skb(struct veth_rq *rq,
 
 static int veth_xdp_rcv(struct veth_rq *rq, int budget,
 			struct veth_xdp_tx_bq *bq,
-			struct veth_stats *stats)
+			struct veth_stats *stats,
+			struct netdev_queue *peer_txq)
 {
 	int i, done = 0, n_xdpf = 0;
 	void *xdpf[VETH_XDP_BATCH];

drivers/net/veth.c +22 -1

+
 			skb = veth_xdp_rcv_skb(rq, skb, bq, stats);
 			if (skb) {
 				if (skb_shared(skb) || skb_unclone(skb, GFP_ATOMIC))

@@ -976,7 +1020,7 @@ static int veth_poll(struct napi_struct *napi, int budget)
 		   netdev_get_tx_queue(peer_dev, queue_idx) : NULL;
 
 	xdp_set_return_frame_no_direct();
-	done = veth_xdp_rcv(rq, budget, &bq, &stats);
+	done = veth_xdp_rcv(rq, budget, &bq, &stats, peer_txq);
 
 	if (stats.xdp_redirect > 0)
 		xdp_do_flush();

@@ -1074,6 +1118,7 @@ static int __veth_napi_enable(struct net_device *dev)
 static void veth_napi_del_range(struct net_device *dev, int start, int end)
 {
 	struct veth_priv *priv = netdev_priv(dev);
+	struct net_device *peer;
 	int i;
 
 	for (i = start; i < end; i++) {

@@ -1092,6 +1137,24 @@ static void veth_napi_del_range(struct net_device *dev, int start, int end)
 		ptr_ring_cleanup(&rq->xdp_ring, veth_ptr_free);
 	}
 
+	/* Reset BQL and wake stopped peer txqs.  A concurrent veth_xmit()
+	 * may have set DRV_XOFF between rcu_assign_pointer(napi, NULL) and
+	 * synchronize_net(), and NAPI can no longer clear it.
+	 * Only wake when the device is still up.
+	 */
+	peer = rtnl_dereference(priv->peer);
+	if (peer) {
+		int peer_end = min_t(int, end, peer->real_num_tx_queues);
+
+		for (i = start; i < peer_end; i++) {
+			struct netdev_queue *txq = netdev_get_tx_queue(peer, i);
+
+			netdev_tx_reset_queue(txq);
+			if (netif_running(dev))
+				netif_tx_wake_queue(txq);
+		}
+	}
+
 	for (i = start; i < end; i++) {
 		page_pool_destroy(priv->rq[i].page_pool);
 		priv->rq[i].page_pool = NULL;

@@ -1741,6 +1804,7 @@ static void veth_setup(struct net_device *dev)
 	dev->priv_flags |= IFF_PHONY_HEADROOM;
 	dev->priv_flags |= IFF_DISABLE_NETPOLL;
 	dev->lltx = true;
+	dev->bql = true;
 
 	dev->netdev_ops = &veth_netdev_ops;
 	dev->xdp_metadata_ops = &veth_xdp_metadata_ops;

drivers/net/veth.c static int veth_xdp_rcv(struct veth_rq *rq, int budget, · line 976

Quoted 11 lines

drivers/net/veth.c +4 -1

@@ -928,9 +968,13 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
 			}
 		} else {
 			/* ndo_start_xmit */
-			struct sk_buff *skb = ptr;
+			bool bql_charged = veth_ptr_is_bql(ptr);
+			struct sk_buff *skb = veth_ptr_to_skb(ptr);
 
 			stats->xdp_bytes += skb->len;
+			if (peer_txq && bql_charged)
+				netdev_tx_completed_queue(peer_txq, 1, VETH_BQL_UNIT);

In the discussion with Jonas [1], I left a comment explaining why I think this doesn’t work.

I still think first that adding an option to modify the hard-coded VETH_RING_SIZE is the way to go.

drivers/net/veth.c static int veth_xdp_rcv(struct veth_rq *rq, int budget, · line 976

Quoted 11 lines · On 5/5/26 15:21, [email protected] wrote:

drivers/net/veth.c +4 -1

@@ -928,9 +968,13 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
 			}
 		} else {
 			/* ndo_start_xmit */
-			struct sk_buff *skb = ptr;
+			bool bql_charged = veth_ptr_is_bql(ptr);
+			struct sk_buff *skb = veth_ptr_to_skb(ptr);
 
 			stats->xdp_bytes += skb->len;
+			if (peer_txq && bql_charged)
+				netdev_tx_completed_queue(peer_txq, 1, VETH_BQL_UNIT);

In the discussion with Jonas [1], I left a comment explaining why I think this doesn’t work.

I still think first that adding an option to modify the hard-coded VETH_RING_SIZE is the way to go.

Thanks!

[1] Link: https://lore.kernel.org/netdev/[email protected]/

Quoted 21 lines · Simon Schippers

drivers/net/veth.c static int veth_xdp_rcv(struct veth_rq *rq, int budget, · line 976

Quoted 11 lines · On 5/5/26 15:21, [email protected] wrote:

drivers/net/veth.c +4 -1

@@ -928,9 +968,13 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
 			}
 		} else {
 			/* ndo_start_xmit */
-			struct sk_buff *skb = ptr;
+			bool bql_charged = veth_ptr_is_bql(ptr);
+			struct sk_buff *skb = veth_ptr_to_skb(ptr);
 
 			stats->xdp_bytes += skb->len;
+			if (peer_txq && bql_charged)
+				netdev_tx_completed_queue(peer_txq, 1, VETH_BQL_UNIT);

In the discussion with Jonas [1], I left a comment explaining why I think this doesn’t work.

I still think first that adding an option to modify the hard-coded VETH_RING_SIZE is the way to go.

Thanks!

Quoted 1 line

[1] Link: https://lore.kernel.org/netdev/[email protected]/

In the above discussion a 20% regression is reported, which IMHO can't be ignored. Still the tput figures in the data are extremely low, something is possibly off?!? I would expect a few Mpps with pktgen on top of veth, while the reported data is ~20-30Kpps.

/P

Quoted 10 lines · Paolo Abeni

Quoted 9 lines · Simon Schippers

Quoted 8 lines · On 5/5/26 15:21, [email protected] wrote:

drivers/net/veth.c +2 -1

@@ -928,9 +968,13 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
  			}
  		} else {
  			/* ndo_start_xmit */
-			struct sk_buff *skb = ptr;
+			bool bql_charged = veth_ptr_is_bql(ptr);
+			struct sk_buff *skb = veth_ptr_to_skb(ptr);
 

Quoted 7 lines

Quoted 7 lines

drivers/net/veth.c static int veth_xdp_rcv(struct veth_rq *rq, int budget, · line 976

Quoted 3 lines

drivers/net/veth.c +2 -0

  			stats->xdp_bytes += skb->len;
+			if (peer_txq && bql_charged)
+				netdev_tx_completed_queue(peer_txq, 1, VETH_BQL_UNIT);

In the discussion with Jonas [1], I left a comment explaining why I think this doesn’t work.

Quoted 3 lines

Quoted 3 lines

I still think first that adding an option to modify the hard-coded VETH_RING_SIZE is the way to go.

Quoted 9 lines

Quoted 3 lines

Thanks!

[1] Link: https://lore.kernel.org/netdev/[email protected]/

In the above discussion a 20% regression is reported, which IMHO can't be ignored. Still the tput figures in the data are extremely low, something is possibly off?!? I would expect a few Mpps with pktgen on top of veth, while the reported data is ~20-30Kpps.

Quoted 2 lines

/P

The ~20-30Kpps occur when thousands of iptables rules are applied and an UDP userspace application is sending.

And there is a 20% pktgen regression (no iptables rules applied).

I am pretty sure the reason is because the BQL limit is stuck at 2 packets (because the completed queue is always called with 1 packet and not in a interrupt/timer with multiple packets...).

"Must be called at most once per TX completion round (and not per
individual packet), so that BQL can adjust its limits appropriately."

"BQL accounting is in the transmit path for every packet, and the
function to recompute the byte limit is run once per transmit completion."

Quoted 5 lines · Simon Schippers

Quoted 2 lines · Paolo Abeni

Quoted 1 line · Simon Schippers

On 5/5/26 15:21, [email protected] wrote:

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/net/veth.c +1 -1

@@ -928,9 +968,13 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
              }
          } else {
              /* ndo_start_xmit */
-            struct sk_buff *skb = ptr;
+            bool bql_charged = veth_ptr_is_bql(ptr);

Quoted 8 lines

Quoted 8 lines

drivers/net/veth.c static int veth_xdp_rcv(struct veth_rq *rq, int budget, · line 976

Quoted 4 lines

drivers/net/veth.c +3 -0

+            struct sk_buff *skb = veth_ptr_to_skb(ptr);
                stats->xdp_bytes += skb->len;
+            if (peer_txq && bql_charged)
+                netdev_tx_completed_queue(peer_txq, 1, VETH_BQL_UNIT);

In the discussion with Jonas [1], I left a comment explaining why I think this doesn’t work.

I've experimented with doing the "completion" at NAPI-end in veth_poll(), but that resulted in BQL limit being 128 packets, which leads to bad latency results (not acceptable).
(See detailed report later)

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

I still think first that adding an option to modify the hard-coded VETH_RING_SIZE is the way to go.

Not against being able to modify VETH_RING_SIZE, but I don't think it is the solution here.

The simply solution is the configure BQL limit_min:
/sys/class/net/<dev>/queues/tx-N/byte_queue_limits/limit_min

My experiments (below) find that limit_min=8 is gives good performance. We can simply set default to 8 as this still allows userspace to change this later if lower latency is preferred.

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

Thanks!

[1] Link: https://lore.kernel.org/netdev/[email protected]/

In the above discussion a 20% regression is reported, which IMHO can't be ignored. Still the tput figures in the data are extremely low, something is possibly off?!? I would expect a few Mpps with pktgen on top of veth, while the reported data is ~20-30Kpps.

Quoted 8 lines

Quoted 2 lines

/P

The ~20-30Kpps occur when thousands of iptables rules are applied and an UDP userspace application is sending.

And there is a 20% pktgen regression (no iptables rules applied).

The pktgen test is a little dubious/weird and Jonas had to modify pktgen
to test this. John Fastabend added a config to pktgen that allows us
to benchmarking egress qdisc path, this might be better to use this. The samples/pktgen/pktgen_bench_xmit_mode_queue_xmit.sh is a demo usage.

If redoing the tests, can you adjust limit_min to see the effect?
/sys/class/net/<dev>/queues/tx-N/byte_queue_limits/limit_min

20% throughput performance regression is of-cause too much, but I will remind us, that adding a qdisc will "cost" some overhead, that is a configuration choice. Our purpose here is to reduce bufferbloat and latency, not optimize for throughput.

Quoted 4 lines

I am pretty sure the reason is because the BQL limit is stuck at 2 packets (because the completed queue is always called with 1 packet and not in a interrupt/timer with multiple packets...).

I've run a lot of experiments, which I made AI write a report over, see attachment. The TL;DR is that best performance vs latency tradeoff is defaulting BQL/DQL limit_min to be 8 packets.

I fear this patchset will stall forever, if we keep searching for a perfect solution without any overhead. The qdisc layer will be a baseline overhead. The limit=2 packets is actually the optimal darkbuffer queue size, but I acknowledge that this causes too many qdisc requeue events (leading to overhead). I suggest that I add another patch in V6, that defaults limit_min to 8 (separate patch to make it easier to revert/adjust later).

I've talked with Jonas, and we want to experiment with different solutions to make BQL/DQL work better with virtual devices.

This patchset helps our (production) use-case reduce mice-flow latency from approx 22ms to 1.3ms for latency under-load. Due to the consumer namespace being the bottleneck the requeue overhead is negligible in comparison.

-Jesper

Quoted 84 lines · Jesper Dangaard Brouer

Quoted 5 lines · Simon Schippers

Quoted 2 lines · Paolo Abeni

Quoted 1 line · Simon Schippers

On 5/5/26 15:21, [email protected] wrote:

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

(quoted diff) +1 -1

@@ -928,9 +968,13 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
               }
           } else {
               /* ndo_start_xmit */
-            struct sk_buff *skb = ptr;
+            bool bql_charged = veth_ptr_is_bql(ptr);

Quoted 8 lines

Quoted 8 lines

Review static int veth_xdp_rcv(struct veth_rq *rq, int budget, · line 975

Quoted 4 lines

(quoted diff) +3 -0

+            struct sk_buff *skb = veth_ptr_to_skb(ptr);
                 stats->xdp_bytes += skb->len;
+            if (peer_txq && bql_charged)
+                netdev_tx_completed_queue(peer_txq, 1, VETH_BQL_UNIT);

In the discussion with Jonas [1], I left a comment explaining why I think this doesn’t work.

I've experimented with doing the "completion" at NAPI-end in veth_poll(), but that resulted in BQL limit being 128 packets, which leads to bad latency results (not acceptable).
(See detailed report later)

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

I still think first that adding an option to modify the hard-coded VETH_RING_SIZE is the way to go.

Not against being able to modify VETH_RING_SIZE, but I don't think it is the solution here.

The simply solution is the configure BQL limit_min:
/sys/class/net/<dev>/queues/tx-N/byte_queue_limits/limit_min

My experiments (below) find that limit_min=8 is gives good performance. We can simply set default to 8 as this still allows userspace to change this later if lower latency is preferred.

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

Thanks!

[1] Link: https://lore.kernel.org/netdev/[email protected]/

In the above discussion a 20% regression is reported, which IMHO can't be ignored. Still the tput figures in the data are extremely low, something is possibly off?!? I would expect a few Mpps with pktgen on top of veth, while the reported data is ~20-30Kpps.

Quoted 8 lines

Quoted 2 lines

/P

The ~20-30Kpps occur when thousands of iptables rules are applied and an UDP userspace application is sending.

And there is a 20% pktgen regression (no iptables rules applied).

The pktgen test is a little dubious/weird and Jonas had to modify pktgen
to test this. John Fastabend added a config to pktgen that allows us
to benchmarking egress qdisc path, this might be better to use this. The samples/pktgen/pktgen_bench_xmit_mode_queue_xmit.sh is a demo usage.

If redoing the tests, can you adjust limit_min to see the effect?
/sys/class/net/<dev>/queues/tx-N/byte_queue_limits/limit_min

20% throughput performance regression is of-cause too much, but I will remind us, that adding a qdisc will "cost" some overhead, that is a configuration choice. Our purpose here is to reduce bufferbloat and latency, not optimize for throughput.

Quoted 4 lines

I am pretty sure the reason is because the BQL limit is stuck at 2 packets (because the completed queue is always called with 1 packet and not in a interrupt/timer with multiple packets...).

I've run a lot of experiments, which I made AI write a report over, see attachment. The TL;DR is that best performance vs latency tradeoff is defaulting BQL/DQL limit_min to be 8 packets.

I fear this patchset will stall forever, if we keep searching for a perfect solution without any overhead. The qdisc layer will be a baseline overhead. The limit=2 packets is actually the optimal darkbuffer queue size, but I acknowledge that this causes too many qdisc requeue events (leading to overhead). I suggest that I add another patch in V6, that defaults limit_min to 8 (separate patch to make it easier to revert/adjust later).

Quoted 8 lines

I've talked with Jonas, and we want to experiment with different solutions to make BQL/DQL work better with virtual devices.

This patchset helps our (production) use-case reduce mice-flow latency from approx 22ms to 1.3ms for latency under-load. Due to the consumer namespace being the bottleneck the requeue overhead is negligible in comparison.

-Jesper

First of all thanks for you work and I really see the advantages of avoiding bufferbloat :)

But the key of the BQL algorithm, which is the *dynamic* adaption of the limit, is not working. Always calling netdev_completed_queue() with 1 packet results in a static limit of 2 packets (as seen by Jonas measurements), which you force up to 8 packets.

So in the end this patchset has the same effect as just setting VETH_RING_SIZE to 8 (and giving an option to change this value).

Quoted 102 lines · Simon Schippers

Quoted 84 lines · Jesper Dangaard Brouer

Quoted 5 lines · Simon Schippers

Quoted 2 lines · Paolo Abeni

Quoted 1 line · Simon Schippers

On 5/5/26 15:21, [email protected] wrote:

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

(quoted diff) +1 -1

@@ -928,9 +968,13 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
               }
           } else {
               /* ndo_start_xmit */
-            struct sk_buff *skb = ptr;
+            bool bql_charged = veth_ptr_is_bql(ptr);

Quoted 8 lines

Quoted 8 lines

Review static int veth_xdp_rcv(struct veth_rq *rq, int budget, · line 975

Quoted 4 lines

(quoted diff) +3 -0

+            struct sk_buff *skb = veth_ptr_to_skb(ptr);
                 stats->xdp_bytes += skb->len;
+            if (peer_txq && bql_charged)
+                netdev_tx_completed_queue(peer_txq, 1, VETH_BQL_UNIT);

In the discussion with Jonas [1], I left a comment explaining why I think this doesn’t work.

I've experimented with doing the "completion" at NAPI-end in veth_poll(), but that resulted in BQL limit being 128 packets, which leads to bad latency results (not acceptable).
(See detailed report later)

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

I still think first that adding an option to modify the hard-coded VETH_RING_SIZE is the way to go.

Not against being able to modify VETH_RING_SIZE, but I don't think it is the solution here.

The simply solution is the configure BQL limit_min:
/sys/class/net/<dev>/queues/tx-N/byte_queue_limits/limit_min

My experiments (below) find that limit_min=8 is gives good performance. We can simply set default to 8 as this still allows userspace to change this later if lower latency is preferred.

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

Thanks!

[1] Link: https://lore.kernel.org/netdev/[email protected]/

In the above discussion a 20% regression is reported, which IMHO can't be ignored. Still the tput figures in the data are extremely low, something is possibly off?!? I would expect a few Mpps with pktgen on top of veth, while the reported data is ~20-30Kpps.

Quoted 8 lines

Quoted 2 lines

/P

The ~20-30Kpps occur when thousands of iptables rules are applied and an UDP userspace application is sending.

And there is a 20% pktgen regression (no iptables rules applied).

The pktgen test is a little dubious/weird and Jonas had to modify pktgen
to test this. John Fastabend added a config to pktgen that allows us
to benchmarking egress qdisc path, this might be better to use this. The samples/pktgen/pktgen_bench_xmit_mode_queue_xmit.sh is a demo usage.

If redoing the tests, can you adjust limit_min to see the effect?
/sys/class/net/<dev>/queues/tx-N/byte_queue_limits/limit_min

20% throughput performance regression is of-cause too much, but I will remind us, that adding a qdisc will "cost" some overhead, that is a configuration choice. Our purpose here is to reduce bufferbloat and latency, not optimize for throughput.

Quoted 4 lines

I am pretty sure the reason is because the BQL limit is stuck at 2 packets (because the completed queue is always called with 1 packet and not in a interrupt/timer with multiple packets...).

I've run a lot of experiments, which I made AI write a report over, see attachment. The TL;DR is that best performance vs latency tradeoff is defaulting BQL/DQL limit_min to be 8 packets.

I fear this patchset will stall forever, if we keep searching for a perfect solution without any overhead. The qdisc layer will be a baseline overhead. The limit=2 packets is actually the optimal darkbuffer queue size, but I acknowledge that this causes too many qdisc requeue events (leading to overhead). I suggest that I add another patch in V6, that defaults limit_min to 8 (separate patch to make it easier to revert/adjust later).

Quoted 8 lines

I've talked with Jonas, and we want to experiment with different solutions to make BQL/DQL work better with virtual devices.

This patchset helps our (production) use-case reduce mice-flow latency from approx 22ms to 1.3ms for latency under-load. Due to the consumer namespace being the bottleneck the requeue overhead is negligible in comparison.

-Jesper

First of all thanks for you work and I really see the advantages of avoiding bufferbloat :)

But the key of the BQL algorithm, which is the *dynamic* adaption of the limit, is not working. Always calling netdev_completed_queue() with 1 packet results in a static limit of 2 packets (as seen by Jonas measurements), which you force up to 8 packets.

Quoted 3 lines

So in the end this patchset has the same effect as just setting VETH_RING_SIZE to 8 (and giving an option to change this value).

I've code up a time based BQL implementation, see attachment. WDYT?

--Jesper

Quoted 112 lines · Jesper Dangaard Brouer

Quoted 97 lines · Simon Schippers

Quoted 84 lines · Jesper Dangaard Brouer

Quoted 5 lines · Simon Schippers

Quoted 2 lines · Paolo Abeni

Quoted 1 line · Simon Schippers

On 5/5/26 15:21, [email protected] wrote:

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

(quoted diff) +1 -1

@@ -928,9 +968,13 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget,
               }
           } else {
               /* ndo_start_xmit */
-            struct sk_buff *skb = ptr;
+            bool bql_charged = veth_ptr_is_bql(ptr);

Quoted 8 lines

Quoted 8 lines

Review static int veth_xdp_rcv(struct veth_rq *rq, int budget, · line 975

Quoted 4 lines

(quoted diff) +3 -0

+            struct sk_buff *skb = veth_ptr_to_skb(ptr);
                 stats->xdp_bytes += skb->len;
+            if (peer_txq && bql_charged)
+                netdev_tx_completed_queue(peer_txq, 1, VETH_BQL_UNIT);

In the discussion with Jonas [1], I left a comment explaining why I think this doesn’t work.

I've experimented with doing the "completion" at NAPI-end in veth_poll(), but that resulted in BQL limit being 128 packets, which leads to bad latency results (not acceptable).
(See detailed report later)

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

I still think first that adding an option to modify the hard-coded VETH_RING_SIZE is the way to go.

Not against being able to modify VETH_RING_SIZE, but I don't think it is the solution here.

The simply solution is the configure BQL limit_min:
/sys/class/net/<dev>/queues/tx-N/byte_queue_limits/limit_min

My experiments (below) find that limit_min=8 is gives good performance. We can simply set default to 8 as this still allows userspace to change this later if lower latency is preferred.

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

Thanks!

[1] Link: https://lore.kernel.org/netdev/[email protected]/

In the above discussion a 20% regression is reported, which IMHO can't be ignored. Still the tput figures in the data are extremely low, something is possibly off?!? I would expect a few Mpps with pktgen on top of veth, while the reported data is ~20-30Kpps.

Quoted 8 lines

Quoted 2 lines

/P

The ~20-30Kpps occur when thousands of iptables rules are applied and an UDP userspace application is sending.

And there is a 20% pktgen regression (no iptables rules applied).

The pktgen test is a little dubious/weird and Jonas had to modify pktgen
to test this. John Fastabend added a config to pktgen that allows us
to benchmarking egress qdisc path, this might be better to use this. The samples/pktgen/pktgen_bench_xmit_mode_queue_xmit.sh is a demo usage.

If redoing the tests, can you adjust limit_min to see the effect?
/sys/class/net/<dev>/queues/tx-N/byte_queue_limits/limit_min

20% throughput performance regression is of-cause too much, but I will remind us, that adding a qdisc will "cost" some overhead, that is a configuration choice. Our purpose here is to reduce bufferbloat and latency, not optimize for throughput.

Quoted 4 lines

I am pretty sure the reason is because the BQL limit is stuck at 2 packets (because the completed queue is always called with 1 packet and not in a interrupt/timer with multiple packets...).

I've run a lot of experiments, which I made AI write a report over, see attachment. The TL;DR is that best performance vs latency tradeoff is defaulting BQL/DQL limit_min to be 8 packets.

I fear this patchset will stall forever, if we keep searching for a perfect solution without any overhead. The qdisc layer will be a baseline overhead. The limit=2 packets is actually the optimal darkbuffer queue size, but I acknowledge that this causes too many qdisc requeue events (leading to overhead). I suggest that I add another patch in V6, that defaults limit_min to 8 (separate patch to make it easier to revert/adjust later).

Quoted 8 lines

I've talked with Jonas, and we want to experiment with different solutions to make BQL/DQL work better with virtual devices.

This patchset helps our (production) use-case reduce mice-flow latency from approx 22ms to 1.3ms for latency under-load. Due to the consumer namespace being the bottleneck the requeue overhead is negligible in comparison.

-Jesper

First of all thanks for you work and I really see the advantages of avoiding bufferbloat :)

Quoted 8 lines

But the key of the BQL algorithm, which is the *dynamic* adaption of the limit, is not working. Always calling netdev_completed_queue() with 1 packet results in a static limit of 2 packets (as seen by Jonas measurements), which you force up to 8 packets.

So in the end this patchset has the same effect as just setting VETH_RING_SIZE to 8 (and giving an option to change this value).

I've code up a time based BQL implementation, see attachment. WDYT?

Quoted 2 lines

--Jesper

A step in the right direction, but I dislike that you call netdev_sent_queue() with at least 1 packet (never 0 packets). I am not sure if it works, and I am not sure about the parameter.

I would propose doing it like other BQL implementations do (for example usbnet for which I adapted BQL [1] :) ):

Call netdev_sent_queue() with n_bql in a periodic work. n_bql would still be counted in veth_xdp_rcv() like you currently do (synchronized with the work via ring.consumer_lock?).

The only weird thing that remains is that BQL's inflight != number of packets in the ring and BQL's limit != "current ring size". Instead the BQL limit describes the number of maximal allowed packets between calls of netdev_sent_queue(), which occur periodically in a somewhat fixed time interval.
I guess that could be fine, but it surely needs testing.

[1] Link: https://lore.kernel.org/netdev/[email protected]/

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I still think first that adding an option to modify the hard-coded VETH_RING_SIZE is the way to go.

Not against being able to modify VETH_RING_SIZE, but I don't think it is the solution here.

Quoted 5 lines · On Thu, 7 May 2026 21:09:09 +0200 Jesper Dangaard Brouer wrote:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I still think first that adding an option to modify the hard-coded VETH_RING_SIZE is the way to go.

Not against being able to modify VETH_RING_SIZE, but I don't think it is the solution here.

Was it evaluated, tho?

It's obviously super easy these days have AI spew no end of complex code. So it'd be great to have some solid, ideally production-like data to back this all up.

VETH_RING_SIZE seems trivial, ethtool set ringparam

Quoted 6 lines · Jakub Kicinski

Quoted 2 lines · On Thu, 7 May 2026 21:09:09 +0200 Jesper Dangaard Brouer wrote:

Not against being able to modify VETH_RING_SIZE, but I don't think it is the solution here.

Was it evaluated, tho?

Quoted 5 lines

It's obviously super easy these days have AI spew no end of complex code. So it'd be great to have some solid, ideally production-like data to back this all up.

VETH_RING_SIZE seems trivial, ethtool set ringparam

No, unfortunately we cannot just decrease the VETH_RING_SIZE.

The reason is that XDP-redirect into veth don't have any back-pressure and would simply drop packets if queue size becomes less than the NAPI budget (64). (Yes, we use both normal path and XDP-redirect in production).

My benchmarking shows that an optimal BQL limit is dynamically adjusted between 17-55 depending on veth consumer namespace overhead/speed, when balancing throughput and latency.

Quoted 11 lines · On Sat, 9 May 2026 11:09:51 +0200 Jesper Dangaard Brouer wrote:

Quoted 10 lines · Jakub Kicinski

Quoted 2 lines · On Thu, 7 May 2026 21:09:09 +0200 Jesper Dangaard Brouer wrote:

Not against being able to modify VETH_RING_SIZE, but I don't think it is the solution here.

Was it evaluated, tho?

It's obviously super easy these days have AI spew no end of complex code. So it'd be great to have some solid, ideally production-like data to back this all up.

Quoted 3 lines

Quoted 1 line

VETH_RING_SIZE seems trivial, ethtool set ringparam

No, unfortunately we cannot just decrease the VETH_RING_SIZE.

To be clear - I said may it configurable with ethtool -G not change the default.

Quoted 4 lines

The reason is that XDP-redirect into veth don't have any back-pressure and would simply drop packets if queue size becomes less than the NAPI budget (64). (Yes, we use both normal path and XDP-redirect in production).

Doesn't this mean you have a queue which is not under BQL control?

Quoted 3 lines

My benchmarking shows that an optimal BQL limit is dynamically adjusted between 17-55 depending on veth consumer namespace overhead/speed, when balancing throughput and latency.

Testing with prod-approximating traffic pattern and load would be great.

Quoted 12 lines · Jakub Kicinski

Quoted 11 lines · On Sat, 9 May 2026 11:09:51 +0200 Jesper Dangaard Brouer wrote:

Quoted 10 lines · Jakub Kicinski

Quoted 2 lines · On Thu, 7 May 2026 21:09:09 +0200 Jesper Dangaard Brouer wrote:

Not against being able to modify VETH_RING_SIZE, but I don't think it is the solution here.

Was it evaluated, tho?

It's obviously super easy these days have AI spew no end of complex code. So it'd be great to have some solid, ideally production-like data to back this all up.

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

VETH_RING_SIZE seems trivial, ethtool set ringparam

No, unfortunately we cannot just decrease the VETH_RING_SIZE.

To be clear - I said may it configurable with ethtool -G not change the default.

Sure, I understand the desire to make VETH_RING_SIZE configurable. If doing so we are making Linux network stack harder to tune and setup correctly. E.g. adding a qdisc to veth would also require changing the ring size, but if system also uses XDP then tuning below 64 (likely 128) will lead to hard-to-find packet drops.

I prefer adding something (like BQL) that auto-tune how much of the ring queue we are using. Good queues function as shock absorbers when concurrent processes in the OS have scheduling noise.

I acknowledge that Simon Schippers found that the BQL implementation was actually not auto-tuning. We need to work on this, my prototype implementation [1] [2] works surprisingly well.

- [1] https://lore.kernel.org/all/[email protected]/2-09-veth-time-based-bql-coalescing.patch
- [2] https://lore.kernel.org/all/[email protected]/

Quoted 5 lines

Quoted 4 lines

The reason is that XDP-redirect into veth don't have any back-pressure and would simply drop packets if queue size becomes less than the NAPI budget (64). (Yes, we use both normal path and XDP-redirect in production).

Quoted 2 lines

Doesn't this mean you have a queue which is not under BQL control?

It is a matter of perspective. BQL needs between 17-55 elements in the 256 queue. At the same time we handle if the ring runs full, e.g. due to a sudden burst of XDP redirected packets, which pushes packets into the qdisc layer.

Quoted 7 lines

Quoted 5 lines

My benchmarking shows that an optimal BQL limit is dynamically adjusted between 17-55 depending on veth consumer namespace overhead/speed, when balancing throughput and latency.

Testing with prod-approximating traffic pattern and load would be great.

That is what I'm doing. I'm testing with prod-approximating traffic pattern and changing the number of iptables rules to simulate the overhead I measured from production. I think I explained this in the cover letter. We are going to use this in a production environment (to be clear).

Simon found an issue testing the overload scenario.

--Jesper

Quoted 23 lines · Jesper Dangaard Brouer

Quoted 19 lines · Jakub Kicinski

Quoted 7 lines · On Sat, 9 May 2026 11:09:51 +0200 Jesper Dangaard Brouer wrote:

Quoted 6 lines · Jakub Kicinski

Quoted 2 lines · On Thu, 7 May 2026 21:09:09 +0200 Jesper Dangaard Brouer wrote:

Not against being able to modify VETH_RING_SIZE, but I don't think it is the solution here.

Was it evaluated, tho?

Quoted 7 lines

Quoted 5 lines

It's obviously super easy these days have AI spew no end of complex code. So it'd be great to have some solid, ideally production-like data to back this all up.

VETH_RING_SIZE seems trivial, ethtool set ringparam

No, unfortunately we cannot just decrease the VETH_RING_SIZE.

To be clear - I said may it configurable with ethtool -G not change the default.

Quoted 5 lines

Sure, I understand the desire to make VETH_RING_SIZE configurable. If doing so we are making Linux network stack harder to tune and setup correctly. E.g. adding a qdisc to veth would also require changing the ring size, but if system also uses XDP then tuning below 64 (likely 128) will lead to hard-to-find packet drops.

I mean 64 still could be a 4x improvement at least.

Quoted 22 lines

I prefer adding something (like BQL) that auto-tune how much of the ring queue we are using. Good queues function as shock absorbers when concurrent processes in the OS have scheduling noise.

I acknowledge that Simon Schippers found that the BQL implementation was actually not auto-tuning. We need to work on this, my prototype implementation [1] [2] works surprisingly well.

- [1] https://lore.kernel.org/all/[email protected]/2-09-veth-time-based-bql-coalescing.patch
- [2] https://lore.kernel.org/all/[email protected]/

Quoted 7 lines

Quoted 4 lines

The reason is that XDP-redirect into veth don't have any back-pressure and would simply drop packets if queue size becomes less than the NAPI budget (64). (Yes, we use both normal path and XDP-redirect in production).

Doesn't this mean you have a queue which is not under BQL control?

Quoted 4 lines

It is a matter of perspective. BQL needs between 17-55 elements in the 256 queue. At the same time we handle if the ring runs full, e.g. due to a sudden burst of XDP redirected packets, which pushes packets into the qdisc layer.

You are checking inflight/limit in /sys directory to get the 17-55 number, right?

Quoted 24 lines · Simon Schippers

Quoted 23 lines · Jesper Dangaard Brouer

Quoted 19 lines · Jakub Kicinski

Quoted 7 lines · On Sat, 9 May 2026 11:09:51 +0200 Jesper Dangaard Brouer wrote:

Quoted 6 lines · Jakub Kicinski

Quoted 2 lines · On Thu, 7 May 2026 21:09:09 +0200 Jesper Dangaard Brouer wrote:

Not against being able to modify VETH_RING_SIZE, but I don't think it is the solution here.

Was it evaluated, tho?

Quoted 7 lines

Quoted 5 lines

It's obviously super easy these days have AI spew no end of complex code. So it'd be great to have some solid, ideally production-like data to back this all up.

VETH_RING_SIZE seems trivial, ethtool set ringparam

No, unfortunately we cannot just decrease the VETH_RING_SIZE.

To be clear - I said may it configurable with ethtool -G not change the default.

Quoted 8 lines

Quoted 5 lines

Sure, I understand the desire to make VETH_RING_SIZE configurable. If doing so we are making Linux network stack harder to tune and setup correctly. E.g. adding a qdisc to veth would also require changing the ring size, but if system also uses XDP then tuning below 64 (likely 128) will lead to hard-to-find packet drops.

I mean 64 still could be a 4x improvement at least.

No not really, setting it to 64 will give same (bad) latency from "BQL off" which that patchset is trying to address.

Quoted 27 lines

Quoted 26 lines

I prefer adding something (like BQL) that auto-tune how much of the ring queue we are using. Good queues function as shock absorbers when concurrent processes in the OS have scheduling noise.

I acknowledge that Simon Schippers found that the BQL implementation was actually not auto-tuning. We need to work on this, my prototype implementation [1] [2] works surprisingly well.

- [1] https://lore.kernel.org/all/[email protected]/2-09-veth-time-based-bql-coalescing.patch
- [2] https://lore.kernel.org/all/[email protected]/

Quoted 7 lines

Quoted 4 lines

The reason is that XDP-redirect into veth don't have any back-pressure and would simply drop packets if queue size becomes less than the NAPI budget (64). (Yes, we use both normal path and XDP-redirect in production).

Doesn't this mean you have a queue which is not under BQL control?

It is a matter of perspective. BQL needs between 17-55 elements in the 256 queue. At the same time we handle if the ring runs full, e.g. due to a sudden burst of XDP redirected packets, which pushes packets into the qdisc layer.

Quoted 3 lines

You are checking inflight/limit in /sys directory to get the 17-55 number, right?

Nope, I'm using a bpftrace program to keep track of the inflight/limit in a BPF hashmap. Reading from /sys will not be accurate.

I moved the selftests into a github repo [1] to allow us to collaborate and evaluate the changes more easily. I explicitly kept the new BPF based BQL tracking as a commit[2] for your benefit.

[1] https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

[2] https://github.com/netoptimizer/veth-backpressure-performance-testing/commit/f25c5dc92977

Sorry for cutting the remaining of the message, but I ran out of time, as things are a bit challenging/hectic here at Cloudflare at the moment.

--Jesper

Quoted 66 lines · Jesper Dangaard Brouer

Quoted 24 lines · Simon Schippers

Quoted 23 lines · Jesper Dangaard Brouer

Quoted 19 lines · Jakub Kicinski

Quoted 7 lines · On Sat, 9 May 2026 11:09:51 +0200 Jesper Dangaard Brouer wrote:

Quoted 6 lines · Jakub Kicinski

Quoted 2 lines · On Thu, 7 May 2026 21:09:09 +0200 Jesper Dangaard Brouer wrote:

Not against being able to modify VETH_RING_SIZE, but I don't think it is the solution here.

Was it evaluated, tho?

Quoted 7 lines

Quoted 5 lines

It's obviously super easy these days have AI spew no end of complex code. So it'd be great to have some solid, ideally production-like data to back this all up.

VETH_RING_SIZE seems trivial, ethtool set ringparam

No, unfortunately we cannot just decrease the VETH_RING_SIZE.

To be clear - I said may it configurable with ethtool -G not change the default.

Quoted 8 lines

Quoted 5 lines

Sure, I understand the desire to make VETH_RING_SIZE configurable. If doing so we are making Linux network stack harder to tune and setup correctly. E.g. adding a qdisc to veth would also require changing the ring size, but if system also uses XDP then tuning below 64 (likely 128) will lead to hard-to-find packet drops.

I mean 64 still could be a 4x improvement at least.

No not really, setting it to 64 will give same (bad) latency from "BQL off" which that patchset is trying to address.

Quoted 27 lines

Quoted 26 lines

I prefer adding something (like BQL) that auto-tune how much of the ring queue we are using. Good queues function as shock absorbers when concurrent processes in the OS have scheduling noise.

I acknowledge that Simon Schippers found that the BQL implementation was actually not auto-tuning. We need to work on this, my prototype implementation [1] [2] works surprisingly well.

- [1] https://lore.kernel.org/all/[email protected]/2-09-veth-time-based-bql-coalescing.patch
- [2] https://lore.kernel.org/all/[email protected]/

Quoted 7 lines

Quoted 4 lines

The reason is that XDP-redirect into veth don't have any back-pressure and would simply drop packets if queue size becomes less than the NAPI budget (64). (Yes, we use both normal path and XDP-redirect in production).

Doesn't this mean you have a queue which is not under BQL control?

It is a matter of perspective. BQL needs between 17-55 elements in the 256 queue. At the same time we handle if the ring runs full, e.g. due to a sudden burst of XDP redirected packets, which pushes packets into the qdisc layer.

Quoted 6 lines

Quoted 3 lines

You are checking inflight/limit in /sys directory to get the 17-55 number, right?

Nope, I'm using a bpftrace program to keep track of the inflight/limit in a BPF hashmap. Reading from /sys will not be accurate.

Ah nice.

Quoted 8 lines

I moved the selftests into a github repo [1] to allow us to collaborate and evaluate the changes more easily. I explicitly kept the new BPF based BQL tracking as a commit[2] for your benefit.

[1] https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

[2] https://github.com/netoptimizer/veth-backpressure-performance-testing/commit/f25c5dc92977

Thanks for sharing. After minor issues I was able to set it up (currently I am just using plain v5, will look at the coalescing patch when I find the time):

Can confirm the latency reduction with the default settings, in my case 4.888ms to 0.241ms.

With the same script I was also able to see a performance slow down:
veth_bql_test_virtme.sh --qdisc fq_codel --nrules 0
--> ~510 Kpps
Same with --bql-disable
--> ~570 Kpps
--> 12% faster

Quoted 5 lines

Sorry for cutting the remaining of the message, but I ran out of time, as things are a bit challenging/hectic here at Cloudflare at the moment.

--Jesper

All good, just ignore it. I think I misunderstood something anyway.

Quoted 4 lines

Quoted 2 lines

Nope, I'm using a bpftrace program to keep track of the inflight/limit in a BPF hashmap. Reading from /sys will not be accurate.

Ah nice.

Add the option --hist to have both NAPI and BQL histograms printed when script ends. This will give you an accurate pattern of how inflight and limit evolves.

Quoted 16 lines

Quoted 8 lines

I moved the selftests into a github repo [1] to allow us to collaborate and evaluate the changes more easily. I explicitly kept the new BPF based BQL tracking as a commit[2] for your benefit.

[1] https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

[2] https://github.com/netoptimizer/veth-backpressure-performance-testing/commit/f25c5dc92977

Thanks for sharing. After minor issues I was able to set it up (currently I am just using plain v5, will look at the coalescing patch when I find the time):

Can confirm the latency reduction with the default settings, in my case 4.888ms to 0.241ms.

Quoted 7 lines

With the same script I was also able to see a performance slow down:
veth_bql_test_virtme.sh --qdisc fq_codel --nrules 0
--> ~510 Kpps
Same with --bql-disable
--> ~570 Kpps
--> 12% faster

Thanks for running these benchmarks.

Notice that --nrules 0 can easily result in no-queuing (on average), because the veth NAPI consumer is faster than the producer. You will likely see BQL inflight=1 and sink reported avg latency very low (remember it okay that sink get high latency penalty as long at ping latency remains low, as that show AQM is working).

There is an important gotcha. We actually have micro-burst of queuing (likely due to scheduling noise). Reading BQL stats from /sys will show BQL inflight=1, but when using the option --hist is it visible that @inflight have a long tail (see below signature). The "qdisc" output line also shows this happening via requeues increasing (approx 17/sec in a test with 567Kpps). (this was with the time-based BQL impl).

Quoted 9 lines

Quoted 7 lines

Sorry for cutting the remaining of the message, but I ran out of time, as things are a bit challenging/hectic here at Cloudflare at the moment.

--Jesper

All good, just ignore it. I think I misunderstood something anyway.

Okay, I'll ignore it as I couldn't make sense of it ;-) --Jesper

--- BQL inflight histogram (VETH_BQL_UNIT=1, values = packets) --- @inflight:

[0, 1)            306565 |@      |
[1, 2)           9250454 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
[2, 3)           5561919 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@      |
[3, 4)            354341 |@      |
[4, 5)             50137 |      |
[5, 6)             16771 |      |
[6, 7)              6001 |      |
[7, 8)              3076 |      |
[8, 9)              1949 |      |
[9, 10)             1965 |      |
[10, 11)            1954 |      |
[11, 12)            1914 |      |
[12, 13)            1732 |      |
[13, 14)            1559 |      |
[14, 15)            1405 |      |
[15, 16)            1269 |      |
[16, 17)            1194 |      |
[17, 18)            1190 |      |
[18, 19)            1148 |      |
[19, 20)            1079 |      |
[20, 21)            1008 |      |
[21, 22)             951 |      |
[22, 23)             870 |      |
[23, 24)             826 |      |
[24, 25)             775 |      |
[25, 26)             764 |      |
[26, 27)             740 |      |
[27, 28)             714 |      |
[28, 29)             665 |      |
[29, 30)             626 |      |
[30, 31)             607 |      |
[31, 32)             601 |      |
[32, 33)             583 |      |
[33, 34)             593 |      |
[34, 35)             574 |      |
[35, 36)             562 |      |
[36, 37)             554 |      |
[37, 38)             538 |      |
[38, 39)             528 |      |
[39, 40)             525 |      |
[40, 41)             512 |      |
[41, 42)             542 |      |
[42, 43)             529 |      |
[43, 44)             526 |      |
[44, 45)             513 |      |
[45, 46)             503 |      |
[46, 47)             485 |      |
[47, 48)             480 |      |
[48, 49)             473 |      |
[49, 50)             474 |      |
[50, 51)             476 |      |
[51, 52)             476 |      |
[52, 53)             465 |      |
[53, 54)             454 |      |
[54, 55)             446 |      |
[55, 56)             430 |      |
[56, 57)             425 |      |
[57, 58)             425 |      |
[58, 59)             422 |      |
[59, 60)             407 |      |
[60, 61)             390 |      |
[61, 62)             370 |      |
[62, 63)             354 |      |
[63, 64)             343 |      |
[64, 65)             325 |      |
[65, 66)             303 |      |
[66, 67)             158 |      |
[67, 68)             136 |      |
[68, 69)             124 |      |
[69, 70)             110 |      |
[70, 71)              99 |      |
[71, 72)              94 |      |
[72, 73)              82 |      |
[73, 74)              74 |      |
[74, 75)              58 |      |
[75, 76)              52 |      |
[76, 77)              45 |      |
[77, 78)              40 |      |
[78, 79)              39 |      |
[79, 80)              38 |      |
[80, 81)              21 |      |
[81, 82)               4 |      |
[82, 83)               4 |      |
[83, 84)               4 |      |
[84, 85)               2 |      |
[85, 86)               2 |      |
[86, 87)               2 |      |
[87, 88)               2 |      |
[88, 89)               1 |      |

--- BQL limit histogram (auto-tuned, values = packets) --- @limit_val:

[61, 62)          221346 |@      |
[62, 63)               0 |      |
[63, 64)          772169 |@@@      |
[64, 65)        10053949 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
[65, 66)               0 |      |
[66, 67)               0 |      |
[67, 68)               0 |      |
[68, 69)               0 |      |
[69, 70)               0 |      |
[70, 71)          457838 |@@      |
[71, 72)               0 |      |
[72, 73)          610198 |@@@      |
[73, 74)               0 |      |
[74, 75)               0 |      |
[75, 76)               0 |      |
[76, 77)               0 |      |
[77, 78)               0 |      |
[78, 79)         2328284 |@@@@@@@@@@@@      |
[79, 80)         1150181 |@@@@@      |

@inflight_stats: count 15593965, average 1, total 23078061

@limit_stats: count 15593965, average 67, total 1054054856

Quoted 33 lines · Jesper Dangaard Brouer

Quoted 4 lines

Quoted 2 lines

Nope, I'm using a bpftrace program to keep track of the inflight/limit in a BPF hashmap. Reading from /sys will not be accurate.

Ah nice.

Add the option --hist to have both NAPI and BQL histograms printed when script ends. This will give you an accurate pattern of how inflight and limit evolves.

Quoted 16 lines

Quoted 8 lines

I moved the selftests into a github repo [1] to allow us to collaborate and evaluate the changes more easily. I explicitly kept the new BPF based BQL tracking as a commit[2] for your benefit.

[1] https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

[2] https://github.com/netoptimizer/veth-backpressure-performance-testing/commit/f25c5dc92977

Thanks for sharing. After minor issues I was able to set it up (currently I am just using plain v5, will look at the coalescing patch when I find the time):

Can confirm the latency reduction with the default settings, in my case 4.888ms to 0.241ms.

Quoted 7 lines

With the same script I was also able to see a performance slow down:
veth_bql_test_virtme.sh --qdisc fq_codel --nrules 0
--> ~510 Kpps
Same with --bql-disable
--> ~570 Kpps
--> 12% faster

Quoted 7 lines

Thanks for running these benchmarks.

Notice that --nrules 0 can easily result in no-queuing (on average), because the veth NAPI consumer is faster than the producer. You will likely see BQL inflight=1 and sink reported avg latency very low (remember it okay that sink get high latency penalty as long at ping latency remains low, as that show AQM is working).

I ran the benchmarks with --hist and I see what you mean. I have very similar results.

Is Jonas way [1] of modifiying pktgen maybe the best option to ensure that the producer is faster than the consumer?

[1] Link: https://lore.kernel.org/netdev/[email protected]/

Review

Quoted 7 lines

Hi, so what I found is that pktgen does not respect __QUEUE_STATE_STACK_OFF. So the test data above is invalid, since it just sent packets even if the BQL "stopped" the queue. So I patched pktgen with the following:

(quoted diff) +1 -1

-       if (unlikely(netif_xmit_frozen_or_drv_stopped(txq))) {
+       if (unlikely(netif_xmit_frozen_or_stopped(txq))) {

After thinking more about the implementation I see possible issues:

1. netdev_tx_completed_queue() never reports more than burst=64 packets:

BQL only increments the limit if the queue was starved. That means:
"The queue was over-limit in the last interval (the last time completion processing ran), and there is no more data in the queue (i.e. it’s empty)" [2]
But as only 64 packets are reported at max, the queue can only grow when it is <= 64 packets. And then it can only stay at a limit >64 until the next decrease of the limit.

2. netdev_tx_completed_queue() is called in irregular intervals:

If the consumer is slow it is called approx each tx_coal_usecs. But if the consumer is fast it is called way more frequent, probably in irregular intervals depending on the scheduling. However, "BQL depends on periodic completion interrupts" [2].

--> How about adding something like an interrupt that triggers every
10us and calls netdev_tx_completed_queue() with n_bql collected from (multiple) veth_xdp_rcv runs? That could solve 1. and 2.

[2] Link: https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83

Quoted 1 line

Quoted 6 lines

There is an important gotcha. We actually have micro-burst of queuing (likely due to scheduling noise). Reading BQL stats from /sys will show BQL inflight=1, but when using the option --hist is it visible that @inflight have a long tail (see below signature). The "qdisc" output line also shows this happening via requeues increasing (approx 17/sec in a test with 567Kpps). (this was with the time-based BQL impl).

I understand..

Quoted 67 lines · Simon Schippers

Quoted 28 lines · Jesper Dangaard Brouer

Quoted 3 lines

Quoted 2 lines

Nope, I'm using a bpftrace program to keep track of the inflight/limit in a BPF hashmap. Reading from /sys will not be accurate.

Ah nice.

Add the option --hist to have both NAPI and BQL histograms printed when script ends. This will give you an accurate pattern of how inflight and limit evolves.

Quoted 14 lines

Quoted 7 lines

I moved the selftests into a github repo [1] to allow us to collaborate and evaluate the changes more easily. I explicitly kept the new BPF based BQL tracking as a commit[2] for your benefit.

[1] https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

[2] https://github.com/netoptimizer/veth-backpressure-performance-testing/commit/f25c5dc92977

Thanks for sharing. After minor issues I was able to set it up (currently I am just using plain v5, will look at the coalescing patch when I find the time):

Can confirm the latency reduction with the default settings, in my case 4.888ms to 0.241ms.

Quoted 7 lines

With the same script I was also able to see a performance slow down:
veth_bql_test_virtme.sh --qdisc fq_codel --nrules 0
--> ~510 Kpps
Same with --bql-disable
--> ~570 Kpps
--> 12% faster

Quoted 7 lines

Thanks for running these benchmarks.

Notice that --nrules 0 can easily result in no-queuing (on average), because the veth NAPI consumer is faster than the producer. You will likely see BQL inflight=1 and sink reported avg latency very low (remember it okay that sink get high latency penalty as long at ping latency remains low, as that show AQM is working).

I ran the benchmarks with --hist and I see what you mean. I have very similar results.

Is Jonas way [1] of modifiying pktgen maybe the best option to ensure that the producer is faster than the consumer?

[1] Link: https://lore.kernel.org/netdev/[email protected]/

Review

Quoted 7 lines

Hi, so what I found is that pktgen does not respect __QUEUE_STATE_STACK_OFF. So the test data above is invalid, since it just sent packets even if the BQL "stopped" the queue. So I patched pktgen with the following:

(quoted diff) +1 -1

-       if (unlikely(netif_xmit_frozen_or_drv_stopped(txq))) {
+       if (unlikely(netif_xmit_frozen_or_stopped(txq))) {

After thinking more about the implementation I see possible issues:

1. netdev_tx_completed_queue() never reports more than burst=64 packets:

BQL only increments the limit if the queue was starved. That means:
"The queue was over-limit in the last interval (the last time completion processing ran), and there is no more data in the queue (i.e. it’s empty)" [2]
But as only 64 packets are reported at max, the queue can only grow when it is <= 64 packets. And then it can only stay at a limit >64 until the next decrease of the limit.

2. netdev_tx_completed_queue() is called in irregular intervals:

Quoted 8 lines

If the consumer is slow it is called approx each tx_coal_usecs. But if the consumer is fast it is called way more frequent, probably in irregular intervals depending on the scheduling. However, "BQL depends on periodic completion interrupts" [2].

--> How about adding something like an interrupt that triggers every
10us and calls netdev_tx_completed_queue() with n_bql collected from (multiple) veth_xdp_rcv runs? That could solve 1. and 2.

Hi,

I worked on a new version (see attachment) that addresses both issues.

The major change is that instead of tracking the timestamp and packet count as local variables in veth_xdp_rcv(), they are now stored persistently in veth_rq as struct veth_bql_state. This allows completions to accumulate across multiple NAPI poll calls, so netdev_tx_completed_queue() can report more than 64 packets at once (see point 1). To get the time I am using (the fast) sched_clock() with a trick to avoid issues when switching between CPUs.

For point 2, the coalescing deadline is now checked both before the receive loop (to flush completions that timed out since the previous poll) and after each consumed packet, making completion intervals more regular. Still the intervals can be smaller than VETH_BQL_COAL_TX_USECS, but I guess this is fine.

I also found out that the BQL limit correlates closely with VETH_BQL_COAL_TX_USECS. It essentially reflects the latency we are targeting. I raised the default to 100 µs to allow DQL to converge to a higher limit (for reaching 255 in the testing below).

With the patched pktgen (respecting __QUEUE_STATE_STACK_XOFF), testing shows:
- --nrules 0: DQL limit reaches (up to) ~255
- --nrules 10000: DQL limit converges to ~0 (with --gro-disable)

These results are what I would expect from a BQL algorithm, but more testing is needed of course.

What do you think?

Thanks!

BTW: I think that this implementation could also work for other
software interfaces.

Quoted 10 lines

[2] Link: https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83

Quoted 6 lines

There is an important gotcha. We actually have micro-burst of queuing (likely due to scheduling noise). Reading BQL stats from /sys will show BQL inflight=1, but when using the option --hist is it visible that @inflight have a long tail (see below signature). The "qdisc" output line also shows this happening via requeues increasing (approx 17/sec in a test with 567Kpps). (this was with the time-based BQL impl).

I understand..

Quoted 104 lines · Simon Schippers

Quoted 67 lines · Simon Schippers

Quoted 28 lines · Jesper Dangaard Brouer

Quoted 3 lines

Quoted 2 lines

Nope, I'm using a bpftrace program to keep track of the inflight/limit in a BPF hashmap. Reading from /sys will not be accurate.

Ah nice.

Add the option --hist to have both NAPI and BQL histograms printed when script ends. This will give you an accurate pattern of how inflight and limit evolves.

Quoted 14 lines

Quoted 7 lines

I moved the selftests into a github repo [1] to allow us to collaborate and evaluate the changes more easily. I explicitly kept the new BPF based BQL tracking as a commit[2] for your benefit.

[1]https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

[2]https://github.com/netoptimizer/veth-backpressure-performance-testing/commit/f25c5dc92977

Thanks for sharing. After minor issues I was able to set it up (currently I am just using plain v5, will look at the coalescing patch when I find the time):

Can confirm the latency reduction with the default settings, in my case 4.888ms to 0.241ms.

Quoted 7 lines

With the same script I was also able to see a performance slow down:
veth_bql_test_virtme.sh --qdisc fq_codel --nrules 0
--> ~510 Kpps
Same with --bql-disable
--> ~570 Kpps
--> 12% faster

Quoted 7 lines

Thanks for running these benchmarks.

Notice that --nrules 0 can easily result in no-queuing (on average), because the veth NAPI consumer is faster than the producer. You will likely see BQL inflight=1 and sink reported avg latency very low (remember it okay that sink get high latency penalty as long at ping latency remains low, as that show AQM is working).

I ran the benchmarks with --hist and I see what you mean. I have very similar results.

Is Jonas way [1] of modifiying pktgen maybe the best option to ensure that the producer is faster than the consumer?

[1] Link:https://lore.kernel.org/netdev/[email protected]/

Review

Quoted 7 lines

Hi, so what I found is that pktgen does not respect __QUEUE_STATE_STACK_OFF. So the test data above is invalid, since it just sent packets even if the BQL "stopped" the queue. So I patched pktgen with the following:

(quoted diff) +1 -1

-       if (unlikely(netif_xmit_frozen_or_drv_stopped(txq))) {
+       if (unlikely(netif_xmit_frozen_or_stopped(txq))) {

After thinking more about the implementation I see possible issues:

1. netdev_tx_completed_queue() never reports more than burst=64 packets:

BQL only increments the limit if the queue was starved. That means:
"The queue was over-limit in the last interval (the last time completion processing ran), and there is no more data in the queue (i.e. it’s empty)" [2]
But as only 64 packets are reported at max, the queue can only grow when it is <= 64 packets. And then it can only stay at a limit >64 until the next decrease of the limit.

2. netdev_tx_completed_queue() is called in irregular intervals:

Quoted 8 lines

If the consumer is slow it is called approx each tx_coal_usecs. But if the consumer is fast it is called way more frequent, probably in irregular intervals depending on the scheduling. However, "BQL depends on periodic completion interrupts" [2].

--> How about adding something like an interrupt that triggers every
10us and calls netdev_tx_completed_queue() with n_bql collected from (multiple) veth_xdp_rcv runs? That could solve 1. and 2.

Hi,

I worked on a new version (see attachment) that addresses both issues.

The major change is that instead of tracking the timestamp and packet count as local variables in veth_xdp_rcv(), they are now stored persistently in veth_rq as struct veth_bql_state. This allows completions to accumulate across multiple NAPI poll calls, so netdev_tx_completed_queue() can report more than 64 packets at once (see point 1). To get the time I am using (the fast) sched_clock() with a trick to avoid issues when switching between CPUs.

For point 2, the coalescing deadline is now checked both before the receive loop (to flush completions that timed out since the previous poll) and after each consumed packet, making completion intervals more regular. Still the intervals can be smaller than VETH_BQL_COAL_TX_USECS, but I guess this is fine.

I also found out that the BQL limit correlates closely with VETH_BQL_COAL_TX_USECS. It essentially reflects the latency we are targeting. I raised the default to 100 µs to allow DQL to converge to a higher limit (for reaching 255 in the testing below).

With the patched pktgen (respecting __QUEUE_STATE_STACK_XOFF), testing shows:
- --nrules 0: DQL limit reaches (up to) ~255
- --nrules 10000: DQL limit converges to ~0 (with --gro-disable)

Quoted 4 lines

These results are what I would expect from a BQL algorithm, but more testing is needed of course.

What do you think?

Hi,

This is exactly what I had in mind for implementing the BQL algorithm in this case. I did some testing with pktgen of this patch and also compared it to the v5 version.

You can find an extension of the benchmark script with pktgen here [1], as well as a wrapper script (veth_bql_bench.sh) to run the test script with and without --bql-disable to report the difference. I also configured pktgen to use the qdisc as suggested by Jesper.

Note: bpftrace needs to be disabled, otherwise it becomes the bottleneck (at least on my machine) and pktgen throughput is halved when enabled.

Here are the results:

v5 (not time-based):
--nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================

                       BQL on    BQL off
---                    ------    -------
Throughput (pps)      1980871    2169898

Ping RTT avg (ms) 0.065 0.162
Throughput diff -8.7% // BQL 8.7% lower throughput
RTT diff -59.9% // BQL 60% lower latency
========================================

Simon's time-based version:

Test args: --nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================

                           BQL on      BQL off
---                        ------      -------
Throughput (pps)          2166335      2153398

Ping RTT avg (ms) 0.165 0.165
Throughput diff 0.6%
RTT diff 0.0%

--pktgen --no-bpftrace --nrules 3500
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------

Throughput (pps)            28569        28696
Ping RTT avg (ms)           1.327        8.409
Throughput diff             -0.4%

RTT diff -84.2%

Seems to work now as expected.

[1]https://github.com/jkoeppeler/veth-backpressure-performance-testing/tree/pktgen-and-benchmark

Thanks,
Jonas

Quoted 15 lines

Thanks!

BTW: I think that this implementation could also work for other
software interfaces.

Quoted 10 lines

[2] Link:https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83

Quoted 6 lines

There is an important gotcha. We actually have micro-burst of queuing (likely due to scheduling noise). Reading BQL stats from /sys will show BQL inflight=1, but when using the option --hist is it visible that @inflight have a long tail (see below signature). The "qdisc" output line also shows this happening via requeues increasing (approx 17/sec in a test with 567Kpps). (this was with the time-based BQL impl).

I understand..

Quoted 114 lines · Jonas Köppeler

Quoted 103 lines · Simon Schippers

Quoted 66 lines · Simon Schippers

Quoted 28 lines · Jesper Dangaard Brouer

Quoted 3 lines

Quoted 2 lines

Nope, I'm using a bpftrace program to keep track of the inflight/limit in a BPF hashmap. Reading from /sys will not be accurate.

Ah nice.

Add the option --hist to have both NAPI and BQL histograms printed when script ends. This will give you an accurate pattern of how inflight and limit evolves.

Quoted 14 lines

Quoted 7 lines

I moved the selftests into a github repo [1] to allow us to collaborate and evaluate the changes more easily. I explicitly kept the new BPF based BQL tracking as a commit[2] for your benefit.

[1]https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

[2]https://github.com/netoptimizer/veth-backpressure-performance-testing/commit/f25c5dc92977

Thanks for sharing. After minor issues I was able to set it up (currently I am just using plain v5, will look at the coalescing patch when I find the time):

Can confirm the latency reduction with the default settings, in my case 4.888ms to 0.241ms.

Quoted 7 lines

With the same script I was also able to see a performance slow down:
veth_bql_test_virtme.sh --qdisc fq_codel --nrules 0
--> ~510 Kpps
Same with --bql-disable
--> ~570 Kpps
--> 12% faster

Quoted 7 lines

Thanks for running these benchmarks.

Notice that --nrules 0 can easily result in no-queuing (on average), because the veth NAPI consumer is faster than the producer. You will likely see BQL inflight=1 and sink reported avg latency very low (remember it okay that sink get high latency penalty as long at ping latency remains low, as that show AQM is working).

I ran the benchmarks with --hist and I see what you mean. I have very similar results.

Is Jonas way [1] of modifiying pktgen maybe the best option to ensure that the producer is faster than the consumer?

[1] Link:https://lore.kernel.org/netdev/[email protected]/

Review

Quoted 7 lines

Hi, so what I found is that pktgen does not respect __QUEUE_STATE_STACK_OFF. So the test data above is invalid, since it just sent packets even if the BQL "stopped" the queue. So I patched pktgen with the following:

(quoted diff) +1 -1

-       if (unlikely(netif_xmit_frozen_or_drv_stopped(txq))) {
+       if (unlikely(netif_xmit_frozen_or_stopped(txq))) {

After thinking more about the implementation I see possible issues:

1. netdev_tx_completed_queue() never reports more than burst=64 packets:

BQL only increments the limit if the queue was starved. That means:
"The queue was over-limit in the last interval (the last time completion processing ran), and there is no more data in the queue (i.e. it’s empty)" [2]
But as only 64 packets are reported at max, the queue can only grow when it is <= 64 packets. And then it can only stay at a limit >64 until the next decrease of the limit.

2. netdev_tx_completed_queue() is called in irregular intervals:

Quoted 8 lines

If the consumer is slow it is called approx each tx_coal_usecs. But if the consumer is fast it is called way more frequent, probably in irregular intervals depending on the scheduling. However, "BQL depends on periodic completion interrupts" [2].

--> How about adding something like an interrupt that triggers every
10us and calls netdev_tx_completed_queue() with n_bql collected from (multiple) veth_xdp_rcv runs? That could solve 1. and 2.

Hi,

I worked on a new version (see attachment) that addresses both issues.

The major change is that instead of tracking the timestamp and packet count as local variables in veth_xdp_rcv(), they are now stored persistently in veth_rq as struct veth_bql_state. This allows completions to accumulate across multiple NAPI poll calls, so netdev_tx_completed_queue() can report more than 64 packets at once (see point 1). To get the time I am using (the fast) sched_clock() with a trick to avoid issues when switching between CPUs.

For point 2, the coalescing deadline is now checked both before the receive loop (to flush completions that timed out since the previous poll) and after each consumed packet, making completion intervals more regular. Still the intervals can be smaller than VETH_BQL_COAL_TX_USECS, but I guess this is fine.

I also found out that the BQL limit correlates closely with VETH_BQL_COAL_TX_USECS. It essentially reflects the latency we are targeting. I raised the default to 100 µs to allow DQL to converge to a higher limit (for reaching 255 in the testing below).

With the patched pktgen (respecting __QUEUE_STATE_STACK_XOFF), testing shows:
- --nrules 0: DQL limit reaches (up to) ~255
- --nrules 10000: DQL limit converges to ~0 (with --gro-disable)

Quoted 4 lines

These results are what I would expect from a BQL algorithm, but more testing is needed of course.

What do you think?

Hi,

This is exactly what I had in mind for implementing the BQL algorithm in this case. I did some testing with pktgen of this patch and also compared it to the v5 version.

Quoted 4 lines

You can find an extension of the benchmark script with pktgen here [1], as well as a wrapper script (veth_bql_bench.sh) to run the test script with and without --bql-disable to report the difference. I also configured pktgen to use the qdisc as suggested by Jesper.

Great, I will use your pktgen solution from now on.

Didn't know about the qdisc option, is there a performance difference with/without it? Or is it to have ping working next to pktgen?

Consider to do a pull request :)

Quoted 3 lines

Note: bpftrace needs to be disabled, otherwise it becomes the bottleneck (at least on my machine) and pktgen throughput is halved when enabled.

Good to know.

Quoted 34 lines

Here are the results:

v5 (not time-based):
--nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------
Throughput (pps) 1980871 2169898
Ping RTT avg (ms) 0.065 0.162
Throughput diff -8.7% // BQL 8.7% lower throughput
RTT diff -59.9% // BQL 60% lower latency
========================================

Simon's time-based version:

Test args: --nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------
Throughput (pps) 2166335 2153398
Ping RTT avg (ms) 0.165 0.165
Throughput diff 0.6%
RTT diff 0.0%

--pktgen --no-bpftrace --nrules 3500
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------

Quoted 5 lines

Throughput (pps)            28569        28696
Ping RTT avg (ms)           1.327        8.409
Throughput diff             -0.4%

RTT diff -84.2%

I think we should run benchmarks against the stock net-next to be safe.

Quoted 1 line

Seems to work now as expected.

Yes, but I think we have to keep these points in mind:

1. Limit/Inflight can be bigger than VETH_RING_SIZE, because packets can be enqueued in the same time as they are read out, so netdev_tx_completed_queue() can theoretically be called with many number of packets.
I do not think it is deal-breaking though.
I could see such high limits/inflights when looking at the /sys BQL statistics..

2. sched_clock() is only valid on the same CPU. When a different CPU starts executing its sched_clock() can be in the past compared to the sched_clock() value saved by the previous CPU. My trick...

min(s->time, sched_clock())
... avoids potentially extremely long intervals between netdev_tx_completed_queue() calls but is not perfect of course. I think CPU hopping happens rarely enough for this to matter.. And also we have to keep this in mind [1]:
"An architecture may or may not provide an implementation of sched_clock() on its own. If a local implementation is not provided, the system jiffy counter will be used as sched_clock()."

3. Inflight can be stuck at a value>0 for a long time when packet enqueueing stops. Only when packets are enqueued again, (on the next veth_xdp_rcv() call,) netdev_tx_completed_queue() is executed and inflight is set to 0 again.
Can also be seen when looking at the /sys BQL statistics.

BTW: Yesterday, I worked on and refactored the code into its own .h
file as a library and it also works fine for TUN/TAP (+vhost-net) for me :)

Thanks for your work!
Simon

[1] Link: https://docs.kernel.org/timers/timekeeping.html

Quoted 20 lines

[1]https://github.com/jkoeppeler/veth-backpressure-performance-testing/tree/pktgen-and-benchmark

Thanks,
Jonas

Quoted 15 lines

Thanks!

BTW: I think that this implementation could also work for other
software interfaces.

Quoted 10 lines

[2] Link:https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83

Quoted 6 lines

There is an important gotcha. We actually have micro-burst of queuing (likely due to scheduling noise). Reading BQL stats from /sys will show BQL inflight=1, but when using the option --hist is it visible that @inflight have a long tail (see below signature). The "qdisc" output line also shows this happening via requeues increasing (approx 17/sec in a test with 567Kpps). (this was with the time-based BQL impl).

I understand..

Quoted 115 lines · Simon Schippers

Quoted 114 lines · Jonas Köppeler

Quoted 103 lines · Simon Schippers

Quoted 66 lines · Simon Schippers

Quoted 28 lines · Jesper Dangaard Brouer

Quoted 3 lines

Quoted 2 lines

Nope, I'm using a bpftrace program to keep track of the inflight/limit in a BPF hashmap. Reading from /sys will not be accurate.

Ah nice.

Add the option --hist to have both NAPI and BQL histograms printed when script ends. This will give you an accurate pattern of how inflight and limit evolves.

Quoted 14 lines

Quoted 7 lines

I moved the selftests into a github repo [1] to allow us to collaborate and evaluate the changes more easily. I explicitly kept the new BPF based BQL tracking as a commit[2] for your benefit.

[1]https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

[2]https://github.com/netoptimizer/veth-backpressure-performance-testing/commit/f25c5dc92977

Thanks for sharing. After minor issues I was able to set it up (currently I am just using plain v5, will look at the coalescing patch when I find the time):

Can confirm the latency reduction with the default settings, in my case 4.888ms to 0.241ms.

Quoted 7 lines

With the same script I was also able to see a performance slow down:
veth_bql_test_virtme.sh --qdisc fq_codel --nrules 0
--> ~510 Kpps
Same with --bql-disable
--> ~570 Kpps
--> 12% faster

Quoted 7 lines

Thanks for running these benchmarks.

Notice that --nrules 0 can easily result in no-queuing (on average), because the veth NAPI consumer is faster than the producer. You will likely see BQL inflight=1 and sink reported avg latency very low (remember it okay that sink get high latency penalty as long at ping latency remains low, as that show AQM is working).

I ran the benchmarks with --hist and I see what you mean. I have very similar results.

Is Jonas way [1] of modifiying pktgen maybe the best option to ensure that the producer is faster than the consumer?

[1] Link:https://lore.kernel.org/netdev/[email protected]/

Review

Quoted 7 lines

Hi, so what I found is that pktgen does not respect __QUEUE_STATE_STACK_OFF. So the test data above is invalid, since it just sent packets even if the BQL "stopped" the queue. So I patched pktgen with the following:

(quoted diff) +1 -1

-       if (unlikely(netif_xmit_frozen_or_drv_stopped(txq))) {
+       if (unlikely(netif_xmit_frozen_or_stopped(txq))) {

After thinking more about the implementation I see possible issues:

1. netdev_tx_completed_queue() never reports more than burst=64 packets:

BQL only increments the limit if the queue was starved. That means:
"The queue was over-limit in the last interval (the last time completion processing ran), and there is no more data in the queue (i.e. it’s empty)" [2]
But as only 64 packets are reported at max, the queue can only grow when it is <= 64 packets. And then it can only stay at a limit >64 until the next decrease of the limit.

2. netdev_tx_completed_queue() is called in irregular intervals:

Quoted 8 lines

If the consumer is slow it is called approx each tx_coal_usecs. But if the consumer is fast it is called way more frequent, probably in irregular intervals depending on the scheduling. However, "BQL depends on periodic completion interrupts" [2].

--> How about adding something like an interrupt that triggers every
10us and calls netdev_tx_completed_queue() with n_bql collected from (multiple) veth_xdp_rcv runs? That could solve 1. and 2.

Hi,

I worked on a new version (see attachment) that addresses both issues.

The major change is that instead of tracking the timestamp and packet count as local variables in veth_xdp_rcv(), they are now stored persistently in veth_rq as struct veth_bql_state. This allows completions to accumulate across multiple NAPI poll calls, so netdev_tx_completed_queue() can report more than 64 packets at once (see point 1). To get the time I am using (the fast) sched_clock() with a trick to avoid issues when switching between CPUs.

For point 2, the coalescing deadline is now checked both before the receive loop (to flush completions that timed out since the previous poll) and after each consumed packet, making completion intervals more regular. Still the intervals can be smaller than VETH_BQL_COAL_TX_USECS, but I guess this is fine.

I also found out that the BQL limit correlates closely with VETH_BQL_COAL_TX_USECS. It essentially reflects the latency we are targeting. I raised the default to 100 µs to allow DQL to converge to a higher limit (for reaching 255 in the testing below).

With the patched pktgen (respecting __QUEUE_STATE_STACK_XOFF), testing shows:
- --nrules 0: DQL limit reaches (up to) ~255
- --nrules 10000: DQL limit converges to ~0 (with --gro-disable)

Quoted 4 lines

These results are what I would expect from a BQL algorithm, but more testing is needed of course.

What do you think?

Hi,

This is exactly what I had in mind for implementing the BQL algorithm in this case. I did some testing with pktgen of this patch and also compared it to the v5 version.

Quoted 8 lines

Quoted 4 lines

You can find an extension of the benchmark script with pktgen here [1], as well as a wrapper script (veth_bql_bench.sh) to run the test script with and without --bql-disable to report the difference. I also configured pktgen to use the qdisc as suggested by Jesper.

Great, I will use your pktgen solution from now on.

Didn't know about the qdisc option, is there a performance difference with/without it? Or is it to have ping working next to pktgen?

Did not see any big difference performance-wise, but as you say, ping works better with pktgen then.

Quoted 46 lines

Consider to do a pull request :)

Quoted 3 lines

Note: bpftrace needs to be disabled, otherwise it becomes the bottleneck (at least on my machine) and pktgen throughput is halved when enabled.

Good to know.

Quoted 34 lines

Here are the results:

v5 (not time-based):
--nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------
Throughput (pps) 1980871 2169898
Ping RTT avg (ms) 0.065 0.162
Throughput diff -8.7% // BQL 8.7% lower throughput
RTT diff -59.9% // BQL 60% lower latency
========================================

Simon's time-based version:

Test args: --nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------
Throughput (pps) 2166335 2153398
Ping RTT avg (ms) 0.165 0.165
Throughput diff 0.6%
RTT diff 0.0%

--pktgen --no-bpftrace --nrules 3500
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------

Quoted 5 lines

Throughput (pps)            28569        28696
Ping RTT avg (ms)           1.327        8.409
Throughput diff             -0.4%

RTT diff -84.2%

Quoted 2 lines

I think we should run benchmarks against the stock net-next to be safe.

--nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
net-next

---                  -------
Throughput (pps)     2285421
Ping RTT avg (ms)    0.161

(slightly adjusted the output to better communicate the results)

So in my case this means BQL implementation has ~5% lower throughput compared to net-next. But please double check.

Quoted 10 lines

Quoted 1 line

Seems to work now as expected.

Yes, but I think we have to keep these points in mind:

1. Limit/Inflight can be bigger than VETH_RING_SIZE, because packets can be enqueued in the same time as they are read out, so netdev_tx_completed_queue() can theoretically be called with many number of packets.
I do not think it is deal-breaking though.
I could see such high limits/inflights when looking at the /sys BQL statistics..

For me this makes sense, that inflight just means the number of packets not yet 'completed' or the number of packets that you can send between two completion calls. I think this is not specific

to this implementation. But for long intervals this might result in some problems because you can just fill the veth_ring to its capacity quickly, and increasing latency if the receiver is slow.

To illustrate this have a look at [1]. There are some plots that show the rtt vs. tx-usec config depending on nrules.

[1] https://github.com/jkoeppeler/veth-backpressure-performance-testing/tree/pktgen-and-benchmark/results/tx-usecs

Quoted 4 lines

2. sched_clock() is only valid on the same CPU. When a different CPU starts executing its sched_clock() can be in the past compared to the sched_clock() value saved by the previous CPU. My trick...

Quoted 8 lines

min(s->time, sched_clock())
... avoids potentially extremely long intervals between netdev_tx_completed_queue() calls but is not perfect of course. I think CPU hopping happens rarely enough for this to matter.. And also we have to keep this in mind [1]:
"An architecture may or may not provide an implementation of sched_clock() on its own. If a local implementation is not provided, the system jiffy counter will be used as sched_clock()."

So the problem with this is that with jiffies you have like millisecond interval granularity, which might be too long in order to work properly. Given the receiver completes 128 packet in 1ms (queue_completed interval), the bql will set the limit at 256. Then the tx thread can quickly fill the ring, and it then basically stopped until the 1ms interval is over.

I don't know how much this matters in practice. Not sure which architectures still hit the jiffies fallback.When in doubt we could disable BQL for those, or pin it to a "virtual queue size" via limit_min / limit_max similar to the v5 behavior and call queue_completed for every packet. Wdyt?

Quoted 6 lines

3. Inflight can be stuck at a value>0 for a long time when packet enqueueing stops. Only when packets are enqueued again, (on the next veth_xdp_rcv() call,) netdev_tx_completed_queue() is executed and inflight is set to 0 again.
Can also be seen when looking at the /sys BQL statistics.

This happens if the ring empties before the next queue_completed fires, right?

Another thing: Is it counterintuitive to set the tx_usec_coal on the receive device? Because this is a bql related config that is normally configured on the TX side?

Quoted 4 lines

BTW: Yesterday, I worked on and refactored the code into its own .h
file as a library and it also works fine for TUN/TAP (+vhost-net) for me :)

Nice! Thank you.
Jonas

Quoted 26 lines

Thanks for your work!
Simon

[1] Link: https://docs.kernel.org/timers/timekeeping.html

Quoted 20 lines

[1]https://github.com/jkoeppeler/veth-backpressure-performance-testing/tree/pktgen-and-benchmark

Thanks,
Jonas

Quoted 15 lines

Thanks!

BTW: I think that this implementation could also work for other
software interfaces.

Quoted 10 lines

[2] Link:https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83

Quoted 6 lines

There is an important gotcha. We actually have micro-burst of queuing (likely due to scheduling noise). Reading BQL stats from /sys will show BQL inflight=1, but when using the option --hist is it visible that @inflight have a long tail (see below signature). The "qdisc" output line also shows this happening via requeues increasing (approx 17/sec in a test with 567Kpps). (this was with the time-based BQL impl).

I understand..

Quoted 180 lines · Jonas Köppeler

Quoted 115 lines · Simon Schippers

Quoted 114 lines · Jonas Köppeler

Quoted 103 lines · Simon Schippers

Quoted 66 lines · Simon Schippers

Quoted 28 lines · Jesper Dangaard Brouer

Quoted 3 lines

Quoted 2 lines

Nope, I'm using a bpftrace program to keep track of the inflight/limit in a BPF hashmap. Reading from /sys will not be accurate.

Ah nice.

Add the option --hist to have both NAPI and BQL histograms printed when script ends. This will give you an accurate pattern of how inflight and limit evolves.

Quoted 14 lines

Quoted 7 lines

I moved the selftests into a github repo [1] to allow us to collaborate and evaluate the changes more easily. I explicitly kept the new BPF based BQL tracking as a commit[2] for your benefit.

[1]https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

[2]https://github.com/netoptimizer/veth-backpressure-performance-testing/commit/f25c5dc92977

Thanks for sharing. After minor issues I was able to set it up (currently I am just using plain v5, will look at the coalescing patch when I find the time):

Can confirm the latency reduction with the default settings, in my case 4.888ms to 0.241ms.

Quoted 7 lines

With the same script I was also able to see a performance slow down:
veth_bql_test_virtme.sh --qdisc fq_codel --nrules 0
--> ~510 Kpps
Same with --bql-disable
--> ~570 Kpps
--> 12% faster

Quoted 7 lines

Thanks for running these benchmarks.

Notice that --nrules 0 can easily result in no-queuing (on average), because the veth NAPI consumer is faster than the producer. You will likely see BQL inflight=1 and sink reported avg latency very low (remember it okay that sink get high latency penalty as long at ping latency remains low, as that show AQM is working).

I ran the benchmarks with --hist and I see what you mean. I have very similar results.

Is Jonas way [1] of modifiying pktgen maybe the best option to ensure that the producer is faster than the consumer?

[1] Link:https://lore.kernel.org/netdev/[email protected]/

Review

Quoted 7 lines

Hi, so what I found is that pktgen does not respect __QUEUE_STATE_STACK_OFF. So the test data above is invalid, since it just sent packets even if the BQL "stopped" the queue. So I patched pktgen with the following:

(quoted diff) +1 -1

-       if (unlikely(netif_xmit_frozen_or_drv_stopped(txq))) {
+       if (unlikely(netif_xmit_frozen_or_stopped(txq))) {

After thinking more about the implementation I see possible issues:

1. netdev_tx_completed_queue() never reports more than burst=64 packets:

BQL only increments the limit if the queue was starved. That means:
"The queue was over-limit in the last interval (the last time completion processing ran), and there is no more data in the queue (i.e. it’s empty)" [2]
But as only 64 packets are reported at max, the queue can only grow when it is <= 64 packets. And then it can only stay at a limit >64 until the next decrease of the limit.

2. netdev_tx_completed_queue() is called in irregular intervals:

Quoted 8 lines

If the consumer is slow it is called approx each tx_coal_usecs. But if the consumer is fast it is called way more frequent, probably in irregular intervals depending on the scheduling. However, "BQL depends on periodic completion interrupts" [2].

--> How about adding something like an interrupt that triggers every
10us and calls netdev_tx_completed_queue() with n_bql collected from (multiple) veth_xdp_rcv runs? That could solve 1. and 2.

Hi,

I worked on a new version (see attachment) that addresses both issues.

The major change is that instead of tracking the timestamp and packet count as local variables in veth_xdp_rcv(), they are now stored persistently in veth_rq as struct veth_bql_state. This allows completions to accumulate across multiple NAPI poll calls, so netdev_tx_completed_queue() can report more than 64 packets at once (see point 1). To get the time I am using (the fast) sched_clock() with a trick to avoid issues when switching between CPUs.

For point 2, the coalescing deadline is now checked both before the receive loop (to flush completions that timed out since the previous poll) and after each consumed packet, making completion intervals more regular. Still the intervals can be smaller than VETH_BQL_COAL_TX_USECS, but I guess this is fine.

I also found out that the BQL limit correlates closely with VETH_BQL_COAL_TX_USECS. It essentially reflects the latency we are targeting. I raised the default to 100 µs to allow DQL to converge to a higher limit (for reaching 255 in the testing below).

With the patched pktgen (respecting __QUEUE_STATE_STACK_XOFF), testing shows:
- --nrules 0: DQL limit reaches (up to) ~255
- --nrules 10000: DQL limit converges to ~0 (with --gro-disable)

Quoted 4 lines

These results are what I would expect from a BQL algorithm, but more testing is needed of course.

What do you think?

Hi,

This is exactly what I had in mind for implementing the BQL algorithm in this case. I did some testing with pktgen of this patch and also compared it to the v5 version.

Quoted 8 lines

Quoted 4 lines

You can find an extension of the benchmark script with pktgen here [1], as well as a wrapper script (veth_bql_bench.sh) to run the test script with and without --bql-disable to report the difference. I also configured pktgen to use the qdisc as suggested by Jesper.

Great, I will use your pktgen solution from now on.

Didn't know about the qdisc option, is there a performance difference with/without it? Or is it to have ping working next to pktgen?

Did not see any big difference performance-wise, but as you say, ping works better with pktgen then.

Quoted 46 lines

Consider to do a pull request :)

Quoted 3 lines

Note: bpftrace needs to be disabled, otherwise it becomes the bottleneck (at least on my machine) and pktgen throughput is halved when enabled.

Good to know.

Quoted 34 lines

Here are the results:

v5 (not time-based):
--nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------
Throughput (pps) 1980871 2169898
Ping RTT avg (ms) 0.065 0.162
Throughput diff -8.7% // BQL 8.7% lower throughput
RTT diff -59.9% // BQL 60% lower latency
========================================

Simon's time-based version:

Test args: --nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------
Throughput (pps) 2166335 2153398
Ping RTT avg (ms) 0.165 0.165
Throughput diff 0.6%
RTT diff 0.0%

--pktgen --no-bpftrace --nrules 3500
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------

Quoted 5 lines

Throughput (pps)            28569        28696
Ping RTT avg (ms)           1.327        8.409
Throughput diff             -0.4%

RTT diff -84.2%

Quoted 2 lines

I think we should run benchmarks against the stock net-next to be safe.

--nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
net-next

Quoted 9 lines

---                  -------
Throughput (pps)     2285421
Ping RTT avg (ms)    0.161

(slightly adjusted the output to better communicate the results)

So in my case this means BQL implementation has ~5% lower throughput compared to net-next. But please double check.

Yes, I will benchmark myself.

There are probably some places where we can gain performance. For example, I see ptr_ring_empty() which could be swapped for __ptr_ring_empty() which would save a spinlock and unlock.

Quoted 10 lines

Quoted 10 lines

Quoted 1 line

Seems to work now as expected.

Yes, but I think we have to keep these points in mind:

1. Limit/Inflight can be bigger than VETH_RING_SIZE, because packets can be enqueued in the same time as they are read out, so netdev_tx_completed_queue() can theoretically be called with many number of packets.
I do not think it is deal-breaking though.
I could see such high limits/inflights when looking at the /sys BQL statistics..

Quoted 3 lines

For me this makes sense, that inflight just means the number of packets not yet 'completed' or the number of packets that you can send between two completion calls. I think this is not specific

From my understanding, I do think that this behavior is
pretty specific.

Typically BQL-enabled NIC drivers clear packets out of some internal buffer in their completion interrupt (or something similar). And after that they call netdev_tx_completed_queue().

Quoted 3 lines

to this implementation. But for long intervals this might result in some problems because you can just fill the veth_ring to its capacity quickly, and increasing latency if the receiver is slow.

Yes, but I think the latency can only be approx. as big as the interval (can be higher with GRO enabled).

Quoted 5 lines

To illustrate this have a look at [1]. There are some plots that show the rtt vs. tx-usec config depending on nrules.

[1] https://github.com/jkoeppeler/veth-backpressure-performance-testing/tree/pktgen-and-benchmark/results/tx-usecs

Nice plots! Great for finding a sane default value.

Quoted 12 lines

Quoted 12 lines

2. sched_clock() is only valid on the same CPU. When a different CPU starts executing its sched_clock() can be in the past compared to the sched_clock() value saved by the previous CPU.
My trick...
min(s->time, sched_clock())
... avoids potentially extremely long intervals between netdev_tx_completed_queue() calls but is not perfect of course. I think CPU hopping happens rarely enough for this to matter.. And also we have to keep this in mind [1]:
"An architecture may or may not provide an implementation of sched_clock() on its own. If a local implementation is not provided, the system jiffy counter will be used as sched_clock()."

Quoted 6 lines

So the problem with this is that with jiffies you have like millisecond interval granularity, which might be too long in order to work properly. Given the receiver completes 128 packet in 1ms (queue_completed interval), the bql will set the limit at 256. Then the tx thread can quickly fill the ring, and it then basically stopped until the 1ms interval is over.

Linux Mint only has 100Hz jiffies :^)

I think you misunderstand something.

drivers/net/veth.c +5 -0

+	if (peer_txq && state->n_bql && ptr_ring_empty(&rq->xdp_ring)) {
+		smp_rmb();
+		if (test_bit(__QUEUE_STATE_STACK_XOFF, &peer_txq->state))
+			veth_bql_complete(state, peer_txq);
+	}

This snippet completes whenever the queue is empty and the netdev queue is stopped due to BQL.
So then the interval is smaller than 1ms.
I think, that is the reason why the pps is fine for 1ms in your benchmarks.

Quoted 1 line

I don't know how much this matters in practice. Not sure which architectures still hit the jiffies fallback.When in doubt we could disable BQL for those, or pin it to a "virtual queue size" via limit_min / limit_max similar to the v5 behavior and call queue_completed for every packet. Wdyt?

Yes, probably not relevant, I think I would just disable it for those architectures, avoiding possible regressions.

Quoted 8 lines

Quoted 5 lines

3. Inflight can be stuck at a value>0 for a long time when packet enqueueing stops. Only when packets are enqueued again, (on the next veth_xdp_rcv() call,) netdev_tx_completed_queue() is executed and inflight is set to 0 again.
Can also be seen when looking at the /sys BQL statistics.

This happens if the ring empties before the next queue_completed fires, right?

Exactly.

Quoted 3 lines

Another thing: Is it counterintuitive to set the tx_usec_coal on the receive device? Because this is a bql related config that is normally configured on the TX side?

Yes, I would say so.

Quoted 31 lines

Quoted 3 lines

BTW: Yesterday, I worked on and refactored the code into its own .h
file as a library and it also works fine for TUN/TAP (+vhost-net) for me :)

Nice! Thank you.
Jonas

Quoted 25 lines

Thanks for your work!
Simon

[1] Link: https://docs.kernel.org/timers/timekeeping.html

Quoted 20 lines

[1]https://github.com/jkoeppeler/veth-backpressure-performance-testing/tree/pktgen-and-benchmark

Thanks,
Jonas

Quoted 15 lines

Thanks!

BTW: I think that this implementation could also work for other
software interfaces.

Quoted 10 lines

[2] Link:https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83

Quoted 6 lines

There is an important gotcha. We actually have micro-burst of queuing (likely due to scheduling noise). Reading BQL stats from /sys will show BQL inflight=1, but when using the option --hist is it visible that @inflight have a long tail (see below signature). The "qdisc" output line also shows this happening via requeues increasing (approx 17/sec in a test with 567Kpps). (this was with the time-based BQL impl).

I understand..

Quoted 209 lines · Simon Schippers

Quoted 180 lines · Jonas Köppeler

Quoted 115 lines · Simon Schippers

Quoted 114 lines · Jonas Köppeler

Quoted 103 lines · Simon Schippers

Quoted 66 lines · Simon Schippers

Quoted 28 lines · Jesper Dangaard Brouer

Quoted 3 lines

Quoted 2 lines

Nope, I'm using a bpftrace program to keep track of the inflight/limit in a BPF hashmap. Reading from /sys will not be accurate.

Ah nice.

Add the option --hist to have both NAPI and BQL histograms printed when script ends. This will give you an accurate pattern of how inflight and limit evolves.

Quoted 14 lines

Quoted 7 lines

I moved the selftests into a github repo [1] to allow us to collaborate and evaluate the changes more easily. I explicitly kept the new BPF based BQL tracking as a commit[2] for your benefit.

[1]https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

[2]https://github.com/netoptimizer/veth-backpressure-performance-testing/commit/f25c5dc92977

Thanks for sharing. After minor issues I was able to set it up (currently I am just using plain v5, will look at the coalescing patch when I find the time):

Can confirm the latency reduction with the default settings, in my case 4.888ms to 0.241ms.

Quoted 7 lines

With the same script I was also able to see a performance slow down:
veth_bql_test_virtme.sh --qdisc fq_codel --nrules 0
--> ~510 Kpps
Same with --bql-disable
--> ~570 Kpps
--> 12% faster

Quoted 7 lines

Thanks for running these benchmarks.

Notice that --nrules 0 can easily result in no-queuing (on average), because the veth NAPI consumer is faster than the producer. You will likely see BQL inflight=1 and sink reported avg latency very low (remember it okay that sink get high latency penalty as long at ping latency remains low, as that show AQM is working).

I ran the benchmarks with --hist and I see what you mean. I have very similar results.

Is Jonas way [1] of modifiying pktgen maybe the best option to ensure that the producer is faster than the consumer?

[1] Link:https://lore.kernel.org/netdev/[email protected]/

Review

Quoted 7 lines

Hi, so what I found is that pktgen does not respect __QUEUE_STATE_STACK_OFF. So the test data above is invalid, since it just sent packets even if the BQL "stopped" the queue. So I patched pktgen with the following:

(quoted diff) +1 -1

-       if (unlikely(netif_xmit_frozen_or_drv_stopped(txq))) {
+       if (unlikely(netif_xmit_frozen_or_stopped(txq))) {

After thinking more about the implementation I see possible issues:

1. netdev_tx_completed_queue() never reports more than burst=64 packets:

BQL only increments the limit if the queue was starved. That means:
"The queue was over-limit in the last interval (the last time completion processing ran), and there is no more data in the queue (i.e. it’s empty)" [2]
But as only 64 packets are reported at max, the queue can only grow when it is <= 64 packets. And then it can only stay at a limit >64 until the next decrease of the limit.

2. netdev_tx_completed_queue() is called in irregular intervals:

Quoted 8 lines

If the consumer is slow it is called approx each tx_coal_usecs. But if the consumer is fast it is called way more frequent, probably in irregular intervals depending on the scheduling. However, "BQL depends on periodic completion interrupts" [2].

--> How about adding something like an interrupt that triggers every
10us and calls netdev_tx_completed_queue() with n_bql collected from (multiple) veth_xdp_rcv runs? That could solve 1. and 2.

Hi,

I worked on a new version (see attachment) that addresses both issues.

The major change is that instead of tracking the timestamp and packet count as local variables in veth_xdp_rcv(), they are now stored persistently in veth_rq as struct veth_bql_state. This allows completions to accumulate across multiple NAPI poll calls, so netdev_tx_completed_queue() can report more than 64 packets at once (see point 1). To get the time I am using (the fast) sched_clock() with a trick to avoid issues when switching between CPUs.

For point 2, the coalescing deadline is now checked both before the receive loop (to flush completions that timed out since the previous poll) and after each consumed packet, making completion intervals more regular. Still the intervals can be smaller than VETH_BQL_COAL_TX_USECS, but I guess this is fine.

I also found out that the BQL limit correlates closely with VETH_BQL_COAL_TX_USECS. It essentially reflects the latency we are targeting. I raised the default to 100 µs to allow DQL to converge to a higher limit (for reaching 255 in the testing below).

With the patched pktgen (respecting __QUEUE_STATE_STACK_XOFF), testing shows:
- --nrules 0: DQL limit reaches (up to) ~255
- --nrules 10000: DQL limit converges to ~0 (with --gro-disable)

Quoted 4 lines

These results are what I would expect from a BQL algorithm, but more testing is needed of course.

What do you think?

Hi,

This is exactly what I had in mind for implementing the BQL algorithm in this case. I did some testing with pktgen of this patch and also compared it to the v5 version.

Quoted 8 lines

Quoted 4 lines

You can find an extension of the benchmark script with pktgen here [1], as well as a wrapper script (veth_bql_bench.sh) to run the test script with and without --bql-disable to report the difference. I also configured pktgen to use the qdisc as suggested by Jesper.

Great, I will use your pktgen solution from now on.

Didn't know about the qdisc option, is there a performance difference with/without it? Or is it to have ping working next to pktgen?

Did not see any big difference performance-wise, but as you say, ping works better with pktgen then.

Quoted 46 lines

Consider to do a pull request :)

Quoted 3 lines

Note: bpftrace needs to be disabled, otherwise it becomes the bottleneck (at least on my machine) and pktgen throughput is halved when enabled.

Good to know.

Quoted 34 lines

Here are the results:

v5 (not time-based):
--nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------
Throughput (pps) 1980871 2169898
Ping RTT avg (ms) 0.065 0.162
Throughput diff -8.7% // BQL 8.7% lower throughput
RTT diff -59.9% // BQL 60% lower latency
========================================

Simon's time-based version:

Test args: --nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------
Throughput (pps) 2166335 2153398
Ping RTT avg (ms) 0.165 0.165
Throughput diff 0.6%
RTT diff 0.0%

--pktgen --no-bpftrace --nrules 3500
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------

Quoted 5 lines

Throughput (pps)            28569        28696
Ping RTT avg (ms)           1.327        8.409
Throughput diff             -0.4%

RTT diff -84.2%

Quoted 2 lines

I think we should run benchmarks against the stock net-next to be safe.

--nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
net-next

Quoted 9 lines

---                  -------
Throughput (pps)     2285421
Ping RTT avg (ms)    0.161

(slightly adjusted the output to better communicate the results)

So in my case this means BQL implementation has ~5% lower throughput compared to net-next. But please double check.

Yes, I will benchmark myself.

There are probably some places where we can gain performance. For example, I see ptr_ring_empty() which could be swapped for __ptr_ring_empty() which would save a spinlock and unlock.

Quoted 10 lines

Quoted 10 lines

Quoted 1 line

Seems to work now as expected.

Yes, but I think we have to keep these points in mind:

1. Limit/Inflight can be bigger than VETH_RING_SIZE, because packets can be enqueued in the same time as they are read out, so netdev_tx_completed_queue() can theoretically be called with many number of packets.
I do not think it is deal-breaking though.
I could see such high limits/inflights when looking at the /sys BQL statistics..

Quoted 3 lines

For me this makes sense, that inflight just means the number of packets not yet 'completed' or the number of packets that you can send between two completion calls. I think this is not specific

Quoted 6 lines

From my understanding, I do think that this behavior is
pretty specific.

Typically BQL-enabled NIC drivers clear packets out of some internal buffer in their completion interrupt (or something similar). And after that they call netdev_tx_completed_queue().

You are right, because descriptors are freed at the same time when they are accounted for.

Quoted 5 lines

Quoted 3 lines

to this implementation. But for long intervals this might result in some problems because you can just fill the veth_ring to its capacity quickly, and increasing latency if the receiver is slow.

Yes, but I think the latency can only be approx. as big as the interval (can be higher with GRO enabled).

I think it's more like ~2.x times added latency in the worst case, which is normal BQL behavior, but for large intervals 2x is a lot. Of course this value is also capped by the time it takes to process the 256 packets that fit in the ring buffer.

For example, for a test run with fq_codel and 20K rules, and a ping baseline of 1.7 ms without pktgen load, the results are:
tx-usecs p99_ping_rtt (ms)
-------- -----------------

     0           5.223
   100           6.910
   500           7.197
  1000           6.967
  5000          16.233
 10000          25.033
 20000          44.133

Also, the interval is sometimes dominated by the processing time of a packet batch (and other effects). The default is 8 packets/batch (see gro_normal_batch). This means that for 7 packets you see processing times on the order of a few hundred nanoseconds, but for the 8th you see the processing time of the whole batch (see table below). With a ruleset > 1000 entries, this exceeds the default tx-usecs value, which is why the p99 RTT increase is even larger than 2x tx-usecs -- as you can see in the table for tx-usecs values 0-5000.

tx-usecs 0 nrules 0
Percentile per-packet processing time (µs)
---------- -------------

    p5             0.316
   p25             0.382
   p50             0.442
   p75             0.508 -- fast enqueue to skb_list

p95 68.304 -- batch processing time

Setting gro_normal_batch to 1 makes the interval times more accurate and improves p99 RTT slightly, though it's still > 2x the tx-usecs latency increase.

However, using smaller values > 0 means that you can benefit from bql, but the actual interval might be larger than the configured value. I do not think this is a deal breaker, but something worth noting where the latencies come from.

Also worth noting: using fq_codel is better to use in this case because it can fast track sparse flows. using sfq the ping packet can get head of line blocked by the current quantum of sfq, if my understanding is correct.

Quoted 7 lines

Quoted 6 lines

To illustrate this have a look at [1]. There are some plots that show the rtt vs. tx-usec config depending on nrules.

[1] https://github.com/jkoeppeler/veth-backpressure-performance-testing/tree/pktgen-and-benchmark/results/tx-usecs

Nice plots! Great for finding a sane default value.

I did some more measurements to better understand whats happening. I will upload the results latest by tomorrow to the github repository mentioned above.

Quoted 19 lines

Quoted 18 lines

Quoted 4 lines

2. sched_clock() is only valid on the same CPU. When a different CPU starts executing its sched_clock() can be in the past compared to the sched_clock() value saved by the previous CPU. My trick...

Quoted 8 lines

min(s->time, sched_clock())
... avoids potentially extremely long intervals between netdev_tx_completed_queue() calls but is not perfect of course. I think CPU hopping happens rarely enough for this to matter.. And also we have to keep this in mind [1]:
"An architecture may or may not provide an implementation of sched_clock() on its own. If a local implementation is not provided, the system jiffy counter will be used as sched_clock()."

So the problem with this is that with jiffies you have like millisecond interval granularity, which might be too long in order to work properly. Given the receiver completes 128 packet in 1ms (queue_completed interval), the bql will set the limit at 256. Then the tx thread can quickly fill the ring, and it then basically stopped until the 1ms interval is over.

Quoted 1 line

Linux Mint only has 100Hz jiffies :^)

ok so 10ms granularity. Effectively disables BQL for the most part. However, you can still benefit if your receive side is very slow.

Quoted 4 lines

I think you misunderstand something.

+ if (peer_txq && state->n_bql && ptr_ring_empty(&rq->xdp_ring)) {

Quoted 10 lines

drivers/net/veth.c +4 -0

+        smp_rmb();
+        if (test_bit(__QUEUE_STATE_STACK_XOFF, &peer_txq->state))
+            veth_bql_complete(state, peer_txq);
+    }

This snippet completes whenever the queue is empty and the netdev queue is stopped due to BQL.
So then the interval is smaller than 1ms.
I think, that is the reason why the pps is fine for 1ms in your benchmarks.

Without that snippet the queue stalls during ramp-up and throughput never gets off the ground. But once the BQL limit has stabilized, the snippet is no longer invoked -- with --nrules 0 the queue is never stopped, and with a slow receiver the queue is not empty. So the snippet is needed for correct ramp-up, but it doesn't affect steady-state throughput in these tests. This is also confirmed by a test where I measure the time between intervals. For 1000us across all nrules configurations the time ranges between 1000-1400us.

The variance of the time between intervals is also quite good in general, but not always exact the configured value. For most cases +-5% from p50 value of the real interval.
Migrations between CPUs can also shorten an interval: if the new CPU's sched_clock() is ahead of the previous one (say by 15 ms), the apparent elapsed time within the current interval is inflated, so only ~5 ms of real time remains before the deadline fires (with the configured tx-usecs at 20 ms). Without migrations the interval stays consistently close to the configured value. I don't think this is a problem in practice -- it just adds some variance to the p5 of the measured interval.

Quoted 51 lines

Quoted 2 lines

I don't know how much this matters in practice. Not sure which architectures still hit the jiffies fallback.When in doubt we could disable BQL for those, or pin it to a "virtual queue size" via limit_min / limit_max similar to the v5 behavior and call queue_completed for every packet. Wdyt?

Yes, probably not relevant, I think I would just disable it for those architectures, avoiding possible regressions.

Quoted 8 lines

Quoted 5 lines

3. Inflight can be stuck at a value>0 for a long time when packet enqueueing stops. Only when packets are enqueued again, (on the next veth_xdp_rcv() call,) netdev_tx_completed_queue() is executed and inflight is set to 0 again.
Can also be seen when looking at the /sys BQL statistics.

This happens if the ring empties before the next queue_completed fires, right?

Exactly.

Quoted 3 lines

Another thing: Is it counterintuitive to set the tx_usec_coal on the receive device? Because this is a bql related config that is normally configured on the TX side?

Yes, I would say so.

Quoted 31 lines

Quoted 3 lines

BTW: Yesterday, I worked on and refactored the code into its own .h
file as a library and it also works fine for TUN/TAP (+vhost-net) for me :)

Nice! Thank you.
Jonas

Quoted 25 lines

Thanks for your work!
Simon

[1] Link: https://docs.kernel.org/timers/timekeeping.html

Quoted 20 lines

[1]https://github.com/jkoeppeler/veth-backpressure-performance-testing/tree/pktgen-and-benchmark

Thanks,
Jonas

Quoted 15 lines

Thanks!

BTW: I think that this implementation could also work for other
software interfaces.

Quoted 10 lines

[2] Link:https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83

Quoted 6 lines

There is an important gotcha. We actually have micro-burst of queuing (likely due to scheduling noise). Reading BQL stats from /sys will show BQL inflight=1, but when using the option --hist is it visible that @inflight have a long tail (see below signature). The "qdisc" output line also shows this happening via requeues increasing (approx 17/sec in a test with 567Kpps). (this was with the time-based BQL impl).

I understand..

Quoted 258 lines · Jonas Köppeler

Quoted 209 lines · Simon Schippers

Quoted 180 lines · Jonas Köppeler

Quoted 115 lines · Simon Schippers

Quoted 114 lines · Jonas Köppeler

Quoted 103 lines · Simon Schippers

Quoted 66 lines · Simon Schippers

Quoted 28 lines · Jesper Dangaard Brouer

Quoted 3 lines

Quoted 2 lines

Nope, I'm using a bpftrace program to keep track of the inflight/limit in a BPF hashmap. Reading from /sys will not be accurate.

Ah nice.

Add the option --hist to have both NAPI and BQL histograms printed when script ends. This will give you an accurate pattern of how inflight and limit evolves.

Quoted 14 lines

Quoted 7 lines

I moved the selftests into a github repo [1] to allow us to collaborate and evaluate the changes more easily. I explicitly kept the new BPF based BQL tracking as a commit[2] for your benefit.

[1]https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

[2]https://github.com/netoptimizer/veth-backpressure-performance-testing/commit/f25c5dc92977

Thanks for sharing. After minor issues I was able to set it up (currently I am just using plain v5, will look at the coalescing patch when I find the time):

Can confirm the latency reduction with the default settings, in my case 4.888ms to 0.241ms.

Quoted 7 lines

With the same script I was also able to see a performance slow down:
veth_bql_test_virtme.sh --qdisc fq_codel --nrules 0
--> ~510 Kpps
Same with --bql-disable
--> ~570 Kpps
--> 12% faster

Quoted 7 lines

Thanks for running these benchmarks.

Notice that --nrules 0 can easily result in no-queuing (on average), because the veth NAPI consumer is faster than the producer. You will likely see BQL inflight=1 and sink reported avg latency very low (remember it okay that sink get high latency penalty as long at ping latency remains low, as that show AQM is working).

I ran the benchmarks with --hist and I see what you mean. I have very similar results.

Is Jonas way [1] of modifiying pktgen maybe the best option to ensure that the producer is faster than the consumer?

[1] Link:https://lore.kernel.org/netdev/[email protected]/

Review

Quoted 7 lines

Hi, so what I found is that pktgen does not respect __QUEUE_STATE_STACK_OFF. So the test data above is invalid, since it just sent packets even if the BQL "stopped" the queue. So I patched pktgen with the following:

(quoted diff) +1 -1

-       if (unlikely(netif_xmit_frozen_or_drv_stopped(txq))) {
+       if (unlikely(netif_xmit_frozen_or_stopped(txq))) {

After thinking more about the implementation I see possible issues:

1. netdev_tx_completed_queue() never reports more than burst=64 packets:

BQL only increments the limit if the queue was starved. That means:
"The queue was over-limit in the last interval (the last time completion processing ran), and there is no more data in the queue (i.e. it’s empty)" [2]
But as only 64 packets are reported at max, the queue can only grow when it is <= 64 packets. And then it can only stay at a limit >64 until the next decrease of the limit.

2. netdev_tx_completed_queue() is called in irregular intervals:

Quoted 8 lines

If the consumer is slow it is called approx each tx_coal_usecs. But if the consumer is fast it is called way more frequent, probably in irregular intervals depending on the scheduling. However, "BQL depends on periodic completion interrupts" [2].

--> How about adding something like an interrupt that triggers every
10us and calls netdev_tx_completed_queue() with n_bql collected from (multiple) veth_xdp_rcv runs? That could solve 1. and 2.

Hi,

I worked on a new version (see attachment) that addresses both issues.

The major change is that instead of tracking the timestamp and packet count as local variables in veth_xdp_rcv(), they are now stored persistently in veth_rq as struct veth_bql_state. This allows completions to accumulate across multiple NAPI poll calls, so netdev_tx_completed_queue() can report more than 64 packets at once (see point 1). To get the time I am using (the fast) sched_clock() with a trick to avoid issues when switching between CPUs.

For point 2, the coalescing deadline is now checked both before the receive loop (to flush completions that timed out since the previous poll) and after each consumed packet, making completion intervals more regular. Still the intervals can be smaller than VETH_BQL_COAL_TX_USECS, but I guess this is fine.

I also found out that the BQL limit correlates closely with VETH_BQL_COAL_TX_USECS. It essentially reflects the latency we are targeting. I raised the default to 100 µs to allow DQL to converge to a higher limit (for reaching 255 in the testing below).

With the patched pktgen (respecting __QUEUE_STATE_STACK_XOFF), testing shows:
- --nrules 0: DQL limit reaches (up to) ~255
- --nrules 10000: DQL limit converges to ~0 (with --gro-disable)

Quoted 4 lines

These results are what I would expect from a BQL algorithm, but more testing is needed of course.

What do you think?

Hi,

This is exactly what I had in mind for implementing the BQL algorithm in this case. I did some testing with pktgen of this patch and also compared it to the v5 version.

Quoted 8 lines

Quoted 4 lines

You can find an extension of the benchmark script with pktgen here [1], as well as a wrapper script (veth_bql_bench.sh) to run the test script with and without --bql-disable to report the difference. I also configured pktgen to use the qdisc as suggested by Jesper.

Great, I will use your pktgen solution from now on.

Didn't know about the qdisc option, is there a performance difference with/without it? Or is it to have ping working next to pktgen?

Did not see any big difference performance-wise, but as you say, ping works better with pktgen then.

Quoted 46 lines

Consider to do a pull request :)

Quoted 3 lines

Note: bpftrace needs to be disabled, otherwise it becomes the bottleneck (at least on my machine) and pktgen throughput is halved when enabled.

Good to know.

Quoted 34 lines

Here are the results:

v5 (not time-based):
--nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------
Throughput (pps) 1980871 2169898
Ping RTT avg (ms) 0.065 0.162
Throughput diff -8.7% // BQL 8.7% lower throughput
RTT diff -59.9% // BQL 60% lower latency
========================================

Simon's time-based version:

Test args: --nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------
Throughput (pps) 2166335 2153398
Ping RTT avg (ms) 0.165 0.165
Throughput diff 0.6%
RTT diff 0.0%

--pktgen --no-bpftrace --nrules 3500
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------

Quoted 5 lines

Throughput (pps)            28569        28696
Ping RTT avg (ms)           1.327        8.409
Throughput diff             -0.4%

RTT diff -84.2%

Quoted 2 lines

I think we should run benchmarks against the stock net-next to be safe.

--nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
net-next

Quoted 9 lines

---                  -------
Throughput (pps)     2285421
Ping RTT avg (ms)    0.161

(slightly adjusted the output to better communicate the results)

So in my case this means BQL implementation has ~5% lower throughput compared to net-next. But please double check.

Yes, I will benchmark myself.

There are probably some places where we can gain performance. For example, I see ptr_ring_empty() which could be swapped for __ptr_ring_empty() which would save a spinlock and unlock.

Quoted 10 lines

Quoted 10 lines

Quoted 1 line

Seems to work now as expected.

Yes, but I think we have to keep these points in mind:

1. Limit/Inflight can be bigger than VETH_RING_SIZE, because packets can be enqueued in the same time as they are read out, so netdev_tx_completed_queue() can theoretically be called with many number of packets.
I do not think it is deal-breaking though.
I could see such high limits/inflights when looking at the /sys BQL statistics..

Quoted 3 lines

For me this makes sense, that inflight just means the number of packets not yet 'completed' or the number of packets that you can send between two completion calls. I think this is not specific

Quoted 6 lines

From my understanding, I do think that this behavior is
pretty specific.

Typically BQL-enabled NIC drivers clear packets out of some internal buffer in their completion interrupt (or something similar). And after that they call netdev_tx_completed_queue().

You are right, because descriptors are freed at the same time when they are accounted for.

Quoted 5 lines

Quoted 3 lines

to this implementation. But for long intervals this might result in some problems because you can just fill the veth_ring to its capacity quickly, and increasing latency if the receiver is slow.

Yes, but I think the latency can only be approx. as big as the interval (can be higher with GRO enabled).

I think it's more like ~2.x times added latency in the worst case, which is normal BQL behavior, but for large intervals 2x is a lot. Of course this value is also capped by the time it takes to process the 256 packets that fit in the ring buffer.

For example, for a test run with fq_codel and 20K rules, and a ping baseline of 1.7 ms without pktgen load, the results are:
tx-usecs p99_ping_rtt (ms)
-------- -----------------

      0           5.223
    100           6.910
    500           7.197
   1000           6.967
   5000          16.233
  10000          25.033
  20000          44.133

Also, the interval is sometimes dominated by the processing time of a packet batch (and other effects). The default is 8 packets/batch (see gro_normal_batch). This means that for 7 packets you see processing times on the order of a few hundred nanoseconds, but for the 8th you see the processing time of the whole batch (see table below). With a ruleset > 1000 entries, this exceeds the default tx-usecs value, which is why the p99 RTT increase is even larger than 2x tx-usecs -- as you can see in the table for tx-usecs values 0-5000.

tx-usecs 0 nrules 0
Percentile per-packet processing time (µs)
---------- -------------

     p5             0.316
    p25             0.382
    p50             0.442
    p75             0.508 -- fast enqueue to skb_list

p95 68.304 -- batch processing time

Quoted 8 lines

Setting gro_normal_batch to 1 makes the interval times more accurate and improves p99 RTT slightly, though it's still > 2x the tx-usecs latency increase.

However, using smaller values > 0 means that you can benefit from bql, but the actual interval might be larger than the configured value. I do not think this is a deal breaker, but something worth noting where the latencies come from.

I understand and agree with your points.
I also do not see anything deal-breaking. Yes, there is no guarantee for the latency, but BQL is best-effort anyway.

I will wait for your new measurements, but there is no argument against a default tx-usecs of ~100us for now, right?

Quoted 4 lines

Also worth noting: using fq_codel is better to use in this case because it can fast track sparse flows. using sfq the ping packet can get head of line blocked by the current quantum of sfq, if my understanding is correct.

Why don't we use a prio qdisc where ping gets prioritized? Then we do not have to worry about head of line blocking on the qdisc layer and instead can concentrate on veth.

Quoted 28 lines

Quoted 6 lines

Quoted 5 lines

To illustrate this have a look at [1]. There are some plots that show the rtt vs. tx-usec config depending on nrules.

[1] https://github.com/jkoeppeler/veth-backpressure-performance-testing/tree/pktgen-and-benchmark/results/tx-usecs

Nice plots! Great for finding a sane default value.

I did some more measurements to better understand whats happening. I will upload the results latest by tomorrow to the github repository mentioned above.

Quoted 18 lines

Quoted 18 lines

Quoted 4 lines

2. sched_clock() is only valid on the same CPU. When a different CPU starts executing its sched_clock() can be in the past compared to the sched_clock() value saved by the previous CPU. My trick...

Quoted 8 lines

min(s->time, sched_clock())
... avoids potentially extremely long intervals between netdev_tx_completed_queue() calls but is not perfect of course. I think CPU hopping happens rarely enough for this to matter.. And also we have to keep this in mind [1]:
"An architecture may or may not provide an implementation of sched_clock() on its own. If a local implementation is not provided, the system jiffy counter will be used as sched_clock()."

So the problem with this is that with jiffies you have like millisecond interval granularity, which might be too long in order to work properly. Given the receiver completes 128 packet in 1ms (queue_completed interval), the bql will set the limit at 256. Then the tx thread can quickly fill the ring, and it then basically stopped until the 1ms interval is over.

Quoted 7 lines

Quoted 1 line

Linux Mint only has 100Hz jiffies :^)

ok so 10ms granularity. Effectively disables BQL for the most part. However, you can still benefit if your receive side is very slow.

Quoted 3 lines

I think you misunderstand something.

+ if (peer_txq && state->n_bql && ptr_ring_empty(&rq->xdp_ring)) {

             // Pairs with smp_wmb() in __ptr_ring_produce()

Quoted 10 lines

Quoted 10 lines

(quoted diff) +4 -0

+        smp_rmb();
+        if (test_bit(__QUEUE_STATE_STACK_XOFF, &peer_txq->state))
+            veth_bql_complete(state, peer_txq);
+    }

This snippet completes whenever the queue is empty and the netdev queue is stopped due to BQL.
So then the interval is smaller than 1ms.
I think, that is the reason why the pps is fine for 1ms in your benchmarks.

Quoted 9 lines

Without that snippet the queue stalls during ramp-up and throughput never gets off the ground. But once the BQL limit has stabilized, the snippet is no longer invoked -- with --nrules 0 the queue is never stopped, and with a slow receiver the queue is not empty. So the snippet is needed for correct ramp-up, but it doesn't affect steady-state throughput in these tests. This is also confirmed by a test where I measure the time between intervals. For 1000us across all nrules configurations the time ranges between 1000-1400us.

Yes, I agree. It only acts in the case of starvation (see [1]) and I think it is an advantage that we react faster compared to other BQL implementations.

Additionally the snippet is *required* because if the ring is empty and the txq stopped due to BQL, there are no more veth_xdp_recv() calls, permanently stalling everything. That is the reason for the memory barrier pairing in the snippet.

[1] Link: https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83

Quoted 11 lines

The variance of the time between intervals is also quite good in general, but not always exact the configured value. For most cases +-5% from p50 value of the real interval.
Migrations between CPUs can also shorten an interval: if the new CPU's sched_clock() is ahead of the previous one (say by 15 ms), the apparent elapsed time within the current interval is inflated, so only ~5 ms of real time remains before the deadline fires (with the configured tx-usecs at 20 ms). Without migrations the interval stays consistently close to the configured value. I don't think this is a problem in practice -- it just adds some variance to the p5 of the measured interval.

Yes, and then we mistakenly decrease the BQL limit once. But we react fast on that starvation. So its fine.

Quoted 50 lines

Quoted 50 lines

Quoted 1 line

I don't know how much this matters in practice. Not sure which architectures still hit the jiffies fallback.When in doubt we could disable BQL for those, or pin it to a "virtual queue size" via limit_min / limit_max similar to the v5 behavior and call queue_completed for every packet. Wdyt?

Yes, probably not relevant, I think I would just disable it for those architectures, avoiding possible regressions.

Quoted 8 lines

Quoted 5 lines

3. Inflight can be stuck at a value>0 for a long time when packet enqueueing stops. Only when packets are enqueued again, (on the next veth_xdp_rcv() call,) netdev_tx_completed_queue() is executed and inflight is set to 0 again.
Can also be seen when looking at the /sys BQL statistics.

This happens if the ring empties before the next queue_completed fires, right?

Exactly.

Quoted 3 lines

Another thing: Is it counterintuitive to set the tx_usec_coal on the receive device? Because this is a bql related config that is normally configured on the TX side?

Yes, I would say so.

Quoted 31 lines

Quoted 3 lines

BTW: Yesterday, I worked on and refactored the code into its own .h
file as a library and it also works fine for TUN/TAP (+vhost-net) for me :)

Nice! Thank you.
Jonas

Quoted 25 lines

Thanks for your work!
Simon

[1] Link: https://docs.kernel.org/timers/timekeeping.html

Quoted 20 lines

[1]https://github.com/jkoeppeler/veth-backpressure-performance-testing/tree/pktgen-and-benchmark

Thanks,
Jonas

Quoted 15 lines

Thanks!

BTW: I think that this implementation could also work for other
software interfaces.

Quoted 10 lines

[2] Link:https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83

Quoted 6 lines

There is an important gotcha. We actually have micro-burst of queuing (likely due to scheduling noise). Reading BQL stats from /sys will show BQL inflight=1, but when using the option --hist is it visible that @inflight have a long tail (see below signature). The "qdisc" output line also shows this happening via requeues increasing (approx 17/sec in a test with 567Kpps). (this was with the time-based BQL impl).

I understand..

Quoted 272 lines · Simon Schippers

Quoted 258 lines · Jonas Köppeler

Quoted 209 lines · Simon Schippers

Quoted 180 lines · Jonas Köppeler

Quoted 115 lines · Simon Schippers

Quoted 114 lines · Jonas Köppeler

Quoted 103 lines · Simon Schippers

Quoted 66 lines · Simon Schippers

Quoted 28 lines · Jesper Dangaard Brouer

Quoted 3 lines

Quoted 2 lines

Nope, I'm using a bpftrace program to keep track of the inflight/limit in a BPF hashmap. Reading from /sys will not be accurate.

Ah nice.

Add the option --hist to have both NAPI and BQL histograms printed when script ends. This will give you an accurate pattern of how inflight and limit evolves.

Quoted 14 lines

Quoted 7 lines

I moved the selftests into a github repo [1] to allow us to collaborate and evaluate the changes more easily. I explicitly kept the new BPF based BQL tracking as a commit[2] for your benefit.

[1]https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

[2]https://github.com/netoptimizer/veth-backpressure-performance-testing/commit/f25c5dc92977

Thanks for sharing. After minor issues I was able to set it up (currently I am just using plain v5, will look at the coalescing patch when I find the time):

Can confirm the latency reduction with the default settings, in my case 4.888ms to 0.241ms.

Quoted 7 lines

With the same script I was also able to see a performance slow down:
veth_bql_test_virtme.sh --qdisc fq_codel --nrules 0
--> ~510 Kpps
Same with --bql-disable
--> ~570 Kpps
--> 12% faster

Quoted 7 lines

Thanks for running these benchmarks.

Notice that --nrules 0 can easily result in no-queuing (on average), because the veth NAPI consumer is faster than the producer. You will likely see BQL inflight=1 and sink reported avg latency very low (remember it okay that sink get high latency penalty as long at ping latency remains low, as that show AQM is working).

I ran the benchmarks with --hist and I see what you mean. I have very similar results.

Is Jonas way [1] of modifiying pktgen maybe the best option to ensure that the producer is faster than the consumer?

[1] Link:https://lore.kernel.org/netdev/[email protected]/

Review

Quoted 7 lines

Hi, so what I found is that pktgen does not respect __QUEUE_STATE_STACK_OFF. So the test data above is invalid, since it just sent packets even if the BQL "stopped" the queue. So I patched pktgen with the following:

(quoted diff) +1 -1

-       if (unlikely(netif_xmit_frozen_or_drv_stopped(txq))) {
+       if (unlikely(netif_xmit_frozen_or_stopped(txq))) {

After thinking more about the implementation I see possible issues:

1. netdev_tx_completed_queue() never reports more than burst=64 packets:

BQL only increments the limit if the queue was starved. That means:
"The queue was over-limit in the last interval (the last time completion processing ran), and there is no more data in the queue (i.e. it’s empty)" [2]
But as only 64 packets are reported at max, the queue can only grow when it is <= 64 packets. And then it can only stay at a limit >64 until the next decrease of the limit.

2. netdev_tx_completed_queue() is called in irregular intervals:

Quoted 8 lines

If the consumer is slow it is called approx each tx_coal_usecs. But if the consumer is fast it is called way more frequent, probably in irregular intervals depending on the scheduling. However, "BQL depends on periodic completion interrupts" [2].

--> How about adding something like an interrupt that triggers every
10us and calls netdev_tx_completed_queue() with n_bql collected from (multiple) veth_xdp_rcv runs? That could solve 1. and 2.

Hi,

I worked on a new version (see attachment) that addresses both issues.

The major change is that instead of tracking the timestamp and packet count as local variables in veth_xdp_rcv(), they are now stored persistently in veth_rq as struct veth_bql_state. This allows completions to accumulate across multiple NAPI poll calls, so netdev_tx_completed_queue() can report more than 64 packets at once (see point 1). To get the time I am using (the fast) sched_clock() with a trick to avoid issues when switching between CPUs.

For point 2, the coalescing deadline is now checked both before the receive loop (to flush completions that timed out since the previous poll) and after each consumed packet, making completion intervals more regular. Still the intervals can be smaller than VETH_BQL_COAL_TX_USECS, but I guess this is fine.

I also found out that the BQL limit correlates closely with VETH_BQL_COAL_TX_USECS. It essentially reflects the latency we are targeting. I raised the default to 100 µs to allow DQL to converge to a higher limit (for reaching 255 in the testing below).

With the patched pktgen (respecting __QUEUE_STATE_STACK_XOFF), testing shows:
- --nrules 0: DQL limit reaches (up to) ~255
- --nrules 10000: DQL limit converges to ~0 (with --gro-disable)

Quoted 4 lines

These results are what I would expect from a BQL algorithm, but more testing is needed of course.

What do you think?

Hi,

This is exactly what I had in mind for implementing the BQL algorithm in this case. I did some testing with pktgen of this patch and also compared it to the v5 version.

Quoted 8 lines

Quoted 4 lines

You can find an extension of the benchmark script with pktgen here [1], as well as a wrapper script (veth_bql_bench.sh) to run the test script with and without --bql-disable to report the difference. I also configured pktgen to use the qdisc as suggested by Jesper.

Great, I will use your pktgen solution from now on.

Didn't know about the qdisc option, is there a performance difference with/without it? Or is it to have ping working next to pktgen?

Did not see any big difference performance-wise, but as you say, ping works better with pktgen then.

Quoted 46 lines

Consider to do a pull request :)

Quoted 3 lines

Note: bpftrace needs to be disabled, otherwise it becomes the bottleneck (at least on my machine) and pktgen throughput is halved when enabled.

Good to know.

Quoted 34 lines

Here are the results:

v5 (not time-based):
--nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------
Throughput (pps) 1980871 2169898
Ping RTT avg (ms) 0.065 0.162
Throughput diff -8.7% // BQL 8.7% lower throughput
RTT diff -59.9% // BQL 60% lower latency
========================================

Simon's time-based version:

Test args: --nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------
Throughput (pps) 2166335 2153398
Ping RTT avg (ms) 0.165 0.165
Throughput diff 0.6%
RTT diff 0.0%

--pktgen --no-bpftrace --nrules 3500
========================================
Results (average over 10 runs):
========================================
BQL on BQL off
--- ------ -------

Quoted 5 lines

Throughput (pps)            28569        28696
Ping RTT avg (ms)           1.327        8.409
Throughput diff             -0.4%

RTT diff -84.2%

Quoted 2 lines

I think we should run benchmarks against the stock net-next to be safe.

--nrules 0 --pktgen --no-bpftrace
========================================
Results (average over 10 runs):
========================================
net-next

Quoted 9 lines

---                  -------
Throughput (pps)     2285421
Ping RTT avg (ms)    0.161

(slightly adjusted the output to better communicate the results)

So in my case this means BQL implementation has ~5% lower throughput compared to net-next. But please double check.

Yes, I will benchmark myself.

There are probably some places where we can gain performance. For example, I see ptr_ring_empty() which could be swapped for __ptr_ring_empty() which would save a spinlock and unlock.

Quoted 10 lines

Quoted 10 lines

Quoted 1 line

Seems to work now as expected.

Yes, but I think we have to keep these points in mind:

1. Limit/Inflight can be bigger than VETH_RING_SIZE, because packets can be enqueued in the same time as they are read out, so netdev_tx_completed_queue() can theoretically be called with many number of packets.
I do not think it is deal-breaking though.
I could see such high limits/inflights when looking at the /sys BQL statistics..

Quoted 3 lines

For me this makes sense, that inflight just means the number of packets not yet 'completed' or the number of packets that you can send between two completion calls. I think this is not specific

Quoted 6 lines

From my understanding, I do think that this behavior is
pretty specific.

Typically BQL-enabled NIC drivers clear packets out of some internal buffer in their completion interrupt (or something similar). And after that they call netdev_tx_completed_queue().

You are right, because descriptors are freed at the same time when they are accounted for.

Quoted 5 lines

Quoted 3 lines

to this implementation. But for long intervals this might result in some problems because you can just fill the veth_ring to its capacity quickly, and increasing latency if the receiver is slow.

Yes, but I think the latency can only be approx. as big as the interval (can be higher with GRO enabled).

I think it's more like ~2.x times added latency in the worst case, which is normal BQL behavior, but for large intervals 2x is a lot. Of course this value is also capped by the time it takes to process the 256 packets that fit in the ring buffer.

For example, for a test run with fq_codel and 20K rules, and a ping baseline of 1.7 ms without pktgen load, the results are:
tx-usecs p99_ping_rtt (ms)
-------- -----------------

      0           5.223
    100           6.910
    500           7.197
   1000           6.967
   5000          16.233
  10000          25.033
  20000          44.133

Also, the interval is sometimes dominated by the processing time of a packet batch (and other effects). The default is 8 packets/batch (see gro_normal_batch). This means that for 7 packets you see processing times on the order of a few hundred nanoseconds, but for the 8th you see the processing time of the whole batch (see table below). With a ruleset > 1000 entries, this exceeds the default tx-usecs value, which is why the p99 RTT increase is even larger than 2x tx-usecs -- as you can see in the table for tx-usecs values 0-5000.

tx-usecs 0 nrules 0
Percentile per-packet processing time (µs)
---------- -------------

     p5             0.316
    p25             0.382
    p50             0.442
    p75             0.508 -- fast enqueue to skb_list

p95 68.304 -- batch processing time

Quoted 8 lines

Setting gro_normal_batch to 1 makes the interval times more accurate and improves p99 RTT slightly, though it's still > 2x the tx-usecs latency increase.

However, using smaller values > 0 means that you can benefit from bql, but the actual interval might be larger than the configured value. I do not think this is a deal breaker, but something worth noting where the latencies come from.

I understand and agree with your points.
I also do not see anything deal-breaking. Yes, there is no guarantee for the latency, but BQL is best-effort anyway.

Quoted 2 lines

I will wait for your new measurements, but there is no argument against a default tx-usecs of ~100us for now, right?

Yes, I think 100us is perfectly fine. I guess most of it was just my curiosity why the latency values are as they are :)

But it feels like this will need some documentation, because as we have seen, some values are a little different from what you expect from bql. Inflight > veth_ring_size, tx-usecs not necessarily achieving the configured value, inflight can get stuck > 0. Wdyt?
But I think it works nicely overall.

Quoted 139 lines

Quoted 4 lines

Also worth noting: using fq_codel is better to use in this case because it can fast track sparse flows. using sfq the ping packet can get head of line blocked by the current quantum of sfq, if my understanding is correct.

Why don't we use a prio qdisc where ping gets prioritized? Then we do not have to worry about head of line blocking on the qdisc layer and instead can concentrate on veth.

Quoted 28 lines

Quoted 6 lines

Quoted 5 lines

To illustrate this have a look at [1]. There are some plots that show the rtt vs. tx-usec config depending on nrules.

[1] https://github.com/jkoeppeler/veth-backpressure-performance-testing/tree/pktgen-and-benchmark/results/tx-usecs

Nice plots! Great for finding a sane default value.

I did some more measurements to better understand whats happening. I will upload the results latest by tomorrow to the github repository mentioned above.

Quoted 18 lines

Quoted 18 lines

Quoted 4 lines

2. sched_clock() is only valid on the same CPU. When a different CPU starts executing its sched_clock() can be in the past compared to the sched_clock() value saved by the previous CPU. My trick...

Quoted 8 lines

min(s->time, sched_clock())
... avoids potentially extremely long intervals between netdev_tx_completed_queue() calls but is not perfect of course. I think CPU hopping happens rarely enough for this to matter.. And also we have to keep this in mind [1]:
"An architecture may or may not provide an implementation of sched_clock() on its own. If a local implementation is not provided, the system jiffy counter will be used as sched_clock()."

So the problem with this is that with jiffies you have like millisecond interval granularity, which might be too long in order to work properly. Given the receiver completes 128 packet in 1ms (queue_completed interval), the bql will set the limit at 256. Then the tx thread can quickly fill the ring, and it then basically stopped until the 1ms interval is over.

Quoted 7 lines

Quoted 1 line

Linux Mint only has 100Hz jiffies :^)

ok so 10ms granularity. Effectively disables BQL for the most part. However, you can still benefit if your receive side is very slow.

Quoted 3 lines

I think you misunderstand something.

+ if (peer_txq && state->n_bql && ptr_ring_empty(&rq->xdp_ring)) {

             // Pairs with smp_wmb() in __ptr_ring_produce()

Quoted 10 lines

Quoted 10 lines

(quoted diff) +4 -0

+        smp_rmb();
+        if (test_bit(__QUEUE_STATE_STACK_XOFF, &peer_txq->state))
+            veth_bql_complete(state, peer_txq);
+    }

This snippet completes whenever the queue is empty and the netdev queue is stopped due to BQL.
So then the interval is smaller than 1ms.
I think, that is the reason why the pps is fine for 1ms in your benchmarks.

Quoted 9 lines

Without that snippet the queue stalls during ramp-up and throughput never gets off the ground. But once the BQL limit has stabilized, the snippet is no longer invoked -- with --nrules 0 the queue is never stopped, and with a slow receiver the queue is not empty. So the snippet is needed for correct ramp-up, but it doesn't affect steady-state throughput in these tests. This is also confirmed by a test where I measure the time between intervals. For 1000us across all nrules configurations the time ranges between 1000-1400us.

Yes, I agree. It only acts in the case of starvation (see [1]) and I think it is an advantage that we react faster compared to other BQL implementations.

Additionally the snippet is *required* because if the ring is empty and the txq stopped due to BQL, there are no more veth_xdp_recv() calls, permanently stalling everything. That is the reason for the memory barrier pairing in the snippet.

[1] Link: https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83

Quoted 11 lines

The variance of the time between intervals is also quite good in general, but not always exact the configured value. For most cases +-5% from p50 value of the real interval.
Migrations between CPUs can also shorten an interval: if the new CPU's sched_clock() is ahead of the previous one (say by 15 ms), the apparent elapsed time within the current interval is inflated, so only ~5 ms of real time remains before the deadline fires (with the configured tx-usecs at 20 ms). Without migrations the interval stays consistently close to the configured value. I don't think this is a problem in practice -- it just adds some variance to the p5 of the measured interval.

Yes, and then we mistakenly decrease the BQL limit once. But we react fast on that starvation. So its fine.

Quoted 50 lines

Quoted 50 lines

Quoted 1 line

I don't know how much this matters in practice. Not sure which architectures still hit the jiffies fallback.When in doubt we could disable BQL for those, or pin it to a "virtual queue size" via limit_min / limit_max similar to the v5 behavior and call queue_completed for every packet. Wdyt?

Yes, probably not relevant, I think I would just disable it for those architectures, avoiding possible regressions.

Quoted 8 lines

Quoted 5 lines

3. Inflight can be stuck at a value>0 for a long time when packet enqueueing stops. Only when packets are enqueued again, (on the next veth_xdp_rcv() call,) netdev_tx_completed_queue() is executed and inflight is set to 0 again.
Can also be seen when looking at the /sys BQL statistics.

This happens if the ring empties before the next queue_completed fires, right?

Exactly.

Quoted 3 lines

Another thing: Is it counterintuitive to set the tx_usec_coal on the receive device? Because this is a bql related config that is normally configured on the TX side?

Yes, I would say so.

Quoted 31 lines

Quoted 3 lines

BTW: Yesterday, I worked on and refactored the code into its own .h
file as a library and it also works fine for TUN/TAP (+vhost-net) for me :)

Nice! Thank you.
Jonas

Quoted 25 lines

Thanks for your work!
Simon

[1] Link: https://docs.kernel.org/timers/timekeeping.html

Quoted 20 lines

[1]https://github.com/jkoeppeler/veth-backpressure-performance-testing/tree/pktgen-and-benchmark

Thanks,
Jonas

Quoted 15 lines

Thanks!

BTW: I think that this implementation could also work for other
software interfaces.

Quoted 10 lines

[2] Link:https://medium.com/@tom_84912/byte-queue-limits-the-unauthorized-biography-61adc5730b83

Quoted 6 lines

There is an important gotcha. We actually have micro-burst of queuing (likely due to scheduling noise). Reading BQL stats from /sys will show BQL inflight=1, but when using the option --hist is it visible that @inflight have a long tail (see below signature). The "qdisc" output line also shows this happening via requeues increasing (approx 17/sec in a test with 567Kpps). (this was with the time-based BQL impl).

I understand..

Quoted 21 lines · Jonas Köppeler

Quoted 20 lines · Simon Schippers

Quoted 19 lines · Jonas Köppeler

Quoted 18 lines · Simon Schippers

Quoted 17 lines · Jonas Köppeler

Quoted 16 lines · Simon Schippers

Quoted 15 lines · Jonas Köppeler

Quoted 14 lines · Simon Schippers

Quoted 13 lines · Simon Schippers

Quoted 12 lines · Jesper Dangaard Brouer

Quoted 3 lines

Quoted 2 lines

Nope, I'm using a bpftrace program to keep track of the inflight/limit
in a BPF hashmap. Reading from /sys will not be accurate.

Ah nice.

Add the option --hist to have both NAPI and BQL histograms printed when script ends. This will give you an accurate pattern of how inflight and limit evolves.

Quoted 5 lines

Quoted 5 lines

I moved the selftests into a github repo [1] to allow us to collaborate
and evaluate the changes more easily. I explicitly kept the new BPF
based BQL tracking as a commit[2] for your benefit.

[1] https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

Quoted 5 lines

Quoted 3 lines

I will wait for your new measurements, but there is no argument against a default tx-usecs of ~100us for now, right?

Yes, I think 100us is perfectly fine. I guess most of it was just my curiosity why the latency values are as they are 🙂

Which is great, because I was wondering the same 🙂

Quoted 7 lines

But it feels like this will need some documentation, because as we have seen, some values are a little different from what you expect from bql. Inflight > veth_ring_size, tx-usecs not necessarily achieving the configured value, inflight can get stuck > 0. Wdyt?
But I think it works nicely overall.

Exactly, we should get ready for a v6 soon.

And I think we should move the BQL logic into a seperate .h file as a library. Then it is also usable for TUN/TAP in the future.

Let's amend the commits. Should we do this on Github?

Quoted 22 lines · Simon Schippers

Quoted 21 lines · Jonas Köppeler

Quoted 20 lines · Simon Schippers

Quoted 19 lines · Jonas Köppeler

Quoted 18 lines · Simon Schippers

Quoted 17 lines · Jonas Köppeler

Quoted 16 lines · Simon Schippers

Quoted 15 lines · Jonas Köppeler

Quoted 14 lines · Simon Schippers

Quoted 13 lines · Simon Schippers

Quoted 12 lines · Jesper Dangaard Brouer

Quoted 3 lines

Nope, I'm using a bpftrace program to keep track of the inflight/limit
in a BPF hashmap. Reading from /sys will not be accurate.
Ah nice.

Add the option --hist to have both NAPI and BQL histograms printed when script ends. This will give you an accurate pattern of how inflight and limit evolves.

Quoted 5 lines

I moved the selftests into a github repo [1] to allow us to collaborate
and evaluate the changes more easily. I explicitly kept the new BPF
based BQL tracking as a commit[2] for your benefit.

[1] https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

[... cut ...]

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

I will wait for your new measurements, but there is no argument against a default tx-usecs of ~100us for now, right?

Yes, I think 100us is perfectly fine. I guess most of it was just my curiosity why the latency values are as they are 🙂

Which is great, because I was wondering the same 🙂

Thank you Jonas and Simon for testing this via[1] on your systems.

One performance concern from my side is if/when BQL limit goes below 8 packets. This will cause cache-line bouncing and many qdisc requeues between the two CPUs. Notice that 8 packets for the ptr_ring is one cache-line. This is why I suggested defaulting BQL min_limit to be 8. This would work in combination with the tx-usecs coalesce tuning as a lower bound.

Quoted 9 lines

Quoted 7 lines

But it feels like this will need some documentation, because as we have seen, some values are a little different from what you expect from bql. Inflight > veth_ring_size, tx-usecs not necessarily achieving the configured value, inflight can get stuck > 0. Wdyt?
But I think it works nicely overall.

Exactly, we should get ready for a v6 soon.

I want to send out a V6 today (and I'm busy rest of week). I'm simply going to include Simon's patch[2]. Then we can iterate on that email thread, making it easier for people to reply inline to the code changes.

[2] https://lore.kernel.org/all/[email protected]/

Quoted 3 lines

And I think we should move the BQL logic into a seperate .h file as a library. Then it is also usable for TUN/TAP in the future.

I like the idea, but refactoring into a library seems out-of-scope for this patchset. As a first step, let us see if this works for veth driver use-case. I do encourage Simon to refactor when doing the work for TUN/TAP in the future.

Alternatively netdev maintainers can choose to apply V5 patchset and we can iterate on finding the right ethtool coalesce parameters for veth and create a .h "library" file for this.

Quoted 1 line

Let's amend the commits. Should we do this on Github?

Kernel development for netdev [3] happens on the mailing list with patches via email.

--Jesper

[3] https://kernel.org/doc/html/latest/process/maintainer-netdev.html

Quoted 22 lines · Simon Schippers

Quoted 21 lines · Jonas Köppeler

Quoted 20 lines · Simon Schippers

Quoted 19 lines · Jonas Köppeler

Quoted 18 lines · Simon Schippers

Quoted 17 lines · Jonas Köppeler

Quoted 16 lines · Simon Schippers

Quoted 15 lines · Jonas Köppeler

Quoted 14 lines · Simon Schippers

Quoted 13 lines · Simon Schippers

Quoted 12 lines · Jesper Dangaard Brouer

Quoted 3 lines

Nope, I'm using a bpftrace program to keep track of the inflight/limit
in a BPF hashmap. Reading from /sys will not be accurate.
Ah nice.

Add the option --hist to have both NAPI and BQL histograms printed when script ends. This will give you an accurate pattern of how inflight and limit evolves.

Quoted 5 lines

I moved the selftests into a github repo [1] to allow us to collaborate
and evaluate the changes more easily. I explicitly kept the new BPF
based BQL tracking as a commit[2] for your benefit.

[1] https://github.com/netoptimizer/veth-backpressure-performance-testing/tree/main/selftests

[... cut ...]

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

I will wait for your new measurements, but there is no argument against a default tx-usecs of ~100us for now, right?

Yes, I think 100us is perfectly fine. I guess most of it was just my curiosity why the latency values are as they are 🙂