From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202410 header.b=bGx52uT/; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id C0A705A004C for ; Thu, 24 Oct 2024 05:38:42 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202410; t=1729741110; bh=62RI4wIrAOMJ7kuboiw63bOAJQ34tzsEmqK3Yy8oDhI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=bGx52uT/q4LycLQ6l6DcTVzTVXV7Qf+7ZjNFiJykgUeOG5wYpKyD4VzQ78MIVQEle je5YL897ichJB7pNk/Fu3+o6x8y+MCN35T6ZVwdXssk9esqLa0MrG9ucSBgKYTWTGA T8PEXTn90DVVdxI4iztywhHooxieWs84/+MVBcnC7FRY9XPdxwtsKaPxG9Tj0osQNB 2tLiLdPPIeCHOqwx1ywmnEh1N0MjmUiBMHAJQLAllR6wHSEmxMDY4YI/llIdr1TLBO ChxiVCj7JDekrj52ISBkjEpwF+cQrbTLYx78JC6gZfKBfEpg2BlthFTjQ9H/yodjy2 pAaCLe24o9DdA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4XYs863qVkz4wc3; Thu, 24 Oct 2024 14:38:30 +1100 (AEDT) Date: Thu, 24 Oct 2024 13:02:45 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 2/2] tcp: Use runtime tests for TCP_INFO fields Message-ID: References: <20241023004253.1729124-1-david@gibson.dropbear.id.au> <20241023004253.1729124-3-david@gibson.dropbear.id.au> <20241023094139.4e94b322@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="U3PvfMMfnf9UD2H6" Content-Disposition: inline In-Reply-To: <20241023094139.4e94b322@elisabeth> Message-ID-Hash: BI3G5P6JEE3RFZEUEQMMGY74F356EI5Z X-Message-ID-Hash: BI3G5P6JEE3RFZEUEQMMGY74F356EI5Z X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --U3PvfMMfnf9UD2H6 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Oct 23, 2024 at 09:41:39AM +0200, Stefano Brivio wrote: > On Wed, 23 Oct 2024 11:42:53 +1100 > David Gibson wrote: >=20 > > 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. > >=20 > > 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. > >=20 > > Signed-off-by: David Gibson > > --- > > tcp.c | 65 +++++++++++++++++++++++++++++++++++------------------------ > > 1 file changed, 39 insertions(+), 26 deletions(-) > >=20 > > 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 0df48= c26d84) */ > > +#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 cd9b2660= 95f4) */ >=20 > Nit: "in TCP_INFO". This becomes too long, but perhaps: >=20 > /* Kernel reports minimum RTT in TCP_INFO (kernel commit cd9b266095f4) */ >=20 > (and similar above) is just as informative. Good idea, adjusted. >=20 > > +#define min_rtt_cap tcp_info_cap(min_rtt) > > +#else > > +#define min_rtt_cap (false) > > +#endif > > + > > =20 > > /* sendmsg() to socket */ > > static struct iovec tcp_iov [UIO_MAXIOV]; > > @@ -687,11 +700,10 @@ static int tcp_rtt_dst_low(const struct tcp_tap_c= onn *conn) > > static void tcp_rtt_dst_check(const struct tcp_tap_conn *conn, > > const struct tcp_info *tinfo) > > { > > -#ifdef HAS_MIN_RTT >=20 > I think we should keep those conditionals as well, because if > tcpi_min_rtt is not defined in the headers: >=20 > > const struct flowside *tapside =3D TAPFLOW(conn); > > int i, hole =3D -1; > > =20 > > - if (!tinfo->tcpi_min_rtt || > > + if (!min_rtt_cap || > > (int)tinfo->tcpi_min_rtt > LOW_RTT_THRESHOLD) >=20 > ...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; > > =20 > > @@ -712,10 +724,6 @@ static void tcp_rtt_dst_check(const struct tcp_tap= _conn *conn, > > if (hole =3D=3D LOW_RTT_TABLE_SIZE) > > hole =3D 0; > > inany_from_af(low_rtt_dst + hole, AF_INET6, &in6addr_any); > > -#else > > - (void)conn; > > - (void)tinfo; > > -#endif /* HAS_MIN_RTT */ > > } > > =20 > > /** > > @@ -1131,30 +1139,29 @@ int tcp_update_seqack_wnd(const struct ctx *c, = struct tcp_tap_conn *conn, > > uint32_t new_wnd_to_tap =3D prev_wnd_to_tap; > > int s =3D conn->sock; > > =20 > > -#ifndef HAS_BYTES_ACKED >=20 > Same here: >=20 > > - (void)force_seq; > > - > > - conn->seq_ack_to_tap =3D conn->seq_from_tap; > > - if (SEQ_LT(conn->seq_ack_to_tap, prev_ack_to_tap)) > > - conn->seq_ack_to_tap =3D 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 =3D conn->seq_from_tap; > > - } else if (conn->seq_ack_to_tap !=3D conn->seq_from_tap) { > > - if (!tinfo) { > > - tinfo =3D &tinfo_new; > > - if (getsockopt(s, SOL_TCP, TCP_INFO, tinfo, &sl)) > > - return 0; > > - } > > - > > - conn->seq_ack_to_tap =3D 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 =3D 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 =3D conn->seq_from_tap; > > + } else if (conn->seq_ack_to_tap !=3D conn->seq_from_tap) { > > + if (!tinfo) { > > + tinfo =3D &tinfo_new; > > + if (getsockopt(s, SOL_TCP, TCP_INFO, tinfo, &sl)) > > + return 0; > > + } > > + > > + conn->seq_ack_to_tap =3D tinfo->tcpi_bytes_acked + >=20 > ...this won't build. >=20 > > + conn->seq_init_from_tap; > > + > > + if (SEQ_LT(conn->seq_ack_to_tap, prev_ack_to_tap)) > > + conn->seq_ack_to_tap =3D prev_ack_to_tap; > > + } > > } > > -#endif /* !HAS_BYTES_ACKED */ > > =20 > > 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 > > =20 > > return 0; >=20 > Everything else looks good to me. >=20 --=20 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 --U3PvfMMfnf9UD2H6 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmcZqqIACgkQzQJF27ox 2Gc+OQ/+OKnkCIp9uOZOJhEGUe+wrzqfyGy1KSo7GemoxpydHFv7IvIQutaEDr+g UTmQ8W3mILZJS4zo+lb7OK4DgIugXdsDZj3oeDGkwVCrIm4q6MOZEttuGQAwPP2r jQXDui6A6rUd+MrF0PeKV6LS0gePgUKQZc5Uu7OdLKuYNeT1qJNcQ5Cx9GGCgu3a oNQMiV+TS3Cs79mBtLujuk1cWytpJbxSFnakoLx18MMJ5gAqiIbJBEtG1Rob2YoM 5hD6RQ/GuaK1PyInRnY87QBXE3rTSTSNHF+8O1waRWboEIm3MmarGxZwvOImGg1Y DVV6offRwWCbnUyR1HJhtNxmBQ+/cA5WxoqELtQNs+ME5VmAog1CzR08NijHXLG/ aN/CGMxQJf5ag8rn4i8L04u1Go9jYjf7iYDWKe4VeIqPPWuKNKEiIaM57pO3ogb4 DW5exR+0uisHEbxw6mzSfVZr3/Yuzl70ypWJwMjq6UDiMD3mMM3sQKcmWq8fPg/W jeLlhJtGyFIwoIVMUO2DXfAMwQkIUg7TMnyQnAi4Mj3WlPZAmzc7EogIKSg2Xjnl FB3S6SC6ek5X9pfF/FLrmYiiN8MUnZnH56cOd2CLf6qk8o/oGIdd6IWMPz+myZcN 35hkE1539uXVYEgcozaH7Z4QD4Ny8cCAMlP68aU03CVKr/ImWKU= =6wN2 -----END PGP SIGNATURE----- --U3PvfMMfnf9UD2H6--