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=B9UUaF85; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 243135A026F for ; Fri, 24 Oct 2025 05:30:33 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202510; t=1761276630; bh=iAvR3mf1yYinsOzB0+ctHhE7gGE8bfdZDxdY6k6z21c=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=B9UUaF856uAl775G/G0nQsmqcqxXb2Dcb2EN0A5Xaeboi5wqMGIHPyz9BlpQY/7+3 b9WmYsXLYx2oGu42wCrb7j7F/zHG6rAR2YUecCREvvKQ9IqW4Yo42P7vvINVez27Od YKvcvDOyzfBeZ5wds9K55IgYsdd5wn+TkfoH3h6M4wjlqUAZ4CBIp7g5Qt8Gpnd1nR +RIVTsgDU8u/qW1Vgco677YrpdFds96P4MR5/Edj6uJQwpGHPXzfaDstphAQNcUAxv Tkzv/onvbHVezEDGg5+eA4mTfUyJHleFyRpvLk+kijzmk8EpYxM3dlL4ZxI3UB2HsO Ab6ae8PWWHADA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4ct7hQ0434z4wCV; Fri, 24 Oct 2025 14:30:30 +1100 (AEDT) Date: Fri, 24 Oct 2025 14:30:09 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v6 3/4] tcp: Resend SYN for inbound connections Message-ID: References: <20251017062838.21041-1-yuhuang@redhat.com> <20251017062838.21041-4-yuhuang@redhat.com> <20251024010431.4329a843@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="EHIudMeI/fW8QS9v" Content-Disposition: inline In-Reply-To: <20251024010431.4329a843@elisabeth> Message-ID-Hash: 2DB4YYEZL2VG7NZA7H3P6RLXZX5LNQX4 X-Message-ID-Hash: 2DB4YYEZL2VG7NZA7H3P6RLXZX5LNQX4 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: --EHIudMeI/fW8QS9v Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Oct 24, 2025 at 01:04:31AM +0200, Stefano Brivio wrote: > On Fri, 17 Oct 2025 14:28:37 +0800 > Yumei Huang wrote: >=20 > > If a client connects while guest is not connected or ready yet, > > resend SYN instead of just resetting connection after 10 seconds. > >=20 > > Use the same backoff calculation for the timeout as linux kernel. >=20 > Linux. >=20 > >=20 > > Link: https://bugs.passt.top/show_bug.cgi?id=3D153 > > Signed-off-by: Yumei Huang > > --- > > tcp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++-------- > > tcp.h | 5 +++++ > > 2 files changed, 52 insertions(+), 8 deletions(-) > >=20 > > diff --git a/tcp.c b/tcp.c > > index 2ec4b0c..9385132 100644 > > --- a/tcp.c > > +++ b/tcp.c > > @@ -179,9 +179,11 @@ > > * > > * Timeouts are implemented by means of timerfd timers, set based on f= lags: > > * > > - * - SYN_TIMEOUT: if no ACK is received from tap/guest during handshak= e (flag > > - * ACK_FROM_TAP_DUE without ESTABLISHED event) within this time, res= et the > > - * connection > > + * - SYN_TIMEOUT_INIT: if no ACK is received from tap/guest during han= dshake > > + * (flag ACK_FROM_TAP_DUE without ESTABLISHED event) within this tim= e, resend > > + * SYN. It's the starting timeout for the first SYN retry. If this p= ersists >=20 > "If this persists" makes sense for the existing ACK_TIMEOUT > description but not here, because it looks like it refers to "starting > timeout". >=20 > Coupled with the next patch, it becomes increasingly difficult to > understand what "this" persisting thing is. Yeah. This was my suggested wording, based on the existing wording for ACK_TIMEOUT. It's not great, but I struggled a bit to find better wording. > Maybe directly say "Retry for ..., then reset the connection"? It's > shorter and clearer. There it is :). "Retry (NNN) times, then reset the connection". [snip] > > @@ -2409,8 +2419,17 @@ void tcp_timer_handler(const struct ctx *c, unio= n epoll_ref ref) > > tcp_timer_ctl(c, conn); > > } else if (conn->flags & ACK_FROM_TAP_DUE) { > > if (!(conn->events & ESTABLISHED)) { > > - flow_dbg(conn, "handshake timeout"); > > - tcp_rst(c, conn); > > + if (conn->retries >=3D TCP_MAX_RETRIES || > > + conn->retries >=3D (c->tcp.tcp_syn_retries + > > + c->tcp.syn_linear_timeouts)) { > > + flow_dbg(conn, "handshake timeout"); > > + tcp_rst(c, conn); > > + } else { > > + flow_trace(conn, "SYN timeout, retry"); > > + tcp_send_flag(c, conn, SYN); > > + conn->retries++; >=20 > I think I already raised this point on a previous revision: this needs > to be zeroed as the connection is established, but I don't see that in > the current version. Yes, you raised that, but then I realised it's already handled. I think I put that in the thread, not just direct to Yumei, but maybe not? Or it just got lost in the minutiae. When we receive a SYN-ACK, it will have th->ack_seq advanced a byte acknowledging the SYN. tcp_tap_handler() calls tcp_update_seqack_from_tap() in the !ESTABLISHED case which will see the new ack_seq and clear retries (retrans before this series). > > + tcp_timer_ctl(c, conn); > > + } > > } else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) { > > flow_dbg(conn, "FIN timeout"); > > tcp_rst(c, conn); > > @@ -2766,6 +2785,24 @@ static socklen_t tcp_probe_tcp_info(void) > > return sl; > > } > > =20 > > +/** > > + * tcp_syn_params_init() - Get initial SYN parameters for inbound conn= ection >=20 > They're not initial, they'll be used for all the connections if I > understand correctly. >=20 > Maybe "Get SYN retries sysctl values"? I think the _init() in the > function name is also somewhat misleading. "Get host kernel RTO parameters"? Since we're thinking of extending this to cover the RTO upper bound as well as the SYN specific parameters. >=20 > > + * @c: Execution context > > +*/ > > +void tcp_syn_params_init(struct ctx *c) > > +{ > > + intmax_t tcp_syn_retries, syn_linear_timeouts; > > + > > + tcp_syn_retries =3D read_file_integer(TCP_SYN_RETRIES, 8); >=20 > Why 8? Perhaps a #define would help? >=20 > > + syn_linear_timeouts =3D read_file_integer(TCP_SYN_LINEAR_TIMEOUTS, 1); > > + > > + c->tcp.tcp_syn_retries =3D MIN(tcp_syn_retries, UINT8_MAX); > > + c->tcp.syn_linear_timeouts =3D MIN(syn_linear_timeouts, UINT8_MAX); > > + > > + debug("TCP SYN parameters: retries=3D%"PRIu8", linear_timeouts=3D%"PR= Iu8, >=20 > Similar to the comment above: these are not parameters of SYN segments > (which would seem to imply TCP options, such as the MSS). >=20 > We typically don't print C assignments, rather human-readable messages, > so that could be "Read sysctl values tcp_syn_retries: ..., > syn_linear_timeouts: ...". >=20 >=20 > > + c->tcp.tcp_syn_retries, c->tcp.syn_linear_timeouts); > > +} > > + > > /** > > * tcp_init() - Get initial sequence, hash secret, initialise per-sock= et data > > * @c: Execution context > > @@ -2776,6 +2813,8 @@ int tcp_init(struct ctx *c) > > { > > ASSERT(!c->no_tcp); > > =20 > > + tcp_syn_params_init(c); > > + > > tcp_sock_iov_init(c); > > =20 > > memset(init_sock_pool4, 0xff, sizeof(init_sock_pool4)); > > diff --git a/tcp.h b/tcp.h > > index 234a803..4369b52 100644 > > --- a/tcp.h > > +++ b/tcp.h > > @@ -59,12 +59,17 @@ union tcp_listen_epoll_ref { > > * @fwd_out: Port forwarding configuration for outbound packets > > * @timer_run: Timestamp of most recent timer run > > * @pipe_size: Size of pipes for spliced connections > > + * @tcp_syn_retries: Number of SYN retries during handshake > > + * @syn_linear_timeouts: Number of SYN retries using linear backoff ti= meout > > + * before switching to exponential backoff timeout >=20 > Maybe more compact: >=20 > * @syn_linear_timeouts: SYN retries before using exponential timeout >=20 > > */ > > struct tcp_ctx { > > struct fwd_ports fwd_in; > > struct fwd_ports fwd_out; > > struct timespec timer_run; > > size_t pipe_size; > > + uint8_t tcp_syn_retries; > > + uint8_t syn_linear_timeouts; > > }; > > =20 > > #endif /* TCP_H */ >=20 > --=20 > Stefano >=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 --EHIudMeI/fW8QS9v Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmj68sAACgkQzQJF27ox 2Ge0Ug/6AzFdDf96XzYO4ZZ50uyF+P5yDBk75qV0xrXAnzuWJkLmKAIDO+08irGe ZmG9OQ7q2N5hMV1f4Oyu+f0FhyJVtgu2JMFxzYQbtxQgF52vniY9x4IjXLov6z3B GHLEhM6pVm8HXuOITOW1uHutVe5/ljvn9+vRynG3edZ4rkItUq2wQdhlWsc1R10x BfBaSj9Mbd9pJxOz7hraEA61g5nf0kikNBLXuosXQTwBJI4xNMS7+w2VdvaRzdO9 ZkSWc5p96kAQy5fNcl9rBMe30OBeZNkekrjrkWzTvMAaAASELQLQYTAYxR5a72VW 0JuLX2UCDOiRm0BNnQBWEy9/rM3+ghTXLZnNV0j9LW8aFC9ucOp5Hzs6KfQhSYAa Wbr7wt9EBgL+EhF5y0WD9cpxY6OxbWRTSOqLwDBnzXLMskUNV1uaPVPNojaXChNb 82c0uN05jB4iTjhBAImNfwZCnblTQgLQVMxqnpQvetBd2mXQqcTz8N1F/VMX8eKn lzf+m5yljlFIrBuKQ5kPLcMDnxLy6mysWi1HCYIuJyEenGs9vxip72JqIEW/FBZ1 O1ijMTZkYsN7gyPbyYtuCdJ8quLiOc6Gj9NujD6JTCSVmkWnrSOKA3XRKeplAZ92 FRHxLQpNJ2fJBua+BNqjupUKzojyc6gOFhR4bPAuQz67EEthgTY= =7JgM -----END PGP SIGNATURE----- --EHIudMeI/fW8QS9v--