Re: [PATCH v2 3/3] tcp: Update data retransmission timeout

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

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

On Fri, Oct 10, 2025 at 03:47:00PM +0800, Yumei Huang wrote:
> Use an exponential backoff timeout with the initial timeout 1s
> and total timeout 60s.

The commit message needs more information on why making this change to
behaviour is desirable (e.g. referencing RFCs).

> 
> Signed-off-by: Yumei Huang <yuhuang@redhat.com>
> ---
>  tcp.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 85bbdac..0db7f30 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -185,10 +185,15 @@
>   *   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
> - *   socket and reset sequence to what was acknowledged. If this persists for
> - *   more than TCP_MAX_RETRIES times in a row, reset the connection
> + * - ACK_TIMEOUT_INIT: 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 socket and reset sequence to what was acknowledged. It's the
> + *   starting timeout for the first retransmission. If this persists for more
> + *   than TCP_MAX_RETRIES times in a row, reset the connection
> + *
> + * - ACK_TIMEOUT_TOTAL: if no ACK segment was received from tap/guest after
> + *   retransmitting data repeatedly from the socket within this time, reset
> + *   the connection
>   *
>   * - 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
> @@ -344,7 +349,8 @@ enum {
>  
>  #define ACK_INTERVAL			10		/* ms */
>  #define SYN_TIMEOUT_INIT		1		/* s */
> -#define ACK_TIMEOUT			2
> +#define ACK_TIMEOUT_INIT		1
> +#define ACK_TIMEOUT_TOTAL		60
>  #define FIN_TIMEOUT			60
>  #define ACT_TIMEOUT			7200
>  
> @@ -358,6 +364,9 @@ enum {
>  	 ((conn)->events & (SOCK_FIN_RCVD | TAP_FIN_RCVD)))
>  #define CONN_HAS(conn, set)	(((conn)->events & (set)) == (set))
>  
> +#define RETRY_ELAPSED(timeout_init, retries)		\
> +	((timeout_init) * ((1 << ((retries) + 1)) - 2))
> +
>  /* Buffers to migrate pending data from send and receive queues. No, they don't
>   * use memory if we don't use them. And we're going away after this, so splurge.
>   */
> @@ -596,7 +605,7 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
>  					(conn->retries - c->tcp.syn_linear_timeouts);
>  		}
>  		else
> -			it.it_value.tv_sec = ACK_TIMEOUT;
> +			it.it_value.tv_sec = ACK_TIMEOUT_INIT << conn->retries;
>  	} else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) {
>  		it.it_value.tv_sec = FIN_TIMEOUT;
>  	} else {
> @@ -2438,6 +2447,10 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
>  		} else if (conn->retries == TCP_MAX_RETRIES) {
>  			flow_dbg(conn, "retransmissions count exceeded");
>  			tcp_rst(c, conn);
> +		} else if(RETRY_ELAPSED(ACK_TIMEOUT_INIT, conn->retries) >=
> +			ACK_TIMEOUT_TOTAL) {
> +			flow_dbg(conn, "retransmissions timeout exceeded");
> +			tcp_rst(c, conn);

Having a test both for number of retries and time elapsed seems
redundant.  RETRY_ELAPSED is pure function of the number of retries,
so it should be possible to just have a threshold on the number of
retries, which can be calculated from the target total timeout.


>  		} else {
>  			flow_dbg(conn, "ACK timeout, retry");
>  
> -- 
> 2.47.0
> 

-- 
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 --]