From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202512 header.b=ltzR/PPw; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id C13765A004E for ; Sat, 27 Dec 2025 05:43:51 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202512; t=1766810629; bh=OskkMHB0x8Wgh03gNVYNLenShHfYG8iIhLDoOg8jfws=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ltzR/PPwpe2LusTB4jXdApxnUPr9ftiCH+Uuekjk8K1ymlNmMOxa9oLDHQRcfcvyl u9zce/lmQ/lJm7qunrKJDJsJ/CQstk+PF+G2LVib7cO1gmsEWlGWmTcQmY3eFVTfzi I2x17cFo/pc6qcnUTGjnS9xddAwsvRHMUxOckaJ9E/Pj8T23YlWrTtVirIftmMvzdn 3f1Sp/yIDBAA2Tm+MQKthJnyKHfU2ZGBcetNwV4Tajh6cmqa9BYBztVVgTNX7MxdpR CZGWc2mGs6azMYcywLbwfTIw4VlHlAOqKhrgPUkWyfIcOQEgyXfiDoyLLhhxGUrCnG QmosbybXVATJA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4ddVHT3LMYz4wB8; Sat, 27 Dec 2025 15:43:49 +1100 (AEDT) Date: Sat, 27 Dec 2025 15:43:42 +1100 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH 7/7] flow: Have flow_epoll_set() retrieve file descriptor from flow structure Message-ID: References: <20251219164518.930012-1-lvivier@redhat.com> <20251219164518.930012-8-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="sek7Mq7MvMQLYEjO" Content-Disposition: inline In-Reply-To: <20251219164518.930012-8-lvivier@redhat.com> Message-ID-Hash: HGOKI5MODP7WNFZ72CXKVHF4JOIRI6BW X-Message-ID-Hash: HGOKI5MODP7WNFZ72CXKVHF4JOIRI6BW X-MailFrom: dgibson@gandalf.ozlabs.org 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: --sek7Mq7MvMQLYEjO Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Dec 19, 2025 at 05:45:18PM +0100, Laurent Vivier wrote: > Currently, callers of flow_epoll_set() pass the file descriptor as an > explicit parameter. However, the function already has access to the flow > structure and knows the epoll type, so it can retrieve the appropriate fd > itself. >=20 > Remove the fd parameter and have flow_epoll_set() extract the file > descriptor directly from the flow based on the type: > - EPOLL_TYPE_TCP_SPLICE: conn->s[sidei] > - EPOLL_TYPE_TCP: conn->sock > - EPOLL_TYPE_TCP_TIMER: conn->timer > - EPOLL_TYPE_PING: pingf->sock > - EPOLL_TYPE_UDP: uflow->s[sidei] As with the previous patch, this seems like a layering violation to me, and I'm not seeing a strong enough case for this change to outweight that. > For TCP timers, the previous logic determined ADD vs MOD by checking > whether conn->timer was -1. Since the timer fd must now be assigned to t= he > flow before calling flow_epoll_set(), this check would no longer work. > Instead, introduce FLOW_EPOLL_TIMER_ADD and FLOW_EPOLL_TIMER_MOD constants > and have callers specify the operation explicitly via the sidei parameter. Blech. > This requires callers to assign the socket or timer fd to the flow > structure before calling flow_epoll_set(), with appropriate cleanup > (resetting to -1) if the epoll registration fails. >=20 > Signed-off-by: Laurent Vivier > --- > flow.c | 26 +++++++++++++++++++------- > flow.h | 4 +++- > icmp.c | 3 +-- > tcp.c | 12 ++++++------ > tcp_splice.c | 4 ++-- > udp_flow.c | 6 ++++-- > 6 files changed, 35 insertions(+), 20 deletions(-) >=20 > diff --git a/flow.c b/flow.c > index 6b9ccca6ef50..7d6f9f2bf1a9 100644 > --- a/flow.c > +++ b/flow.c > @@ -451,13 +451,14 @@ static uint32_t tcp_conn_epoll_events(uint8_t event= s, uint8_t conn_flags) > * flow_epoll_set() - Add or modify epoll registration for a flow socket > * @type: epoll type > * @f: Flow to register socket for > - * @fd: File descriptor to register > - * @sidei: Side index of the flow > + * @sidei: Side index of the flow (for most types), or for EPOLL_TYPE_TC= P_TIMER: > + * FLOW_EPOLL_TIMER_ADD to add the timer to epoll, > + * FLOW_EPOLL_TIMER_MOD to modify an existing registration > * > * Return: 0 on success, -1 on error (from epoll_ctl()) > */ > int flow_epoll_set(enum epoll_type type, const struct flow_common *f, > - int fd, unsigned int sidei) > + unsigned int sidei) > { > struct epoll_event ev; > union epoll_ref ref; > @@ -465,13 +466,13 @@ int flow_epoll_set(enum epoll_type type, const stru= ct flow_common *f, > int epollfd; > int m; > =20 > - ref.fd =3D fd; > ref.type =3D type; > =20 > switch (type) { > case EPOLL_TYPE_TCP_SPLICE: { > const struct tcp_splice_conn *conn =3D &((union flow *)f)->tcp_splice; > =20 > + ref.fd =3D conn->s[sidei]; > ref.flowside =3D flow_sidx(f, sidei); > if (flow_in_epoll(f)) { > m =3D EPOLL_CTL_MOD; > @@ -487,6 +488,7 @@ int flow_epoll_set(enum epoll_type type, const struct= flow_common *f, > case EPOLL_TYPE_TCP: { > const struct tcp_tap_conn *conn =3D &((union flow *)f)->tcp; > =20 > + ref.fd =3D conn->sock; > ref.flowside =3D flow_sidx(f, sidei); > if (flow_in_epoll(f)) { > m =3D EPOLL_CTL_MOD; > @@ -502,24 +504,34 @@ int flow_epoll_set(enum epoll_type type, const stru= ct flow_common *f, > case EPOLL_TYPE_TCP_TIMER: { > const struct tcp_tap_conn *conn =3D &((union flow *)f)->tcp; > =20 > + ref.fd =3D conn->timer; > ref.flow =3D flow_idx(f); > - m =3D conn->timer =3D=3D -1 ? EPOLL_CTL_ADD : EPOLL_CTL_MOD; > + if (sidei =3D=3D FLOW_EPOLL_TIMER_ADD) > + m =3D EPOLL_CTL_ADD; > + else > + m =3D EPOLL_CTL_MOD; > epollfd =3D flow_epollfd(f); > events =3D EPOLLIN | EPOLLET; > break; > } > - case EPOLL_TYPE_PING: > + case EPOLL_TYPE_PING: { > + const struct icmp_ping_flow *pingf =3D &((union flow *)f)->ping; > + > + ref.fd =3D pingf->sock; > ref.flowside =3D flow_sidx(f, sidei); > m =3D EPOLL_CTL_ADD; > epollfd =3D flow_epollfd(f); > events =3D EPOLLIN; > break; > + } > case EPOLL_TYPE_UDP: { > + const struct udp_flow *uflow =3D &((union flow *)f)->udp; > union { > flow_sidx_t sidx; > uint32_t data; > } fref =3D { .sidx =3D flow_sidx(f, sidei) }; > =20 > + ref.fd =3D uflow->s[sidei]; > ref.data =3D fref.data; > m =3D EPOLL_CTL_ADD; > epollfd =3D flow_epollfd(f); > @@ -533,7 +545,7 @@ int flow_epoll_set(enum epoll_type type, const struct= flow_common *f, > ev.events =3D events; > ev.data.u64 =3D ref.u64; > =20 > - return epoll_ctl(epollfd, m, fd, &ev); > + return epoll_ctl(epollfd, m, ref.fd, &ev); > } > =20 > /** > diff --git a/flow.h b/flow.h > index ed1e16139a4d..3a8224dba3b0 100644 > --- a/flow.h > +++ b/flow.h > @@ -266,7 +266,9 @@ int flow_epollfd(const struct flow_common *f); > void flow_epollid_set(struct flow_common *f, int epollid); > void flow_epollid_clear(struct flow_common *f); > int flow_epoll_set(enum epoll_type type, const struct flow_common *f, > - int fd, unsigned int sidei); > + unsigned int sidei); > +#define FLOW_EPOLL_TIMER_MOD 0 > +#define FLOW_EPOLL_TIMER_ADD 1 > void flow_epollid_register(int epollid, int epollfd); > void flow_defer_handler(const struct ctx *c, const struct timespec *now); > int flow_migrate_source_early(struct ctx *c, const struct migrate_stage = *stage, > diff --git a/icmp.c b/icmp.c > index 5487a904117d..2f2fb56ed726 100644 > --- a/icmp.c > +++ b/icmp.c > @@ -210,8 +210,7 @@ static struct icmp_ping_flow *icmp_ping_new(const str= uct ctx *c, > goto cancel; > =20 > flow_epollid_set(&pingf->f, EPOLLFD_ID_DEFAULT); > - if (flow_epoll_set(EPOLL_TYPE_PING, &pingf->f, pingf->sock, > - TGTSIDE) < 0) { > + if (flow_epoll_set(EPOLL_TYPE_PING, &pingf->f, TGTSIDE) < 0) { > close(pingf->sock); > goto cancel; > } > diff --git a/tcp.c b/tcp.c > index 2e8a7d516273..730da457081c 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -507,15 +507,14 @@ static int tcp_epoll_ctl(const struct ctx *c, struc= t tcp_tap_conn *conn) > return 0; > } > =20 > - if (flow_epoll_set(EPOLL_TYPE_TCP, &conn->f, conn->sock, > - !TAPSIDE(conn))) > + if (flow_epoll_set(EPOLL_TYPE_TCP, &conn->f, !TAPSIDE(conn))) > return -errno; > =20 > flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT); > =20 > if (conn->timer !=3D -1) { > if (flow_epoll_set(EPOLL_TYPE_TCP_TIMER, &conn->f, > - conn->timer, 0)) > + FLOW_EPOLL_TIMER_MOD)) > return -errno; > } > =20 > @@ -546,13 +545,14 @@ static void tcp_timer_ctl(const struct ctx *c, stru= ct tcp_tap_conn *conn) > return; > } > =20 > - if (flow_epoll_set(EPOLL_TYPE_TCP_TIMER, &conn->f, fd, 0)) { > + conn->timer =3D fd; > + if (flow_epoll_set(EPOLL_TYPE_TCP_TIMER, &conn->f, > + FLOW_EPOLL_TIMER_ADD)) { > flow_dbg_perror(conn, "failed to add timer"); > close(fd); > + conn->timer =3D -1; > return; > } > - > - conn->timer =3D fd; > } > =20 > if (conn->flags & ACK_TO_TAP_DUE) { > diff --git a/tcp_splice.c b/tcp_splice.c > index 1eeb678d534b..964a559782a3 100644 > --- a/tcp_splice.c > +++ b/tcp_splice.c > @@ -117,8 +117,8 @@ static struct tcp_splice_conn *conn_at_sidx(flow_sidx= _t sidx) > */ > static int tcp_splice_epoll_ctl(struct tcp_splice_conn *conn) > { > - if (flow_epoll_set(EPOLL_TYPE_TCP_SPLICE, &conn->f, conn->s[0], 0) || > - flow_epoll_set(EPOLL_TYPE_TCP_SPLICE, &conn->f, conn->s[1], 1)) { > + if (flow_epoll_set(EPOLL_TYPE_TCP_SPLICE, &conn->f, 0) || > + flow_epoll_set(EPOLL_TYPE_TCP_SPLICE, &conn->f, 1)) { > int ret =3D -errno; > flow_perror(conn, "ERROR on epoll_ctl()"); > return ret; > diff --git a/udp_flow.c b/udp_flow.c > index afacfb356261..9a67ad701c34 100644 > --- a/udp_flow.c > +++ b/udp_flow.c > @@ -83,10 +83,12 @@ static int udp_flow_sock(const struct ctx *c, > return s; > } > =20 > + uflow->s[sidei] =3D s; > flow_epollid_set(&uflow->f, EPOLLFD_ID_DEFAULT); > - rc =3D flow_epoll_set(EPOLL_TYPE_UDP, &uflow->f, s, sidei); > + rc =3D flow_epoll_set(EPOLL_TYPE_UDP, &uflow->f, sidei); > if (rc < 0) { > close(s); > + uflow->s[sidei] =3D -1; > return rc; > } > =20 > @@ -95,11 +97,11 @@ static int udp_flow_sock(const struct ctx *c, > =20 > epoll_del(flow_epollfd(&uflow->f), s); > close(s); > + uflow->s[sidei] =3D -1; > =20 > flow_dbg_perror(uflow, "Couldn't connect flow socket"); > return rc; > } > - uflow->s[sidei] =3D s; > =20 > /* It's possible, if unlikely, that we could receive some packets in > * between the bind() and connect() which may or may not be for this > --=20 > 2.51.1 >=20 --=20 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 --sek7Mq7MvMQLYEjO Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmlPY/EACgkQzQJF27ox 2GftHQ/8DK7nJPlGSQwzqcHuAN0+xLD7opFvsLBHvJYNYU+MX0TJO/tUDu/EzyXH h3Dk9GJKSwWFbzzKuoMiNMJVvs0NjwOa4FsI8n1gvWJmUsNEu4vWzWWfKjxHFrHV uusXMSpJgoIQs4n/E0ugtQNaN0Nd8fZYhNv7JpH3LpfVhGVVhEZQFlcXbgdcGrJy 7OKFyJvRqFeCP5SJAD40UirG1SVAtpOpybBHYJzAo/Y4pABSi6lXkqKfy43HMvyH jOOKf4Xfmhq4nyIDr8Yp5p5EihGJg18+OWPDBkQoXiZ02YRrSLfZpLYF4KXpWbbn VZawwhG5TTMBSY6tHCEdTa5BTe9DO1v8iJU5QvMR6sz+j5Ji/PjKqLduIiP2Ni8E qUV9mhYSRRslG3L3Od6XiJczaceZRJZgBauOJfcEmblQ6rN6cH4ZY+rNKZbYELo2 UYslOjbKEQW7G6LAaLQQrlg4fXeNrHb4ygEJKoYHN6ukJvYnJUU2JXq2S/WCu+v2 HEPVfphviOTWy/hcIduCm1gRlCsE38e9CsxZOxM7K3BbWa5s6ZGZjtIygXr79sHP UjEnd8l4oBEGLU5CcSwAnwiIXwZFTIWcEe+q93gBglx6jOGVOX7fqtmefzwBLjgf LmdPeYANLxxHbpEXSzx9MAvtnEvsvdAFQQFhM/pqomiNMk0dpTc= =Q91J -----END PGP SIGNATURE----- --sek7Mq7MvMQLYEjO--