On Fri, Aug 15, 2025 at 06:10:38PM +0200, Stefano Brivio wrote:
> ...as I'm going to need a third occurrence of this in the next change.
> 
> This introduces a small functional change in tcp_data_from_tap(): the
> sequence was previously rewound to the highest ACK number we found in
> the current packet batch, and not to the current value of
> seq_ack_from_tap.
> 
> The two might differ in case tcp_sock_consume() failed, because in
> that case we're ignoring that ACK altogether. But if we're ignoring
> it, it looks more correct to me to start retransmitting from an
> earlier sequence anyway.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  tcp.c | 47 ++++++++++++++++++++++++++++++++---------------
>  1 file changed, 32 insertions(+), 15 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 7c1f237..1402ca2 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1097,6 +1097,26 @@ static void tcp_update_seqack_from_tap(const struct ctx *c,
>  	}
>  }
>  
> +/**
> + * tcp_rewind_seq() - Rewind sequence to tap and socket offset to current ACK
> + * @c:		Execution context
> + * @conn:	Connection pointer
> + *
> + * Return: 0 on success, -1 on failure, with connection reset
> + */
> +static int tcp_rewind_seq(const struct ctx *c, struct tcp_tap_conn *conn)
> +{
> +	conn->seq_to_tap = conn->seq_ack_from_tap;
> +	conn->events &= ~TAP_FIN_SENT;
> +
> +	if (tcp_set_peek_offset(conn, 0)) {
> +		tcp_rst(c, conn);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * tcp_prepare_flags() - Prepare header for flags-only segment (no payload)
>   * @c:		Execution context
> @@ -1757,13 +1777,11 @@ static int tcp_data_from_tap(const struct ctx *c, struct tcp_tap_conn *conn,
>  	if (retr) {
>  		flow_trace(conn,
>  			   "fast re-transmit, ACK: %u, previous sequence: %u",
> -			   max_ack_seq, conn->seq_to_tap);
> -		conn->seq_to_tap = max_ack_seq;
> -		conn->events &= ~TAP_FIN_SENT;
> -		if (tcp_set_peek_offset(conn, 0)) {
> -			tcp_rst(c, conn);
> +			   conn->seq_ack_from_tap, conn->seq_to_tap);
> +
> +		if (tcp_rewind_seq(c, conn))
>  			return -1;
> -		}
> +
>  		tcp_data_from_sock(c, conn);
>  	}
>  
> @@ -2285,17 +2303,16 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
>  			tcp_rst(c, conn);
>  		} else {
>  			flow_dbg(conn, "ACK timeout, retry");
> -			conn->retrans++;
> -			conn->seq_to_tap = conn->seq_ack_from_tap;
> -			conn->events &= ~TAP_FIN_SENT;
> +
>  			if (!conn->wnd_from_tap)
>  				conn->wnd_from_tap = 1; /* Zero-window probe */
> -			if (tcp_set_peek_offset(conn, 0)) {
> -				tcp_rst(c, conn);
> -			} else {
> -				tcp_data_from_sock(c, conn);
> -				tcp_timer_ctl(c, conn);
> -			}
> +
> +			conn->retrans++;
> +			if (tcp_rewind_seq(c, conn))
> +				return;
> +
> +			tcp_data_from_sock(c, conn);
> +			tcp_timer_ctl(c, conn);
>  		}
>  	} else {
>  		struct itimerspec new = { { 0 }, { ACT_TIMEOUT, 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