Re: [PATCH v3 04/10] tcp: Adaptive interval based on RTT for socket-side acknowledgement checks

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 9 Dec 2025 16:10:36 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Dec 08, 2025 at 08:20:17AM +0100, Stefano Brivio wrote:
> > A fixed 10 ms ACK_INTERVAL timer value served us relatively well until
> > the previous change, because we would generally cause retransmissions
> > for non-local outbound transfers with relatively high (> 100 Mbps)
> > bandwidth and non-local but low (< 5 ms) RTT.
> > 
> > Now that retransmissions are less frequent, we don't have a proper
> > trigger to check for acknowledged bytes on the socket, and will
> > generally block the sender for a significant amount of time while
> > we could acknowledge more data, instead.
> > 
> > Store the RTT reported by the kernel using an approximation (exponent),
> > to keep flow storage size within two (typical) cachelines. Check for
> > socket updates when half of this time elapses: it should be a good
> > indication of the one-way delay we're interested in (peer to us).
> > 
> > Representable values are between 100 us and 3.2768 s, and any value
> > outside this range is clamped to these bounds. This choice appears
> > to be a good trade-off between additional overhead and throughput.
> > 
> > This mechanism partially overlaps with the "low RTT" destinations,
> > which we use to infer that a socket is connected to an endpoint to
> > the same machine (while possibly in a different namespace) if the
> > RTT is reported as 10 us or less.
> > 
> > This change doesn't, however, conflict with it: we are reading
> > TCP_INFO parameters for local connections anyway, so we can always
> > store the RTT approximation opportunistically.
> > 
> > Then, if the RTT is "low", we don't really need a timer to
> > acknowledge data as we'll always acknowledge everything to the
> > sender right away. However, we have limited space in the array where
> > we store addresses of local destination, so the low RTT property of a
> > connection might toggle frequently. Because of this, it's actually
> > helpful to always have the RTT approximation stored.
> > 
> > This could probably benefit from a future rework, though, introducing
> > a more integrated approach between these two mechanisms.
> > 
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  tcp.c      | 30 +++++++++++++++++++++++-------
> >  tcp_conn.h |  9 +++++++++
> >  util.c     | 14 ++++++++++++++
> >  util.h     |  1 +
> >  4 files changed, 47 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tcp.c b/tcp.c
> > index 28d3304..0167121 100644
> > --- a/tcp.c
> > +++ b/tcp.c
> > @@ -202,9 +202,13 @@
> >   * - ACT_TIMEOUT, in the presence of any event: if no activity is detected on
> >   *   either side, the connection is reset
> >   *
> > - * - ACK_INTERVAL elapsed after data segment received from tap without having
> > + * - RTT / 2 elapsed after data segment received from tap without having
> >   *   sent an ACK segment, or zero-sized window advertised to tap/guest (flag
> > - *   ACK_TO_TAP_DUE): forcibly check if an ACK segment can be sent
> > + *   ACK_TO_TAP_DUE): forcibly check if an ACK segment can be sent.
> > + *
> > + *   RTT, here, is an approximation of the RTT value reported by the kernel via
> > + *   TCP_INFO, with a representable range from RTT_STORE_MIN (100 us) to
> > + *   RTT_STORE_MAX (3276.8 ms). The timeout value is clamped accordingly.
> >   *
> >   *
> >   * Summary of data flows (with ESTABLISHED event)
> > @@ -341,7 +345,6 @@ enum {
> >  #define MSS_DEFAULT			536
> >  #define WINDOW_DEFAULT			14600		/* RFC 6928 */
> >  
> > -#define ACK_INTERVAL			10		/* ms */
> >  #define RTO_INIT			1		/* s, RFC 6298 */
> >  #define RTO_INIT_AFTER_SYN_RETRIES	3		/* s, RFC 6298 */
> >  #define FIN_TIMEOUT			60
> > @@ -593,7 +596,8 @@ 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;
> > +		it.it_value.tv_sec = RTT_GET(conn) / 2 / (1000 * 1000);
> > +		it.it_value.tv_nsec = RTT_GET(conn) / 2 % (1000 * 1000) * 1000;
> >  	} else if (conn->flags & ACK_FROM_TAP_DUE) {
> >  		int exp = conn->retries, timeout = RTO_INIT;
> >  		if (!(conn->events & ESTABLISHED))
> > @@ -608,9 +612,17 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
> >  		it.it_value.tv_sec = ACT_TIMEOUT;
> >  	}
> >  
> > -	flow_dbg(conn, "timer expires in %llu.%03llus",
> > -		 (unsigned long long)it.it_value.tv_sec,
> > -		 (unsigned long long)it.it_value.tv_nsec / 1000 / 1000);
> > +	if (conn->flags & ACK_TO_TAP_DUE) {
> > +		flow_trace(conn, "timer expires in %llu.%03llums",
> > +			   (unsigned long)it.it_value.tv_sec * 1000 +
> > +			   (unsigned long long)it.it_value.tv_nsec %
> > +					       (1000 * 1000),
> > +			   (unsigned long long)it.it_value.tv_nsec / 1000);
> 
> This is the wrong way around.  The ms part needs to be:

Ouch, I... half swapped them. Almost. This risks haunting us for a bit.

Luckily we have discrete values there so I could make this small table
for --trace logs from the broken implementation:

#include <stdio.h>

int main()
{
	unsigned long rtt, n, s;

	printf("before fix\t\tafter fix\tRTT\n");
	for (rtt = 100; rtt < 100 << 16; rtt <<= 1) {
		s = rtt / 2 / (1000 * 1000);
		n = rtt / 2 % (1000 * 1000) * 1000;

		printf("%llu.%03llu\t\t%lu.%02lu\t\t%lu\n",
		       s * 1000 + n % (1000 * 1000), n / 1000,
		       s * 1000 + n / (1000 * 1000), (n / 10000) % 100,
		       rtt);
	}
}

before fix		after fix	RTT
50000.050		0.05		100
100000.100		0.10		200
200000.200		0.20		400
400000.400		0.40		800
800000.800		0.80		1600
600000.1600		1.60		3200
200000.3200		3.20		6400
400000.6400		6.40		12800
800000.12800		12.80		25600
600000.25600		25.60		51200
200000.51200		51.20		102400
400000.102400		102.40		204800
800000.204800		204.80		409600
600000.409600		409.60		819200
200000.819200		819.20		1638400
401000.638400		1638.40		3276800

> 	tv_sec * 1000 + tv_nsec / 1000000
> and the fractional (us) part:
> 	(tv_nsec / 1000) % 1000
> 
> (or if you did just want a single digit after the ., then:
> 	tv_nsec / 100000 % 10
> )

I guess I'll display two digits instead, I never had a ~100 ns RTT in
my tests so I didn't notice until now that the resulting timeout would
be displayed as 0.0ms with one fractional digit.

Thanks for the snippet, I'll post a patch soon (of course, feel free to
do so meanwhile if you already have a local change...).

-- 
Stefano