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

Enable SOCK_NONBLOCK on system call socket() #13093

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from

Conversation

panjf2000
Copy link
Contributor

@panjf2000 panjf2000 commented Feb 27, 2024

What this PR mainly does is:

  1. Refactor the anetCreateSocket() to make it more generic for more socket arguments, and use SOCK_NONBLOCK if available, which will reduce two system calls (F_GETFL and F_SETFL) of enabling the non-blocking mode on each newly created socket.
  2. Clean up the unused anetUnixGenericConnect() that calls anetCreateSocket().

SOCK_NONBLOCK for system call socket() is supported on most UNIX-like platforms (linux, dragonfly, freebsd, netbsd, openbsd, solaris, etc.). This improvement will significantly reduce the system calls considering how massively anetTcpGenericConnect() will be called when needed.

As for the cleanup, anetUnixGenericConnect was introduced in c61e692 and the only reference back then was from the createClient() in redis-benchmark.c which had been removed in ec8f066 and made it the dead code. Most of that dead code was also cleaned up in f657315, and it seems that the anetUnixGenericConnect got left out. Therefore, I also cleaned it up, but I'm not so certain about doing this cleanup in this PR. Maybe you would prefer to do it in a separate PR?

References

@panjf2000
Copy link
Contributor Author

panjf2000 commented Feb 27, 2024

@oranagra @yossigo

@panjf2000
Copy link
Contributor Author

Kindly ping @oranagra @yossigo

@panjf2000
Copy link
Contributor Author

Ping @oranagra @yossigo

src/anet.c Outdated Show resolved Hide resolved
src/anet.c Outdated Show resolved Hide resolved
src/anet.c Outdated Show resolved Hide resolved
src/anet.c Outdated Show resolved Hide resolved
src/anet.c Outdated Show resolved Hide resolved
src/anet.c Outdated Show resolved Hide resolved
src/unix.c Outdated Show resolved Hide resolved
@panjf2000 panjf2000 force-pushed the socket-opt branch 2 times, most recently from 9789896 to 97bfc8e Compare March 11, 2024 17:28
@panjf2000 panjf2000 requested a review from oranagra March 11, 2024 17:28
src/anet.c Outdated
Comment on lines 438 to 442
#ifdef SOCK_NONBLOCK
if (flags & ANET_CONNECT_NONBLOCK) p->ai_socktype |= SOCK_NONBLOCK;
#else
if (flags & ANET_CONNECT_NONBLOCK) sockflags |= O_NONBLOCK;
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm confused.
why do we have to pass two different flags for the same purpose, rather than let anetCreateSocket handle it implicitly with ifdefs?
as far as i remember that was your previous approach, why did you abandon it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we do care about CLOEXEC for the listen sockets, maybe we should handle them like NONBLOCK REUSEADDR, and let anetCreateSocket get flags and handle them all internally?

Did I misunderstand your previous comment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe i misunderstood something and my comment isn't applicable..

when i gave that comment it was on a piece of code that explicitly called anetCloexec, and you added an #ifdef for SOCK_CLOEXEC, but code was far from the logical "#else. so i'm trying to suggest that all the logic about these 3 flags, (whether to pass them as flags to socketor call an explicit system call after it), should all be be hidden insideanetCreateSocket` that abstracts all the nuances about how to use the system calls.

i hope it's doable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check out the latest commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oranagra Any thoughts about the latest commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @oranagra

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @oranagra

@panjf2000 panjf2000 requested a review from oranagra March 12, 2024 09:05
@panjf2000 panjf2000 changed the title Use SOCK_NONBLOCK to reduce system calls from enabling non-blocking sockets Enable SOCK_NONBLOCK on system call socket() Mar 12, 2024
@CLAassistant
Copy link

CLAassistant commented Mar 24, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

…ockets

Signed-off-by: Andy Pan <i@andypan.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Awaits merge
Development

Successfully merging this pull request may close these issues.

None yet

3 participants