Re: [PATCH v4 2/7] epoll_ctl: Extract epoll operations

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2992 bytes --]

On Tue, Oct 21, 2025 at 01:52:25PM +0200, Laurent Vivier wrote:
> On 20/10/2025 03:20, David Gibson wrote:
> > On Fri, Oct 17, 2025 at 12:31:24PM +0200, Laurent Vivier wrote:
> > > Centralize epoll_add() and epoll_del() helper functions into new
> > > epoll_ctl.c/h files.
> > > 
> > > This also moves the union epoll_ref definition from passt.h to
> > > epoll_ctl.h where it's more logically placed.
> > > 
> > > The new epoll_add() helper simplifies adding file descriptors to epoll
> > > by taking an epoll_ref and events, handling error reporting
> > > consistently across all call sites.
> > > 
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > 
> > Concept looks good, some minor details in execution.
> > 
> > > ---
> > >   Makefile     | 22 +++++++++++-----------
> > >   epoll_ctl.c  | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > >   epoll_ctl.h  | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >   icmp.c       |  4 +---
> > >   passt.c      |  2 +-
> > >   passt.h      | 34 ----------------------------------
> > >   pasta.c      |  7 +++----
> > >   repair.c     | 14 +++++---------
> > >   tap.c        | 13 ++++---------
> > >   tcp.c        |  2 +-
> > >   tcp_splice.c |  2 +-
> > >   udp.c        |  2 +-
> > >   udp_flow.c   |  1 +
> > >   util.c       | 22 +++-------------------
> > >   util.h       |  4 +++-
> > >   vhost_user.c |  8 ++------
> > >   vu_common.c  |  2 +-
> > >   17 files changed, 134 insertions(+), 101 deletions(-)
> > >   create mode 100644 epoll_ctl.c
> > >   create mode 100644 epoll_ctl.h
> ...
> > > @@ -1327,14 +1327,12 @@ static void tap_backend_show_hints(struct ctx *c)
> > >   static void tap_sock_unix_init(const struct ctx *c)
> > >   {
> > >   	union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN };
> > > -	struct epoll_event ev = { 0 };
> > >   	listen(c->fd_tap_listen, 0);
> > >   	ref.fd = c->fd_tap_listen;
> > > -	ev.events = EPOLLIN | EPOLLET;
> > > -	ev.data.u64 = ref.u64;
> > > -	epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev);
> > > +
> > > +	epoll_add(c->epollfd, EPOLLIN | EPOLLET, &ref);
> > 
> > Preexisting, but we should probably check for errors here.
> > 
> 
> To do what? die() ? err() to report the place where the error happens?

The case I'm concerned about is this fails, but we carry on.  Then we
lose 1/Nth of our packets because we're not getting events for one
queue, and weird failures follow.  It takes us ages to debug, because
we don't consider that the fd might have silently failed to make it
into the epoll set.

err() would be probably be enough to make this debuggable.  But
honestly, if we can't put our tap fds into the epoll, we're in a
sufficiently bad state that die() makes sense.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]