From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=iXH6TfpT; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id E15E15A026F for ; Fri, 17 Oct 2025 15:05:36 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1760706335; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=TnGUKMMdjuOcbK2/HeaHKDoxqbaEmMruBtWmQBA10N4=; b=iXH6TfpTjNY6SIMwBs7Ehh+iHxG+PcxFljv7RRMMO9I1ia7MLFXToEutpjmdgXAyUyIaBj oJPoXZlDppe1LovfxcTcfhXp0/YDwQyK+m3dAGLKYUgKuPE6vmftkhm8AlyCcIlgMmCV9q xusIq52iPLcnlKwThqFhUk0Ot7FTjyk= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-661-adwP1HlVOsqZ6xF0BQdsJw-1; Fri, 17 Oct 2025 09:05:34 -0400 X-MC-Unique: adwP1HlVOsqZ6xF0BQdsJw-1 X-Mimecast-MFC-AGG-ID: adwP1HlVOsqZ6xF0BQdsJw_1760706333 Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-426ce339084so1817324f8f.0 for ; Fri, 17 Oct 2025 06:05:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1760706332; x=1761311132; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=TnGUKMMdjuOcbK2/HeaHKDoxqbaEmMruBtWmQBA10N4=; b=CEMcUSQbOYao3b0vl8LBPLBWahpj+moIi1ZUllyvrnRwftP8K+p3Nw79ms0Yr5rCIq aIvXqQYbwphqdwvZPUEtKCWF2q0+XNF+10geHVkysh5V/n6v6IjhdNWcTJ69taojYgDJ JZkPFGDCk/ZquQyG/Q8rlqbbXwnVf/AmgGM8ZkXugqFc0Klw/ELAYrmTSWOpHJHPUK++ enOWrx2f3yuKR5AmaW0ShUBYSSZvqyI3oyLP+/2SFkL+iZdIsM0Z1YcVGGRBzB8Z8fML Ic2syMXvu+XuxIazo6gV/6v2gNP1DDYdBfo52cUIBuobXj3gaL5ZYh8RGOXGQ4RZ2rOM Tm1A== X-Gm-Message-State: AOJu0YwAiyrwhUG9URXykJj59O+nwLE2kOpAjRq+rntjcV6aQr7cNwKe 86evOHY3f+d7TJXD3wMFhChslzwSd5UoimqNKNgGi2g8OTRg1LD+DsA5aISFF7llS6O0yb9dqW5 6l2vDIg5PlFRbIYgkyAf4SCuu7+8vUPXU3iEOwqRjtz98mVTg98citnNfzSpnY50DWBNMmOcg8J uItbU1hr+MG5MnFStGYnTzOn11w3/wz5jsbIiI X-Gm-Gg: ASbGncvlTz7ls5iKdjvXd3UgMJJBv00znrajxiCRlZC2Rko1vyaQVWn2v/YI6P8yq9i c33SiAJKR7AaEDlzxPR3TY/mgSGgP7y12pecZyZ93BWJMPGVSaN6Oe+9Nvr8A+FFNHfQAyKa10V ZUdCJnKYN5Nm3nRrfC3tQm90Yt6PL8z7vdiTcB+/SloyszaBqI1u3kuToWhHul4qIDZsTZ96laJ 7ESvZEWPy3mT6HdUCHaDz0zr/j+BfXgM20PKHFsGE0lSc9/FV/lXxK/B3E7GFs73Bk11nJDmX3+ OsFzpVuxmmecduot7Bb+DgPWbncnogg+PK615TFpDiFJ/YIyTyUMcmOjDxhI4oYpBfEIzXTATif BJZM60zfV/QEgi2yW0c8wcPbtTJw= X-Received: by 2002:a5d:5d02:0:b0:3f7:b7ac:f3d2 with SMTP id ffacd0b85a97d-42704d9b22amr2469816f8f.43.1760706332347; Fri, 17 Oct 2025 06:05:32 -0700 (PDT) X-Google-Smtp-Source: AGHT+IH6B5YqgbpTpiycDu+BIeJYgxvaZQY6u0Lzqnz6hl/+7ZcS3Mz3tX+vkVrFvOUmJ4hzN2CIbA== X-Received: by 2002:a5d:5d02:0:b0:3f7:b7ac:f3d2 with SMTP id ffacd0b85a97d-42704d9b22amr2469775f8f.43.1760706331648; Fri, 17 Oct 2025 06:05:31 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-4270607009fsm4638179f8f.49.2025.10.17.06.05.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 17 Oct 2025 06:05:30 -0700 (PDT) Date: Fri, 17 Oct 2025 15:05:29 +0200 From: Stefano Brivio To: Laurent Vivier Subject: Re: [PATCH v4 2/7] epoll_ctl: Extract epoll operations Message-ID: <20251017150529.5d15bade@elisabeth> In-Reply-To: References: <20251017103129.229412-1-lvivier@redhat.com> <20251017103129.229412-3-lvivier@redhat.com> <20251017134832.70b0e58d@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 2nqPk29jltg7E0lAlegOJ0J9RGqniWnqbTT9t7uTUp0_1760706333 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: RMG7Q6C26MMV5OTT76RLHL4IDG4CB44L X-Message-ID-Hash: RMG7Q6C26MMV5OTT76RLHL4IDG4CB44L X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Fri, 17 Oct 2025 14:21:35 +0200 Laurent Vivier 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 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 > >> --- > >> 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 \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 > >> + */ > >> + > >> +#include > >> + > >> +#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 > >> + */ > >> + > >> +#ifndef EPOLL_CTL_H > >> +#define EPOLL_CTL_H > >> + > >> +#include > >> + > >> +#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