Skip to content

subsystem: net tcp #3198

Closed
theanarkh wants to merge 6 commits into
libuv:masterfrom
theanarkh:reuseport
Closed

subsystem: net tcp #3198
theanarkh wants to merge 6 commits into
libuv:masterfrom
theanarkh:reuseport

Conversation

@theanarkh

@theanarkh theanarkh commented Jun 9, 2021

Copy link
Copy Markdown
Contributor

support reuserport flag for tcp socket, improve performance and load balancing

reuserport flag in linux will make process accept and handle the connection better

@theanarkh theanarkh closed this Jun 9, 2021
@theanarkh theanarkh reopened this Jun 9, 2021
support reuserport flag for tcp socket, improve performance and load balancing

reuserport flag in linux will make process accept and handle the connection better

@vtjnash vtjnash left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This cannot be enabled by default, as it is a functional change

My understanding is that linux also requires this to be after the bind call, while freebsd requires it to be before the bind call.

gc added 2 commits June 11, 2021 01:48
support reuserport flag for tcp socket, improve performance and load balancing

reuserport flag in linux will make process accept and handle the connection better
support reuserport flag for tcp socket, improve performance and load balancing

reuserport flag in linux will make process accept and handle the connection better
@theanarkh

Copy link
Copy Markdown
Contributor Author

This cannot be enabled by default, as it is a functional change

My understanding is that linux also requires this to be after the bind call, while freebsd requires it to be before the bind call.

Thanks for review 。I have add environment variable SO_REUSEPORT to control this flag。In addition
。In Linux,setsockopt need to be called before bind。Below is the document。

SO_REUSEPORT (since Linux 3.9)
Permits multiple AF_INET or AF_INET6 sockets to be bound
to an identical socket address. This option must be set
on each socket (including the first socket) prior to
calling bind(2) on the socket.

@vtjnash

vtjnash commented Jun 10, 2021

Copy link
Copy Markdown
Member

Ah, you are right, I had that backwards (FreeBSD must be afterwards)

@theanarkh

Copy link
Copy Markdown
Contributor Author

Ah, you are right, I had that backwards (FreeBSD must be afterwards)

OS usually check the port in bind call。so i think we should call setsockopt before bind。And i find this document(https://www.freebsd.org/cgi/man.cgi?query=setsockopt&sektion=2&format=html) that also describe this。

@theanarkh theanarkh requested a review from vtjnash June 15, 2021 03:12
@stale

stale Bot commented Jul 8, 2021

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the stale label Jul 8, 2021
@yunnysunny

Copy link
Copy Markdown

I think it's a good resolution to resolve the issue of nodejs/node#37343

@stale stale Bot removed the stale label Jul 15, 2021

@santigimeno santigimeno left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think using an environment variable is the libuv way to do this. I think a better option would be defining a specific uv_tcp_flags flag: UV_TCP_REUSEPORT that can be passed to uv_tcp_bind().

@theanarkh

Copy link
Copy Markdown
Contributor Author

I don't think using an environment variable is the libuv way to do this. I think a better option would be defining a specific uv_tcp_flags flag: UV_TCP_REUSEPORT that can be passed to uv_tcp_bind().

Thanks for review. I have change the code. Looking forward to your review again

@theanarkh theanarkh requested a review from santigimeno July 21, 2021 17:41
@theanarkh

Copy link
Copy Markdown
Contributor Author

I think it's a good resolution to resolve the issue of nodejs/node#37343

I have add this flags into my Node.js branch(branch add-reuseport of https://github.com/theanarkh/node). You can test the performance in Linux 3.9 and above.

@santigimeno

santigimeno commented Jul 23, 2021

Copy link
Copy Markdown
Member

Codewise, except some style nits I think it's good, but I've been thinking about it and I'm not sure we should add this option as the SO_REUSEPORT seems to be implemented differently across platforms. (see: https://stackoverflow.com/a/14388707). From what I understand this PR is focused in the Linux implementation which only seems to be implemented also on FreeBSD 12+ by using the SO_REUSEPORT_LB option. So I see two options:

  1. Don't add this as you can accomplish the same by calling setsockopt() yourself before calling uv_tcp_bind().
  2. Rename the option to UV_TCP_REUSEPORT_LB to be clearer what's the purpose of the flag and have it implemented only where supported (linux 3.9+, FreeBSD 12+).

My choice would be 1 but let's see what other @libuv/collaborators think.

@theanarkh

Copy link
Copy Markdown
Contributor Author

Codewise, except some style nits I think it's good, but I've been thinking about it and I'm not sure we should add this option as the SO_REUSEPORT seems to be implemented differently across platforms. (see: https://stackoverflow.com/a/14388707). From what I understand this PR is focused in the Linux implementation which only seems to be implemented also on FreeBSD 12+ by using the SO_REUSEPORT_LB option. So I see two options:

  1. Don't add this as you can accomplish the same by calling setsockopt() yourself before calling uv_tcp_bind().
  2. Rename the option to UV_TCP_REUSEPORT_LB to be clearer what's the purpose of the flag and have it implemented only where supported (linux 3.9+, FreeBSD 12+).

My choice would be 1 but let's see what other @libuv/collaborators think.

Thanks for your advice, What I think is that Node.js Server runs under Linux in most cases, so supporting this flag allows Node.js to benefit from Linux. For systems that do not support this flag, I also added compatible code for Node.js(demo code: https://github.com/theanarkh/node/pull/1/files). Do you mean not to support this flag in Libuv, or do not modify the code in uv_tcp_bind? Nginx supports this flag already.

@santigimeno

Copy link
Copy Markdown
Member

Do you mean not to support this flag in Libuv, or do not modify the code in uv_tcp_bind?

What I mean is that the consumer of libuv (nodejs in this case) could call setsockopt() directly only on specific platforms before calling uv_tcp_bind()

@theanarkh

Copy link
Copy Markdown
Contributor Author

Do you mean not to support this flag in Libuv, or do not modify the code in uv_tcp_bind?

What I mean is that the consumer of libuv (nodejs in this case) could call setsockopt() directly only on specific platforms before calling uv_tcp_bind()

before uv_tcp_bind the socket have not been made(make in uv__tcp_bind). consumer make the socket?

@santigimeno

Copy link
Copy Markdown
Member

Yeah, I forgot to add, use uv_tcp_init_ex() instead of uv_tcp_init() to create the socket

@theanarkh

Copy link
Copy Markdown
Contributor Author

Yeah, I forgot to add, use uv_tcp_init_ex() instead of uv_tcp_init() to create the socket

I think it should be that the bottom layer supports certain features, and then the upper layer decides whether to enable it, rather than when the upper layer needs to use the feature, it needs to write similar repetitive code, which will increase the burden on the upper layer.

@santigimeno

Copy link
Copy Markdown
Member

I understand, but the issue here is that it's not a feature that can be implemented cross-platform. BTW, as a reference here's an issue from some years ago about this very same topic: #1229 (comment). The solution it refers to is also using uv_tcp_init_ex().

@theanarkh

Copy link
Copy Markdown
Contributor Author

I understand, but the issue here is that it's not a feature that can be implemented cross-platform. BTW, as a reference here's an issue from some years ago about this very same topic: #1229 (comment). The solution it refers to is also using uv_tcp_init_ex().

Thanks again, i get it, i try to support this in C++ and js of Node.js。

@stale

stale Bot commented Aug 13, 2021

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the stale label Aug 13, 2021
@santigimeno

Copy link
Copy Markdown
Member

I'm going to close this. Please feel free to re-open in case you thing there's more to discuss. Thanks

@yunnysunny

Copy link
Copy Markdown

I have read the source code of libuv, and found that the function of uv__set_reuse was called in uv__udp_bind. Does this make harmful for cross-platform envrionment?

libuv/src/unix/udp.c

Lines 562 to 566 in 0f696da

if (flags & UV_UDP_REUSEADDR) {
err = uv__set_reuse(fd);
if (err)
return err;
}

@bnoordhuis

Copy link
Copy Markdown
Member

@yunnysunny I'm not sure I understand your question. Can you elaborate?

@yunnysunny

Copy link
Copy Markdown

@yunnysunny I'm not sure I understand your question. Can you elaborate?

@santigimeno said we should use uv_tcp_init_ex to set the option of SO_REUSEPORT instead of setting it in the function of uv__tcp_bind directly. But when use the option of SO_REUSEPORT, it sets it just in uv__udp_bind. I think we should use the same design to resolve the similar problems in our code.

@theanarkh

Copy link
Copy Markdown
Contributor Author

SO_REUSEPORT is different between tcp and udp. For udp it means mutilcast which is supported in multiple platform. So i think this is why the Libuv supports it.

@yunnysunny

yunnysunny commented Nov 19, 2021

Copy link
Copy Markdown

SO_REUSEPORT is different between tcp and udp. For udp it means mutilcast which is supported in multiple platform. So i think this is why the Libuv supports it.

👌

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants