Re: [PATCH v3 2/6] epoll_ctl: Extract epoll operations

[Stefano Brivio <sbrivio@redhat.com>]

On Thu,  9 Oct 2025 15:04:05 +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..0a06350f87a5
> --- /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;
> +		warn("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;

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.

> +		union {
> +			uint32_t flow;
> +			flow_sidx_t flowside;
> +			union tcp_listen_epoll_ref tcp_listen;
> +			union udp_listen_epoll_ref udp;
> +			uint32_t data;
> +			int nsdir_fd;
> +			int queue;
> +		};
> +	};
> +	uint64_t u64;
> +};
> +static_assert(sizeof(union epoll_ref) <= sizeof(union epoll_data),
> +	      "epoll_ref must have same size as epoll_data");

Either the comment is misleading, or it should be sizeof(...) != ...

The rest of the patch looks good to me, I didn't finish reviewing the
series though.

-- 
Stefano