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=lYGQ4ixS; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id B0F095A061A for ; Tue, 07 Oct 2025 01:34:13 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202510; t=1759793650; bh=C6yQFo/uORzhulWje3zZU2IvnS/m26GUNOofa2Qw5JY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=lYGQ4ixS0tdj24JFJfq1+XYvaKifrkN6mX9poyBlQZ3T9mvc1mauffmbUFwpOxdM8 EyGTxK4urZQcvRca6bki8dPshuPFzkQ1wucaOXGtKyBlLY3aBRrdWIIqdm/mtKOfbm vPdSealeQmwIfU3xMoBcgDAARwroiQt9aLyuIu4zzBEeJt0pyaiCKrjSqdOhm6lCfu kkIZY9PbPXfOteOXoEr2ejO8SNmuBpnv6Ml3ElawYAqHHgOicVVNQJkn3QMINNiLNy GDenxsHTiQ4OdpWgYRAzw64s4rFUhZr2PiyKRgGpuzWwyphTwiMvjSQ4x2ldxDO+G4 LCtzodpI4J3vg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4cgbFZ3fmWz4w2P; Tue, 7 Oct 2025 10:34:10 +1100 (AEDT) Date: Tue, 7 Oct 2025 10:31:11 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 1/4] tcp: Fix ACK sequence on FIN to tap Message-ID: References: <20251002000646.2136202-1-sbrivio@redhat.com> <20251002000646.2136202-2-sbrivio@redhat.com> <20251002135841.112eb4d3@elisabeth> <20251007003219.3f286b1d@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="mJ/EUkdxu+kmuV86" Content-Disposition: inline In-Reply-To: <20251007003219.3f286b1d@elisabeth> Message-ID-Hash: QTKEYYYDG5UG6P7SNI74PPSG2YPKS2ZR X-Message-ID-Hash: QTKEYYYDG5UG6P7SNI74PPSG2YPKS2ZR 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: --mJ/EUkdxu+kmuV86 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 07, 2025 at 12:32:19AM +0200, Stefano Brivio wrote: > On Fri, 3 Oct 2025 13:19:17 +1000 > David Gibson wrote: >=20 > > On Thu, Oct 02, 2025 at 01:58:41PM +0200, Stefano Brivio wrote: > > > On Thu, 2 Oct 2025 12:41:08 +1000 > > > David Gibson wrote: > > > =20 > > > > On Thu, Oct 02, 2025 at 02:06:43AM +0200, Stefano Brivio wrote: =20 > > > > > If we reach end-of-file on a socket (or get EPOLLRDHUP / EPOLLHUP= ) and > > > > > send a FIN segment to the guest / container acknowledging a seque= nce > > > > > number that's behind what we received so far, we won't have any > > > > > further trigger to send an updated ACK segment, as we are now > > > > > switching the epoll socket monitoring to edge-triggered mode. > > > > >=20 > > > > > To avoid this situation, in tcp_update_seqack_wnd(), we set the n= ext > > > > > acknowledgement sequence to the current observed sequence, regard= less > > > > > of what was acknowledged socket-side. =20 > > > >=20 > > > > To double check my understanding: things should work if we always > > > > acknowledged everything we've received. Acknowledging only what the > > > > peer has acked is a refinement to give the guest a view that's clos= er > > > > to what it would be end-to-end with the peer (which might improve t= he > > > > operation of flow control). =20 > > >=20 > > > Right. > > > =20 > > > > We can't use that refined mechanism when the socket is closing > > > > (amongst other cases), because while we can get the peer acked bytes > > > > from TCP_INFO, we can't get events when that changes, so we have no > > > > mechanism to provide updates to the guest at the right time. So we > > > > fall back to the simpler method. > > > >=20 > > > > Is that correct? =20 > > >=20 > > > Also correct, yes. If you have a better idea to summarise this in the > > > comment in tcp_buf_data_from_sock() let me know. =20 > >=20 > > Hm, I might. Or actually a way to reorganise the code that I think > > will be a bit clearer and probably allow a clearer comment too. >=20 > I would keep reworks for a later moment. Right now, it's already taking > me long enough to find a moment to investigate these issues, write these > fixes, and test them. I mean... the change I'm proposing reduces lines of code (excepting the big new comment), makes it easier to reason about and is localised to the immediately surrounding code. But whatever, I don't particularly care about the order we do things. > > > Maybe I could mention > > > EPOLLET explicitly there? =20 > >=20 > > I don't think EPOLLET is actually relevant. Even if we had level > > triggered events, a change in bytes_acked doesn't count as an event > > (AFAIK). >=20 > It doesn't count, but with level-triggered events, we would be busy > polling bytes_acked, as you noted. I was mentioning EPOLLET because it > could be taken, intuitively, as a "stop listening for events" (almost) > step. I'll leave that out then. I mean, if busy polling were acceptable we could accomplish that easily enough by doing it in tcp_defer_handler() regardless of EPOLLET. > > So either some other event is on, in which case we'd > > effectively be busy polling bytes_acked, or it's not in which case we > > don't get updates, just like now. > >=20 > > I principle we could implement some sort of timer based polling, but > > that sounds like way more trouble than it's worth. >=20 > We already have something similar, based on ACK_INTERVAL and > ACK_TO_TAP_DUE, and it shouldn't be overly complicated to extend that > to a new FIN_TO_TAP_DUE flag. But indeed beyond the scope of this > series. Certainly. > > > > > However, we don't necessarily call tcp_update_seqack_wnd() before > > > > > sending the FIN segment, which might potentially lead to a situat= ion, > > > > > not observed in practice, where we unnecessarily cause a > > > > > retransmission at some point after our FIN segment. > > > > >=20 > > > > > Avoid that by setting the ACK sequence to whatever we received fr= om > > > > > the container / guest, before sending a FIN segment and switching= to > > > > > EPOLLET. > > > > >=20 > > > > > Signed-off-by: Stefano Brivio =20 > > > >=20 > > > > Based on my understanding above, this looks correct to me, so, > > > >=20 > > > > Reviewed-by: David Gibson > > > >=20 > > > > My only concern is whether we could instead insert an extra call to > > > > tcp_update_seqack_wnd() to reduce duplicated logic. =20 > > >=20 > > > Hmm, maybe, but on the other hand we're closing the connection. Should > > > we really spend time querying TCP_INFO to recalculate the window at > > > this point? I wouldn't. =20 > >=20 > > Good point. I mean tcp_update_seqack_wnd() could skip the TCP_INFO in > > that case, but that does look a bit fiddly. > >=20 > > On the other hand, in favour of not duplicating logic... > >=20 > > [snip] > > > > > @@ -368,7 +368,19 @@ int tcp_buf_data_from_sock(const struct ctx = *c, struct tcp_tap_conn *conn) > > > > > conn_flag(c, conn, STALLED); > > > > > } else if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) =3D= =3D > > > > > SOCK_FIN_RCVD) { > > > > > - int ret =3D tcp_buf_send_flag(c, conn, FIN | ACK); > > > > > + int ret; > > > > > + > > > > > + /* On TAP_FIN_SENT, we won't get further data events > > > > > + * from the socket, and this might be the last ACK > > > > > + * segment we send to the tap, so update its sequence to > > > > > + * include everything we received until now. > > > > > + * > > > > > + * See also the special handling on CONN_IS_CLOSING() in > > > > > + * tcp_update_seqack_wnd(). > > > > > + */ > > > > > + conn->seq_ack_to_tap =3D conn->seq_from_tap; =20 > >=20 > > ... the equivalent bits in tcp_update_seqack_wnd() have after them: > > if (SEQ_LT(conn->seq_ack_to_tap, prev_ack_to_tap)) > > conn->seq_ack_to_tap =3D prev_ack_to_tap; > >=20 > > Don't we need that here as well, in case the guest is retransmitting > > when we get the sock side FIN? >=20 > Not really, because we don't rewind conn->seq_from_tap, so we don't > risk jumping back here. Ah, because this one's on the sock->tap data path, whereas the other calls are on the tap->sock data path. Good point. > In tcp_update_seqack_wnd(), we might jump back (that should be double > checked eventually, I'm not sure it's still the case) if we happened to > acknowledge more than acknowledged socket-side while handling some > particular condition, and then we switch back to acknowledging only > bytes_acked. >=20 > It should happen if the destination is/was a low RTT one, but we run > out of slots in low_rtt_dst in favour of other entries. I don't > remember any other case. --=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 --mJ/EUkdxu+kmuV86 Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmjkUS8ACgkQzQJF27ox 2GeWCg/+KjitXJ76AgDregJfYY0EozHtvSugiST356kCq5JaHz8ZtbVLmNlEIsfK 4P9VR3+DJ0nX5FG1T9iS5pykb4aVFixn6oJDCAdl9GngvvqmPPQRsN/8FuNbVwPS gTHoBUbDMom9feHauRkCSJHY/YArMrMsELt9DgOCWyQl6Z2ga+IGJMHvO2MJrORA 0JPk7VT0xFzqxlMO/MnIKxGn7Ya27ZwwORxdhIYJ5TZP3bwmfz45JrVEop+Cu0dN RLbwTaY6wNHR4hiq8PsfwLPZbFv9d27NfVDCieim/4vm5bWcNKHYtxdf6zIWfSEk OzSoaeVbJalAzlMYPRJsQnRptq0o7c5oMrAInaBWZgwFKYQYMcVq737PUV/qIvCz iO2PgYnzvWE72QXLysLZGbRQ4u1ujfi2sxwPt8HpVXy9i+4ehizn+1VWofoGeVPI GB6qLcaGgoOc+0sMGL6eCcapUUi12eFJdYGn2MGFoOWgYTCEBrLG88DU4wfZuLdV xFLQ0feNWEOpVyQ1gZrqwZqBv1kRsMFva3BhNxPLCnuA5LHUjCS5OCW1ZFNaLza1 B9FC9LA2a9keUAn51m4tJ932e1EAfLgbXGxuuB+jYPdq+0l1ayPdAOtG5SaF/ylP 95Mq1YXIzj2amIuklau+gwj/69XZ6uX8V8Ojr4/yDOFph04ZKgE= =7pjy -----END PGP SIGNATURE----- --mJ/EUkdxu+kmuV86--