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

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

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

On Tue, Nov 04, 2025 at 05:42:33AM +0100, Stefano Brivio wrote:
> On Fri, 31 Oct 2025 13:42:40 +0800
> Yumei Huang <yuhuang@redhat.com> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]