Re: [PATCH v8 6/6] tcp: Clamp the retry timeout

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 9847 bytes --]

On Fri, Nov 14, 2025 at 11:05:51AM +0800, Yumei Huang wrote:
> On Fri, Nov 14, 2025 at 8:47 AM David Gibson
> <david@gibson.dropbear.id.au> wrote:
> >
> > On Fri, Nov 14, 2025 at 01:01:21AM +0100, Stefano Brivio wrote:
> > > On Mon, 10 Nov 2025 21:56:39 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >
> > > > On Mon, Nov 10, 2025 at 05:31:37PM +0800, Yumei Huang wrote:
> > > > > Clamp the TCP retry timeout as Linux kernel does. If a retry occurs
> > > > > during the handshake and the RTO is below 3 seconds, re-initialise
> > > > > it to 3 seconds for data retransmissions according to RFC 6298.
> > > > >
> > > > > Suggested-by: Stefano Brivio <sbrivio@redhat.com>
> > > > > Signed-off-by: Yumei Huang <yuhuang@redhat.com>
> > > >
> > > > Looks correct, so
> > > >
> > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > > >
> > > > A few possible improvements (mostly comments/names) below.
> > > >
> > > > > ---
> > > > >  tcp.c      | 25 ++++++++++++++++++++-----
> > > > >  tcp.h      |  2 ++
> > > > >  tcp_conn.h |  1 +
> > > > >  3 files changed, 23 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/tcp.c b/tcp.c
> > > > > index ee111e0..b015658 100644
> > > > > --- a/tcp.c
> > > > > +++ b/tcp.c
> > > > > @@ -187,6 +187,9 @@
> > > > >   *   for established connections, or (syn_retries + syn_linear_timeouts) times
> > > > >   *   during the handshake, reset the connection
> > > > >   *
> > > > > + * - RTO_INIT_ACK: if the RTO is less than this, re-initialise RTO to this for
> > > > > + *   data retransmissions
> > > > > + *
> > > >
> > > > Now that this is conditional on SYN being retried, the description
> > > > doesn't look entirely accurate any more.
> > >
> > > If I read the whole thing again I would actually say it becomes
> > > misleading and worth fixing before applying this. Also because the
> > > relationship to the text in the RFC is not obvious anymore.
> > >
> > > This should be "if SYN retries happened during handshake, ...".
> > >
> > > > >   * - FIN_TIMEOUT: if a FIN segment was sent to tap/guest (flag ACK_FROM_TAP_DUE
> > > > >   *   with TAP_FIN_SENT event), and no ACK is received within this time, reset
> > > > >   *   the connection
> > > > > @@ -340,6 +343,7 @@ enum {
> > > > >
> > > > >  #define ACK_INTERVAL                     10              /* ms */
> > > > >  #define RTO_INIT                 1               /* s, RFC 6298 */
> > > > > +#define RTO_INIT_ACK                     3               /* s, RFC 6298 */
> > > >
> > > > The relevance of "_ACK" in the name is not obvious to me.
> > >
> > > What about RTO_INIT_AFTER_SYN_RETRIES? We use it only once, and it
> > > looks fine there.
> >
> > Works for me.
> >
> > > > >  #define FIN_TIMEOUT                      60
> > > > >  #define ACT_TIMEOUT                      7200
> > > > >
> > > > -> @@ -365,9 +369,11 @@ uint8_t tcp_migrate_rcv_queue               [TCP_MIGRATE_RCV_QUEUE_MAX];
> > > > >
> > > > >  #define SYN_RETRIES              "/proc/sys/net/ipv4/tcp_syn_retries"
> > > > >  #define SYN_LINEAR_TIMEOUTS      "/proc/sys/net/ipv4/tcp_syn_linear_timeouts"
> > > > > +#define RTO_MAX_MS               "/proc/sys/net/ipv4/tcp_rto_max_ms"
> > > > >
> > > > >  #define SYN_RETRIES_DEFAULT              6
> > > > >  #define SYN_LINEAR_TIMEOUTS_DEFAULT      4
> > > > > +#define RTO_MAX_MS_DEFAULT               120000
> > > > >  #define MAX_SYNCNT                       127 /* derived from kernel's limit */
> > > > >
> > > > >  /* "Extended" data (not stored in the flow table) for TCP flow migration */
> > > > > @@ -392,7 +398,7 @@ static const char *tcp_state_str[] __attribute((__unused__)) = {
> > > > >
> > > > >  static const char *tcp_flag_str[] __attribute((__unused__)) = {
> > > > >   "STALLED", "LOCAL", "ACTIVE_CLOSE", "ACK_TO_TAP_DUE",
> > > > > - "ACK_FROM_TAP_DUE", "ACK_FROM_TAP_BLOCKS",
> > > > > + "ACK_FROM_TAP_DUE", "ACK_FROM_TAP_BLOCKS", "SYN_RETRIED",
> > > > >  };
> > > > >
> > > > >  /* Listening sockets, used for automatic port forwarding in pasta mode only */
> > > > > @@ -590,10 +596,13 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
> > > > >   if (conn->flags & ACK_TO_TAP_DUE) {
> > > > >           it.it_value.tv_nsec = (long)ACK_INTERVAL * 1000 * 1000;
> > > > >   } else if (conn->flags & ACK_FROM_TAP_DUE) {
> > > > > -         int exp = conn->retries;
> > > > > +         int exp = conn->retries, timeout = RTO_INIT;
> > > > >           if (!(conn->events & ESTABLISHED))
> > > > >                   exp -= c->tcp.syn_linear_timeouts;
> > > > > -         it.it_value.tv_sec = RTO_INIT << MAX(exp, 0);
> > > > > +         else if (conn->flags & SYN_RETRIED)
> > > > > +                 timeout = MAX(timeout, RTO_INIT_ACK);
> > > > > +         timeout <<= MAX(exp, 0);
> > > > > +         it.it_value.tv_sec = MIN(timeout, c->tcp.rto_max);
> > > > >   } else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) {
> > > > >           it.it_value.tv_sec = FIN_TIMEOUT;
> > > > >   } else {
> > > > > @@ -2440,6 +2449,7 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
> > > > >                           flow_trace(conn, "SYN timeout, retry");
> > > > >                           tcp_send_flag(c, conn, SYN);
> > > > >                           conn->retries++;
> > > > > +                         conn_flag(c, conn, SYN_RETRIED);
> > > > >                           tcp_timer_ctl(c, conn);
> > > > >                   }
> > > > >           } else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) {
> > > > > @@ -2811,10 +2821,15 @@ void tcp_get_rto_params(struct ctx *c)
> > > > >   v = read_file_integer(SYN_LINEAR_TIMEOUTS, SYN_LINEAR_TIMEOUTS_DEFAULT);
> > > > >   c->tcp.syn_linear_timeouts = MIN(v, MAX_SYNCNT);
> > > > >
> > > > > + v = read_file_integer(RTO_MAX_MS, RTO_MAX_MS_DEFAULT);
> > > > > + c->tcp.rto_max = MIN(DIV_ROUND_CLOSEST(v, 1000), INT_MAX);
> > > >
> > > > Possibly we should verify this is => RTO_INIT.
> > >
> > > As a sanity check, maybe, but I don't see any harmful effect if it's
> > > < RTO_INIT, right? So I'm not sure if we should. No preference from my
> > > side really.
> >
> > Sorry, describing this as >= RTO_INIT was misleading.  What I'm
> > concerned about here is if the kernel value is set to 400ms, we'll
> > round it to... 0s.
> >
> > So, really what I'm concerned about is that we ensure this is > 0.
> 
> That's a good point.

Actually, thinking about it, I wonder if makes more sense to always
round up to 1s, rather than to the nearest 1s.


> > > > > +
> > > > >   debug("Read sysctl values syn_retries: %"PRIu8
> > > > > -       ", syn_linear_timeouts: %"PRIu8,
> > > > > +       ", syn_linear_timeouts: %"PRIu8
> > > > > +       ", rto_max: %d",
> > > > >         c->tcp.syn_retries,
> > > > > -       c->tcp.syn_linear_timeouts);
> > > > > +       c->tcp.syn_linear_timeouts,
> > > > > +       c->tcp.rto_max);
> > > > >  }
> > > > >
> > > > >  /**
> > > > > diff --git a/tcp.h b/tcp.h
> > > > > index 37d7758..c4945c3 100644
> > > > > --- a/tcp.h
> > > > > +++ b/tcp.h
> > > > > @@ -60,6 +60,7 @@ union tcp_listen_epoll_ref {
> > > > >   * @fwd_out:             Port forwarding configuration for outbound packets
> > > > >   * @timer_run:           Timestamp of most recent timer run
> > > > >   * @pipe_size:           Size of pipes for spliced connections
> > > > > + * @rto_max:             Maximal retry timeout (in s)
> > >
> > > As pointed out earlier, I think "maximum" is slightly more appropriate
> > > here.
> >
> > Agreed, with nuance discussed earlier.
> 
> Well, I must have misunderstood something in last week's meeting. I
> joined the second half meeting a few mins later and I caught Stefano
> saying he was wrong about maximal. Probably he was talking about the
> French meaning. Anyway, I will update.

Sorry, yeah, the discussion there was probably fairly confusing - I
think both Stefano and I got a bit distracted from the question at
hand by language-nerding.

The upshot is that while I think both "maximal" and "maximum" would be
correct, I think "maximum" is slightly clearer.

> >
> > >
> > > > >   * @syn_retries: SYN retries using exponential backoff timeout
> > > > >   * @syn_linear_timeouts: SYN retries before using exponential backoff timeout
> > > > >   */
> > > > > @@ -68,6 +69,7 @@ struct tcp_ctx {
> > > > >   struct fwd_ports fwd_out;
> > > > >   struct timespec timer_run;
> > > > >   size_t pipe_size;
> > > > > + int rto_max;
> > > > >   uint8_t syn_retries;
> > > > >   uint8_t syn_linear_timeouts;
> > > > >  };
> > > > > diff --git a/tcp_conn.h b/tcp_conn.h
> > > > > index 923af36..e36910c 100644
> > > > > --- a/tcp_conn.h
> > > > > +++ b/tcp_conn.h
> > > > > @@ -77,6 +77,7 @@ struct tcp_tap_conn {
> > > > >  #define ACK_TO_TAP_DUE           BIT(3)
> > > > >  #define ACK_FROM_TAP_DUE BIT(4)
> > > > >  #define ACK_FROM_TAP_BLOCKS      BIT(5)
> > > > > +#define SYN_RETRIED              BIT(6)
> > > > >
> > > > >  #define SNDBUF_BITS              24
> > > > >   unsigned int    sndbuf          :SNDBUF_BITS;
> > > > > --
> > > > > 2.51.0
> > >
> > > --
> > > Stefano
> > >
> >
> > --
> > David Gibson (he or they)       | I'll have my music baroque, and my code
> > david AT gibson.dropbear.id.au  | minimalist, thank you, not the other way
> >                                 | around.
> > http://www.ozlabs.org/~dgibson
> 
> 
> 
> -- 
> Thanks,
> 
> Yumei Huang
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]