Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

socketpair: add eventfd and use SOCK_NONBLOCK for socketpair() #13874

Closed
wants to merge 5 commits into from

Conversation

panjf2000
Copy link
Contributor

@panjf2000 panjf2000 commented Jun 4, 2024

Currently, we use pipe for wakeup_create, which requires two file descriptors. Furthermore, given its complexity inside, pipe is a bit heavyweight for just a simple event wait/notify mechanism.

eventfd would be a more suitable solution for this kind of scenario, kernel also advocates for developers to use eventfd instead of pipe for some simple use cases:

Applications can use an eventfd file descriptor instead of a pipe
(see pipe(2)) in all cases where a pipe is used simply to signal
events. The kernel overhead of an eventfd file descriptor is
much lower than that of a pipe, and only one file descriptor is
required (versus the two required for a pipe).

This PR adds the new backend of eventfd for wakeup_create and uses it where available, eliminating the overhead of pipe. Also, it optimizes the wakeup_create to eliminate the system calls that make file descriptors non-blocking by moving the logic of setting non-blocking flags on file descriptors to socketpair.c and using SOCK_NONBLOCK for socketpair(2), EFD_NONBLOCK for eventfd(2).

Ref:
https://man7.org/linux/man-pages/man7/pipe.7.html
https://man7.org/linux/man-pages/man2/eventfd.2.html
https://man7.org/linux/man-pages/man2/socketpair.2.html
https://www.gnu.org/software/gnulib/manual/html_node/eventfd.html

@panjf2000 panjf2000 force-pushed the wakeup-opt branch 4 times, most recently from b5b4453 to afef94a Compare June 4, 2024 08:07
lib/socketpair.h Outdated Show resolved Hide resolved
Currently, we use `pipe` for `wakeup_create`, which requires ***two***
file descriptors. Furthermore, given its complexity inside, `pipe` is a
bit heavyweight for just a simple event wait/notify mechanism.

`eventfd` would be a more suitable solution for this kind of scenario,
kernel also advocates for developers to use `eventfd` instead of `pipe`
in some simple use cases:
> Applications can use an eventfd file descriptor instead of a pipe
   (see pipe(2) in all cases where a pipe is used simply to signal
    events.  The kernel overhead of an eventfd file descriptor is
    much lower than that of a pipe, and only one file descriptor is
    required (versus the two required for a pipe).

This PR adds the new backend of `eventfd` for `wakeup_create` and uses
it where available, eliminating the overhead of `pipe`. Also, it optimizes
the `wakeup_create` to eliminate the system calls that make file descriptors
non-blocking by moving the logic of setting non-blocking flags on file
descriptors to `socketpair.c` and using `SOCK_NONBLOCK` for `socketpair(2)`,
`EFD_NONBLOCK` for `eventfd(2)`.

Ref:
https://man7.org/linux/man-pages/man7/pipe.7.html
https://man7.org/linux/man-pages/man2/eventfd.2.html
https://man7.org/linux/man-pages/man2/socketpair.2.html
https://www.gnu.org/software/gnulib/manual/html_node/eventfd.html
lib/asyn-thread.c Outdated Show resolved Hide resolved
lib/asyn-thread.c Outdated Show resolved Hide resolved
lib/multi.c Outdated Show resolved Hide resolved
lib/asyn-thread.c Show resolved Hide resolved
@bagder bagder closed this in 23fe1a5 Jun 4, 2024
@bagder
Copy link
Member

bagder commented Jun 4, 2024

Thanks!

@panjf2000 panjf2000 deleted the wakeup-opt branch June 4, 2024 22:00
@MarcelRaad
Copy link
Member

Unfortunately, this breaks --disable-socketpair / CURL_DISABLE_SOCKETPAIR, and there's a curl_socket_t to int narrowing conversion warning in 64-bit MinGW builds:
https://curl.se/dev/log.cgi?id=20240605035856-3529577
I wonder why the latter didn't show up in the GitHub CI.

@panjf2000
Copy link
Contributor Author

How did it break? What's the error info?

@panjf2000
Copy link
Contributor Author

I'm investigating this.

@bagder
Copy link
Member

bagder commented Jun 5, 2024

Unfortunately, this breaks --disable-socketpair / CURL_DISABLE_SOCKETPAIR

Is this breakage perhaps specific to Windows? I built ./configure --disable-socketpair but I can't spot any problems...

@panjf2000
Copy link
Contributor Author

Unfortunately, this breaks --disable-socketpair / CURL_DISABLE_SOCKETPAIR

Is this breakage perhaps specific to Windows? I built ./configure --disable-socketpair but I can't spot any problems...

I've run ./configure --disable-socketpair --with-openssl && make && make test on my Linux server, and it passed all tests without reporting any errors. I assume that issue is MinGW-specific.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants