Add IBUC network adaptor support - #265
Conversation
Summary of ChangesHello @chencjcj, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new InfiniBand Unreliable Connection (IBUC) network adaptor, enhancing the system's networking capabilities. It involves significant refactoring of existing InfiniBand-related code into a shared common header, streamlining the codebase and enabling easier integration of future InfiniBand adaptors. The changes also update the build system to support the new adaptor and modify the network selection logic to incorporate IBUC. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new network adaptor for InfiniBand Unreliable Connected (IBUC) transport. A significant part of this work involves refactoring common InfiniBand code from the existing IBRC adaptor into a shared header ib_common.h, which is a good improvement for maintainability.
However, the new IBUC adaptor implementation has a critical design flaw. It attempts to use RDMA operations (like RDMA_WRITE) with Unreliable Connected (UC) queue pairs. The InfiniBand UC transport type does not support RDMA; it is a message-based transport that relies on send/recv semantics. This fundamental misunderstanding makes the current IBUC implementation non-functional. The communication protocol for this adaptor needs to be redesigned to correctly use ibv_post_send and ibv_post_recv for data transfer.
I've also found a smaller issue with unreachable code in the ibuc_adaptor.cc file. Please see the detailed comments for specifics.
|
|
||
| FLAGCX_PARAM(IbucSplitDataOnQps, "IBUC_SPLIT_DATA_ON_QPS", 0); | ||
|
|
||
| flagcxResult_t flagcxIbucMultiSend(struct flagcxIbSendComm *comm, int slot) { |
There was a problem hiding this comment.
The implementation of flagcxIbucMultiSend incorrectly uses RDMA opcodes (IBV_WR_RDMA_WRITE and IBV_WR_RDMA_WRITE_WITH_IMM). The Unreliable Connected (UC) transport type, which this adaptor is intended for, does not support RDMA operations. Data transfer over UC queue pairs must use a message-passing model with IBV_WR_SEND.
This implementation appears to be a copy of the Reliable Connected (RC) logic and will fail at runtime. The entire data transfer mechanism for the IBUC adaptor needs to be redesigned to use a send/recv paradigm instead of RDMA.
| wr.sg_list = &comm->devs[ctsQp->devIndex].fifoSge; | ||
| wr.num_sge = 1; | ||
|
|
||
| wr.opcode = IBV_WR_RDMA_WRITE; // UC mode supports RDMA_WRITE |
There was a problem hiding this comment.
The comment on this line is incorrect. Unreliable Connected (UC) mode does not support IBV_WR_RDMA_WRITE. This operation is only valid for Reliable Connected (RC) and Reliable Datagram (RD) queue pairs. Using this opcode with a UC QP will result in a runtime error.
This indicates a fundamental misunderstanding of the UC transport's capabilities and invalidates the current FIFO notification mechanism, which relies on RDMA.
test-ibuc.txt