Skip to content
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

feat: add simple RDMA communication support #2

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

moui0
Copy link

@moui0 moui0 commented Nov 28, 2022

Motivation

Currently tonic only supports HTTP, so I tried to add optional RDMA communication feature.

Using RDMA(if machine supports) can improve communication efficiency.

Solution

RDMA communication based on async-rdma crate. RDMA establishes a connection via TCP, then communicates using SEND/RECV operations.

Server handles both HTTP and RDMA requests through serve_with_rdma. This function listens to two TCP ports, which are used for HTTP and RDMA connection establishment respectively.

Client has two options to communicate with the server: either HTTP via Channel(connect) or RDMA via RdmaChannel(connect_rdma).

Copy link

@rogercloud rogercloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, can we merge the rdma code into the original logic, especially for the code generation part? There are too many duplicate code. And we can add an rdma feature flag to support the rdma statically.

@@ -146,6 +168,27 @@ fn generate_connect(service_ident: &syn::Ident, enabled: bool) -> TokenStream {
}
}

#[cfg(feature = "transport")]
fn generate_connect_rdma(service_ident: &syn::Ident, enable: bool) -> TokenStream {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http version ignores the enable parameter, why do we care about it?

);

// if !disable_comments.contains(&format!(
// "{}{}{}.{}",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the commented code.

@@ -197,6 +240,139 @@ fn generate_methods<T: Service>(
stream
}

fn generate_methods_rdma<T: Service>(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try not to duplicate the code if many are the same or similar.

// body
source
.for_each(|item| {
len += encoder.encode_into_slice(item, &mut buf).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to make sure the len is within mr's len?

If you ensure that by MAX_MSG_LEN, you can add a comment to explain why is it that number.

pub trait RdmaService {
///
fn alloc_mr(&self) -> io::Result<LocalMr>;
///
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forget to add comments?


// TODO: handle this error
async fn rdma_serve_inner(rdma: Arc<Rdma>, mut routes: RdmaRoutes) -> Result<(), io::Error> {
println!("[server] connected!");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug! might be better?

@@ -0,0 +1,202 @@
use std::collections::HashMap;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add this example to github action as a test and show the usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants