enable brpc use rdma #1836

Tuvie · 2022-07-13T11:19:11Z

Trying to merge rdma implementation into master.
This implementation is little different from previous rdma branch.
I remove some proved useless code and try to improve the handshake process.

wwbmmm · 2022-08-02T06:51:16Z

CMakeLists.txt

+set(WITH_RDMA_VAL "0")
+if(WITH_RDMA)
+    set(WITH_RDMA_VAL "1")
+    set(BRPC_WITH_RDMA 1)


这个有用到吗

应该是没用的，我删掉

wwbmmm · 2022-08-02T08:25:22Z

src/brpc/channel.cpp

@@ -149,6 +169,16 @@ int Channel::InitChannelOptions(const ChannelOptions* options) {
        LOG(ERROR) << "Channel does not support the protocol";
        return -1;
    }
+
+#if BRPC_WITH_RDMA


没有BRPC_WITH_RDMA宏的时候，如果用户设置options里的use_rdma，是不是应该报错

已调整为报错

wwbmmm · 2022-08-02T08:26:28Z

src/brpc/input_messenger.cpp

@@ -85,6 +86,15 @@ ParseResult InputMessenger::CutInputMessage(
                    << " bytes, the connection will be closed."
                    " Set max_body_size to allow bigger messages";
                return result;
+            } else {
+                if (m->_read_buf.size() >= 4) {
+                char data[4];


缩进有问题

wwbmmm · 2022-08-02T08:27:22Z

src/brpc/input_messenger.cpp

+                if (m->_read_buf.size() >= 4) {
+                char data[4];
+                m->_read_buf.copy_to_cstr(data, 4);
+                if (strncmp(data, "RDMA", 4) == 0 && m->_rdma_state == Socket::RDMA_OFF) {


这个 4 能否定义成常量

wwbmmm · 2022-08-02T09:03:24Z

src/brpc/socket.cpp

+        }
+        m->_rdma_state = RDMA_UNKNOWN;
+    } else {
+        delete m->_rdma_ep;


同厂内评论，确保这里一定是NULL

zyearn · 2022-08-03T08:48:55Z

@Tuvie 可以简单介绍一下代码结构和改动思路吗？文件有点多，有个整体的overview的话review起来会方便一些，谢谢。

wwbmmm · 2022-08-02T11:31:59Z

src/brpc/server.cpp

+        LOG(WARNING) << "SSL is not supported by RDMA";
+        return false;
+    }
+    if (opt->nshead_service) {


http_master_service是不是也不支持

可以支持，之前的版本也允许。但是事实上不建议这么做，因为http头本身也很重。所以这个版本给取消了

wwbmmm · 2022-08-02T11:42:05Z

src/brpc/rdma/rdma_endpoint.cpp

+static butil::Mutex* g_rdma_resource_mutex = NULL;
+static RdmaResource* g_rdma_resource_list = NULL;
+
+struct HelloMessage {


这个叫HelloMessage可能会让人误以为是测试代码

这个是握手协议，发送的第一个信息命名为Hello

wwbmmm · 2022-08-03T09:28:11Z

src/brpc/rdma/block_pool.cpp

+
+// This callback is used when extending a new region
+// Generally, it is a memory region register call
+typedef uint32_t (*Callback)(void*, size_t);


这个是不是定义在头文件中比较合适？
另外，这个命名不是很容易让人理解这个callback的作用

是。这个改了

wwbmmm · 2022-08-03T11:11:44Z

src/brpc/rdma/block_pool.cpp

+        node = g_info->idle_list[block_type][index];
+        if (!node) {
+            // There is no block left, extend a new region
+            if (!ExtendBlockPool(FLAGS_rdma_memory_pool_increase_size_mb,


在当前bucket上找不到，为什么不换个bucket再找找，而是直接Extend?

设立多个bucket还是希望尽量减少相互的竞争。考虑到本身各个bucket随机使用，相对比较平均，一个bucket没有空间了，则大概率其他bucket也快没有了，这时不如直接extend出来。从实际使用建议上看，不建议依赖中途extend的机制，这个会引发Lkey的动态查找。所以暂时也没有太多优化这里。

wwbmmm · 2022-08-03T11:14:08Z

src/brpc/rdma/block_pool.cpp

+    // Move more blocks from global list to tls list
+    if (block_type == 0) {
+        node = g_info->idle_list[0][index];
+        tls_idle_list = node;


iobuf内部就有一个tls的block缓存，这里的tls_idle_list有什么不一样吗？

这个block pool是为了做block分配前的thread local。iobuf里面的tls是分配出来以后的thread local。实际上iobuf里面不少操作并没有用到里面的tls cache。

wwbmmm · 2022-08-03T11:28:14Z

src/brpc/rdma/rdma_endpoint.cpp

+
+void HelloMessage::Serialize(void* data) {
+    // Note serialization does include magic str
+    memcpy(data, MAGIC_STR, 4);


deserialize是不含这个MAGIC_STR的，所以serialize也不包含是不是更合理？

tsyw1987 · 2022-08-09T05:14:04Z

后续是否支持同一个进程中启动多个brpc server，并分别绑定到不同的rdma网卡？

yanjianglu · 2022-08-11T08:42:17Z

src/brpc/rdma/rdma_endpoint.cpp

+    }
+}
+
+bool HelloNegotiationValid(HelloMessage msg) {


参数是否应该使用引用(const HelloMessage&) 避免拷贝？

这个函数为何单独放在这里，不再HelloMessage结构体呢？

yanjianglu · 2022-08-11T09:04:45Z

src/brpc/rdma/rdma_endpoint.h

+    int ReadFromFd(void* data, size_t len);
+
+
+    // Wrute at most len bytes from data to fd in _socket


Wrute 拼写错误

Tuvie · 2022-08-11T11:36:29Z

后续是否支持同一个进程中启动多个brpc server，并分别绑定到不同的rdma网卡？

这个可以在后续支持

yanjianglu · 2022-08-12T02:58:30Z

src/brpc/rdma/rdma_endpoint.cpp

+static RdmaResource* g_rdma_resource_list = NULL;
+
+struct HelloMessage {
+    void Serialize(void* data);


Serialize 是否可以设为常量成员函数呢？

Tuvie · 2022-08-16T05:41:57Z

@Tuvie 可以简单介绍一下代码结构和改动思路吗？文件有点多，有个整体的overview的话review起来会方便一些，谢谢。

已在docs中添加相应描述

wwbmmm · 2022-08-16T07:01:54Z

LGTM

zyearn · 2022-08-17T19:46:23Z

@Tuvie 可以简单介绍一下代码结构和改动思路吗？文件有点多，有个整体的overview的话review起来会方便一些，谢谢。

已在docs中添加相应描述

感谢。目前正在review，文件有点多，看完需要些时间。

zyearn · 2022-08-31T18:28:40Z

LGTM.

@Tuvie 从上次提交到现在有什么bugfix吗？没有的话等 @wwbmmm 看看线上运行的稳定程度，没什么问题的话可以考虑合入了。

zyearn · 2022-08-31T18:29:20Z

@Tuvie 有conflicts，需要merge master重新提交一下

Tuvie · 2022-09-03T02:55:27Z

LGTM.

@Tuvie 从上次提交到现在有什么bugfix吗？没有的话等 @wwbmmm 看看线上运行的稳定程度，没什么问题的话可以考虑合入了。

目前暂时没有，也可以先等等

Tuvie · 2022-09-05T01:33:41Z

@Tuvie 有conflicts，需要merge master重新提交一下

done

trevor211 · 2022-09-20T07:21:23Z

example/rdma_performance/CMakeLists.txt

+    ${OPENSSL_CRYPTO_LIBRARY}
+    ${OPENSSL_SSL_LIBRARY}
+    ${THRIFT_LIB}
+    ${THRIFTNB_LIB}


We should consider glog library. If brpc was compiled with WITH_GLOG set to ON, rdma_performace won't link successfully.

trevor211 · 2022-09-20T07:28:20Z

example/rdma_performance/client.cpp

+        brpc::rdma::GlobalRdmaInitializeOrDie();
+    }
+
+    brpc::StartDummyServerAt(8001);


Maybe we can make dummy port configurable by adding a gflag say dummy_port.

trevor211 · 2022-09-22T12:15:14Z

src/brpc/rdma/rdma_endpoint.cpp

+        return wc.byte_len;
+    }
+    default:
+        CHECK(false) << "This should not happen";


Suggested change

CHECK(false) << "This should not happen";

// Do not CHECK abort since wc.opcode could be IBV_WC_DRIVER2(136) which is

// a bug already fixed by rdma-core.

// FYI: https://github.com/linux-rdma/rdma-core/commit/4c905646de3e75bdccada4abe9f0d273d76eaf50

LOG(WARNING) << "This should not happen, wc.opcode = " << wc.opcode;

这个mlx驱动的bug会在什么情况下触发呢？如果确实是一个严重问题，仅仅抛出WARNING应该也是不行的。

Tuvie force-pushed the master branch 3 times, most recently from 4549318 to e377e07 Compare July 14, 2022 05:10

enable brpc use rdma

7d38c8f

Tuvie force-pushed the master branch 3 times, most recently from eed41a9 to 4e57fd6 Compare July 20, 2022 05:49

wwbmmm mentioned this pull request Jul 28, 2022

Feature request: Merge RDMA branch into master #1788

Closed

wwbmmm reviewed Aug 2, 2022

View reviewed changes

Tuvie force-pushed the master branch from 4e57fd6 to 4fc0393 Compare August 3, 2022 05:42

zyearn self-requested a review August 3, 2022 08:47

wwbmmm reviewed Aug 3, 2022

View reviewed changes

Tuvie force-pushed the master branch 2 times, most recently from 263ae9d to 6758bcb Compare August 6, 2022 03:49

This was referenced Aug 9, 2022

Rdma typo #1398

Closed

Fixed rdma multi device connect failed issue #1402

Closed

Fix rdma char buf overflow #1703

Closed

fix rdma bond mode issue #1276

Closed

yanjianglu reviewed Aug 11, 2022

View reviewed changes

Tuvie force-pushed the master branch from 6758bcb to 344c3bb Compare August 11, 2022 12:36

yanjianglu reviewed Aug 12, 2022

View reviewed changes

Tuvie force-pushed the master branch from 344c3bb to 5c68021 Compare August 15, 2022 03:45

Merge branch 'master' into master

568b1ac

Tuvie force-pushed the master branch from 5c68021 to 568b1ac Compare August 15, 2022 05:56

This was referenced Aug 16, 2022

rdma 有开源计划吗？ #1453

Closed

brpc rdma multithread polling mode in pthread #1395

Closed

How to run code in incubator-brpc/example/rdma_performance? #1387

Closed

This was referenced Aug 18, 2022

RDMA的代码为什么没有何如到master分支 #782

Closed

fix: stack buffer overflow #1367

Closed

wwbmmm mentioned this pull request Aug 26, 2022

RDMA分支版本可以在生产环境中用吗，和master分支版本在功能和性能上有什么差别 #1215

Closed

loongs-zhang mentioned this pull request Aug 27, 2022

Is there a plan to support RDMA ? alibaba/PhotonLibOS#59

Closed

zyearn approved these changes Sep 1, 2022

View reviewed changes

Tuvie closed this Sep 3, 2022

Tuvie reopened this Sep 3, 2022

Merge branch 'master' into master

546337c

Tuvie force-pushed the master branch from cbc0a50 to 546337c Compare September 4, 2022 10:55

serverglen added the feature new feature label Sep 14, 2022

trevor211 reviewed Sep 20, 2022

View reviewed changes

trevor211 reviewed Sep 22, 2022

View reviewed changes

Tuvie added 3 commits September 22, 2022 23:56

Merge branch 'master' into master

71af04c

Merge branch 'master' of https://github.com/Tuvie/brpc

7aeae8c

Merge branch 'apache:master' into master

866a9eb

Tuvie force-pushed the master branch 2 times, most recently from 4159b9c to 1499b6a Compare September 23, 2022 06:27

enable brpc use rdma

4f63a27

Tuvie force-pushed the master branch from 1499b6a to 4f63a27 Compare September 23, 2022 06:27

wwbmmm mentioned this pull request Oct 14, 2022

rdma分支存在内存泄漏问题 #1945

Closed

wwbmmm merged commit 791516c into apache:master Oct 25, 2022

		int ReadFromFd(void* data, size_t len);


		// Wrute at most len bytes from data to fd in _socket

-        CHECK(false) << "This should not happen";
+        // Do not CHECK abort since wc.opcode could be IBV_WC_DRIVER2(136) which is
+        // a bug already fixed by rdma-core.
+        // FYI: https://github.com/linux-rdma/rdma-core/commit/4c905646de3e75bdccada4abe9f0d273d76eaf50
+        LOG(WARNING) << "This should not happen, wc.opcode = " << wc.opcode;

enable brpc use rdma #1836

enable brpc use rdma #1836

Conversation

Tuvie commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyearn commented Aug 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsyw1987 commented Aug 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tuvie commented Aug 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tuvie commented Aug 16, 2022

wwbmmm commented Aug 16, 2022

zyearn commented Aug 17, 2022

zyearn commented Aug 31, 2022

zyearn commented Aug 31, 2022

Tuvie commented Sep 3, 2022

Tuvie commented Sep 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment