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=202510 header.b=rcCYXb54; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id C0B495A0619 for ; Mon, 03 Nov 2025 10:37:47 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202510; t=1762162665; bh=QK7tUhRIyaaTCZJjdwGshUyWKj3Wb3ruk5RYeiJ/F9E=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=rcCYXb54+06uYUbwV5hLrmamyks0zu4vugtkWUI6S7xf5CwkYxVrJNST9LZtycfT3 7cIFILnNOYHWwdbxwQ61ZI+LPLQK9fcF2g1erYqxegsBLfT/FwuwjtqOu62OqP59WZ 2DZxMaz+ndUmi5GvyjDAydJUWw3tkLTpN8yHLsD5ovJ270EgdKSsZw0wYUYiM9YWKm /W7IP1tl+xtgGVorsgirl5Gb3kbhCT0k/b7bcZLt8UXrPIaZmuacx6KAzDxEtuo4AI tbpkPO9jItY4vLZv+dK5+yzHWMvEd+WyaVHz5fJoGMJXelOVPYCGNxHw9aXruKCk5o GUnVjhmS+MyBw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4d0RMY5pj9z4w93; Mon, 03 Nov 2025 20:37:45 +1100 (AEDT) Date: Mon, 3 Nov 2025 20:32:14 +1100 From: David Gibson To: Yumei Huang Subject: Re: [PATCH v7 4/5] tcp: Update data retransmission timeout Message-ID: References: <20251031054242.7334-1-yuhuang@redhat.com> <20251031054242.7334-5-yuhuang@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="+MFURZe4Vb102YYo" Content-Disposition: inline In-Reply-To: Message-ID-Hash: KM2MBDYCQIQPJ2W5VMXEYEYMZX6X6VT5 X-Message-ID-Hash: KM2MBDYCQIQPJ2W5VMXEYEYMZX6X6VT5 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, sbrivio@redhat.com 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: --+MFURZe4Vb102YYo Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Nov 03, 2025 at 10:57:27AM +0800, Yumei Huang wrote: > On Mon, Nov 3, 2025 at 9:38=E2=80=AFAM David Gibson wrote: > > > > On Fri, Oct 31, 2025 at 01:42:41PM +0800, Yumei Huang wrote: > > > Use an exponential backoff timeout for data retransmission according > > > to RFC 2988 and RFC 6298. Set the initial RTO to one second as discus= sed > > > in Appendix A of RFC 6298. > > > > > > Also combine the macros defining the initial RTO for both SYN and ACK. > > > > > > Signed-off-by: Yumei Huang > > > Reviewed-by: David Gibson > > > > As reported, the carried over R-b was a minor mistake, since the code > > has changed, but here's a new one: > > > > Reviewed-by: David Gibson > > > > Small comment below, though. > > > > > --- > > > tcp.c | 30 ++++++++++++------------------ > > > 1 file changed, 12 insertions(+), 18 deletions(-) > > > > > > diff --git a/tcp.c b/tcp.c > > > index bada88a..96ee56a 100644 > > > --- a/tcp.c > > > +++ b/tcp.c > > > @@ -179,16 +179,13 @@ > > > * > > > * Timeouts are implemented by means of timerfd timers, set based on= flags: > > > * > > > - * - SYN_TIMEOUT_INIT: if no ACK is received from tap/guest during h= andshake > > > - * (flag ACK_FROM_TAP_DUE without ESTABLISHED event) within this t= ime, resend > > > - * SYN. It's the starting timeout for the first SYN retry. Retry f= or > > > - * TCP_MAX_RETRIES or (tcp_syn_retries + tcp_syn_linear_timeouts) = times, > > > - * reset the connection > > > - * > > > - * - ACK_TIMEOUT: if no ACK segment was received from tap/guest, aft= er sending > > > - * data (flag ACK_FROM_TAP_DUE with ESTABLISHED event), re-send da= ta from the > > > - * socket and reset sequence to what was acknowledged. If this per= sists for > > > - * more than TCP_MAX_RETRIES times in a row, reset the connection > > > + * - RTO_INIT: if no ACK segment was received from tap/guest, either= during > > > + * handshake (flag ACK_FROM_TAP_DUE without ESTABLISHED event) or = after > > > + * sending data (flag ACK_FROM_TAP_DUE with ESTABLISHED event), re= -send data > > > + * from the socket and reset sequence to what was acknowledged. Th= is is the > > > + * timeout for the first retry, in seconds. Retry for TCP_MAX_RETR= IES times > > > + * for established connections, or (tcp_syn_retries + > > > + * tcp_syn_linear_timeouts) times during the handshake, reset the = connection > > > * > > > * - FIN_TIMEOUT: if a FIN segment was sent to tap/guest (flag ACK_F= ROM_TAP_DUE > > > * with TAP_FIN_SENT event), and no ACK is received within this ti= me, reset > > > @@ -342,8 +339,7 @@ enum { > > > #define WINDOW_DEFAULT 14600 /* RFC = 6928 */ > > > > > > #define ACK_INTERVAL 10 /* ms */ > > > -#define SYN_TIMEOUT_INIT 1 /* s, RFC 6928 = */ > > > -#define ACK_TIMEOUT 2 > > > +#define RTO_INIT 1 /* s, RFC 6298 = */ > > > #define FIN_TIMEOUT 60 > > > #define ACT_TIMEOUT 7200 > > > > > > @@ -589,12 +585,10 @@ static void tcp_timer_ctl(const struct ctx *c, = struct tcp_tap_conn *conn) > > > if (conn->flags & ACK_TO_TAP_DUE) { > > > it.it_value.tv_nsec =3D (long)ACK_INTERVAL * 1000 * 100= 0; > > > } else if (conn->flags & ACK_FROM_TAP_DUE) { > > > - if (!(conn->events & ESTABLISHED)) { > > > - int exp =3D conn->retries - c->tcp.syn_linear_t= imeouts; > > > > I didn't spot it in the previous patch, but this is (theoretically) > > buggy. conn->retries is unsigned, so the subtraction will be > > performed unsigned and only then cast to signed. I think that will > > probably do the right thing in practice, but I don't think that's > > guaranteed by the C standard (and might even be UB). >=20 > I'm not sure, but I just googled it. IIUC, the uint8_t (conn->retries > and c->tcp.syn_linear_timeouts) will go through integer promotion > before subtraction. So the line is like: >=20 > int exp =3D (int) conn->retries - (int) c->tcp.syn_linear_timeouts; >=20 > Please correct me if I'm wrong. Huh, I thought it would only be promoted if one of the operands was an int. But C promotion rules are really confusing, so I could well be wrong. >=20 > > > > > - it.it_value.tv_sec =3D SYN_TIMEOUT_INIT << MAX(= exp, 0); > > > - } > > > - else > > > - it.it_value.tv_sec =3D ACK_TIMEOUT; > > > + int exp =3D conn->retries; > > > > This change fixes it, by forcing the cast to a signed int before the > > subtraction. It also removes the minor style error I noted in the > > previous patch. Given that, I don't think we need to worry about > > either of them. > > > > > + if (!(conn->events & ESTABLISHED)) > > > + exp -=3D c->tcp.syn_linear_timeouts; > > > + it.it_value.tv_sec =3D RTO_INIT << MAX(exp, 0); > > > } else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) { > > > it.it_value.tv_sec =3D FIN_TIMEOUT; > > > } else { > > > -- > > > 2.49.0 > > > > > > > -- > > David Gibson (he or they) | I'll have my music baroque, and my co= de > > david AT gibson.dropbear.id.au | minimalist, thank you, not the other = way > > | around. > > http://www.ozlabs.org/~dgibson >=20 >=20 >=20 > --=20 > Thanks, >=20 > Yumei Huang >=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 --+MFURZe4Vb102YYo Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmkIdp0ACgkQzQJF27ox 2Gd/2Q/+IsERnxgguSmN7nFwQzwyCxybvrstpMD5Wx4tZvb+PZuymLLVY8KwS6UA 1kgaXE3mNm38to6hkL64W7zSpkEqocY/c8NVn2Ow0LVRuq8mCShEALE0H7kg0m11 7Qfmzx261AdShY/rvkX4DbU2/c8+yeUmO44GN4rKqiQLJhoy0bFEzp846BbikIjG 3A4+B/JJcCY3STfr8aM2jIVaMKBtZwV3k3JZb3TazUqrdZQIFDrM/PFq/LFzmP3s OfLk30JS4yUAdvKDnuPVy9kYRyvovoeP3Kgiyc7ZQB5+IMVhYS84RTpEQ5CdMh/7 e6ZXZhjIQNLvM43vr3ZaBOOZM2fk1TaThZ0VMMIZvxgRySHXfMTic/A6D8axCa+K 7nllvHILsHGgsiL8ReaSXq454wyaws+JO+wP+qZczxxf57gDdgT1MaKz0sNDUrX7 DfrGSANhl0clqLya/0cAaOpmn/Rrln/On2szarIsWhKPSL9DutvepG5sB2zhxdTH Ts0nTRh+yapSmAEn92sGDRW5lNmhldIgAWUCsuucMrZJRlQcXDeKY2II6ZFelzK1 WmXB5P+G4EOfx2yHcIr7I97M0CPwmwFy13l0JRTlx9CU86ov6E7JJKu+g5+HGjnA gdEd9JGol6j++9rRkOMYndMVSvbOewOF6Utn0QzLTfQosKKh4ww= =NrFo -----END PGP SIGNATURE----- --+MFURZe4Vb102YYo--