Ibverbs-based RDMA path by junshi15 · Pull Request #8943 · tensorflow/tensorflow

junshi15 · 2017-04-04T01:18:40Z

This patch introduces an ibverbs-based RDMA path between servers for tensor transfer (weights, gradients, etc). The existing gRPC path remains and is responsible for "administrative" tasks, such as setting up the Rdma path, exchanging computation graphs, etc. Design details can be found in the README file below:
https://github.com/yahoo/tensorflow/blob/verbs_rdma/tensorflow/contrib/verbs/README.md

tensorflow-jenkins · 2017-04-04T01:18:41Z

Can one of the admins verify this patch?

poxvoculi · 2017-04-05T18:13:19Z

Via another channel I am in communication wtih @junshi15 about this PR. It needs to be updated slightly to the most recent version of the main repository, then he will submit again.

poxvoculi

Just starting to enter a few comments one by one, because this tool will lose all unsubmitted comments on various minor browser events.

poxvoculi · 2017-04-05T18:54:01Z

+  // analyzing request
+  // the channel setting part is redundant.
+  string remote_host_name = request->host_name();
+  RdmaChannel* rc = rdma_mgr_->FindChannel(remote_host_name);


poxvoculi · 2017-04-05T18:57:06Z

+  int i = 0;
+  int idx[] = {1, 0, 3, 2};
+  std::vector<RdmaBuffer*> mb(rc->message_buffers());
+  for (const auto& mr : request->mr()) {


CHECK whether request->mr() has more than size 4?

poxvoculi · 2017-04-05T19:22:34Z

+    }
+
+    // Initiate recv
+    for (int i = 0; i < 100; i++) {


I suppose eventually this should be some kind of config option.

yes, in the future revision.

poxvoculi · 2017-04-05T19:25:06Z

+      rb = new RdmaMessageBuffer(this, name);
+    } else if (buffer_type == ACK) {
+      rb = new RdmaAckBuffer(this, name); 
+    }


CHECK(!rb) ?

added CHECK(rb)

poxvoculi · 2017-04-05T19:28:38Z

+//   None
+void RdmaChannel::InsertRecvCallback(string& key, 
+        std::function<void()> recv_done) {
+  ct_mu_.lock();


If you include tensorflow/core/platform/mutex.h you can use the scoped mutex_lock instead of explicit unlocks like here. It's a little less error prone in practice.

changed most places to scoped mutex_lock.

poxvoculi · 2017-04-05T19:53:07Z

+            << "send dev name: " << src_dev->name()
+            << " gpu_info: " << src_dev->tensorflow_gpu_device_info();
+        // "val" is on a GPU. Uses GPUUtil to fill the proto.
+        s = VerbsUtil::SetProtoFromGPUSync(in, src_dev, 


Can this stall the CQ processing thread?

In theory, it can. will optimize in a future revision.

poxvoculi · 2017-04-05T20:35:49Z

+        return;
+      }
+      AllocatorAttributes src_alloc_attr;
+      src_alloc_attr.set_on_host(true);             


I don't see where this is used.

poxvoculi · 2017-04-05T20:40:24Z

+class VerbsServerFactory : public ServerFactory {
+ public:
+  bool AcceptsOptions(const ServerDef& server_def) override {
+    return server_def.protocol() == "verbs";


Maybe use "grpc+verbs" as the protocol, since this is a hybrid.

changed to "grpc+verbs"

poxvoculi · 2017-04-05T20:59:10Z

+                        GetRemoteAddressResponse* response) {
+  // analyzing request
+  // the channel setting part is redundant.
+  string remote_host_name = request->host_name();


const string

poxvoculi · 2017-04-05T21:05:14Z

+    srcs = ["verbs_service.proto"],
+    cc_api_version = 2,
+    visibility = [
+        "//tensorflow:internal",


If it works, prefer tensorflow:subpackages

changed to tensorflow:subpackages

junshi15 · 2017-04-07T16:20:24Z

rebased to master branch, conflicts resolved.

poxvoculi · 2017-04-07T17:42:20Z

Jenkins, test this please.

jhseu · 2017-04-07T18:05:33Z

Jenkins, test this please

poxvoculi · 2017-04-07T18:16:39Z

@junshi15 : the buildifier sanity check is failing, and that appears to be blocking all further testing.

It looks like you can run
run buildifier tensorflow/contrib/verbs/BUILD
or fix the errors by hand. It looks like some build rule deps are not ordered properly.

poxvoculi · 2017-04-07T18:46:21Z

  TF_CHECK_OK(Stop());
  TF_CHECK_OK(Join());
-
+  


delete empty line

poxvoculi · 2017-04-07T18:47:47Z

  worker_service_ =
      NewGrpcWorkerService(worker_impl_.get(), &builder).release();
+  // extra service:
+  if (service_func != NULL)


use { } whenever 'if' construct spans more than one line.

poxvoculi · 2017-04-07T18:49:27Z

  std::unique_ptr<GrpcServer> ret(
      new GrpcServer(server_def, env == nullptr ? Env::Default() : env));
-  TF_RETURN_IF_ERROR(ret->Init());
+  ServiceCreationFunction service_func = NULL;


s/NULL/nullptr

junshi15 · 2017-04-07T18:52:43Z

working on tensorflow/contrib/verbs/BUILD, buildifier (https://github.com/bazelbuild/buildtools) does not build for me, so I will edit it manually.

poxvoculi · 2017-04-07T18:56:18Z

+          const std::string& worker_name, WorkerCacheInterface* worker_cache)> 
+        RendezvousMgrCreationFunction;
+
+typedef std::function<void(const WorkerEnv*, ::grpc::ServerBuilder*)> 


Could you document what this is supposed to do? I think it's an initialization for a derived class. So maybe call it ServiceInitFunction?

"ServiceCreationFunction" registers a service on the server. This is one way to register a service by the derived class. VerbsServer::Init() calls GrpcServer::Init(). But the server is already built and run after GrpcServer::Init(), so I have to pass a function to GrpcServer::Init() to register an extra service.

I am ok with your naming.

junshi15 · 2017-04-07T20:11:56Z

Jenkins, test this please.

poxvoculi · 2017-04-07T20:13:40Z

Jenkins, test this please.

Add missing comma in verbs/BUILD file

poxvoculi · 2017-04-07T20:19:26Z

Jenkins, test this please.

junshi15 · 2017-04-07T20:35:01Z

What's the problem with two BUILD files that failed sanity checks? indentation?

poxvoculi · 2017-04-07T20:41:22Z

Not sure, trying to figure that out.

…

On Fri, Apr 7, 2017 at 1:36 PM, Jun Shi ***@***.***> wrote: What's the problem with two BUILD files that failed sanity checks? indentation? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#8943 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AO818e4BFtUkrUAwSSpqvX03yORDYEPLks5rtp47gaJpZM4MyR40> .

junshi15 · 2017-04-09T16:29:28Z

Do we have an example where link option can be included/excluded with a flag?
"#define" only takes care of header files. I need to disable a link option below:
https://github.com/yahoo/tensorflow/blob/5776b4ffc851cbcb742952c6721739f45f0dbf6f/tensorflow/contrib/verbs/BUILD#L138-L140

junshi15 · 2017-04-09T16:43:53Z

I found one here, will give it a try.
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/BUILD#L1297-L1300

junshi15 · 2017-04-09T23:42:17Z

Added a switch to turn on/off link option "-libverbs". Please initiate a build. Thank you.

poxvoculi · 2017-04-10T03:08:29Z

Jenkins test this please

tensorflow-jenkins · 2017-04-10T15:08:36Z

Can one of the admins verify this patch?

junshi15 · 2017-04-10T19:23:57Z

Added "#ifdef TENSORFLOW_USE_VERBS" to two more files. Please initiate the build. Thanks.

jhseu · 2017-04-10T19:33:55Z

Jenkins, test this please

drpngx · 2017-04-15T02:57:06Z

@poxvoculi are we good with this then?

poxvoculi · 2017-04-19T19:41:07Z

Jenkins, test this please.

poxvoculi

ok, thanks

wydwww · 2017-04-20T04:03:38Z

Typos in README.md:
fro in line 17
transimssion in line 39

drpngx · 2017-04-20T16:17:34Z

Right. @wydwww could you please send a follow-up PR?

junshi15 · 2017-04-20T21:28:45Z

@wydwww Thanks for catching the typos.

fanlu · 2017-05-18T02:16:20Z

#9926
When I training inception model, I got the following error
2017-05-18 09:50:50.534662: I tensorflow/contrib/verbs/rdma_mgr.cc:56] connecting to remote node /job:worker/replica:0/task:2
2017-05-18 09:51:02.357013: I tensorflow/contrib/verbs/rdma.cc:518] channel already connected
2017-05-18 09:51:02.357102: I tensorflow/contrib/verbs/rdma_mgr.cc:56] connecting to remote node /job:worker/replica:0/task:0
2017-05-18 09:51:02.358775: I tensorflow/contrib/verbs/rdma_mgr.cc:56] connecting to remote node /job:ps/replica:0/task:0
2017-05-18 09:51:07.867035: I tensorflow/contrib/verbs/rdma.cc:518] channel already connected
2017-05-18 09:51:08.095058: I tensorflow/contrib/verbs/rdma.cc:518] channel already connected
INFO:tensorflow:SyncReplicasV2: replicas_to_aggregate=3; total_num_replicas=3
INFO:tensorflow:2017-05-18 09:52:16.455855 Supervisor
2017-05-18 09:52:19.322077: I tensorflow/core/distributed_runtime/master_session.cc:999] Start master session 126a016806cf6873 with config:
allow_soft_placement: true

2017-05-18 09:52:27.325014: F tensorflow/contrib/verbs/rdma.cc:129] Check failed: wc_[i].status == IBV_WC_SUCCESS Failed status
transport retry counter exceeded 12 40500432 129
Aborted (core dumped)
this is my command line
CUDA_VISIBLE_DEVICES=1 bazel-bin/inception/imagenet_distributed_train \

--batch_size=32
--data_dir=/data0/imagenet-data
--job_name='worker'
--task_id=1
--ps_hosts='localhost:2222'
--worker_hosts='localhost:2223,localhost:2224,localhost:2225' --protocol='grpc+verbs'

@junshi15 @llhe Can you help me to solve this problem?

junshi15 · 2017-05-22T20:32:01Z

@fanlu Please use this link #9926

suiyuan2009 · 2017-08-01T05:09:31Z

verbs build failed on current master branch, last commit.

ERROR: /home/dongziming/tensorflow/tensorflow/contrib/verbs/BUILD:158:1: C++ compilation of rule '//tensorflow/contrib/verbs:verbs_server_lib' failed: crosstool_wrapper_driver_is_not_gcc failed: error executing command external/local_config_cuda/crosstool/clang/bin/crosstool_wrapper_driver_is_not_gcc -U_FORTIFY_SOURCE '-D_FORTIFY_SOURCE=1' -fstack-protector -fPIE -Wall -Wunused-but-set-parameter ... (remaining 164 argument(s) skipped): com.google.devtools.build.lib.shell.BadExitStatusException: Process exited with status 1.
tensorflow/contrib/verbs/verbs_server_lib.cc: In constructor 'tensorflow::{anonymous}::VerbsServerRegistrar::VerbsServerRegistrar()':
tensorflow/contrib/verbs/verbs_server_lib.cc:153:5: error: 'gpr_allocation_functions' was not declared in this scope
     gpr_allocation_functions alloc_fns;
     ^
tensorflow/contrib/verbs/verbs_server_lib.cc:154:5: error: 'alloc_fns' was not declared in this scope
     alloc_fns.malloc_fn = port::Malloc;
     ^
tensorflow/contrib/verbs/verbs_server_lib.cc:157:43: error: 'gpr_set_allocation_functions' was not declared in this scope
     gpr_set_allocation_functions(alloc_fns);
                                           ^

vdevaram · 2017-10-11T06:09:44Z

@junshi15 : we are not able to use NIC bonding feature with this PR. is this PR supports NIC bonding? If not is it possible to add NIC bonding feature with verbs. May I know how to handle in code? do you have any plans to add it in near future?

junshi15 · 2017-10-11T06:23:37Z

@vdevaram I do not have experience with NIC bonding and do not have a plan to add this feature in the future. Your contributions are welcome.

googlebot added the cla: yes label Apr 4, 2017

junshi15 mentioned this pull request Apr 4, 2017

Running failed YahooArchive/tensorflow#5

Closed

rohan100jain requested a review from jhseu April 5, 2017 14:55

jhseu requested review from poxvoculi and removed request for jhseu April 5, 2017 18:10

poxvoculi self-assigned this Apr 5, 2017

poxvoculi reviewed Apr 5, 2017

View reviewed changes

initial check-in

a56d7a4

junshi15 force-pushed the verbs_rdma branch 2 times, most recently from af23ab6 to 0a6c80b Compare April 7, 2017 08:02

modifications according to review feedback

830ec09

junshi15 force-pushed the verbs_rdma branch from 0a6c80b to 830ec09 Compare April 7, 2017 08:17

poxvoculi reviewed Apr 7, 2017

View reviewed changes

fix BUILD file

d02d514

Update BUILD

bb48fcb

Add missing comma in verbs/BUILD file

link option fix

c14056e

poxvoculi closed this Apr 10, 2017

poxvoculi reopened this Apr 10, 2017

fix TENSORFLOW_USE_VERBS

b5581b2

poxvoculi approved these changes Apr 19, 2017

View reviewed changes

drpngx merged commit dd40e98 into tensorflow:master Apr 19, 2017

junshi15 deleted the verbs_rdma branch April 20, 2017 16:22

anfeng mentioned this pull request Apr 20, 2017

[Feature] RDMA support for distribued Tensorflow #2916

Closed

wydwww mentioned this pull request Apr 21, 2017

Fix typos #9353

Merged

drpngx mentioned this pull request Apr 24, 2017

Faild to build a pip package for TensorFlow with GPU #9417

Closed

byronyi mentioned this pull request Aug 2, 2017

GPUDirect RDMA Out-of-Band Tensor Transport #11392

Merged

byronyi mentioned this pull request Jul 5, 2019

Add alternative protocols supported by TF and SIG-Networking tensorflow/estimator#36

Closed

zhucan mentioned this pull request Jan 20, 2022

Is there plan to support rdma? grpc/grpc-rust#896

Closed

Conversation

junshi15 commented Apr 4, 2017

Uh oh!

tensorflow-jenkins commented Apr 4, 2017

Uh oh!

poxvoculi commented Apr 5, 2017

Uh oh!

poxvoculi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junshi15 commented Apr 7, 2017

Uh oh!

poxvoculi commented Apr 7, 2017

Uh oh!

jhseu commented Apr 7, 2017

Uh oh!

poxvoculi commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junshi15 commented Apr 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junshi15 commented Apr 7, 2017

Uh oh!

poxvoculi commented Apr 7, 2017

Uh oh!

poxvoculi commented Apr 7, 2017

Uh oh!

junshi15 commented Apr 7, 2017

Uh oh!

poxvoculi commented Apr 7, 2017 via email

poxvoculi commented Apr 7, 2017 •

edited

Loading

fanlu commented May 18, 2017 •

edited

Loading