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=dvAFr+i2; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 01B7A5A0279 for ; Fri, 12 Sep 2025 04:01:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202508; t=1757642502; bh=9GeAPR0EMKO21efAyQnz9Rhn5KnOVvx9YUOlttBtpLI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=dvAFr+i2tFLpgK3Wp8frQhJpRYVN/c3WfUblfbTjvgzaULdfdwqrV30oRTDGPnAXp A/8A52Ik5gtP5NiVof4bxDSJGGMvFKwaqBrXLhgJTblE3J9EufOvtiCnHmq817gpBj Uxg9rH9Zj9MBYZEh2NG3lavBzMrsmQqCtlXim6qZY1JG2R0iIj4HlB7DSxuTReciE9 HTL4IFppNcanF6yU1vQy3c9dMwMtGsRaQxIq6o4ELrdBkAtCJ9gdrB3ZAFyBPpOqvZ 6VrrfCOEgC2av1R2OuoidyX+zyTAa6U8lL5Hj7Sy00+kOLXw8/vbLbXBZ5+xTg0Kt0 tRiNZHDp0K57w== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4cNHjL4knsz4w9Q; Fri, 12 Sep 2025 12:01:42 +1000 (AEST) Date: Fri, 12 Sep 2025 12:01: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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="Y7XxkfzjRYq96C4M" Content-Disposition: inline In-Reply-To: <20250911115425.79eaaac5@elisabeth> Message-ID-Hash: VII6YRRDW6SHI54L5PAL6KYFWWMAH2YT X-Message-ID-Hash: VII6YRRDW6SHI54L5PAL6KYFWWMAH2YT 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: --Y7XxkfzjRYq96C4M Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 > Thanks, the fix itself obviously makes sense, but I have a few questions > 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 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. Putting that context in the commit message would be useful. > - until a while ago, this couldn't happen at all. We were just blocking > 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. 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? 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) # Don't listen() until the tap connection is ready - 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. # Don't accept() until the tap connection is ready - 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. - More work to implement, because essentially every sock-side handler has to check fd_tap and abort early # Drop packets in tap_send_frames(), but return 0 - 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 - 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. > 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 struct = ctx *c, > > * @iov must have total length @bufs_per_frame * @nframes, with each s= et 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 *iov, > > 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 > 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. 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 :). Fwiw, I agree that the comment can probably just be dropped. If kept, I'd suggest: If no client is connected, silently drop the frames --=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 --Y7XxkfzjRYq96C4M Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmjDfwAACgkQzQJF27ox 2GemJg//aNm3wfdSfPHB2M9KCkkA8ZoKqGfknl1NiGS7R3mQNJ8O0whlX8dsXUcI +/fUhJEHe+ntQlj/nh8kCjOVuVjAqqTJJ1AuIbRVf7+ljCepPsUa23alvYfI9pGk Wr/iNky8nt9ZJbg2Q/UkghWtZL9JoEjcPm3iInbMGegU2L9GofGwhlEOfjIVCaPI 1UFJF4jsMfl3ERdCBI/BLXEzboHlIiMrJo9ObGc93xHhAajub1SoqJiZ+5Rs7sO+ Q4ZCmGhC089Ro9wIBlhdn84dvT5lUAXvUAaqlXAXAYSRyqtDjhto21gTAHjdxTt2 DIAfwlHwpPdvwdmVjY2xeXJKiDgnn8ihs4xPSQmQi6obyEdjIKifaDmN0ViKyAuS 4DgJ7PSI349FcD5E0lXEQMpb6kbdrRrE/X7s2EOeyFdgoh/dM+kvNiP62qxmzad/ 9cRcDkmYJBJzap95TctLuR6gG3yzXfWkvVmkxare4krgEih4+4QnDFuqe9OEyuPK haPzV08Z0swAnxNgP0OOKlRaccPiNGP4pR16uE1die6aPhDSm6MMBa+kTW9cuS02 xunf8iS7sTjFw7wuEMJKr6o6nMMzhH/9p7tuFikK02k+swTfMkQofo9MXiYJV4NC mSYyW9SX1iGSTpzKk2RrZlRESn/Y91dWe53eJQcPNnW974rtI0U= =+plm -----END PGP SIGNATURE----- --Y7XxkfzjRYq96C4M--