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=202508 header.b=WkFsz9aY; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 6A5045A068E for ; Thu, 18 Sep 2025 06:28:47 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202508; t=1758169723; bh=uYReF63JXGii0JNAUcRILgx/Z88bloq7IulIA91i5o4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=WkFsz9aY1uRsuuSnzZXH4kyO6j5uf/nqS2+LTD+Sl5tZVSU+zFzS5eCSsx0DT4dlL M8fs63JvcEp3/8JMnypBu29VQv3nZ3DOK0iiGhq5MDsr1fyY0j8UHnHkd4uYjA226a UU6qXqsSmux2Z/oF41R57Ax5sM44z/1VaztT8I63W6QEYYeA4sMwZAANEwmpzJSibm 66uJd0QpbDZrnuHj1KvYf+Va5YJOxkew2ge+lFpII5di+3kK6Nx8N4PGakWB0EDPIr i0mOeJ1Ik8/NCwp3XiVkb6ojVsA4W1RV4hYo/2bSXckpn9whqpwtojlvUPnS2GQsKU GNsM4mWR0eyYA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4cS2hC1vhVz4wCB; Thu, 18 Sep 2025 14:28:43 +1000 (AEST) Date: Thu, 18 Sep 2025 14:28:37 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH] tap: Drop frames if no client connected Message-ID: References: <20250911085519.24395-1-yuhuang@redhat.com> <20250911115425.79eaaac5@elisabeth> <20250915081319.00e72e53@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="23OOLkL+NhzbOMAT" Content-Disposition: inline In-Reply-To: <20250915081319.00e72e53@elisabeth> Message-ID-Hash: OSFVYV2JFTZTBIOOZH6L6PVBLA22JVXT X-Message-ID-Hash: OSFVYV2JFTZTBIOOZH6L6PVBLA22JVXT 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, lvivier@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: --23OOLkL+NhzbOMAT Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 15, 2025 at 08:13:19AM +0200, Stefano Brivio wrote: > On Fri, 12 Sep 2025 12:01:37 +1000 > David Gibson wrote: >=20 > > On Thu, Sep 11, 2025 at 11:54:25AM +0200, Stefano Brivio wrote: > > > On Thu, 11 Sep 2025 16:55:19 +0800 > > > Yumei Huang wrote: > > > =20 > > > > If no client is attached, discard outgoing frames and report them as > > > > sent. This mimics the behavior of a physical host with its network > > > > cable unplugged. > > > >=20 > > > > Suggested-by: David Gibson > > > > Signed-off-by: Yumei Huang =20 > > >=20 > > > Thanks, the fix itself obviously makes sense, but I have a few questi= ons > > > and comments: > > >=20 > > > - first off, what happens if we don't return early in tap_send_frames= ()? > > > Commit messages for fixes (assuming this is a fix) should always say > > > what concrete problem we had, what is going to be fixed, or if we're > > > not aware of any real issue but things are just fragile / wrong =20 > >=20 > > Without this we will get an EBADF in either writev() (pasta) or > > sendmsg() (passt). That's basically harmless, but a bit ugly. > > Explicitly catching this case results in behaviour that's probably a > > bit clearer to debug if we hit it. > >=20 > > Putting that context in the commit message would be useful. > >=20 > > > - until a while ago, this couldn't happen at all. We were just blocki= ng > > > the whole execution as long as the tap / guest / container interface > > > wasn't up and running. > > >=20 > > > I wonder when this changed and if it makes sense to go back to the > > > previous behaviour. I had just a quick look and I wonder if I > > > accidentally broke this in c9b241346569 ("conf, passt, tap: Open > > > socket and PID files before switching UID/GID"). > > >=20 > > > Before that, main() would call tap_sock_init(), which would call > > > tap_sock_unix_open(), a blocking function. > > >=20 > > > Should we make the whole thing blocking again? If not, is there > > > anything else that's breaking with that? Timers, other inputs, etc.= =20 > >=20 > > I don't think we can quite do that. I'm not sure if it's the only > > reason, but for vhost-user I believe we need the epoll loop up and > > running before we have the tap connection fully set up, because we > > need it to process the vhost-user control messages. Laurent, can you > > verify? >=20 > We discussed this in the past, before realising that the execution > continues for whatever reason, and probably before I broke the > assumption that guest connection was blocking. >=20 > Yes, in the vhost-user case, the epoll loop needs to run before we have > a working connection to the guest, but: >=20 > - we can anyway block until the control socket is set up (we used to do > that) The vhost-user control socket? I'm not entirely sure what you mean by "block" here. Since we need the epoll loop up, I don't see how we can block in the conventional sense. > - the vhost-user implementation autonomously throws data away received > before that point Right. It doesn't have anywhere to put it, so it doesn't have much choice. > Now, I don't think we necessarily need to stick to that approach, it > was the obvious choice when passt was much simpler, and it keeps things > simple in the sense that we don't need to care about cases like the > ones this patch is addressing. >=20 > On the other hand, if we want to switch to a different model, we need > to have a look at other possible breakages, I guess. >=20 > > There are several different approaches we can take here. I discussed > > some with Yumei and suggested she take this one. Here's some > > reasoning (maybe this would also be useful in the commit message, > > though it's rather bulky) > >=20 > > # Don't listen() until the tap connection is ready > >=20 > > - It's not clear that the host rejecting the connection is better > > than the host accepting, then the connection stalling until the=20 > > guest is ready. > > - Would require substantial rework because we currently listen() as > > we parse the command line and don't store the information we'd need > > to do it later. >=20 > Right, that looks like a lot of effort for nothing. >=20 > > # Don't accept() until the tap connection is ready > >=20 > > - To the peer, will behave basically the same as this patch - the > > host will complete the TCP handshake, then the connection will stall > > until the guest is ready. >=20 > Same here. >=20 > > - More work to implement, because essentially every sock-side handler > > has to check fd_tap and abort early >=20 > There's one substantial issue at TCP level, though, that we're keeping > with the current approach and with this patch: we'll accept inbound > connections and silently stall them. >=20 > We could mitigate that by making the TCP handler aware of this, and by > resetting the connection if the guest isn't there. This would at least > be consistent with the case where the guest isn't listening on the port > (we accept(), fail to connect to it, eventually call tcp_rst()). True. Arguably less consistent with a non-passt-connected peer that's not there though. Plus with the silently stall approach we have a chance that the TCP connection will recover if the guest attaches reasonably soon. > If we don't do this, I think we should at least check what happens in > terms of race conditions between passt starting and the guest appearing > and accepting the connection. I guess we'll retry for a bit, which is > desirable, but we should check that the whole retrying thing actually > works. >=20 > That's because the current approach just happened by accident. Right. I'm not entirely sure what concrete action you're suggesting at this point, though. > > # Drop packets in tap_send_frames(), but return 0 > >=20 > > - To the peer, would behave basically the same > > - Would make the TCP code do a bunch of busy work attempting to > > resend, probably to no avail >=20 > Right, that's something we certainly want to avoid. >=20 > > - Handling of errors returned by tap_send_frames() is on the basis > > that it's probably a transient fault (buffer full) and we want to > > resend very soon. That approach doesn't make sense for a missing > > guest. > >=20 > > > I didn't really have time to investigate until now, I can try to > > > have another look soon though, unless you find out more meanwhile. > > > =20 > > > > --- > > > > tap.c | 6 +++++- > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > >=20 > > > > diff --git a/tap.c b/tap.c > > > > index 7ba6399..e01219d 100644 > > > > --- a/tap.c > > > > +++ b/tap.c > > > > @@ -507,13 +507,17 @@ static size_t tap_send_frames_passt(const str= uct ctx *c, > > > > * @iov must have total length @bufs_per_frame * @nframes, with ea= ch set of > > > > * @bufs_per_frame contiguous buffers representing a single frame. > > > > * > > > > - * Return: number of frames actually sent > > > > + * Return: number of frames actually sent, or accounted as sent > > > > */ > > > > size_t tap_send_frames(const struct ctx *c, const struct iovec *io= v, > > > > size_t bufs_per_frame, size_t nframes) > > > > { > > > > size_t m; > > > > =20 > > > > + if (c->fd_tap =3D=3D -1) > > > > + /* If no client connected, account the frames have been sent */ = =20 > > >=20 > > > I think the comment is redundant because, well, if c->fd_tap is -1 > > > (obvious, documented), we return 'nframes' (also documented). > > >=20 > > > If it's not redundant, for any reason, "to account" in this sense > > > isn't transitive. You could say: "consider that the frames have been > > > sent" but not "account that the frames have been sent". > > >=20 > > > You can pick a different meaning of "to account" and say "account the > > > frames as sent", though. =20 > >=20 > > It's an amusing truth of the passt project that you'll get more > > English usage notes from the Italian living in Germany than the native > > English speaker living in an English speaking country :). >=20 > I'd rather call that pedantry than usage note, and it's so bad that we > ask gcc to align with that: So would I, but I was being polite :-p. --=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 --23OOLkL+NhzbOMAT Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmjLinQACgkQzQJF27ox 2Gdx1A/9ESWHMqQedzZbhmfRJjQJBep6Jy5XXr85hxRlmxjLnAUBHDTJG34qx9Ta XsfKMR9Tvw+uuSjfasDfWY1d5E4Q0YJ488bg7ufOhsn7sKTJ3CrJxVzgNG/Z95NH EidHkKg/FGvT7IFytzo7DE2jY9aO9Kuirr5Mv1znvD97MMJtG3YBpJ2BCSuoQRSc ENGyn7SOQsHH2lmGMZEpVE8wnyxGlczgDvHzpkFAUF3wacXR7rcV6SGQ6ZDmesvq nQq6wZoYNmGKXHieykHIO4fCQ+XlmJF6nI1V9rCbEpYuFeTbqt527/wSBYOx14Z9 2eqvGeTuvL1zldXlA9rckrZbzKiOFptNR9JZsRdCkKKqswtr4KdGbZLk2jyGtNUJ zu9yRKTy02NwQpo5IcSHbEjWKuk6+mkPJwq3zDLe9Mi9e3Was1mVaVOKANQaTvS6 Qec2b+n9h0Dgw+QB7VHqxfblxN9N7hKf+FEdpp7YW7ut/IDPXa/8jUPTb880sz54 YdrKl1pvI/+ikm2YFIWXJIOtMuQ0aozSFEVc6d5K5YeQi9DWwFwLw6E5b/OQfeWV 5Nq3dNgQD42ne1+Ph16mr2lxqLS/+mi+m2Y7HJy0BN3wK7Q1M8OUAVhfSa1JoSx3 QRNQQXHvvsbI+QE5K3Acbqz6UhaT2KbYLC0u54P1HTcgw7CenaE= =ewL0 -----END PGP SIGNATURE----- --23OOLkL+NhzbOMAT--