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=202510 header.b=GeZtzECG; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 750055A061B for ; Tue, 07 Oct 2025 08:08:27 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202510; t=1759817304; bh=Ugr2f++rDW6HDRrtevySGE2HQidgoRZLUhyE/RUjDtA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=GeZtzECGDaewFa7wnh1qpUMo9Xcernd//NHQWxIGDvXhQ7/0lSN3gJu2iu7cbnJu+ tUODVwr60vMJtkVWu2VniwYsIW2dRwLUXQ8BaZNbY2b19OP5LAf3wSkY109coK0l6O CpFQQzpa1eZ0CisHVsjl9majFpM7dxGPuy3W5qDBsyUEPTW3gHU6QukorQXJ9aeUqo vduY2E2ydpuOibH2UUqxMQT5MbHpYoLlLgU+Y+3IsPoYcessrZMvqERqHwVi3u9vVN XkMleCH/lLlGgw4gjDKjMXD0xu3v1NCHVWsfjmYDKF36VSZS+UFv/yRQ13HVnBZ1CF C761Vif45+vHA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4cgm0S52Jwz4wB6; Tue, 7 Oct 2025 17:08:24 +1100 (AEDT) Date: Tue, 7 Oct 2025 16:57:42 +1100 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH 2/5] util: Move epoll registration out of sock_l4_sa() Message-ID: References: <20251003152717.2437765-1-lvivier@redhat.com> <20251003152717.2437765-3-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="t/ON8tB8RWMTGymj" Content-Disposition: inline In-Reply-To: <20251003152717.2437765-3-lvivier@redhat.com> Message-ID-Hash: LTSSWTGGHMDHE6O34S7YQDJQQL5B3D2Q X-Message-ID-Hash: LTSSWTGGHMDHE6O34S7YQDJQQL5B3D2Q 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: --t/ON8tB8RWMTGymj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Oct 03, 2025 at 05:27:14PM +0200, Laurent Vivier wrote: > Move epoll_ctl() calls from sock_l4_sa() to the protocol-specific code > (icmp.c, pif.c, udp_flow.c) to give callers more control over epoll > registration. This allows sock_l4_sa() to focus solely on socket > creation and binding, while epoll management happens at a higher level. >=20 > Remove the data parameter from sock_l4_sa() and flowside_sock_l4() as > it's no longer needed - callers now construct the full epoll_ref and > register the socket themselves after creation. I like this idea in principle. I've previously thought that doing the epoll registration in sock_l4_sa() was verging on a layering violation. However... [snip] > diff --git a/icmp.c b/icmp.c > index bd3108a21675..d6f0abe68269 100644 > --- a/icmp.c > +++ b/icmp.c > @@ -172,10 +172,11 @@ static struct icmp_ping_flow *icmp_ping_new(const s= truct ctx *c, > { > uint8_t proto =3D af =3D=3D AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6; > uint8_t flowtype =3D af =3D=3D AF_INET ? FLOW_PING4 : FLOW_PING6; > - union epoll_ref ref =3D { .type =3D EPOLL_TYPE_PING }; > union flow *flow =3D flow_alloc(); > struct icmp_ping_flow *pingf; > const struct flowside *tgt; > + struct epoll_event ev; > + union epoll_ref ref; > =20 > if (!flow) > return NULL; > @@ -196,9 +197,7 @@ static struct icmp_ping_flow *icmp_ping_new(const str= uct ctx *c, > =20 > pingf->seq =3D -1; > =20 > - ref.flowside =3D FLOW_SIDX(flow, TGTSIDE); > - pingf->sock =3D flowside_sock_l4(c, EPOLL_TYPE_PING, PIF_HOST, > - tgt, ref.data); > + pingf->sock =3D flowside_sock_l4(c, EPOLL_TYPE_PING, PIF_HOST, tgt); > =20 > if (pingf->sock < 0) { > warn("Cannot open \"ping\" socket. You might need to:"); > @@ -210,6 +209,18 @@ static struct icmp_ping_flow *icmp_ping_new(const st= ruct ctx *c, > if (pingf->sock > FD_REF_MAX) > goto cancel; > =20 > + ref.type =3D EPOLL_TYPE_PING; > + ref.flowside =3D FLOW_SIDX(flow, TGTSIDE); > + ref.fd =3D pingf->sock; > + > + ev.events =3D EPOLLIN; > + ev.data.u64 =3D ref.u64; > + if (epoll_ctl(c->epollfd, EPOLL_CTL_ADD, pingf->sock, &ev) =3D=3D -1) { > + warn_perror("L4 epoll_ctl"); > + close(pingf->sock); > + goto cancel; > + } > + =2E.. this is uncomfortably bulky to have in every protocol. Could we maybe mitigate this with an epoll_add() helper of some sort that does at least some of the bit shuffling to construct the epoll data? --=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 --t/ON8tB8RWMTGymj Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmjkq60ACgkQzQJF27ox 2GfR3g//cBmpKM1FgiKLn2xHoBI0ohMYqqmUUMvN7srrLegVfgWk9R902OHgQu6j 1uIqc8aU7hs5YblYChNGeyStVb9iaJrrTKkJPXTmg3hlylcq1lhYXUsgSZSVw9Yr 57in5nWfoS9orcBYlcpfJX7jR8fOjMIlvdrAnY1lQdLQpOWhQr4ZvymfW3D7pSei xoTRIIRB3caQXq0PcjEjHP9c5fxWlDB4mP5EjCDbvcm4NU8GnhHBYSwl3r3cYhj9 eiEywY7jaHjvZSt9zLJRppbVlbNNDZAdQgi4M6kc2XaZNvOadsUUyjXKpbq4dFpi ud6KrmjRA6WhWBMXymFtr/0suOCgWgR8PK941f6S+UQBq+FjrqWAt4JGvxJEBTLx /GFt0Vc0NqtzaH7/K4WcQRyjI/+NJOIkuY2zmxSTN9YgvYPDtSMdlLMxedQ8PCaC MGw+C8stJPqvnRsOKR/ojBv76b/plwIt2nuJjm9Bcrrumn2e64Q00M9DHZ5Sm8ip 6YxpS10K1gnoav4i7wHSVdx74L0oSKRiCAiGE0TLeFjdK1xoEISWvQXKoApEt3iy cs0IisP/1wZB0eq/3w3ncjH03RdRkmcCA/stTky1mfm0SfMAXADldnIgplIhlLMM //3Ks6t6/huSvzIAYtb/lo0zO8KQ+aSi6YgzjG/GE7fmvpxHv0Q= =tCrV -----END PGP SIGNATURE----- --t/ON8tB8RWMTGymj--