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

[Laurent Vivier <lvivier@redhat.com>]

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.

But I can move it if you prefer.

Thanks,
Laurent