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=EM1DI9Rg; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 9C21D5A0624 for ; Wed, 21 Jan 2026 09:13:28 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1768983207; 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=B3VqSOw3Hq2amQHFpPxSmo3D69lKbikhlNf+nEy/in8=; b=EM1DI9RgKgb8ANJM0hWO/bKa2/uFo4uzuQVJsbjej5HQ9OiZCm3RraPFf/XH3B7n9Aa9nD H791J5DcbKoTWxzKGyeK4kebmVgZdRYyimaSZ5q1ppLzN2IV0+dB7tpmSNwgzXzm7ecgtG 4FvTG1wULijUjSk38i+5SlGrMm8JtOc= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-440-BeFH6ET4MS-k3y5MiqDg9Q-1; Wed, 21 Jan 2026 03:13:25 -0500 X-MC-Unique: BeFH6ET4MS-k3y5MiqDg9Q-1 X-Mimecast-MFC-AGG-ID: BeFH6ET4MS-k3y5MiqDg9Q_1768983205 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-4801c1056c7so29100495e9.2 for ; Wed, 21 Jan 2026 00:13:25 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1768983204; x=1769588004; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=B3VqSOw3Hq2amQHFpPxSmo3D69lKbikhlNf+nEy/in8=; b=Sby5ZoCRIuk0SPcKeCoHUetH9/PXqw7g95m/H72U1IDbZZ3AQ+RhgiMBQQCcaBcGLg zWcIyBwwfkAB8wWDy/WcC77l6ASUrAYLgmJrcj5riuw9DeYeFtacPMq6m2x9EJ0lyYcl 2LxBk6KgDczEXkfqVff0Yf7qM660jB/hkDe7m/k8/r9kQj2u/keEw/kxV29nFL9qP5Sl pbMGwr0SJ3xsBE5NyEvQARD832E0cK5XVTcxEzl/G2DdRHaVGGVwI5VaphpmgsauM6k+ mJX5qJSXGMNzJ4OPYgRzDDNHuxpn5m5+wPckPPtkFDSrkHu1btogdY4ZNStmf4xOMztC n3FA== X-Gm-Message-State: AOJu0YwI25fOghBY4kXb7Xzq0RJxUC8I/AXO61igaT/4G6434iFbO09L LVKGQ+qrYxlk4Cj5WcDlphFCiNpDlM42h5cdUQqYUXPclL++IvNDHDseWR4Hqp9dzSKfmQeMu9c OFtkG365G1gqUw5Sg91GEe/IJRPHMfBo6XXK7gp5yl6Nc+lXRkfZvvQ== X-Gm-Gg: AZuq6aKFWmMo3GLBUWOmp7ic8ri/kKXOKm/40zXI+XugP7K+k2BQgm/Sq0t6JfLhuOr p8e9qFdO9/z84KZB/eTmb3P34KRhOPtDgtrtwVX6W6gEEJceJ7vKJkk+wBAOSK8swi2bAuFODiO r+5qDOM1YnFj5qLatJV7NTs8N0CPkBBerevqzmjNS9Lbr6EwAU4pCeWXXQoYZEJQxcdR/3ewgFM bGZ6+rtYU2uthp8fs3VbBwwFv/5zeiHoFRYvitLRKqbLY2OFanvbV7EiP0KYCPZXHHaNE+GCaai iHm8OLYYLaJX7oh1BzWpjOxOBiGj0ZTqSrFVeoXd/m2TXoYPcGvxMEKi0qiknCBHefaVg4AEpUd MduzWsdjLe4eKUIW7cdy+aM7TGwUAZeTZJ3itaA== X-Received: by 2002:a05:600c:4686:b0:47a:7fdd:2906 with SMTP id 5b1f17b1804b1-4803e7a468emr64907035e9.12.1768983204461; Wed, 21 Jan 2026 00:13:24 -0800 (PST) X-Received: by 2002:a05:600c:4686:b0:47a:7fdd:2906 with SMTP id 5b1f17b1804b1-4803e7a468emr64906645e9.12.1768983203973; Wed, 21 Jan 2026 00:13:23 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-480424b64a8sm17145355e9.3.2026.01.21.00.13.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Jan 2026 00:13:23 -0800 (PST) Date: Wed, 21 Jan 2026 09:13:21 +0100 From: Stefano Brivio To: Laurent Vivier Subject: Re: [PATCH v2 2/3] tcp: Register fds with epoll at flow creation Message-ID: <20260121091321.22fbb246@elisabeth> In-Reply-To: <20260119161915.4014677-3-lvivier@redhat.com> References: <20260119161915.4014677-1-lvivier@redhat.com> <20260119161915.4014677-3-lvivier@redhat.com> 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: a3a3Vju_C37FSvet0-RoZ59vk-X10PVVKguAekPATOY_1768983205 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: LIXKR24IVTIQCM6YM23YGTKLWSNPQPJL X-Message-ID-Hash: LIXKR24IVTIQCM6YM23YGTKLWSNPQPJL 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, David Gibson 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 Mon, 19 Jan 2026 17:19:14 +0100 Laurent Vivier wrote: > Register connection sockets with epoll using empty events > (events=0) in tcp_conn_from_tap(), tcp_tap_conn_from_sock() > and tcp_flow_repair_socket(). > > This allows tcp_epoll_ctl() to always use EPOLL_CTL_MOD, removing > the need to check whether fds are already registered. As a result, the > conditional ADD/MOD logic is no longer needed, simplifying the function. > > Signed-off-by: Laurent Vivier > --- > flow.c | 1 + > tcp.c | 46 ++++++++++++++++++++++++---------------------- > 2 files changed, 25 insertions(+), 22 deletions(-) > > diff --git a/flow.c b/flow.c > index cefe6c8b5b24..532339ce7fe1 100644 > --- a/flow.c > +++ b/flow.c > @@ -357,6 +357,7 @@ static void flow_set_state(struct flow_common *f, enum flow_state state) > * > * Return: true if flow is registered with epoll, false otherwise > */ > +/* cppcheck-suppress unusedFunction */ > bool flow_in_epoll(const struct flow_common *f) > { > return f->epollid != EPOLLFD_ID_INVALID; > diff --git a/tcp.c b/tcp.c > index 1db861705ddb..29d69354bd94 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -528,37 +528,22 @@ static uint32_t tcp_conn_epoll_events(uint8_t events, uint8_t conn_flags) > static int tcp_epoll_ctl(struct tcp_tap_conn *conn) > { > uint32_t events; > - int m; > > if (conn->events == CLOSED) { > - if (flow_in_epoll(&conn->f)) { > - int epollfd = flow_epollfd(&conn->f); > + int epollfd = flow_epollfd(&conn->f); > > - epoll_del(epollfd, conn->sock); > - if (conn->timer != -1) > - epoll_del(epollfd, conn->timer); > - } > + epoll_del(epollfd, conn->sock); > + if (conn->timer != -1) > + epoll_del(epollfd, conn->timer); > > return 0; > } > > events = tcp_conn_epoll_events(conn->events, conn->flags); > > - if (flow_in_epoll(&conn->f)) { > - m = EPOLL_CTL_MOD; > - } else { > - flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT); > - m = EPOLL_CTL_ADD; > - } > - > - if (flow_epoll_set(&conn->f, m, events, conn->sock, > - !TAPSIDE(conn)) < 0) { > - int ret = -errno; > - > - if (m == EPOLL_CTL_ADD) > - flow_epollid_clear(&conn->f); > - return ret; > - } > + if (flow_epoll_set(&conn->f, EPOLL_CTL_MOD, events, conn->sock, > + !TAPSIDE(conn)) < 0) > + return -errno; > > return 0; > } > @@ -1710,6 +1695,11 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af, > conn->sock = s; > conn->timer = -1; > conn->listening_sock = -1; > + flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT); > + if (flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, s, TGTSIDE) < 0) { > + flow_perror(flow, "Can't register with epoll"); > + goto cancel; > + } > conn_event(c, conn, TAP_SYN_RCVD); > > conn->wnd_to_tap = WINDOW_DEFAULT; > @@ -2433,6 +2423,15 @@ static void tcp_tap_conn_from_sock(const struct ctx *c, union flow *flow, > conn->sock = s; > conn->timer = -1; > conn->ws_to_tap = conn->ws_from_tap = 0; > + > + flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT); > + if (flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, s, INISIDE) < 0) { > + flow_perror(flow, "Can't register with epoll"); > + conn_flag(c, conn, CLOSING); > + FLOW_ACTIVATE(conn); > + return; > + } > + > conn_event(c, conn, SOCK_ACCEPTED); > > hash = flow_hash_insert(c, TAP_SIDX(conn)); > @@ -3825,6 +3824,9 @@ int tcp_flow_migrate_target(struct ctx *c, int fd) > return 0; > } > > + flow_epollid_set(&conn->f, EPOLLFD_ID_DEFAULT); > + flow_epoll_set(&conn->f, EPOLL_CTL_ADD, 0, conn->sock, !TAPSIDE(conn)); Oops, I overlooked this, because I missed re-checking on v2 after David pointed it out: this causes Coverity Scan to (reasonably, I think) complain that: /home/sbrivio/passt/tcp.c:3693:2: Type: Unchecked return value (CHECKED_RETURN) /home/sbrivio/passt/tcp.c:3648:2: Unchecked call to function 1. path: Condition "!(flow = flow_alloc())", taking false branch. /home/sbrivio/passt/tcp.c:3653:2: 2. path: Condition "read_all_buf(fd, &t, 112UL /* sizeof (t) */)", taking false branch. /home/sbrivio/passt/tcp.c:3685:2: 3. path: Condition "rc = tcp_flow_repair_socket(c, conn)", taking false branch. /home/sbrivio/passt/tcp.c:3693:2: 4. path: Condition "!(conn->f.pif[1] == PIF_TAP)", taking true branch. /home/sbrivio/passt/tcp.c:3693:2: 5. check_return: Calling "flow_epoll_set" without checking return value (as is done elsewhere 6 out of 7 times). /home/sbrivio/passt/icmp.c:213:2: Examples where return value from this function is checked 6. example_checked: Example 1: "flow_epoll_set(&pingf->f, 1, EPOLLIN, pingf->sock, 1U)" has its value checked in "flow_epoll_set(&pingf->f, 1, EPOLLIN, pingf->sock, 1U) < 0". /home/sbrivio/passt/tcp.c:1695:2: Examples where return value from this function is checked 7. example_checked: Example 2: "flow_epoll_set(&conn->f, 1, 0U, s, 1U)" has its value checked in "flow_epoll_set(&conn->f, 1, 0U, s, 1U) < 0". /home/sbrivio/passt/tcp_splice.c:364:2: Examples where return value from this function is checked 8. example_checked: Example 3: "flow_epoll_set(&conn->f, 1, 0U, conn->s[1], 1U)" has its value checked in "flow_epoll_set(&conn->f, 1, 0U, conn->s[1], 1U)". /home/sbrivio/passt/tcp_splice.c:364:2: Examples where return value from this function is checked 9. example_checked: Example 4: "flow_epoll_set(&conn->f, 1, 0U, conn->s[0], 0U)" has its value checked in "flow_epoll_set(&conn->f, 1, 0U, conn->s[0], 0U)". /home/sbrivio/passt/udp_flow.c:87:2: Examples where return value from this function is checked 10. example_checked: Example 5: "flow_epoll_set(&uflow->f, 1, EPOLLIN, s, sidei)" has its value checked in "flow_epoll_set(&uflow->f, 1, EPOLLIN, s, sidei) < 0". I don't think it's overly problematic but epoll_ctl() can, in theory, fail regardless of the arguments. > + > flow_hash_insert(c, TAP_SIDX(conn)); > FLOW_ACTIVATE(conn); > -- Stefano