[PATCH net] net/sched: fix packet reordering on requeue

[Antonio Quartulli <antonio@mandelbit.com>]

dev_requeue_skb() appends requeued skbs to the tail of q->gso_skb, while
dequeue_skb() takes them back one at a time from the head. If the driver
returns NETDEV_TX_BUSY again for the first of them, it moves behind the
skbs still parked there and is sent after them.

This showed up as heavy TCP reordering while benchmarking over veth,
which returns NETDEV_TX_BUSY whenever its ring is full since
commit dc82a33297fc ("veth: apply qdisc backpressure on full ptr_ring to reduce TX drops").

Requeue the skbs at the head of gso_skb, keeping their order. With 4 TCP
streams over a veth pair with a pfifo qdisc, this removes about 1.3M
out-of-order segments and 15-37k retransmits per 15 seconds, and raises
throughput from about 40 to 76 Gbit/s.

Fixes: a53851e2c321 ("net: sched: explicit locking in gso_cpu fallback")
Fixes: 9540d977618c ("net: sched: fix skb leak in dev_requeue_skb()")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
---
Notes:

- Setup: 8-vCPU VM, veth pair; sender with a pfifo qdisc and TSO off,
  receiver with GRO and threaded NAPI; iperf3, 15 s runs.

- A single stream gets about 7% slower (87 -> 81 Gbit/s), with 0
  retransmits instead of 1-20k. This is not a cost of the change: without
  the losses TCP keeps the veth ring full, and veth_poll() waking the
  queue makes net_tx_action() run the transmit work on the receiver's
  NAPI CPU. With VETH_RING_SIZE raised to 4096 the ring doesn't fill,
  nothing is requeued and both kernels do 86-90 Gbit/s, while 4 streams
  still go from 45 to 70 Gbit/s.

- History: before a53851e2c321, gso_skb was a single pointer.
  dev_requeue_skb() stored the busy skb there with the rest of the list
  still chained through skb->next, and dequeue_skb() handed the whole
  chain back, so the order was kept. a53851e2c321 turned gso_skb into a
  queue that dequeue_skb() drains one skb at a time; requeuing queued
  only the first skb (at the head for locked qdiscs, at the tail for
  TCQ_F_NOLOCK ones) and lost the rest of the list. 9540d977618c fixed
  that leak by queueing every skb of the list, and moved the locked path
  to the tail too, "to avoid skb out of order". Appending keeps a
  requeued list in order while gso_skb is empty, but not when the
  requeued skb was just taken from the head of a non-empty gso_skb,
  which is the case fixed here. So locked qdiscs reorder since
  9540d977618c, TCQ_F_NOLOCK ones since a53851e2c321; both are in 4.16.

 net/sched/sch_generic.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 6f6a6f0d5eb0..e79e9f33efba 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -168,8 +168,11 @@ static inline void qdisc_enqueue_skb_bad_txq(struct Qdisc *q,
 
 static inline void dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
 {
+	struct sk_buff_head requeue;
 	spinlock_t *lock = NULL;
 
+	__skb_queue_head_init(&requeue);
+
 	if (q->flags & TCQ_F_NOLOCK) {
 		lock = qdisc_lock(q);
 		spin_lock(lock);
@@ -178,7 +181,7 @@ static inline void dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
 	while (skb) {
 		struct sk_buff *next = skb->next;
 
-		__skb_queue_tail(&q->gso_skb, skb);
+		__skb_queue_tail(&requeue, skb);
 
 		/* it's still part of the queue */
 		if (qdisc_is_percpu_stats(q)) {
@@ -194,6 +197,11 @@ static inline void dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
 		skb = next;
 	}
 
+	/* the skbs may come from the head of gso_skb, so put them back there:
+	 * appending them would let the ones still parked overtake them
+	 */
+	skb_queue_splice(&requeue, &q->gso_skb);
+
 	if (lock) {
 		spin_unlock(lock);
 		set_bit(__QDISC_STATE_MISSED, &q->state);
-- 
2.55.0