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

Set TCP_USER_TIMEOUT socket option for linux #16419

Merged
merged 8 commits into from Sep 13, 2018

Conversation

Projects
None yet
5 participants
@yashykt
Member

yashykt commented Aug 21, 2018

@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 21, 2018

****************************************************************

libgrpc.so

     VM SIZE                                                           FILE SIZE
 ++++++++++++++ GROWING                                             ++++++++++++++
  +0.0%    +272 [None]                                                 +776  +0.0%
  +9.5%    +352 src/core/lib/iomgr/socket_utils_common_posix.cc        +352  +9.5%
      [NEW]    +348 grpc_set_socket_tcp_user_timeout                       +348  [NEW]
      +2.1%      +4 [Unmapped]                                               +4  +2.1%
  +1.0%     +32 src/core/lib/iomgr/tcp_client_posix.cc                  +32  +1.0%
      +3.1%     +21 grpc_tcp_client_prepare_fd                              +21  +3.1%
       +26%     +11 [Unmapped]                                              +11   +26%
  +0.8%     +16 src/core/lib/iomgr/tcp_server_utils_posix_common.cc     +16  +0.8%
      +2.6%     +23 grpc_tcp_server_prepare_socket                          +23  +2.6%

  +0.0%    +672 TOTAL                                               +1.15Ki  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 21, 2018

[trickle] No significant performance differences
@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 21, 2018

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,953,022      Total (>)      1,952,970

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
10,665,007      Total (>)     10,664,807

 No significant differences in binary sizes


@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 21, 2018

[microbenchmarks] No significant performance differences
@srini100

This could be a behavior change for some users. There should be a way to disable it. Any plan to make it user configurable especially in light of this issue #15889 ?

@srini100

This comment has been minimized.

Contributor

srini100 commented Aug 21, 2018

Is this only for Linux? Is there an equivalent in MacOS?

@yashykt

This comment has been minimized.

Member

yashykt commented Aug 21, 2018

I agree that there should be a way to configure it (including disabling it). I will put out a C++ specific gRFC for that. I am keeping that PR separate from this because that can only go in after the gRFC is approved.

@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 22, 2018

****************************************************************

libgrpc.so

     VM SIZE                                                           FILE SIZE
 ++++++++++++++ GROWING                                             ++++++++++++++
  +0.0%    +256 [None]                                                 +776  +0.0%
  +9.9%    +368 src/core/lib/iomgr/socket_utils_common_posix.cc        +368  +9.9%
      [NEW]    +364 grpc_set_socket_tcp_user_timeout                       +364  [NEW]
      +2.1%      +4 [Unmapped]                                               +4  +2.1%
  +1.0%     +32 src/core/lib/iomgr/tcp_client_posix.cc                  +32  +1.0%
      +3.1%     +21 grpc_tcp_client_prepare_fd                              +21  +3.1%
       +26%     +11 [Unmapped]                                              +11   +26%
  +0.8%     +16 src/core/lib/iomgr/tcp_server_utils_posix_common.cc     +16  +0.8%
      +2.6%     +23 grpc_tcp_server_prepare_socket                          +23  +2.6%

  +0.0%    +672 TOTAL                                               +1.16Ki  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 22, 2018

[trickle] No significant performance differences
@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 22, 2018

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,953,022      Total (>)      1,952,970

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
10,665,261      Total (>)     10,665,046

 No significant differences in binary sizes


@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 22, 2018

[microbenchmarks] No significant performance differences
@@ -242,7 +242,7 @@ grpc_error* grpc_set_socket_tcp_user_timeout(int fd, int val) {
return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
"Failed to set TCP_USER_TIMEOUT");
}
#endif /* GPR_LINUX */
#endif /* GRPC_HAVE_TCP_USER_TIMEOUT */

This comment has been minimized.

@srini100

srini100 Aug 22, 2018

Contributor

Add an info log if this cannot be set.

This comment has been minimized.

@yashykt

yashykt Aug 22, 2018

Member

done!

yashykt added some commits Aug 22, 2018

@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 22, 2018

****************************************************************

libgrpc.so

     VM SIZE                                                           FILE SIZE
 ++++++++++++++ GROWING                                             ++++++++++++++
  +0.0%    +256 [None]                                                 +776  +0.0%
  +9.9%    +368 src/core/lib/iomgr/socket_utils_common_posix.cc        +368  +9.9%
      [NEW]    +364 grpc_set_socket_tcp_user_timeout                       +364  [NEW]
      +2.1%      +4 [Unmapped]                                               +4  +2.1%
  +1.0%     +32 src/core/lib/iomgr/tcp_client_posix.cc                  +32  +1.0%
      +3.1%     +21 grpc_tcp_client_prepare_fd                              +21  +3.1%
       +26%     +11 [Unmapped]                                              +11   +26%
  +0.8%     +16 src/core/lib/iomgr/tcp_server_utils_posix_common.cc     +16  +0.8%
      +2.6%     +23 grpc_tcp_server_prepare_socket                          +23  +2.6%

  +0.0%    +672 TOTAL                                               +1.16Ki  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



1 similar comment
@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 22, 2018

****************************************************************

libgrpc.so

     VM SIZE                                                           FILE SIZE
 ++++++++++++++ GROWING                                             ++++++++++++++
  +0.0%    +256 [None]                                                 +776  +0.0%
  +9.9%    +368 src/core/lib/iomgr/socket_utils_common_posix.cc        +368  +9.9%
      [NEW]    +364 grpc_set_socket_tcp_user_timeout                       +364  [NEW]
      +2.1%      +4 [Unmapped]                                               +4  +2.1%
  +1.0%     +32 src/core/lib/iomgr/tcp_client_posix.cc                  +32  +1.0%
      +3.1%     +21 grpc_tcp_client_prepare_fd                              +21  +3.1%
       +26%     +11 [Unmapped]                                              +11   +26%
  +0.8%     +16 src/core/lib/iomgr/tcp_server_utils_posix_common.cc     +16  +0.8%
      +2.6%     +23 grpc_tcp_server_prepare_socket                          +23  +2.6%

  +0.0%    +672 TOTAL                                               +1.16Ki  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 22, 2018

[trickle] No significant performance differences
1 similar comment
@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 22, 2018

[trickle] No significant performance differences
@srini100

This comment has been minimized.

Contributor

srini100 commented Aug 22, 2018

As discussed, let's hold off on this PR and do a gRFC to add channel arg and sort out default and min values.

@srini100 srini100 added the lang/core label Aug 22, 2018

@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 22, 2018

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,953,107      Total (>)      1,952,970

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
10,665,259      Total (>)     10,665,045

 No significant differences in binary sizes


@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 22, 2018

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,953,107      Total (>)      1,952,970

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
10,665,255      Total (>)     10,665,053

 No significant differences in binary sizes


@sreecha sreecha self-requested a review Aug 22, 2018

@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 22, 2018

[microbenchmarks] No significant performance differences
1 similar comment
@grpc-testing

This comment has been minimized.

grpc-testing commented Aug 22, 2018

[microbenchmarks] No significant performance differences
@@ -222,6 +222,32 @@ grpc_error* grpc_set_socket_low_latency(int fd, int low_latency) {
return GRPC_ERROR_NONE;
}
#define DEFAULT_TCP_USER_TIMEOUT 20000 /* 20 seconds */

This comment has been minimized.

@sreecha

sreecha Aug 22, 2018

Contributor

Add the MILLIS suffix to the name i.e DEFAULT_TCP_USER_TIMEOUT_MILLIS

This comment has been minimized.

@yashykt

yashykt Sep 6, 2018

Member

changed the suffix to _MS following code at other places

@@ -76,6 +76,8 @@ static grpc_error* prepare_socket(const grpc_resolved_address* addr, int fd,
if (!grpc_is_unix_socket(addr)) {
err = grpc_set_socket_low_latency(fd, 1);
if (err != GRPC_ERROR_NONE) goto error;
err = grpc_set_socket_tcp_user_timeout(fd, 0 /* set to gRPC default */);

This comment has been minimized.

@sreecha

sreecha Aug 22, 2018

Contributor

Can you add support to set this as a channel arg as well ?
Fortunately, the required plumbing is already in place - i.e channel_args is passed as an argument to this function and just few lines below , we even have code that parses the args for GRPC_ARG_SOCKET_MUTATOR :)

This comment has been minimized.

@yashykt
@sreecha

A couple of minor changes.

Also, please add a test for this (once you add support to pass it via channel args)

@yashykt

The proposal was submitted in grpc/proposal#95

@@ -222,6 +222,32 @@ grpc_error* grpc_set_socket_low_latency(int fd, int low_latency) {
return GRPC_ERROR_NONE;
}
#define DEFAULT_TCP_USER_TIMEOUT 20000 /* 20 seconds */

This comment has been minimized.

@yashykt

yashykt Sep 6, 2018

Member

changed the suffix to _MS following code at other places

@@ -76,6 +76,8 @@ static grpc_error* prepare_socket(const grpc_resolved_address* addr, int fd,
if (!grpc_is_unix_socket(addr)) {
err = grpc_set_socket_low_latency(fd, 1);
if (err != GRPC_ERROR_NONE) goto error;
err = grpc_set_socket_tcp_user_timeout(fd, 0 /* set to gRPC default */);

This comment has been minimized.

@yashykt
@yashykt

This comment has been minimized.

Member

yashykt commented Sep 6, 2018

It would be hard to create infrastructure that would bring down interfaces to imitate a bad connection, hence manually tested this.

@grpc-testing

This comment has been minimized.

grpc-testing commented Sep 6, 2018

****************************************************************

libgrpc.so

     VM SIZE                                                           FILE SIZE
 ++++++++++++++ GROWING                                             ++++++++++++++
  +0.0%    +288 [None]                                              +1.38Ki  +0.0%
  +1.7%     +64 src/core/lib/iomgr/socket_utils_common_posix.cc         +64  +1.7%
      [NEW]     +42 grpc_set_socket_tcp_user_timeout                        +42  [NEW]
       +10%     +20 [Unmapped]                                              +20   +10%
      [NEW]      +2 config_default_tcp_user_timeout                          +2  [NEW]
  +1.0%     +32 src/core/lib/iomgr/tcp_client_posix.cc                  +32  +1.0%
      +4.0%     +27 grpc_tcp_client_prepare_fd                              +27  +4.0%
       +13%      +5 [Unmapped]                                               +5   +13%
  +1.6%     +32 src/core/lib/iomgr/tcp_server_utils_posix_common.cc     +32  +1.6%
      +3.4%     +31 grpc_tcp_server_prepare_socket                          +31  +3.4%
      +4.2%      +1 [Unmapped]                                               +1  +4.2%

  +0.0%    +416 TOTAL                                               +1.51Ki  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing

This comment has been minimized.

grpc-testing commented Sep 6, 2018

[trickle] No significant performance differences
@grpc-testing

This comment has been minimized.

grpc-testing commented Sep 6, 2018

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,956,113      Total (>)      1,955,968

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
10,844,340      Total (>)     10,843,878

 No significant differences in binary sizes


@grpc-testing

This comment has been minimized.

grpc-testing commented Sep 6, 2018

[microbenchmarks] No significant performance differences
@grpc-testing

This comment has been minimized.

grpc-testing commented Sep 13, 2018

****************************************************************

libgrpc.so

     VM SIZE                                                           FILE SIZE
 ++++++++++++++ GROWING                                             ++++++++++++++
  +0.0%    +320 [None]                                              +5.36Ki  +0.1%
  +1.7%     +64 src/core/lib/iomgr/socket_utils_common_posix.cc         +64  +1.7%
      [NEW]     +42 grpc_set_socket_tcp_user_timeout                        +42  [NEW]
       +10%     +20 [Unmapped]                                              +20   +10%
      [NEW]      +2 config_default_tcp_user_timeout                          +2  [NEW]
  +1.0%     +32 src/core/lib/iomgr/tcp_client_posix.cc                  +32  +1.0%
      +4.0%     +27 grpc_tcp_client_prepare_fd                              +27  +4.0%
       +13%      +5 [Unmapped]                                               +5   +13%
  +1.6%     +32 src/core/lib/iomgr/tcp_server_utils_posix_common.cc     +32  +1.6%
      +3.4%     +31 grpc_tcp_server_prepare_socket                          +31  +3.4%
      +4.2%      +1 [Unmapped]                                               +1  +4.2%

  +0.0%    +448 TOTAL                                               +5.48Ki  +0.1%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing

This comment has been minimized.

grpc-testing commented Sep 13, 2018

[trickle] No significant performance differences
@grpc-testing

This comment has been minimized.

grpc-testing commented Sep 13, 2018

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,961,059      Total (>)      1,960,914

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
10,883,611      Total (>)     10,883,158

 No significant differences in binary sizes


@grpc-testing

This comment has been minimized.

grpc-testing commented Sep 13, 2018

[microbenchmarks] No significant performance differences
@yashykt

This comment has been minimized.

Member

yashykt commented Sep 13, 2018

Known issues : #14128, #15489, #16123
Not sure about the ruby flake but it's possibly the same as #16587

@yashykt

This comment has been minimized.

Member

yashykt commented Sep 13, 2018

Thanks for reviewing!

@yashykt yashykt merged commit edfec1b into grpc:master Sep 13, 2018

33 of 38 checks passed

Bazel UBSAN build for C/C++ Infrastructure error. Please ignore for now.
Details
Basic Tests MacOS [opt] (internal CI) Internal CI build failed
Details
Basic Tests Windows [dbg] (internal CI) Internal CI build failed
Details
Bazel Debug build for C/C++ Internal CI build failed
Details
Bazel Opt build for C/C++ Internal CI build failed
Details
Android (Internal CI) Internal CI build successful
Details
Artifact Build Linux (internal CI) Internal CI build successful
Details
Artifact Build MacOS (internal CI) Internal CI build successful
Details
Artifact Build Windows (internal CI) Internal CI build successful
Details
Asan C (internal CI) Internal CI build successful
Details
Asan C++ (internal CI) Internal CI build successful
Details
Basic Tests C Linux [dbg] (internal CI) Internal CI build successful
Details
Basic Tests C Linux [opt] (internal CI) Internal CI build successful
Details
Basic Tests C++ Linux [dbg] (internal CI) Internal CI build successful
Details
Basic Tests C++ Linux [opt] (internal CI) Internal CI build successful
Details
Basic Tests MacOS [dbg] (internal CI) Internal CI build successful
Details
Basic Tests Multi-language Linux (internal CI) Internal CI build successful
Details
Basic Tests Windows [opt] (internal CI) Internal CI build successful
Details
Bazel ASAN build for C/C++ Internal CI build successful
Details
Bazel Basic build for C/C++ Internal CI build successful
Details
Bazel Basic tests for Python Internal CI build successful
Details
Bazel TSAN build for C/C++ Internal CI build successful
Details
Distribution Tests Linux (standalone subset) Internal CI build successful
Details
Distribution Tests Windows (standalone subset) Internal CI build successful
Details
Interop ALTS Tests (internal CI) Internal CI build successful
Details
Interop Cloud-to-Cloud Tests (internal CI) Internal CI build successful
Details
Interop Cloud-to-Prod Tests (internal CI) Internal CI build successful
Details
Microbenchmarks Diff (internal CI) Internal CI build successful
Details
Msan C (internal CI) Internal CI build successful
Details
Portability Tests Linux [Build Only] (internal CI) Internal CI build successful
Details
Portability Tests Windows [Build Only] (internal CI) Internal CI build successful
Details
Sanity Checks (internal CI) Internal CI build successful
Details
Trickle Diff (internal CI) Internal CI build successful
Details
Tsan C (internal CI) Internal CI build successful
Details
Tsan C++ (internal CI) Internal CI build successful
Details
UBsan C (internal CI) Internal CI build successful
Details
cla/linuxfoundation yashykt authorized
Details
iOS Binary Size Diff (internal CI) Internal CI build successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment