Re: [PATCH v4 3/4] tcp: Resend SYN for inbound connections

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 16 Oct 2025 10:34:22 +0800
Yumei Huang <yuhuang@redhat.com> wrote:

> If a client connects while guest is not connected or ready yet,
> resend SYN instead of just resetting connection after 10 seconds.
> 
> Use the same backoff calculation for the timeout as linux kernel.
> 
> Signed-off-by: Yumei Huang <yuhuang@redhat.com>
> ---
>  tcp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++--------
>  tcp.h |  2 ++
>  2 files changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 2ec4b0c..3003333 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -179,9 +179,11 @@
>   *
>   * Timeouts are implemented by means of timerfd timers, set based on flags:
>   *
> - * - SYN_TIMEOUT: if no ACK is received from tap/guest during handshake (flag
> - *   ACK_FROM_TAP_DUE without ESTABLISHED event) within this time, reset the
> - *   connection
> + * - SYN_TIMEOUT_INIT: if no ACK is received from tap/guest during handshake
> + *   (flag ACK_FROM_TAP_DUE without ESTABLISHED event) within this time, resend
> + *   SYN. It's the starting timeout for the first SYN retry. If this persists
> + *   for more than TCP_MAX_RETRIES or (tcp_syn_retries +
> + *   tcp_syn_linear_timeouts) times in a row, reset the connection
>   *
>   * - ACK_TIMEOUT: if no ACK segment was received from tap/guest, after sending
>   *   data (flag ACK_FROM_TAP_DUE with ESTABLISHED event), re-send data from the
> @@ -340,7 +342,7 @@ enum {
>  #define WINDOW_DEFAULT			14600		/* RFC 6928 */
>  
>  #define ACK_INTERVAL			10		/* ms */
> -#define SYN_TIMEOUT			10		/* s */
> +#define SYN_TIMEOUT_INIT		1		/* s */
>  #define ACK_TIMEOUT			2
>  #define FIN_TIMEOUT			60
>  #define ACT_TIMEOUT			7200
> @@ -365,6 +367,10 @@ uint8_t tcp_migrate_rcv_queue		[TCP_MIGRATE_RCV_QUEUE_MAX];
>  
>  #define TCP_MIGRATE_RESTORE_CHUNK_MIN	1024 /* Try smaller when above this */
>  
> +#define TCP_SYN_RETRIES_SYSCTL		"/proc/sys/net/ipv4/tcp_syn_retries"
> +#define TCP_SYN_LINEAR_TIMEOUTS_SYSCTL						\
> +	"/proc/sys/net/ipv4/tcp_syn_linear_timeouts"

It's quite obvious those are names of sysctl entries, I think we can
drop the _SYSCTL suffix and keep this shorter without losing any
information/indication.

> +
>  /* "Extended" data (not stored in the flow table) for TCP flow migration */
>  static struct tcp_tap_transfer_ext migrate_ext[FLOW_MAX];
>  
> @@ -581,8 +587,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) {
> -		if (!(conn->events & ESTABLISHED))
> -			it.it_value.tv_sec = SYN_TIMEOUT;
> +		if (!(conn->events & ESTABLISHED)) {
> +			if (conn->retries < c->tcp.syn_linear_timeouts)
> +				it.it_value.tv_sec = SYN_TIMEOUT_INIT;
> +			else
> +				it.it_value.tv_sec = SYN_TIMEOUT_INIT <<
> +					(conn->retries - c->tcp.syn_linear_timeouts);
> +		}
>  		else
>  			it.it_value.tv_sec = ACK_TIMEOUT;
>  	} else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) {
> @@ -2409,8 +2420,16 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
>  		tcp_timer_ctl(c, conn);
>  	} else if (conn->flags & ACK_FROM_TAP_DUE) {
>  		if (!(conn->events & ESTABLISHED)) {
> -			flow_dbg(conn, "handshake timeout");
> -			tcp_rst(c, conn);
> +			if (conn->retries >= MIN(TCP_MAX_RETRIES,
> +				(c->tcp.tcp_syn_retries + c->tcp.syn_linear_timeouts))) {

That doesn't seem to match the sysctl documentation for
tcp_syn_retries, which should be the *total* number of retries, not
excluding the ones with "linear timeouts".

This is pretty hard to read, by the way. It could be:

			if (conn->retries >= TCP_MAX_RETRIES ||
			    conn->retries >= ...) {

> +				flow_dbg(conn, "handshake timeout");
> +				tcp_rst(c, conn);
> +			} else {
> +				flow_trace(conn, "SYN timeout, retry");
> +				tcp_send_flag(c, conn, SYN);
> +				conn->retries++;
> +				tcp_timer_ctl(c, conn);
> +			}
>  		} else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) {
>  			flow_dbg(conn, "FIN timeout");
>  			tcp_rst(c, conn);
> @@ -2766,6 +2785,24 @@ static socklen_t tcp_probe_tcp_info(void)
>  	return sl;
>  }
>  
> +/**
> + * tcp_syn_params_init() - Get initial syn params for inbound connection

SYN, parameters

> + * @c:		Execution context
> +*/
> +void tcp_syn_params_init(struct ctx *c)
> +{
> +	long tcp_syn_retries, syn_linear_timeouts;
> +
> +	tcp_syn_retries = read_file_integer(TCP_SYN_RETRIES_SYSCTL, 8);
> +	syn_linear_timeouts = read_file_integer(TCP_SYN_LINEAR_TIMEOUTS_SYSCTL, 1);
> +
> +	c->tcp.tcp_syn_retries = (uint8_t)MIN(tcp_syn_retries, UINT8_MAX);
> +	c->tcp.syn_linear_timeouts = (uint8_t)MIN(syn_linear_timeouts, UINT8_MAX);

I don't think you need those casts, MIN(..., UINT8_MAX) already
guarantees that the number is <= UINT8_MAX, and in any case the cast
won't fix anything here.

> +
> +	debug("TCP SYN parameters: retries=%"PRIu8", linear_timeouts=%"PRIu8,
> +		  c->tcp.tcp_syn_retries, c->tcp.syn_linear_timeouts);
> +}
> +
>  /**
>   * tcp_init() - Get initial sequence, hash secret, initialise per-socket data
>   * @c:		Execution context
> @@ -2776,6 +2813,8 @@ int tcp_init(struct ctx *c)
>  {
>  	ASSERT(!c->no_tcp);
>  
> +	tcp_syn_params_init(c);
> +
>  	tcp_sock_iov_init(c);
>  
>  	memset(init_sock_pool4,		0xff,	sizeof(init_sock_pool4));
> diff --git a/tcp.h b/tcp.h
> index 234a803..df699a4 100644
> --- a/tcp.h
> +++ b/tcp.h
> @@ -65,6 +65,8 @@ struct tcp_ctx {
>  	struct fwd_ports fwd_out;
>  	struct timespec timer_run;
>  	size_t pipe_size;
> +	uint8_t tcp_syn_retries;
> +	uint8_t syn_linear_timeouts;

These should be added to the documentation for struct tcp_ctx, above.

>  };
>  
>  #endif /* TCP_H */

-- 
Stefano