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

[Eric Dumazet]

Le mar. 6 oct. 2026 à 08:42, Eric Dumazet <edumazet@xxxxxxxxxx> a écrit :
>
>
>
> On 10/6/26 07:15, nramaswamy@xxxxxxxxxx wrote:
> > From: Neil Ramaswamy <nramaswamy@xxxxxxxxxx>
> >
> > 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@xxxxxxxxx/
> >
> > Fixes: 043b87d7599e ("tcp: more efficient RACK loss detection")
> > Signed-off-by: Neil Ramaswamy <nramaswamy@xxxxxxxxxx>
> > 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);
>
>
>
> > +}
> > +
> >   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);

> > +                     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);
> >       }
>