Re: [PATCH v7 3/5] tcp: Resend SYN for inbound connections

On Tue, Nov 04, 2025 at 05:42:33AM +0100, Stefano Brivio wrote:

On Fri, 31 Oct 2025 13:42:40 +0800 Yumei Huang wrote: [snip]

...

+/** + * tcp_get_rto_params() - Get host kernel RTO parameters + * @c: Execution context +*/ +void tcp_get_rto_params(struct ctx *c) +{ + intmax_t tcp_syn_retries, syn_linear_timeouts; + + tcp_syn_retries = read_file_integer( + TCP_SYN_RETRIES, TCP_SYN_RETRIES_DEFAULT); + syn_linear_timeouts = read_file_integer( + TCP_SYN_LINEAR_TIMEOUTS, TCP_SYN_LINEAR_TIMEOUTS_DEFAULT);

I think this is a bit hard to read. Now:

tcp_syn_retries = read_file_integer(TCP_SYN_RETRIES, TCP_SYN_RETRIES_DEFAULT); syn_linear_timeouts = read_file_integer(TCP_SYN_LINEAR_TIMEOUTS, TCP_SYN_LINEAR_TIMEOUTS_DEFAULT);

would be a bit closer to the coding style we're adopting, but I wonder:

- does read_file_integer() really need to have "integer" in the name?

In a language where integers are called 'int', perhaps read_file_int() is clear enough?

I think the idea is that read_file_integer() can be used for any (signed) integer type (with range checking performed after the call). read_file_int() might suggest it reads exactly an 'int', not anything bigger or smaller.

- the constants are defined here, in tcp.c, so they obviously refer to TCP, so maybe SYN_LINEAR_TIMEOUTS is clear enough

- you don't really need to store both values in two different variables, one is enough as you're assigning them right away. And:

v = read_file_int(SYN_RETRIES, SYN_RETRIES_DEFAULT); c->tcp.tcp_syn_retries = MIN(v, UINT8_MAX);

v = read_file_int(SYN_LINEAR_TIMEOUTS, SYN_LINEAR_TIMEOUTS_DEFAULT); c->tcp.syn_linear_timeouts = MIN(v, UINT8_MAX);

is four lines instead of six, and more readable if you ask me.

...

+ + c->tcp.tcp_syn_retries = MIN(tcp_syn_retries, UINT8_MAX); + c->tcp.syn_linear_timeouts = MIN(syn_linear_timeouts, UINT8_MAX); + + debug("Read sysctl values tcp_syn_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 +2815,8 @@ int tcp_init(struct ctx *c) { ASSERT(!c->no_tcp);

+ tcp_get_rto_params(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..befedde 100644 --- a/tcp.h +++ b/tcp.h @@ -59,12 +59,16 @@ 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 + * @tcp_syn_retries: SYN retries using exponential backoff timeout + * @syn_linear_timeouts: SYN retries before using exponential backoff timeout */ struct tcp_ctx { struct fwd_ports fwd_in; struct fwd_ports fwd_out; struct timespec timer_run; size_t pipe_size; + uint8_t tcp_syn_retries;

Why 'tcp' again?

...

+ uint8_t syn_linear_timeouts; };

#endif /* TCP_H */

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