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=tadMNfyM; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id C4FF95A026F for ; Mon, 20 Oct 2025 02:21:00 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202510; t=1760919657; bh=isM2k2VPlpjUwoslF2m78cyqqQ306mQdmuXWPtZ+w/k=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=tadMNfyMFC9egR3MIaiav3C7KpXnmjy1p1nemhBSodKsfjTKSPwZuMvPHMI7nHkrk y3eZU5teOYqJLWsaP+SuWvc/XBrKstn6a9vjNLqvdSnp/wGEsvJ0XMePToDWymbCMi nLaagIAEHf4cVxyi0/gXGBKE87Xj4kfVl7TWJfPERpMm7LbdN5UROWB2hIPRnv6BbQ 28hLL3xszaj9e9unKv45yO255LwvJ3ZNImhah4XgwHq5ZsrHjsWgF6iEedY4kL6mYg UNqXbeHvKEtffHg12p7K/7kM0mAsWqDpzOetWrCbX5+jnNM3oIUh/+v81/ZD0IPQvB TX0dWSDAQU6qw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4cqbgY5SY2z4wBJ; Mon, 20 Oct 2025 11:20:57 +1100 (AEDT) Date: Mon, 20 Oct 2025 11:20:19 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 4/4] tcp: Update data retransmission timeout Message-ID: References: <20251014073836.18150-1-yuhuang@redhat.com> <20251014073836.18150-5-yuhuang@redhat.com> <20251017202812.173e9352@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="/wwW2i1BU/5KAPnl" Content-Disposition: inline In-Reply-To: <20251017202812.173e9352@elisabeth> Message-ID-Hash: 7C65EZBBKQGUPCCUOJNIZV346LQ4C4IR X-Message-ID-Hash: 7C65EZBBKQGUPCCUOJNIZV346LQ4C4IR 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: Yumei Huang , 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: --/wwW2i1BU/5KAPnl Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Oct 17, 2025 at 08:28:12PM +0200, Stefano Brivio wrote: > On Thu, 16 Oct 2025 09:54:25 +1100 > David Gibson wrote: >=20 > > On Wed, Oct 15, 2025 at 02:31:27PM +0800, Yumei Huang wrote: > > > On Wed, Oct 15, 2025 at 8:05=E2=80=AFAM David Gibson > > > wrote: =20 > > > > > > > > On Tue, Oct 14, 2025 at 03:38:36PM +0800, Yumei Huang wrote: =20 > > > > > According to RFC 2988 and RFC 6298, we should use an exponential > > > > > backoff timeout for data retransmission starting from one second > > > > > (see Appendix A in RFC 6298), and limit it to about 60 seconds > > > > > as allowed by the same RFC: > > > > > > > > > > (2.5) A maximum value MAY be placed on RTO provided it is at > > > > > least 60 seconds. =20 > > > > > > > > The interpretation of this isn't entirely clear to me. Does it mean > > > > if the total retransmit delay exceeds 60s we give up and RST (what > > > > this patch implements)? Or does it mean that if the retransmit del= ay > > > > reaches 60s we keep retransmitting, but don't increase the delay any > > > > further? > > > > > > > > Looking at tcp_bound_rto() and related code in the kernel suggests = the > > > > second interpretation. > > > > =20 > > > > > Combine the macros defining the initial timeout for both SYN and = ACK. > > > > > And add a macro ACK_RETRIES to limit the total timeout to about 6= 0s. > > > > > > > > > > Signed-off-by: Yumei Huang > > > > > --- > > > > > tcp.c | 32 ++++++++++++++++---------------- > > > > > 1 file changed, 16 insertions(+), 16 deletions(-) > > > > > > > > > > diff --git a/tcp.c b/tcp.c > > > > > index 3ce3991..84da069 100644 > > > > > --- a/tcp.c > > > > > +++ b/tcp.c > > > > > @@ -179,16 +179,12 @@ > > > > > * > > > > > * Timeouts are implemented by means of timerfd timers, set base= d on flags: > > > > > * > > > > > - * - SYN_TIMEOUT_INIT: if no ACK is received from tap/guest duri= ng handshake > > > > > - * (flag ACK_FROM_TAP_DUE without ESTABLISHED event) within th= is time, resend > > > > > - * SYN. It's the starting timeout for the first SYN retry. If = this persists > > > > > - * for more than TCP_MAX_RETRIES or (tcp_syn_retries + > > > > > - * tcp_syn_linear_timeouts) times in a row, reset the connecti= on > > > > > - * > > > > > - * - ACK_TIMEOUT: if no ACK segment was received from tap/guest,= after sending > > > > > - * data (flag ACK_FROM_TAP_DUE with ESTABLISHED event), re-sen= d data from the > > > > > - * socket and reset sequence to what was acknowledged. If this= persists for > > > > > - * more than TCP_MAX_RETRIES times in a row, reset the connect= ion > > > > > + * - ACK_TIMEOUT_INIT: if no ACK segment was received from tap/g= uest, eiher > > > > > + * 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= =2E It's the > > > > > + * starting timeout for the first retry. If this persists for = more than > > > > > + * allowed times in a row, reset the connection > > > > > * > > > > > * - FIN_TIMEOUT: if a FIN segment was sent to tap/guest (flag A= CK_FROM_TAP_DUE > > > > > * with TAP_FIN_SENT event), and no ACK is received within thi= s time, reset > > > > > @@ -342,8 +338,7 @@ enum { > > > > > #define WINDOW_DEFAULT 14600 /* = RFC 6928 */ > > > > > > > > > > #define ACK_INTERVAL 10 /* ms */ > > > > > -#define SYN_TIMEOUT_INIT 1 /* s */ > > > > > -#define ACK_TIMEOUT 2 > > > > > +#define ACK_TIMEOUT_INIT 1 /* s, RFC 6= 298 */ =20 > > > > > > > > I'd suggest calling this RTO_INIT to match the terminology used in = the > > > > RFCs. =20 > > >=20 > > > Sure. =20 > > > > =20 > > > > > #define FIN_TIMEOUT 60 > > > > > #define ACT_TIMEOUT 7200 > > > > > > > > > > @@ -352,6 +347,11 @@ enum { > > > > > > > > > > #define ACK_IF_NEEDED 0 /* See tcp_send_fla= g() */ > > > > > > > > > > +/* Number of retries calculated from the exponential backoff for= mula, limited > > > > > + * by a total timeout of about 60 seconds. > > > > > + */ > > > > > +#define ACK_RETRIES 5 > > > > > + =20 > > > > > > > > As noted above, I think this is based on a misunderstanding of what > > > > the RFC is saying. TCP_MAX_RETRIES should be fine as it is, I thin= k. > > > > We could implement the clamping of the RTO, but it's a "MAY" in the > > > > RFC, so we don't have to, and I don't really see a strong reason to= do > > > > so. =20 > > >=20 > > > If we use TCP_MAX_RETRIES and not clamping RTO, the total timeout > > > could be 255 seconds. > > >=20 > > > Stefano mentioned "Retransmitting data after 256 seconds doesn't make > > > a lot of sense to me" in the previous comment. =20 > >=20 > > That's true, but it's pretty much true for 60s as well. For the local > > link we usually have between passt and guest, even 1s is an eternity. >=20 > Rather than the local link I was thinking of whatever monitor or > liveness probe in KubeVirt which might have a 60-second period, or some > firewall agent, or how long it typically takes for guests to stop and > resume again in KubeVirt. Right, I hadn't considered those. Although.. do those actually re-use a single connection? I would have guessed they use a new connection each time, making the timeouts here irrelevant. > It's usually seconds or maybe minutes but not five minutes. >=20 > > Basically I see no harm, but also no advantage to clamping or limiting > > the RTO, so I'm suggesting going with the simplest code. >=20 > The advantage I see is that we'll recover significantly faster in case > something went wrong. That's a fair point in a more general case. > > Note that there are (rare) situations where we could get a response > > after minutes. > > - The interface on the guest was disabled for a while > > - An error in guest firewall configuration blocked packets for a while > > - A bug on the guest cause the kernel to wedge for a while > > - The user manually suspended the guest for a while (VM/passt only) > >=20 > > These generally indicate something has gone fairly badly wrong, but a > > long RTO gives the user a bit more time to realise their mistake and > > fix things. >=20 > True, it's just that to me five minutes sounds like "broken beyond > repair", while one minute sounds like "oh we tried again and it worked". Eh, maybe. By nature it's always going to be a bit arbitrary. > > These are niche cases, but given the cost of implementing > > it is "do nothing"... >=20 > ...anyway, it's not a strong preference from my side. It's mostly about > experience but I won't be able to really come up with obvious evidence > (at least not quickly), so if the code is significantly simpler... > whatever. It's not provable so I won't insist. It's a bit simpler, I'm not sure I'd go so far as "significantly". > Note: the comments I'm replying to are from yesterday / Thursday, on > v3, and today / Friday we're at v6. I don't expect a week grace period > as you would on the kernel: >=20 > https://docs.kernel.org/process/submitting-patches.html#don-t-get-disco= uraged-or-impatient >=20 > because we can surely move faster than that, but three versions in a > day obviously before I get any chance to have a look means a > substantial overhead for me, and I might miss the meaning and context of > comments of other reviewers (David in this case). There are no > changelogs in cover letters either. >=20 > I plan to skip to v6 but don't expect a review soon, because of that > overhead I just mentioned. --=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 --/wwW2i1BU/5KAPnl Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmj1gEIACgkQzQJF27ox 2GcdVhAAnKI3PNVKThKOnq/6X0rqCmvmyoTg5eYecG4g5Md6RbkW4O9x6kVg8soL KY64nhn4iesIpx21DKCu3qsYbv5G/Aid84nw0ZT58mKpjoc7tWHOo8ipOij9We3f xsBRiPkoGrdmWxvxJ+0R4DQH6+R9ABZQQTNoT/0PeSDD1Rtdy+zptVOKngqwXRGi jnCYzv7kVITB8gRCPUAugrFiuK72FGFsUQyDoaPIKaJCFELP9ooHtr5HX3rJvY0x FN7O+BGQFk0FFf9R/snu7QVhUxT9ugeh6oBH05rS5ZDlUtUbFkePkARCasbjiUfX YBKxPt6y+LirMGHzv6g7VJvtkQDEK/D0r8Dy+EFTjNrC6bOXsdAWjtVMYxBA4cOw PdAHCFgbvkOK+a7xne89R1uQYgh7TzRhn5GKJm5mSWy9jSL2mJkm30masteKhVsw CbcqM6IgDiiIesX8Nvoojl3DQfG0tF/BTHJgNx1ZMQh2lTqSFhTOOE7ilhpkSDYm M30PU2WgE+DDg4UJUsFBsxOItbHajAooFqJ7KjuWeMC7s9eNehyQcg70OBCizHiq 1uMGENPqb32Z8QztFQimUp5sVRFzyHISaqledVSTCxSN1gWKQicxeTjKyw3IQsXS 5rm/Y+8ZVeB2XrNYs8BN72n4tpXHm/ZpqZwC4YRsKTEqQlL0ryw= =Juud -----END PGP SIGNATURE----- --/wwW2i1BU/5KAPnl--