omit setting TCP_NODELAY flag for a socket that inherits the property from an accepting socket#1568

chenbd · 2017-12-23T07:35:18Z

Accepted socket can inherit "TCP_NODELAY" option form listening socket.
So we can set this option on listening socket and remove setting on accepted socket.

kazuho · 2017-12-25T02:00:13Z

Thank you for the PR.

Do you have a benchmark that shows the practical benefit of changing the code?

The reason I ask is because per my understanding whether if TCP_NODELAY is inherited depends on the platform. We want to avoid platform-dependent code unless there is a strong reason.

FWIW, FreeBSD's man page does not mention that (whereas it says that some other options are inherited).

NetBSD's man page states:

In the historical BSD TCP implementation, if the TCP_NODELAY option was set on a passive socket, the sockets returned by accept(2) erroneously did not have the TCP_NODELAY option set; the behavior was corrected to inherit TCP_NODELAY in NetBSD 1.6.

chenbd · 2017-12-25T03:25:48Z

@kazuho
I dig into this and found this:
http://www.openldap.org/lists/openldap-bugs/200009/msg00191.html

FreeBSD 4.5 have TCP_NODELAY inherited from accepted socket.

seems all bsd and linux(not too old version, NetBSD 1.6 guess is too old) should have the same behaviour.
hope this will gain a little performance, but did not do the benchmark.
i'll do the benchmark with "ab" on my pc if you need this.
but should not expect a big performance gain i think.

kazuho · 2017-12-26T01:51:57Z

It's not only FreeBSD that we support. H2O is known to compile on other platforms as well (e.g. NetBSD, OpenBSD, Solaris, AIX, etc.). We also want to keep H2O portable to new platforms; that is why we generally stick to what is defined in POSIX.

Therefore, I would highly appreciate it if you could adjust the PR so that it would work as an optimization on platforms that support inheritance of the property.

For example, how about doing the following?

set TCP_NODELAY in h2o_evloop_socket_create on all platforms (we can ignore the result, as we do now in create_socket_set_nodelay)
skip setting TCP_NODELAY in h2o_evloop_socket_accept on selected platforms (i.e. linux, freebsd)

chenbd · 2017-12-26T03:27:42Z

set TCP_NODELAY in h2o_evloop_socket_create on all platforms (we can ignore the result, as we do now in create_socket_set_nodelay)

h2o_evloop_socket_create will be called with fd from pipe/eventfd/socketpair/unix domain socket.
these fds guess not support this option.
is this really what you want?

skip setting TCP_NODELAY in h2o_evloop_socket_accept on selected platforms (i.e. linux, freebsd)

this make sense, will try to do this.

kazuho · 2017-12-26T18:57:58Z

h2o_evloop_socket_create will be called with fd from pipe/eventfd/socketpair/unix domain socket.
these fds guess not support this option.
is this really what you want?

Yes.

There's no reason to avoid doing that, since none of them would be used in a performance-sensitive way.

Requiring the user of the event loop to set the flag would increase the complexity; it is better to keep the contract between the user and the event loop as simple as possible.

…ening socket

kazuho

Thank you for the changes. I think that we are almost done.

I have left my ideas hoping that we might be able to make the code even simpler. Please let me know what you think.

kazuho · 2017-12-27T21:18:22Z

lib/common/socket/evloop.c.h

+         */
+        setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, &flag, sizeof(flag));
+    }
+
    return &create_socket(loop, fd, flags)->super;


Could we just call create_socket_set_nodelay instead?

yes, we can. fixed

kazuho · 2017-12-27T21:23:50Z

lib/common/socket/evloop.c.h

+#endif
+#endif
+
+    return &create_socket(listener->loop, fd, H2O_SOCKET_FLAG_IS_ACCEPTED_CONNECTION)->super;


How about calling create_socket if H2O_USE_ACCEPT4 is set, and call create_socket_set_nodelay if otherwise? There is a #if H2O_USE_ACCEPT4 ... #else ... #endif block right above, to which you can merge the logic.

For platforms that support inheritance of TCP_NODELAY but do not provide accept4 (maybe it's only NetBSD?), we would be taking a lock and call fcntl in addition to accept anyways. So I do not think that it's necessary to complicate the code in order to have this optimization.

merge logic using H2O_USE_ACCEPT4 for simplicity.

kazuho · 2018-01-05T01:16:43Z

Thank you for all the work. Merged to master.

chenbd added 5 commits December 27, 2017 10:03

move "TCP_NODELAY" from accepted socket to listening socket

98dc0dd

fix ci error: Linking C executable examples-simple

0cab5f5

fix ci error: Failed test at t/40unix-socket.t line 26

3dbe9bc

set "TCP_NODELAY" on accepted socket in case of not inherit from list…

83cd708

…ening socket

move TCP_NODELAY flag setting to h2o_evloop_socket_create()

a388777

chenbd force-pushed the tcp_nodelay_to_listening_socket branch from edf70a4 to a388777 Compare December 27, 2017 02:10

fix coding miss (missing ";")

482a307

kazuho reviewed Dec 27, 2017

View reviewed changes

more clean up about "move TCP_NODELAY"

781c37a

kazuho changed the title ~~move "TCP_NODELAY" from accepted socket to listening socket~~ omit setting TCP_NODELAY flag for a socket that inherits the property from an accepting socket Jan 5, 2018

kazuho merged commit a1cae48 into h2o:master Jan 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

omit setting TCP_NODELAY flag for a socket that inherits the property from an accepting socket#1568

omit setting TCP_NODELAY flag for a socket that inherits the property from an accepting socket #1568

chenbd commented Dec 23, 2017

kazuho commented Dec 25, 2017

chenbd commented Dec 25, 2017

kazuho commented Dec 26, 2017

chenbd commented Dec 26, 2017 •
edited

Loading

kazuho commented Dec 26, 2017 •
edited

Loading

kazuho left a comment

kazuho Dec 27, 2017

chenbd Dec 28, 2017

kazuho Dec 27, 2017

chenbd Dec 28, 2017

kazuho commented Jan 5, 2018

omit setting TCP_NODELAY flag for a socket that inherits the property from an accepting socket#1568

omit setting TCP_NODELAY flag for a socket that inherits the property from an accepting socket #1568

Conversation

chenbd commented Dec 23, 2017

kazuho commented Dec 25, 2017

chenbd commented Dec 25, 2017

kazuho commented Dec 26, 2017

chenbd commented Dec 26, 2017 • edited Loading

kazuho commented Dec 26, 2017 • edited Loading

kazuho left a comment

Choose a reason for hiding this comment

kazuho Dec 27, 2017

Choose a reason for hiding this comment

chenbd Dec 28, 2017

Choose a reason for hiding this comment

kazuho Dec 27, 2017

Choose a reason for hiding this comment

chenbd Dec 28, 2017

Choose a reason for hiding this comment

kazuho commented Jan 5, 2018

chenbd commented Dec 26, 2017 •
edited

Loading

kazuho commented Dec 26, 2017 •
edited

Loading