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

[Stefano Brivio <sbrivio@redhat.com>]

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.

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

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

-- 
Stefano