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: > > > 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 questions > > > 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"). > > > > > > 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? > > 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. > > Yes, in the vhost-user case, the epoll loop needs to run before we have > a working connection to the guest, but: > > - 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. > > 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. > > > 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. > > Right, that looks like a lot of effort for nothing. > > > # 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. > > Same here. > > > - More work to implement, because essentially every sock-side handler > > has to check fd_tap and abort early > > 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. > > 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. > > 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 > > > > - 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 > > Right, that's something we certainly want to avoid. > > > - 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 struct 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 == -1) > > > > + /* If no client connected, account the frames have been sent */ > > > > > > I think the comment is redundant because, well, if c->fd_tap is -1 > > > (obvious, documented), we return 'nframes' (also documented). > > > > > > 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 :). > > 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. -- 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