You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2020/04/10 06:41:03 UTC

[kudu-CR] WIP: rpc: add support for passing an open file descriptor across a unix socket 

Hello Andrew Wong, Grant Henke,

I'd like you to do a code review. Please visit

    http://gerrit.cloudera.org:8080/15706

to review the following change.


Change subject: WIP: rpc: add support for passing an open file descriptor across a unix socket
......................................................................

WIP: rpc: add support for passing an open file descriptor across a unix socket

This adds RpcController::SendFileDescriptor() which allows an RPC client
to pass an open file to the server. The server can retrieve the file
from the RpcContext.

The eventual goal here is that a client can create a memfd file (or an
unlinked file in /dev/shm if memfd isn't supported) and pass it to a
tserver in a Scan RPC. The server can then write the scan results into
the file instead of passing the data back via the normal RPC sidecar
mechanism. Assuming the client has mmapped the file, this should results
in the elimination of a bunch of copies and load on the reactor threads.

WIP because I don't want to commit this until I've proven the idea out
end-to-end and seen it actually produce a noticeable perf gain.

Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
---
M src/kudu/gutil/linux_syscall_support.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/fd.cc
A src/kudu/util/fd.h
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
23 files changed, 439 insertions(+), 21 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15706/1
-- 
To view, visit http://gerrit.cloudera.org:8080/15706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
Gerrit-Change-Number: 15706
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>

[kudu-CR] WIP: rpc: add support for passing an open file descriptor across a unix socket

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Andrew Wong, Kudu Jenkins, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15706

to look at the new patch set (#2).

Change subject: WIP: rpc: add support for passing an open file descriptor across a unix socket
......................................................................

WIP: rpc: add support for passing an open file descriptor across a unix socket

This adds RpcController::SendFileDescriptor() which allows an RPC client
to pass an open file to the server. The server can retrieve the file
from the RpcContext.

The eventual goal here is that a client can create a memfd file (or an
unlinked file in /dev/shm if memfd isn't supported) and pass it to a
tserver in a Scan RPC. The server can then write the scan results into
the file instead of passing the data back via the normal RPC sidecar
mechanism. Assuming the client has mmapped the file, this should results
in the elimination of a bunch of copies and load on the reactor threads.

WIP because I don't want to commit this until I've proven the idea out
end-to-end and seen it actually produce a noticeable perf gain.

Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
---
M src/kudu/gutil/linux_syscall_support.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/fd.cc
A src/kudu/util/fd.h
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
23 files changed, 459 insertions(+), 30 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15706/2
-- 
To view, visit http://gerrit.cloudera.org:8080/15706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
Gerrit-Change-Number: 15706
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] rpc: add support for passing an open file descriptor across a unix socket

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Andrew Wong, Kudu Jenkins, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15706

to look at the new patch set (#4).

Change subject: rpc: add support for passing an open file descriptor across a unix socket
......................................................................

rpc: add support for passing an open file descriptor across a unix socket

This adds RpcController::SendFileDescriptor() which allows an RPC client
to pass an open file to the server. The server can retrieve the file
from the RpcContext.

The eventual goal here is that a client can create an in-memory file
descriptor (using either memfd_create on newer kernels or shm_open on
older ones) and pass it to a tserver in a Scan RPC. The server can then
write the scan results into the file instead of passing the data back
via the normal RPC sidecar mechanism. Assuming the client has mmapped
the file, this eliminates a bunch of copies and load on the reactor
threads.

This includes a benchmark that compares various methods for doing this
shared memory transport. Results from running the benchmark on various
systems show that the shared memory can be substantially (2-5x) faster
than the existing sidecar approach:

88-thread Xeon E5-2699 v4 @ 2.20GHz, el7:
  https://gist.github.com/toddlipcon/c691afcb72ddea7d1c6c1e54904e94a6

8-thread Core i7-7820HQ @ 2.9Ghz, Ubuntu 18:
  https://gist.github.com/toddlipcon/28193dd2d2017bba2aaebdea78bcee6a

AWS r5d.2xlarge 8vCPU (Xeon Platinum 8175M CPU @ 2.50GHz), Ubuntu 18:
  https://gist.github.com/toddlipcon/f84a09a92d24939d9d49dd7747d8aa9e

Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
---
M docs/design-docs/rpc.md
M src/kudu/rpc/connection.cc
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/service_pool.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
A src/kudu/util/fd.h
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
24 files changed, 944 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15706/4
-- 
To view, visit http://gerrit.cloudera.org:8080/15706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
Gerrit-Change-Number: 15706
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] rpc: add support for passing an open file descriptor across a unix socket

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15706 )

Change subject: rpc: add support for passing an open file descriptor across a unix socket
......................................................................


Patch Set 4:

(18 comments)

http://gerrit.cloudera.org:8080/#/c/15706/4/docs/design-docs/rpc.md
File docs/design-docs/rpc.md:

http://gerrit.cloudera.org:8080/#/c/15706/4/docs/design-docs/rpc.md@328
PS4, Line 328: caes
> case
Done


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/connection.cc@472
PS4, Line 472: call->has_outbound_fds() && !remote_.is_unix()
> nit: wrap into PREDICT_FALSE ?
Done


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/connection.cc@663
PS4, Line 663: vector<FileDescriptor> extra_fds;
> Since the code below in the loop calls std::move(extra_fds), maybe it makes
Done


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/outbound_call.h@196
PS4, Line 196: outbound_fds_.empty()
> It is safe to call outbound_fds_.empty() after std::move() was called on it
I'll add a note that the result of this is undefined if called after TakeOutboundFds(). The C++ standard says that the state of an object is "valid but undefined" after move has been called on it.


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc-test-base.h@26
PS4, Line 26: #include <unistd.h>
            : #include <sched.h>
            : #include <sys/mman.h>
            : #include <emmintrin.h>
            : #include <xmmintrin.h>
> style nit: it seems these should precede std C++ headers
Done


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc-test-base.h@376
PS4, Line 376:  
> nit: drop extra space?
Done


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc-test-base.h@386
PS4, Line 386:  
> ditto
Done


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc-test-base.h@414
PS4, Line 414: SendBackDataRequestPB*
> style nit: maybe, use constant reference since 'req' is immutable?
no can do -- this is an implementation of an RPC call, and those pass by pointer since technically you can respond to the call outside the scope of the method


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc-test-base.h@418
PS4, Line 418:       case SendBackDataRequestPB::WRITE_TO_FILE_DESCRIPTOR:
> nit: add FALLTHROUGH_INTENDED ?
I personally think that's a little silly when the fallthrough is on a consecutive line with no intervening code, but added it anyway :)


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc-test-base.h@475
PS4, Line 475:         LOG(FATAL);
> nit: maybe, log a message if this triggers?
Done


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc_controller.h
File src/kudu/rpc/rpc_controller.h:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc_controller.h@228
PS4, Line 228: the
> nit: drop 'the' ?
Done


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc_controller.cc
File src/kudu/rpc/rpc_controller.cc:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc_controller.cc@76
PS4, Line 76:   outbound_sidecars_total_bytes_ = 0;
> Do we need to call outbound_fds_.clear() as a part of Reset()?
good catch.


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc_controller.cc@166
PS4, Line 166: !fd.is_valid()
> nit: wrap into PREDICT_FALSE() ?
Done


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/service_pool.h
File src/kudu/rpc/service_pool.h:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/service_pool.h@93
PS4, Line 93: std::vector<scoped_refptr<kudu::Thread>>
> Is it intentional that it's a copy, not a reference?
Done


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/transfer.h
File src/kudu/rpc/transfer.h:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/transfer.h@81
PS4, Line 81:   // with those extra bytes, they will be stored in 'extra_fds'
> nit: add a stop/period
Done


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/transfer.h@99
PS4, Line 99:   std::vector<FileDescriptor> TakeReceivedFds();
> Add a doc string?
Done


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/security/tls_socket.cc@102
PS4, Line 102: !fds.empty()
> nit: wrap into PREDICT_FALSE() ?
Done


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/util/net/socket.h
File src/kudu/util/net/socket.h:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/util/net/socket.h@150
PS4, Line 150: virtual Status Writev(const struct ::iovec *iov, int iov_len, int64_t *nwritten) final;
> Is the combination of 'virtual' combined with 'final' is here for avoid sha
I inlined the body so it's clear what's going on. This is just avoiding default arguments on virtual methods by instead using an overload with fewer arguments. It's final to be clear that subclasses should be overloading the one with all the arguments, not the one with fewer (since otherwise some call sites might continue to hit the base class)



-- 
To view, visit http://gerrit.cloudera.org:8080/15706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
Gerrit-Change-Number: 15706
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 17 Apr 2020 19:54:23 +0000
Gerrit-HasComments: Yes

[kudu-CR] rpc: add support for passing an open file descriptor across a unix socket

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15706 )

Change subject: rpc: add support for passing an open file descriptor across a unix socket
......................................................................


Patch Set 6:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/outbound_call.h@111
PS6, Line 111:   std::vector<int> TakeOutboundFds() {
nit: not add &&?


http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/outbound_call.cc@148
PS6, Line 148: move
nit: hrm, i'm surprised this works without using std::. Is this perhaps inherited from boost? Or am I missing a using std:: somewhere?


http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/reactor.h
File src/kudu/rpc/reactor.h:

http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/reactor.h@201
PS6, Line 201: pid_for_tests
nit: tid, not pid?


http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/rpc_controller.cc
File src/kudu/rpc/rpc_controller.cc:

http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/rpc_controller.cc@167
PS6, Line 167: SendFileDescriptor
nit: given this isn't actually sending the descriptor yet, consider AddFileDescriptor for parity with AddOutboundSidecar?


http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/util/net/socket.cc
File src/kudu/util/net/socket.cc:

http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/util/net/socket.cc@447
PS6, Line 447:   
nit: extra space


http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/util/net/socket.cc@472
PS6, Line 472:     int aux_len = CMSG_LEN(send_fds.size() * sizeof(int));
Why don't we need to check that send_fds doesn't exceed kMaxPassedFds?


http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/util/net/socket.cc@587
PS6, Line 587: if (cmsg->cmsg_level == SOL_SOCKET &&
             :         cmsg->cmsg_type == SCM_RIGHTS) {
             :       if (cmsg->cmsg_len <= sizeof(struct cmsghdr)) {
             :         // An invalid or empty SCM_RIGHTS.
             :         continue;
             :       }
Do you think it's worth vlogging skipped cmgs?



-- 
To view, visit http://gerrit.cloudera.org:8080/15706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
Gerrit-Change-Number: 15706
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 01 May 2020 05:08:53 +0000
Gerrit-HasComments: Yes

[kudu-CR] rpc: add support for passing an open file descriptor across a unix socket

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15706 )

Change subject: rpc: add support for passing an open file descriptor across a unix socket
......................................................................


Patch Set 6:

(4 comments)

Overall looks good (just some nits), but there is a compatibility issue with macOS.

http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/rpc_stub-test.cc
File src/kudu/rpc/rpc_stub-test.cc:

http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/rpc_stub-test.cc@119
PS6, Line 119: sched_getaffinity
This seems to be Linux-specific and doesn't compile on macOS.  Is it possible to carve out these utility classes and test using these if compiling under macOS?


http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/rpc/rpc_stub-test.cc@128
PS6, Line 128: private:
nit: add indent (one space)


http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/util/net/socket.cc
File src/kudu/util/net/socket.cc:

http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/util/net/socket.cc@452
PS6, Line 452: send_fds.size() > kMaxPassedFds
nit: wrap into PREDICT_FALSE() ?


http://gerrit.cloudera.org:8080/#/c/15706/6/src/kudu/util/net/socket.cc@571
PS6, Line 571: MSG_CMSG_CLOEXEC
This flags seems to be supported on Linux (2.6.23+), FreeBSD (10.0-RELEASE+), but it doesn't compile on my macOS (HighSierra, 10.13.6).  I didn't find a way around this, probably we shouldn't use this flag if compiling on macOS (and allow for races)?



-- 
To view, visit http://gerrit.cloudera.org:8080/15706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
Gerrit-Change-Number: 15706
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 20 Apr 2020 18:44:29 +0000
Gerrit-HasComments: Yes

[kudu-CR] rpc: add support for passing an open file descriptor across a unix socket

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15706 )

Change subject: rpc: add support for passing an open file descriptor across a unix socket
......................................................................


Patch Set 4:

(18 comments)

Initial quick pass.

http://gerrit.cloudera.org:8080/#/c/15706/4/docs/design-docs/rpc.md
File docs/design-docs/rpc.md:

http://gerrit.cloudera.org:8080/#/c/15706/4/docs/design-docs/rpc.md@328
PS4, Line 328: caes
case


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/connection.cc@472
PS4, Line 472: call->has_outbound_fds() && !remote_.is_unix()
nit: wrap into PREDICT_FALSE ?


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/connection.cc@663
PS4, Line 663: vector<FileDescriptor> extra_fds;
Since the code below in the loop calls std::move(extra_fds), maybe it makes sense to move this under the corresponding while(true) scope?

The same for extra_buf?


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/outbound_call.h@196
PS4, Line 196: outbound_fds_.empty()
It is safe to call outbound_fds_.empty() after std::move() was called on it?  From reading https://en.cppreference.com/w/cpp/utility/move , it seems calling empty() should be OK since it hasn't any preconditions.


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc-test-base.h@26
PS4, Line 26: #include <unistd.h>
            : #include <sched.h>
            : #include <sys/mman.h>
            : #include <emmintrin.h>
            : #include <xmmintrin.h>
style nit: it seems these should precede std C++ headers


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc-test-base.h@376
PS4, Line 376:  
nit: drop extra space?


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc-test-base.h@386
PS4, Line 386:  
ditto


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc-test-base.h@414
PS4, Line 414: SendBackDataRequestPB*
style nit: maybe, use constant reference since 'req' is immutable?


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc-test-base.h@418
PS4, Line 418:       case SendBackDataRequestPB::WRITE_TO_FILE_DESCRIPTOR:
nit: add FALLTHROUGH_INTENDED ?


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc-test-base.h@475
PS4, Line 475:         LOG(FATAL);
nit: maybe, log a message if this triggers?


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc_controller.h
File src/kudu/rpc/rpc_controller.h:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc_controller.h@228
PS4, Line 228: the
nit: drop 'the' ?


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc_controller.cc
File src/kudu/rpc/rpc_controller.cc:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc_controller.cc@76
PS4, Line 76:   outbound_sidecars_total_bytes_ = 0;
Do we need to call outbound_fds_.clear() as a part of Reset()?

Also, not a part of this patch, but maybe it's necessary to reset outbound_sidecars_ as well here?


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/rpc_controller.cc@166
PS4, Line 166: !fd.is_valid()
nit: wrap into PREDICT_FALSE() ?


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/service_pool.h
File src/kudu/rpc/service_pool.h:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/service_pool.h@93
PS4, Line 93: std::vector<scoped_refptr<kudu::Thread>>
Is it intentional that it's a copy, not a reference?


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/transfer.h
File src/kudu/rpc/transfer.h:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/transfer.h@81
PS4, Line 81:   // with those extra bytes, they will be stored in 'extra_fds'
nit: add a stop/period


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/rpc/transfer.h@99
PS4, Line 99:   std::vector<FileDescriptor> TakeReceivedFds();
Add a doc string?


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/security/tls_socket.cc@102
PS4, Line 102: !fds.empty()
nit: wrap into PREDICT_FALSE() ?


http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/util/net/socket.h
File src/kudu/util/net/socket.h:

http://gerrit.cloudera.org:8080/#/c/15706/4/src/kudu/util/net/socket.h@150
PS4, Line 150: virtual Status Writev(const struct ::iovec *iov, int iov_len, int64_t *nwritten) final;
Is the combination of 'virtual' combined with 'final' is here for avoid shading or it's something else?  Maybe, add a comment to explain this?



-- 
To view, visit http://gerrit.cloudera.org:8080/15706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
Gerrit-Change-Number: 15706
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 17 Apr 2020 01:45:00 +0000
Gerrit-HasComments: Yes

[kudu-CR] rpc: add support for passing an open file descriptor across a unix socket

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Andrew Wong, Kudu Jenkins, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15706

to look at the new patch set (#5).

Change subject: rpc: add support for passing an open file descriptor across a unix socket
......................................................................

rpc: add support for passing an open file descriptor across a unix socket

This adds RpcController::SendFileDescriptor() which allows an RPC client
to pass an open file to the server. The server can retrieve the file
from the RpcContext.

The eventual goal here is that a client can create an in-memory file
descriptor (using either memfd_create on newer kernels or shm_open on
older ones) and pass it to a tserver in a Scan RPC. The server can then
write the scan results into the file instead of passing the data back
via the normal RPC sidecar mechanism. Assuming the client has mmapped
the file, this eliminates a bunch of copies and load on the reactor
threads.

This includes a benchmark that compares various methods for doing this
shared memory transport. Results from running the benchmark on various
systems show that the shared memory can be substantially (2-5x) faster
than the existing sidecar approach:

88-thread Xeon E5-2699 v4 @ 2.20GHz, el7:
  https://gist.github.com/toddlipcon/c691afcb72ddea7d1c6c1e54904e94a6

8-thread Core i7-7820HQ @ 2.9Ghz, Ubuntu 18:
  https://gist.github.com/toddlipcon/28193dd2d2017bba2aaebdea78bcee6a

AWS r5d.2xlarge 8vCPU (Xeon Platinum 8175M CPU @ 2.50GHz), Ubuntu 18:
  https://gist.github.com/toddlipcon/f84a09a92d24939d9d49dd7747d8aa9e

Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
---
M docs/design-docs/rpc.md
M src/kudu/rpc/connection.cc
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/service_pool.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
A src/kudu/util/fd.h
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
24 files changed, 955 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15706/5
-- 
To view, visit http://gerrit.cloudera.org:8080/15706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
Gerrit-Change-Number: 15706
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] rpc: add support for passing an open file descriptor across a unix socket

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Andrew Wong, Kudu Jenkins, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15706

to look at the new patch set (#6).

Change subject: rpc: add support for passing an open file descriptor across a unix socket
......................................................................

rpc: add support for passing an open file descriptor across a unix socket

This adds RpcController::SendFileDescriptor() which allows an RPC client
to pass an open file to the server. The server can retrieve the file
from the RpcContext.

The eventual goal here is that a client can create an in-memory file
descriptor (using either memfd_create on newer kernels or shm_open on
older ones) and pass it to a tserver in a Scan RPC. The server can then
write the scan results into the file instead of passing the data back
via the normal RPC sidecar mechanism. Assuming the client has mmapped
the file, this eliminates a bunch of copies and load on the reactor
threads.

This includes a benchmark that compares various methods for doing this
shared memory transport. Results from running the benchmark on various
systems show that the shared memory can be substantially (2-5x) faster
than the existing sidecar approach:

88-thread Xeon E5-2699 v4 @ 2.20GHz, el7:
  https://gist.github.com/toddlipcon/c691afcb72ddea7d1c6c1e54904e94a6

8-thread Core i7-7820HQ @ 2.9Ghz, Ubuntu 18:
  https://gist.github.com/toddlipcon/28193dd2d2017bba2aaebdea78bcee6a

AWS r5d.2xlarge 8vCPU (Xeon Platinum 8175M CPU @ 2.50GHz), Ubuntu 18:
  https://gist.github.com/toddlipcon/f84a09a92d24939d9d49dd7747d8aa9e

Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
---
M docs/design-docs/rpc.md
M src/kudu/rpc/connection.cc
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/service_pool.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
A src/kudu/util/fd.h
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
24 files changed, 956 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15706/6
-- 
To view, visit http://gerrit.cloudera.org:8080/15706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
Gerrit-Change-Number: 15706
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] rpc: add support for passing an open file descriptor across a unix socket

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Andrew Wong, Kudu Jenkins, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15706

to look at the new patch set (#3).

Change subject: rpc: add support for passing an open file descriptor across a unix socket
......................................................................

rpc: add support for passing an open file descriptor across a unix socket

This adds RpcController::SendFileDescriptor() which allows an RPC client
to pass an open file to the server. The server can retrieve the file
from the RpcContext.

The eventual goal here is that a client can create a memfd file (or an
unlinked file in /dev/shm if memfd isn't supported) and pass it to a
tserver in a Scan RPC. The server can then write the scan results into
the file instead of passing the data back via the normal RPC sidecar
mechanism. Assuming the client has mmapped the file, this should results
in the elimination of a bunch of copies and load on the reactor threads.

This includes a benchmark that compares various methods for doing this
shared memory transport. Results from running the benchmark on various
systems show that the shared memory can be substantially (2-5x) faster
than the existing sidecar approach:

88-thread Xeon E5-2699 v4 @ 2.20GHz, el7:
  https://gist.github.com/toddlipcon/c691afcb72ddea7d1c6c1e54904e94a6

8-thread Core i7-7820HQ @ 2.9Ghz, Ubuntu 18:
  https://gist.github.com/toddlipcon/28193dd2d2017bba2aaebdea78bcee6a

AWS r5d.2xlarge 8vCPU (Xeon Platinum 8175M CPU @ 2.50GHz), Ubuntu 18:
  https://gist.github.com/toddlipcon/f84a09a92d24939d9d49dd7747d8aa9e

Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
---
M src/kudu/gutil/linux_syscall_support.h
M src/kudu/rpc/connection.cc
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/service_pool.h
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
A src/kudu/util/fd.h
M src/kudu/util/net/socket.cc
M src/kudu/util/net/socket.h
23 files changed, 889 insertions(+), 31 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/15706/3
-- 
To view, visit http://gerrit.cloudera.org:8080/15706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89baff83fa85f9109429fdc29d6eefc9f0f8b4a2
Gerrit-Change-Number: 15706
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)