| Message ID | 174489811513.355490.8155513147018728621.stgit@firesoul (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | veth: qdisc backpressure and qdisc check refactor | expand |
On 2025/04/17 22:55, Jesper Dangaard Brouer wrote: ... > + case NETDEV_TX_BUSY: > + /* If a qdisc is attached to our virtual device, returning > + * NETDEV_TX_BUSY is allowed. > + */ > + txq = netdev_get_tx_queue(dev, rxq); > + > + if (qdisc_txq_has_no_queue(txq)) { > + dev_kfree_skb_any(skb); > + goto drop; > + } > + netif_tx_stop_queue(txq); > + /* Restore Eth hdr pulled by dev_forward_skb/eth_type_trans */ > + __skb_push(skb, ETH_HLEN); > + if (use_napi) > + __veth_xdp_flush(rq); > + /* Cancel TXQ stop for very unlikely race */ > + if (unlikely(__ptr_ring_empty(&rq->xdp_ring))) > + netif_tx_wake_queue(txq); xdp_ring is only initialized when use_napi is not NULL. Should add "if (use_napi)" ? BTW, you added a check for the ring_empty here. so if empty: this function starts the queue by itself else: it is guaranteed that veth_xdp_rcv() consumes the ring after this point. so the rcv side definitely starts the queue. With that, __veth_xdp_flush invocation seems to be unnecessary, if your concern is starting the queue. Toshiaki Makita
On 18/04/2025 14.38, Toshiaki Makita wrote: > On 2025/04/17 22:55, Jesper Dangaard Brouer wrote: > ... >> + case NETDEV_TX_BUSY: >> + /* If a qdisc is attached to our virtual device, returning >> + * NETDEV_TX_BUSY is allowed. >> + */ >> + txq = netdev_get_tx_queue(dev, rxq); >> + >> + if (qdisc_txq_has_no_queue(txq)) { >> + dev_kfree_skb_any(skb); >> + goto drop; >> + } >> + netif_tx_stop_queue(txq); >> + /* Restore Eth hdr pulled by dev_forward_skb/eth_type_trans */ >> + __skb_push(skb, ETH_HLEN); >> + if (use_napi) >> + __veth_xdp_flush(rq); >> + /* Cancel TXQ stop for very unlikely race */ >> + if (unlikely(__ptr_ring_empty(&rq->xdp_ring))) >> + netif_tx_wake_queue(txq); > > xdp_ring is only initialized when use_napi is not NULL. > Should add "if (use_napi)" ? > We actually don't need the "if (use_napi)" check, because this code path cannot be invoked without use_name set. This also means the check before __veth_xdp_flush() is unnecessary. I still added it, because it is subtle that this isn't needed and if code change slightly is will be needed. Regarding xdp_ring is only initialized when use_napi is not NULL, I'm considering not adding a if(use_napi) check, because this code path cannot be called without use_napi is true, and if that change in the future, then it's better that the code crash. Different opinions are welcomed... > BTW, you added a check for the ring_empty here. so > > if empty: > this function starts the queue by itself > else: > it is guaranteed that veth_xdp_rcv() consumes the ring after this point. > so the rcv side definitely starts the queue. > > With that, __veth_xdp_flush invocation seems to be unnecessary, > if your concern is starting the queue. That is actually correct. I'm trying to catch the race in two different ways. The __ptr_ring_empty() will be sufficient, to cover both cases. I'll try to think of a good comment that explains, the parring with the !__ptr_ring_empty() check in veth_poll(). --Jesper
diff --git a/drivers/net/veth.c b/drivers/net/veth.c index 7bb53961c0ea..55be225c4e20 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -307,12 +307,10 @@ static void __veth_xdp_flush(struct veth_rq *rq) static int veth_xdp_rx(struct veth_rq *rq, struct sk_buff *skb) { - if (unlikely(ptr_ring_produce(&rq->xdp_ring, skb))) { - dev_kfree_skb_any(skb); - return NET_RX_DROP; - } + if (unlikely(ptr_ring_produce(&rq->xdp_ring, skb))) + return NETDEV_TX_BUSY; /* signal qdisc layer */ - return NET_RX_SUCCESS; + return NET_RX_SUCCESS; /* same as NETDEV_TX_OK */ } static int veth_forward_skb(struct net_device *dev, struct sk_buff *skb, @@ -346,11 +344,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; - int ret = NETDEV_TX_OK; + struct netdev_queue *txq; struct net_device *rcv; int length = skb->len; bool use_napi = false; - int rxq; + int ret, rxq; rcu_read_lock(); rcv = rcu_dereference(priv->peer); @@ -373,17 +371,43 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev) } skb_tx_timestamp(skb); - if (likely(veth_forward_skb(rcv, skb, rq, use_napi) == NET_RX_SUCCESS)) { + + ret = veth_forward_skb(rcv, skb, rq, use_napi); + switch (ret) { + case NET_RX_SUCCESS: /* same as NETDEV_TX_OK */ if (!use_napi) dev_sw_netstats_tx_add(dev, 1, length); else __veth_xdp_flush(rq); - } else { + break; + case NETDEV_TX_BUSY: + /* If a qdisc is attached to our virtual device, returning + * NETDEV_TX_BUSY is allowed. + */ + txq = netdev_get_tx_queue(dev, rxq); + + if (qdisc_txq_has_no_queue(txq)) { + dev_kfree_skb_any(skb); + goto drop; + } + netif_tx_stop_queue(txq); + /* Restore Eth hdr pulled by dev_forward_skb/eth_type_trans */ + __skb_push(skb, ETH_HLEN); + if (use_napi) + __veth_xdp_flush(rq); + /* Cancel TXQ stop for very unlikely race */ + if (unlikely(__ptr_ring_empty(&rq->xdp_ring))) + netif_tx_wake_queue(txq); + break; + case NET_RX_DROP: /* same as NET_XMIT_DROP */ drop: atomic64_inc(&priv->dropped); ret = NET_XMIT_DROP; + break; + default: + net_crit_ratelimited("%s(%s): Invalid return code(%d)", + __func__, dev->name, ret); } - rcu_read_unlock(); return ret; @@ -874,9 +898,17 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget, struct veth_xdp_tx_bq *bq, struct veth_stats *stats) { + struct veth_priv *priv = netdev_priv(rq->dev); + int queue_idx = rq->xdp_rxq.queue_index; + struct netdev_queue *peer_txq; + struct net_device *peer_dev; int i, done = 0, n_xdpf = 0; void *xdpf[VETH_XDP_BATCH]; + /* NAPI functions as RCU section */ + peer_dev = rcu_dereference_check(priv->peer, rcu_read_lock_bh_held()); + peer_txq = netdev_get_tx_queue(peer_dev, queue_idx); + for (i = 0; i < budget; i++) { void *ptr = __ptr_ring_consume(&rq->xdp_ring); @@ -925,6 +957,9 @@ static int veth_xdp_rcv(struct veth_rq *rq, int budget, rq->stats.vs.xdp_packets += done; u64_stats_update_end(&rq->stats.syncp); + if (unlikely(netif_tx_queue_stopped(peer_txq))) + netif_tx_wake_queue(peer_txq); + return done; }
In production, we're seeing TX drops on veth devices when the ptr_ring fills up. This can occur when NAPI mode is enabled, though it's relatively rare. However, with threaded NAPI - which we use in production - the drops become significantly more frequent. The underlying issue is that with threaded NAPI, the consumer often runs on a different CPU than the producer. This increases the likelihood of the ring filling up before the consumer gets scheduled, especially under load, leading to drops in veth_xmit() (ndo_start_xmit()). This patch introduces backpressure by returning NETDEV_TX_BUSY when the ring is full, signaling the qdisc layer to requeue the packet. The txq (netdev queue) is stopped in this condition and restarted once veth_poll() drains entries from the ring, ensuring coordination between NAPI and qdisc. Backpressure is only enabled when a qdisc is attached. Without a qdisc, the driver retains its original behavior - dropping packets immediately when the ring is full. This avoids unexpected behavior changes in setups without a configured qdisc. With a qdisc in place (e.g. fq, sfq) this allows Active Queue Management (AQM) to fairly schedule packets across flows and reduce collateral damage from elephant flows. A known limitation of this approach is that the full ring sits in front of the qdisc layer, effectively forming a FIFO buffer that introduces base latency. While AQM still improves fairness and mitigates flow dominance, the latency impact is measurable. In hardware drivers, this issue is typically addressed using BQL (Byte Queue Limits), which tracks in-flight bytes needed based on physical link rate. However, for virtual drivers like veth, there is no fixed bandwidth constraint - the bottleneck is CPU availability and the scheduler's ability to run the NAPI thread. It is unclear how effective BQL would be in this context. This patch serves as a first step toward addressing TX drops. Future work may explore adapting a BQL-like mechanism to better suit virtual devices like veth. Reported-by: Yan Zhai <yan@cloudflare.com> Signed-off-by: Jesper Dangaard Brouer <hawk@kernel.org> --- drivers/net/veth.c | 55 +++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 10 deletions(-)