From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Tbxy5yp3; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id AB75E5A0271 for ; Fri, 12 Sep 2025 04:45:49 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1757645148; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=0bmlZYR7KaxXfS24CLVwC7CIc3BL6Z6VJAasLYvcjrc=; b=Tbxy5yp3PLeXudDqiRE3K6I/pYdw92p1Rne/aHUBkMW1zZmwbzvQlb2JSic89OwKy1K2XL qmP1Gal5hMeFzyPsHaF9M5ZoIwngjMk7OXUW5BfPTeX2bRfFmEeL3N/2HTQvmmyz1q1KcF lz89sn+Qf3vUTlg4bWCMBMxo+q/Tm4s= Received: from mail-ed1-f72.google.com (mail-ed1-f72.google.com [209.85.208.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-368-hG6WmFywOiOOB0TQtk9EtQ-1; Thu, 11 Sep 2025 22:45:46 -0400 X-MC-Unique: hG6WmFywOiOOB0TQtk9EtQ-1 X-Mimecast-MFC-AGG-ID: hG6WmFywOiOOB0TQtk9EtQ_1757645145 Received: by mail-ed1-f72.google.com with SMTP id 4fb4d7f45d1cf-621839f460cso1387366a12.2 for ; Thu, 11 Sep 2025 19:45:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1757645144; x=1758249944; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=0bmlZYR7KaxXfS24CLVwC7CIc3BL6Z6VJAasLYvcjrc=; b=nlKGjDAfJusI6oIliHMASZkRXyZckIKUtFtTowC+92UiN8hRPQnmL0BmbtUE1c02XT FYPrUllQaLm6dbV9NkRR7a6LWIMhpbfAR8DTFbRaR5gv7RtoRetKTgJyUrmnrO42yD7g dhZcDkVqzhw7h9KRzvMbYDkTTddxR7VZnTfwhmvDEqQAvFjcBWw1WUspu4okojp7Cfvy h/CxBwcSkptbCRGoA19pP3Oq7XdA23Jlk6CrWjDxOjSXG733akNultvPIOjw3fx1Qc0F U03YhPuqGqY4HR6Tth+QyM9YpoczAFKzEH8DeiFYOLQ44lCG4l7GJuD3bqb1HkAxVXD3 yLTA== X-Forwarded-Encrypted: i=1; AJvYcCVU2tzDvHFnnHJU9AWTgRe6U2L8qZPHsWjpa/8t6WsQvA53FFdP8aXtp4WXnWIym9PM+R/VwYCZgLk=@passt.top X-Gm-Message-State: AOJu0Yyag3Olov7w/H82OMQado6sOW1qec/c1Yl1/hNgfLHGh1uYhZZa leBPi/wMhsf0bwknfo0FAR3yb38y8z8cwwabyDLAqvuSsaYq8Bx4gfMTLKTGOVIkxbS6Gf2qqLF M0ROxT6j3yuwBVyaP1oDLwnqrdxwNlpc2k2FdZaxGb82wTYjHCHfVfdfa+FwNBCqhDgh6IZqtZ9 Acrjk0vWhk0yDjYr10y+pcTX5Jg8gS X-Gm-Gg: ASbGncuXKoG+ElQ7YOgb8OCsI2J/TVk7jWBZEIDGMHNtrPEx3AehFKMqqxP3y11ESHK 3Vh1FyfTVgk6KtyjH8pPF9tcXlmuiXnt9fo6CscGqbSwXoUHw6m4uojWU1iQJSkMAVwYaZ57HhV lPOnFgKvF707Yo+VTOC4FMPw== X-Received: by 2002:a05:6402:2102:b0:628:12e9:5e41 with SMTP id 4fb4d7f45d1cf-62ed851c69dmr1494378a12.29.1757645144537; Thu, 11 Sep 2025 19:45:44 -0700 (PDT) X-Google-Smtp-Source: AGHT+IF2yZpFHqMZAyZ4OutrvQ1Hmiyri0ul3011wuoIT2/6ReOT5WnsE+4QSNe9pOy5qmPsrbx37wUmnVL4tm0OPak= X-Received: by 2002:a05:6402:2102:b0:628:12e9:5e41 with SMTP id 4fb4d7f45d1cf-62ed851c69dmr1494297a12.29.1757645141964; Thu, 11 Sep 2025 19:45:41 -0700 (PDT) MIME-Version: 1.0 References: <20250911085519.24395-1-yuhuang@redhat.com> <20250911115425.79eaaac5@elisabeth> In-Reply-To: From: Yumei Huang Date: Fri, 12 Sep 2025 10:45:30 +0800 X-Gm-Features: AS18NWDM23cP_dkl14nPNy0dVDuK9mr087Kuh65VsEZ37h7GCytT7rAmYH5cwDw Message-ID: Subject: Re: [PATCH] tap: Drop frames if no client connected To: David Gibson X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: Wt4pMPps7GeP84XNPapJyInVBxjtC9yEhTUqSmAPWaM_1757645145 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Message-ID-Hash: CFB4IO5IN2PUWMS5ETOYHLW3DAH3WOVP X-Message-ID-Hash: CFB4IO5IN2PUWMS5ETOYHLW3DAH3WOVP X-MailFrom: yuhuang@redhat.com 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: Stefano Brivio , 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: Thank you David for helping explain all this! On Fri, Sep 12, 2025 at 10:02=E2=80=AFAM David Gibson wrote: > > 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: > > > > > 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. > > > > > > Suggested-by: David Gibson > > > Signed-off-by: Yumei Huang > > > > Thanks, the fix itself obviously makes sense, but I have a few question= s > > and comments: > > > > - 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. > > > > 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"). BTW, I read the commit c9b241346569, it didn't change the behaviour. Before this, tap_sock_init() will call tap_sock_unix_init() without checking if c->fd_tap. > > > > Before that, main() would call tap_sock_init(), which would call > > tap_sock_unix_open(), a blocking function. > > > > 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 > 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. > > > > > --- > > > tap.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > 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 struc= t ctx *c, > > > * @iov must have total length @bufs_per_frame * @nframes, with each= 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 *iov, > > > size_t bufs_per_frame, size_t nframes) > > > { > > > size_t m; > > > > > > + if (c->fd_tap =3D=3D -1) > > > + /* If no client connected, account the frames have been s= ent */ > > > > I think the comment is redundant because, well, if c->fd_tap is -1 > > (obvious, documented), we return 'nframes' (also documented). Agree, I will drop it in v2. > > > > 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". > > > > 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 :). Haha, true! I=E2=80=99m still improving my English, so the suggestions are very helpful and much appreciated:) > > 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 > > -- > 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 wa= y > | around. > http://www.ozlabs.org/~dgibson --=20 Thanks, Yumei Huang