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

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(a)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(a)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