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

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 17 Oct 2025 14:21:35 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 17/10/2025 13:48, Stefano Brivio wrote:
> > I was reviewing v3 but I can seamlessly move on to v4, just two things
> > here:
> > 
> > On Fri, 17 Oct 2025 12:31:24 +0200
> > Laurent Vivier <lvivier@redhat.com> 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>
> >> ---
> >>   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
> >>
> >> diff --git a/Makefile b/Makefile
> >> index 3328f8324140..91e037b8fd3c 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -37,23 +37,23 @@ FLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE)
> >>   FLAGS += -DVERSION=\"$(VERSION)\"
> >>   FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)
> >>   
> >> -PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c flow.c fwd.c \
> >> -	icmp.c igmp.c inany.c iov.c ip.c isolation.c lineread.c log.c mld.c \
> >> -	ndp.c netlink.c migrate.c packet.c passt.c pasta.c pcap.c pif.c \
> >> -	repair.c tap.c tcp.c tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c \
> >> -	udp_vu.c util.c vhost_user.c virtio.c vu_common.c
> >> +PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c epoll_ctl.c \
> >> +	flow.c fwd.c icmp.c igmp.c inany.c iov.c ip.c isolation.c lineread.c \
> >> +	log.c mld.c ndp.c netlink.c migrate.c packet.c passt.c pasta.c pcap.c \
> >> +	pif.c repair.c tap.c tcp.c tcp_buf.c tcp_splice.c tcp_vu.c udp.c \
> >> +	udp_flow.c udp_vu.c util.c vhost_user.c virtio.c vu_common.c
> >>   QRAP_SRCS = qrap.c
> >>   PASST_REPAIR_SRCS = passt-repair.c
> >>   SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS)
> >>   
> >>   MANPAGES = passt.1 pasta.1 qrap.1 passt-repair.1
> >>   
> >> -PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h flow.h fwd.h \
> >> -	flow_table.h icmp.h icmp_flow.h inany.h iov.h ip.h isolation.h \
> >> -	lineread.h log.h migrate.h ndp.h netlink.h packet.h passt.h pasta.h \
> >> -	pcap.h pif.h repair.h siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h \
> >> -	tcp_internal.h tcp_splice.h tcp_vu.h udp.h udp_flow.h udp_internal.h \
> >> -	udp_vu.h util.h vhost_user.h virtio.h vu_common.h
> >> +PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h epoll_ctl.h \
> >> +	flow.h fwd.h flow_table.h icmp.h icmp_flow.h inany.h iov.h ip.h \
> >> +	isolation.h lineread.h log.h migrate.h ndp.h netlink.h packet.h \
> >> +	passt.h pasta.h pcap.h pif.h repair.h siphash.h tap.h tcp.h tcp_buf.h \
> >> +	tcp_conn.h tcp_internal.h tcp_splice.h tcp_vu.h udp.h udp_flow.h \
> >> +	udp_internal.h udp_vu.h util.h vhost_user.h virtio.h vu_common.h
> >>   HEADERS = $(PASST_HEADERS) seccomp.h
> >>   
> >>   C := \#include <sys/random.h>\nint main(){int a=getrandom(0, 0, 0);}
> >> diff --git a/epoll_ctl.c b/epoll_ctl.c
> >> new file mode 100644
> >> index 000000000000..7a520560aeb9
> >> --- /dev/null
> >> +++ b/epoll_ctl.c
> >> @@ -0,0 +1,45 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +/* epoll_ctl.c - epoll manipulation helpers
> >> + *
> >> + * Copyright Red Hat
> >> + * Author: Laurent Vivier <lvivier@redhat.com>
> >> + */
> >> +
> >> +#include <errno.h>
> >> +
> >> +#include "epoll_ctl.h"
> >> +
> >> +/**
> >> + * epoll_add() - Add a file descriptor to an epollfd
> >> + * @epollfd:	epoll file descriptor to add to
> >> + * @events:	epoll events
> >> + * @ref:	epoll reference for the file descriptor (includes fd and metadata)
> >> + *
> >> + * Return: 0 on success, negative errno on failure
> >> + */
> >> +int epoll_add(int epollfd, uint32_t events, union epoll_ref *ref)
> >> +{
> >> +	struct epoll_event ev;
> >> +	int ret;
> >> +
> >> +	ev.events = events;
> >> +	ev.data.u64 = ref->u64;
> >> +
> >> +	ret = epoll_ctl(epollfd, EPOLL_CTL_ADD, ref->fd, &ev);
> >> +	if (ret == -1) {
> >> +		ret = -errno;
> >> +		err("Failed to add fd to epoll: %s", strerror_(-ret));
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +/**
> >> + * epoll_del() - Remove a file descriptor from an epollfd
> >> + * @epollfd:	epoll file descriptor to remove from
> >> + * @fd:		File descriptor to remove
> >> + */
> >> +void epoll_del(int epollfd, int fd)
> >> +{
> >> +	epoll_ctl(epollfd, EPOLL_CTL_DEL, fd, NULL);
> >> +}
> >> diff --git a/epoll_ctl.h b/epoll_ctl.h
> >> new file mode 100644
> >> index 000000000000..cf92b0f63f26
> >> --- /dev/null
> >> +++ b/epoll_ctl.h
> >> @@ -0,0 +1,51 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later
> >> + * Copyright Red Hat
> >> + * Author: Laurent Vivier <lvivier@redhat.com>
> >> + */
> >> +
> >> +#ifndef EPOLL_CTL_H
> >> +#define EPOLL_CTL_H
> >> +
> >> +#include <sys/epoll.h>
> >> +
> >> +#include "util.h"
> >> +#include "passt.h"
> >> +#include "epoll_type.h"
> >> +#include "flow.h"
> >> +#include "tcp.h"
> >> +#include "udp.h"
> >> +
> >> +/**
> >> + * union epoll_ref - Breakdown of reference for epoll fd bookkeeping
> >> + * @type:	Type of fd (tells us what to do with events)
> >> + * @fd:		File descriptor number (implies < 2^24 total descriptors)
> >> + * @flow:	Index of the flow this fd is linked to
> >> + * @tcp_listen:	TCP-specific reference part for listening sockets
> >> + * @udp:	UDP-specific reference part
> >> + * @data:	Data handled by protocol handlers
> >> + * @nsdir_fd:	netns dirfd for fallback timer checking if namespace is gone
> >> + * @queue:	vhost-user queue index for this fd
> >> + * @u64:	Opaque reference for epoll_ctl() and epoll_wait()
> >> + */
> >> +union epoll_ref {
> >> +	struct {
> >> +		enum epoll_type type:8;
> >> +		int32_t		  fd:FD_REF_BITS;  
> > 
> > Same comment as for v3, quoting:
> > 
> > ---
> > Do the definitions of FD_REF_BITS and FD_REF_MAX really belong to
> > util.h? It would be clearer to have them here. I didn't check all the
> > possible places where you would need to include this header though.
> > ---
> >   
> 
> I didn't move them from util.h because it's not only used by epoll but also by 
> tcp_tap_conn. So for me it was a generic definition and that fits with util.h.

Oh, I see your point now, and we limit the number of bits we use in the
two structures because we want to save space in both, separately, so
also the reason for the limit is somehow generic.

> But I can move it if you prefer.

No no, sorry, never mind, I don't know why I missed that.

-- 
Stefano