Re: [PATCH 2/2] tcp: Use runtime tests for TCP_INFO fields

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

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

On Wed, Oct 23, 2024 at 09:41:39AM +0200, Stefano Brivio wrote:
> On Wed, 23 Oct 2024 11:42:53 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > In order to use particular fields from the TCP_INFO getsockopt() we
> > need them to be in the version of the structure we have defined.  We
> > test this in the Makefile, setting HAS_BYTES_ACKED and HAS_MIN_RTT
> > accordingly.
> > 
> > However, we also need the fields to be present in the runtime kernel
> > we're using, which we don't currently check for those fields.  Add
> > logic similar to that for tcpi_snd_wnd to check for these fields too
> > instead of just using the compile time check.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  tcp.c | 65 +++++++++++++++++++++++++++++++++++------------------------
> >  1 file changed, 39 insertions(+), 26 deletions(-)
> > 
> > diff --git a/tcp.c b/tcp.c
> > index 3cca5c6..b5ef1f1 100644
> > --- a/tcp.c
> > +++ b/tcp.c
> > @@ -380,6 +380,19 @@ socklen_t tcp_info_size;
> >  #else
> >  #define snd_wnd_cap	(false)
> >  #endif
> > +#ifdef HAS_BYTES_ACKED
> > +/* Does the kernel report bytes acked in TCP_INFO (kernel commit 0df48c26d84) */
> > +#define bytes_acked_cap	tcp_info_cap(bytes_acked)
> > +#else
> > +#define bytes_acked_cap	(false)
> > +#endif
> > +#ifdef HAS_MIN_RTT
> > +/* Does the kernel report minimum RTT TCP_INFO (kernel commit cd9b266095f4) */
> 
> Nit: "in TCP_INFO". This becomes too long, but perhaps:
> 
> /* Kernel reports minimum RTT in TCP_INFO (kernel commit cd9b266095f4) */
> 
> (and similar above) is just as informative.

Good idea, adjusted.

> 
> > +#define min_rtt_cap	tcp_info_cap(min_rtt)
> > +#else
> > +#define min_rtt_cap	(false)
> > +#endif
> > +
> >  
> >  /* sendmsg() to socket */
> >  static struct iovec	tcp_iov			[UIO_MAXIOV];
> > @@ -687,11 +700,10 @@ static int tcp_rtt_dst_low(const struct tcp_tap_conn *conn)
> >  static void tcp_rtt_dst_check(const struct tcp_tap_conn *conn,
> >  			      const struct tcp_info *tinfo)
> >  {
> > -#ifdef HAS_MIN_RTT
> 
> I think we should keep those conditionals as well, because if
> tcpi_min_rtt is not defined in the headers:
> 
> >  	const struct flowside *tapside = TAPFLOW(conn);
> >  	int i, hole = -1;
> >  
> > -	if (!tinfo->tcpi_min_rtt ||
> > +	if (!min_rtt_cap ||
> >  	    (int)tinfo->tcpi_min_rtt > LOW_RTT_THRESHOLD)
> 
> ...this won't build.

Ah, bother.  Earlier drafts had a field-getter macro which removed the
need for ifdefs outside its definition.  Then I removed it to simplify
things, but forgot that needed the ifdefs to come back.

> >  		return;
> >  
> > @@ -712,10 +724,6 @@ static void tcp_rtt_dst_check(const struct tcp_tap_conn *conn,
> >  	if (hole == LOW_RTT_TABLE_SIZE)
> >  		hole = 0;
> >  	inany_from_af(low_rtt_dst + hole, AF_INET6, &in6addr_any);
> > -#else
> > -	(void)conn;
> > -	(void)tinfo;
> > -#endif /* HAS_MIN_RTT */
> >  }
> >  
> >  /**
> > @@ -1131,30 +1139,29 @@ int tcp_update_seqack_wnd(const struct ctx *c, struct tcp_tap_conn *conn,
> >  	uint32_t new_wnd_to_tap = prev_wnd_to_tap;
> >  	int s = conn->sock;
> >  
> > -#ifndef HAS_BYTES_ACKED
> 
> Same here:
> 
> > -	(void)force_seq;
> > -
> > -	conn->seq_ack_to_tap = conn->seq_from_tap;
> > -	if (SEQ_LT(conn->seq_ack_to_tap, prev_ack_to_tap))
> > -		conn->seq_ack_to_tap = prev_ack_to_tap;
> > -#else
> > -	if ((unsigned)SNDBUF_GET(conn) < SNDBUF_SMALL || tcp_rtt_dst_low(conn)
> > -	    || CONN_IS_CLOSING(conn) || (conn->flags & LOCAL) || force_seq) {
> > +	if (!bytes_acked_cap) {
> >  		conn->seq_ack_to_tap = conn->seq_from_tap;
> > -	} else if (conn->seq_ack_to_tap != conn->seq_from_tap) {
> > -		if (!tinfo) {
> > -			tinfo = &tinfo_new;
> > -			if (getsockopt(s, SOL_TCP, TCP_INFO, tinfo, &sl))
> > -				return 0;
> > -		}
> > -
> > -		conn->seq_ack_to_tap = tinfo->tcpi_bytes_acked +
> > -				       conn->seq_init_from_tap;
> > -
> >  		if (SEQ_LT(conn->seq_ack_to_tap, prev_ack_to_tap))
> >  			conn->seq_ack_to_tap = prev_ack_to_tap;
> > +	} else {
> > +		if ((unsigned)SNDBUF_GET(conn) < SNDBUF_SMALL ||
> > +		    tcp_rtt_dst_low(conn) || CONN_IS_CLOSING(conn) ||
> > +		    (conn->flags & LOCAL) || force_seq) {
> > +			conn->seq_ack_to_tap = conn->seq_from_tap;
> > +		} else if (conn->seq_ack_to_tap != conn->seq_from_tap) {
> > +			if (!tinfo) {
> > +				tinfo = &tinfo_new;
> > +				if (getsockopt(s, SOL_TCP, TCP_INFO, tinfo, &sl))
> > +					return 0;
> > +			}
> > +
> > +			conn->seq_ack_to_tap = tinfo->tcpi_bytes_acked +
> 
> ...this won't build.
> 
> > +				conn->seq_init_from_tap;
> > +
> > +			if (SEQ_LT(conn->seq_ack_to_tap, prev_ack_to_tap))
> > +				conn->seq_ack_to_tap = prev_ack_to_tap;
> > +		}
> >  	}
> > -#endif /* !HAS_BYTES_ACKED */
> >  
> >  	if (!snd_wnd_cap) {
> >  		tcp_get_sndbuf(conn);
> > @@ -2653,6 +2660,12 @@ int tcp_init(struct ctx *c)
> >  #ifdef HAS_SND_WND
> >  	dbg_tcpi(snd_wnd);
> >  #endif
> > +#ifdef HAS_BYTES_ACKED
> > +	dbg_tcpi(bytes_acked);
> > +#endif
> > +#ifdef HAS_MIN_RTT
> > +	dbg_tcpi(min_rtt);
> > +#endif
> >  #undef dbg_tcpi
> >  
> >  	return 0;
> 
> Everything else looks good to me.
> 

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