Re: [PATCH net v2 1/2] tcp: restore RACK list membership when undoing loss

Le mar. 6 oct. 2026 à 08:42, Eric Dumazet [off-list ref] a écrit :

On 10/6/26 07:15, nramaswamy@openai.com wrote:

quoted

From: Neil Ramaswamy <redacted>

Partial undo can clear the TCPCB_LOST flag on segments already removed
from RACK's list, which prevents subsequent RACK loss detection, leading
to segments only being retransmitted after the RTO. Restoring them to
the RACK list as part of partial undo makes sure we can properly
reconsider them for fast retransmission in the future.

To do this, we first sort (by transmission time) the segments whose
TCPCB_LOST flag is being cleared and then linearly insert those back
into the RACK list, which is sorted by transmission time.

My investigation started from seeing repeated TCP stalls in prod and the
mitigation that seemed to prevent these stalls was limiting SO_SNDBUF to
96 KiB. It also seems like others have seen similar symptoms before [1].

[1]
https://lore.kernel.org/netdev/35A4DDAA-7E8D-43CB-A1F5-D1E46A4ED42E@gmail.com/ (local)

Fixes: 043b87d7599e ("tcp: more efficient RACK loss detection")
Signed-off-by: Neil Ramaswamy <redacted>
Assisted-by: LLM sparse
---
  net/ipv4/tcp_input.c | 35 +++++++++++++++++++++++++++++++++++
  1 file changed, 35 insertions(+)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 92bc60716f33..51f04c7474dc 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c

@@ -69,6 +69,7 @@
  #include <linux/module.h>
  #include <linux/sysctl.h>
  #include <linux/kernel.h>
+#include <linux/list_sort.h>
  #include <linux/prefetch.h>
  #include <linux/bitops.h>
  #include <net/dst.h>

@@ -2840,16 +2841,50 @@ static void DBGUNDO(struct sock *sk, const char *msg)
  #endif
  }

+static int tcp_rack_skb_cmp(void *priv, const struct list_head *a,
+                         const struct list_head *b)
+{
+     const struct sk_buff *skb_a = list_entry(a, struct sk_buff,
+                                            tcp_tsorted_anchor);
+     const struct sk_buff *skb_b = list_entry(b, struct sk_buff,
+                                            tcp_tsorted_anchor);
+
+     return tcp_skb_sent_after(tcp_skb_timestamp_us(skb_a),
+                               tcp_skb_timestamp_us(skb_b),
+                               TCP_SKB_CB(skb_a)->end_seq,
+                               TCP_SKB_CB(skb_b)->end_seq);

Patch LGTM, but perhaps we could avoid the two div_u64() calls here.


        return tcp_skb_sent_after(skb_a->skb_mstamp_ns,
                                  skb_b->skb_mstamp_ns,
                                  TCP_SKB_CB(skb_a)->end_seq,
                                  TCP_SKB_CB(skb_b)->end_seq);

quoted

+}
+
  static void tcp_undo_cwnd_reduction(struct sock *sk, bool unmark_loss)
  {
      struct tcp_sock *tp = tcp_sk(sk);

      if (unmark_loss) {
+             LIST_HEAD(restored);
              struct sk_buff *skb;

              skb_rbtree_walk(skb, &sk->tcp_rtx_queue) {
+                     if (TCP_SKB_CB(skb)->sacked & TCPCB_LOST)
+                             list_move_tail(&skb->tcp_tsorted_anchor,
+                                            &restored);
                      TCP_SKB_CB(skb)->sacked &= ~TCPCB_LOST;
              }
+             if (!list_empty(&restored)) {
+                     struct list_head *pos = &tp->tsorted_sent_queue;
+
+                     /* Ensure lost skbs are added in transmission order */
+                     list_sort(NULL, &restored, tcp_rack_skb_cmp);

Second thoughts (sorry I am currently attending LPC in Prague, little
time this week for reviews)

pw-bot: cr

I am quite concerned about calling list_sort() inside
tcp_undo_cwnd_reduction().

In high-BDP environments (like 100G/200G/400G fabrics), a flow can easily
have thousands of packets in flight. If a loss burst affects, say, 4000
skbs:

1. list_sort() will perform ~45,000 comparisons.
2. With indirect function calls (retpoline cost) and cache misses across
   1MB+ of sk_buff structures, this can freeze the CPU in SoftIRQ under
   the socket lock for multiple milliseconds..

We could avoid list_sort() completely by maintaining a second list,
e.g. tp->tsorted_lost_queue.

Consider the following:

1. When RACK detects loss in tcp_rack_detect_loss(), instead of calling
   list_del_init(&skb->tcp_tsorted_anchor), move the skb to the lost
   queue:

--- a/net/ipv4/tcp_recovery.c
+++ b/net/ipv4/tcp_recovery.c

@@ -101,7 +101,7 @@ static void tcp_rack_detect_loss(struct sock *sk,

u32 *reo_timeout)
  remaining = tcp_rack_skb_timeout(tp, skb, reo_wnd);
  if (remaining <= 0) {
  tcp_mark_skb_lost(sk, skb);
- list_del_init(&skb->tcp_tsorted_anchor);
+ list_move_tail(&skb->tcp_tsorted_anchor, &tp->tsorted_lost_queue);
  } else {

   Because RACK scans tsorted_sent_queue from oldest to newest, packets
   appended to tsorted_lost_queue are already guaranteed to be in strictly
   increasing departure timestamp order.

2. When a packet is retransmitted, tcp_update_skb_after_send() already
   calls:

   list_move_tail(&skb->tcp_tsorted_anchor, &tp->tsorted_sent_queue);

   So retransmitted packets automatically leave tsorted_lost_queue and
   re-enter the tail of tsorted_sent_queue in O(1).

3. When a partial undo occurs in tcp_undo_cwnd_reduction(), any
   un-retransmitted packets remaining in tsorted_lost_queue are already
   in sorted order.

   Since these lost packets were originally sent prior to any subsequent
   transmissions in tsorted_sent_queue, restoring them can often be done
   with an O(1) list splice at the head:

   if (unmark_loss && !list_empty(&tp->tsorted_lost_queue))
           list_splice_init(&tp->tsorted_lost_queue, &tp->tsorted_sent_queue);

quoted

+                     while (!list_empty(&restored)) {
+                             struct list_head *entry = restored.next;
+
+                             while (pos->next != &tp->tsorted_sent_queue &&
+                                    !tcp_rack_skb_cmp(NULL, pos->next,
+                                              entry))
+                                     pos = pos->next;
+                             list_move(entry, pos);
+                             pos = entry;
+                     }
+             }
              tp->lost_out = 0;
              tcp_clear_all_retrans_hints(tp);
      }