[PATCH gwproxy v0] epoll: Improve log readability and efficiency

[PATCH gwproxy v0] epoll: Improve log readability and efficiency

[Ahmad Gani]

I noticed from the info log that the connection is reported as closed
twice, even though I only tested with a single curl. After investigating,
I realized it's not an error, just a misleading log message.

This change fixes the confusing log and also improves efficiency:
by moving the accept4 syscall to the top of __handle_ev_accept, we can
eliminate an unnecessary allocation.

Signed-off-by: Ahmad Gani <reyuki@gnuweeb.org>
---
 src/gwproxy/ev/epoll.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/src/gwproxy/ev/epoll.c b/src/gwproxy/ev/epoll.c
index 5cf7910888d7..9202b564ddb2 100644
--- a/src/gwproxy/ev/epoll.c
+++ b/src/gwproxy/ev/epoll.c
@@ -325,24 +325,22 @@ static int __handle_ev_accept(struct gwp_wrk *w)
 	static const int flags = SOCK_NONBLOCK | SOCK_CLOEXEC;
 	struct gwp_ctx *ctx = w->ctx;
 	struct gwp_conn_pair *gcp;
-	struct sockaddr *addr;
+	struct gwp_sockaddr addr;
 	socklen_t addr_len;
 	int fd, r;
 
+	addr_len = sizeof(addr);
+	fd = __sys_accept4(w->tcp_fd, &addr.sa, &addr_len, flags);
+	if (fd < 0)
+		return handle_accept_error(w, fd);
+
 	gcp = gwp_alloc_conn_pair(w);
 	if (unlikely(!gcp)) {
 		pr_err(&ctx->lh, "Failed to allocate connection pair on accept");
 		return handle_accept_error(w, -ENOMEM);
 	}
 
-	addr = &gcp->client_addr.sa;
-	addr_len = sizeof(gcp->client_addr);
-	fd = __sys_accept4(w->tcp_fd, addr, &addr_len, flags);
-	if (fd < 0) {
-		r = handle_accept_error(w, fd);
-		goto out_err;
-	}
-
+	gcp->client_addr = addr;
 	gwp_setup_cli_sock_options(w, fd);
 	gcp->client.fd = fd;
 	pr_dbg(&ctx->lh, "New connection from %s (fd=%d)",
-- 
Ahmad Gani

Re: [PATCH gwproxy v0] epoll: Improve log readability and efficiency

[Alviro Iskandar Setiawan]

On Sun, 17 Aug 2025 10:16:19 +0700, Ahmad Gani wrote:
> --- a/src/gwproxy/ev/epoll.c
> +++ b/src/gwproxy/ev/epoll.c
> @@ -325,24 +325,22 @@ static int __handle_ev_accept(struct gwp_wrk *w)
>         static const int flags = SOCK_NONBLOCK | SOCK_CLOEXEC;
>         struct gwp_ctx *ctx = w->ctx;
>         struct gwp_conn_pair *gcp;
> -       struct sockaddr *addr;
> +       struct gwp_sockaddr addr;
>         socklen_t addr_len;
>         int fd, r;
>
> +       addr_len = sizeof(addr);
> +       fd = __sys_accept4(w->tcp_fd, &addr.sa, &addr_len, flags);
> +       if (fd < 0)
> +               return handle_accept_error(w, fd);
> +
>         gcp = gwp_alloc_conn_pair(w);
>         if (unlikely(!gcp)) {
>                 pr_err(&ctx->lh, "Failed to allocate connection pair on accept");
>                 return handle_accept_error(w, -ENOMEM);
>         }

NAK.

If gwp_alloc_conn_pair() fails, you're leaking a TCP fd from
accept4(). After your changes, no one holds the returned fd except
that local variable.

Re: [PATCH gwproxy v0] epoll: Improve log readability and efficiency

[Ahmad Gani]

On Sun, Aug 17, 2025 at 11:24 AM Alviro Iskandar Setiawan wrote:
> NAK.
>
> If gwp_alloc_conn_pair() fails, you're leaking a TCP fd from
> accept4(). After your changes, no one holds the returned fd except
> that local variable.

Ah, you're right—I forgot to close the fd when gwp_alloc_conn_pair() fails.

I'll add a close(fd) in that path and send a v2 PATCH (and I'll use v1
instead of v0 for the subject next time).

--
Ahmad Gani

Re: [PATCH gwproxy v0] epoll: Improve log readability and efficiency

[Ammar Faizi]

On Sun, Aug 17, 2025 at 11:34:39AM +0700, Ahmad Gani wrote:
> Ah, you're right—I forgot to close the fd when gwp_alloc_conn_pair() fails.
> 
> I'll add a close(fd) in that path and send a v2 PATCH (and I'll use v1
> instead of v0 for the subject next time).

Don't use close(), use __sys_close().

-- 
Ammar Faizi

Re: [PATCH gwproxy v0] epoll: Improve log readability and efficiency

[Ahmad Gani]

On Sun, Aug 17, 2025 at 11:37 AM Ammar Faizi wrote:
> On Sun, Aug 17, 2025 at 11:34:39AM +0700, Ahmad Gani wrote:
> > Ah, you're right—I forgot to close the fd when gwp_alloc_conn_pair() fails.
> >
> > I'll add a close(fd) in that path and send a v2 PATCH (and I'll use v1
> > instead of v0 for the subject next time).
>
> Don't use close(), use __sys_close().

Understood, I'll switch to __sys_close().

By the way, I wonder why GWP_CONN_FLAG_NO_CLOSE_FD exists? Can you explain
the use cases for such a thing to exist?

Wouldn't it cause a file descriptor leak too?

--
Ahmad Gani

Re: [PATCH gwproxy v0] epoll: Improve log readability and efficiency

[Ahmad Gani]

On Sun, Aug 17, 2025 at 11:49 AM Ahmad Gani <reyuki@gnuweeb.org> wrote:
> On Sun, Aug 17, 2025 at 11:37 AM Ammar Faizi wrote:
> > On Sun, Aug 17, 2025 at 11:34:39AM +0700, Ahmad Gani wrote:
> > > Ah, you're right—I forgot to close the fd when gwp_alloc_conn_pair() fails.
> > >
> > > I'll add a close(fd) in that path and send a v2 PATCH (and I'll use v1
> > > instead of v0 for the subject next time).
> >
> > Don't use close(), use __sys_close().
>
> Understood, I'll switch to __sys_close().
>
> By the way, I wonder why GWP_CONN_FLAG_NO_CLOSE_FD exists? Can you explain
> the use cases for such a thing to exist?
>
> Wouldn't it cause a file descriptor leak too?

Oh, it is only used in the io_uring system.

--
Ahmad Gani

Re: [PATCH gwproxy v0] epoll: Improve log readability and efficiency

[Ammar Faizi]

On Sun, Aug 17, 2025 at 11:49:13AM +0700, Ahmad Gani wrote:
> By the way, I wonder why GWP_CONN_FLAG_NO_CLOSE_FD exists? Can you explain
> the use cases for such a thing to exist?
> 
> Wouldn't it cause a file descriptor leak too?

I should have explained it better.

GWP_CONN_FLAG_NO_CLOSE_FD is only used from the io_uring case.

__sys_close() is a heavy syscall and better be done asynchronously via
prep_close() in io_uring context.

GWP_CONN_FLAG_NO_CLOSE_FD allows log_conn_pair_close() to log the fd
numbers without the obligation to call __sys_close() in free_conn().

Previously, it was confusing to debug when the fd is set to -1 by
the gwp_free_conn_pair()'s caller.

GWP_CONN_FLAG_NO_CLOSE_FD simply means the fds are no longer owned by gcp.

-- 
Ammar Faizi

Re: [PATCH gwproxy v0] epoll: Improve log readability and efficiency

[Ahmad Gani]

On Sun, Aug 17, 2025 at 11:57 AM Ammar Faizi wrote:
> On Sun, Aug 17, 2025 at 11:49:13AM +0700, Ahmad Gani wrote:
> > By the way, I wonder why GWP_CONN_FLAG_NO_CLOSE_FD exists? Can you explain
> > the use cases for such a thing to exist?
> >
> > Wouldn't it cause a file descriptor leak too?
>
> I should have explained it better.
>
> GWP_CONN_FLAG_NO_CLOSE_FD is only used from the io_uring case.
>
> __sys_close() is a heavy syscall and better be done asynchronously via
> prep_close() in io_uring context.
>
> GWP_CONN_FLAG_NO_CLOSE_FD allows log_conn_pair_close() to log the fd
> numbers without the obligation to call __sys_close() in free_conn().
>
> Previously, it was confusing to debug when the fd is set to -1 by
> the gwp_free_conn_pair()'s caller.
>
> GWP_CONN_FLAG_NO_CLOSE_FD simply means the fds are no longer owned by gcp.

I see; now it makes sense. Thanks for the explanation!

--
Ahmad Gani