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

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 5 Nov 2025 15:24:44 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

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

Oh, I see. It could be read_file_num() then, even if it's slightly less
accurate, or it can even remain read_file_integer(). If something like
this:

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

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

fits 80 columns, I'm taking it as a sign that the function name isn't
exceedingly long. No particular preference from me.

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