[RFC] Separated sys crate out and applied changes from nevenoomo #8

rdelfin · 2021-04-09T21:38:46Z

I've been using ibverbs for the last few months for an application I went shopping around for Rust wrappers. I found this one, but I found the lack of examples made it difficult to use and fully understand. From there I found @nevenoomo's fork together with his netCAT application, which helped show a lot more clear examples of how to make use of this. I then forked off of that, separated out the direct bindings into a separate crate (which made it a whole lot easier to import this into a Bazel project), switched the build system to the cmake crate (which as an added bonus makes this work when cross-compiling) and have been working off of that branch since.

I also had to add a few minor changes to rdma-core since it has... Compilation errors if the path you build from is too nested (which seems to be an issue with the cmake crate).

This PR is by no means ready to merge, but I wanted to get some feedback on whether this change would even be palatable. If so, I can work on cleaning up further. Let me know what you think!

jonhoo · 2021-04-10T02:53:23Z

ibverbs/src/lib.rs

+        let ok = if !self.ctx.is_null() {
+            unsafe { ffi::ibv_close_device(self.ctx) }
+        } else {
+            0
+        };
        assert_eq!(ok, 0);


Suggested change

let ok = if !self.ctx.is_null() {

unsafe { ffi::ibv_close_device(self.ctx) }

} else {

0

};

assert_eq!(ok, 0);

if !self.ctx.is_null() {

let ok = unsafe { ffi::ibv_close_device(self.ctx) };

assert_eq!(ok, 0);

}

jonhoo · 2021-04-10T02:53:52Z

ibverbs/src/lib.rs

    }
 }

 impl Drop for Context {
    fn drop(&mut self) {
-        let ok = unsafe { ffi::ibv_close_device(self.ctx) };
+        let ok = if !self.ctx.is_null() {


How can ctx even be NULL here? Wouldn't that mean the context failed to be constructed in the first place?

This one I'm not 100% sure on. Let me see if I can find the original commit and whether that explains it

jonhoo · 2021-04-10T02:54:44Z

ibverbs/src/lib.rs

-pub struct CompletionQueue<'ctx> {
-    _phantom: PhantomData<&'ctx ()>,
+pub struct CompletionQueue {
+    // _phantom: PhantomData<&'ctx ()>,


I was under the impression that a CQ must outlive the context, which is what the lifetime bound used to enforce — is that not the case?

Let me double check this one too. The changes on this file are mostly from nevenmoo, so this might just have been them getting around lifetime requirements for ease of use.

Should we tag them in to this PR? They may be able to provide some additional input.

Oh! I did, it's @nevenoomo. With another tag they might see it :P

jonhoo · 2021-04-10T02:56:26Z

ibverbs/src/lib.rs

+        let errno = if !self.cq.is_null() {
+            unsafe { ffi::ibv_destroy_cq(self.cq) }
+        } else {
+            0
+        };


Suggested change

let errno = if !self.cq.is_null() {

unsafe { ffi::ibv_destroy_cq(self.cq) }

} else {

0

};

if self.cq.is_null() {

return;

}

let errno = unsafe { ffi::ibv_destroy_cq(self.cq) };

jonhoo · 2021-04-10T02:56:48Z

ibverbs/src/lib.rs

    fn drop(&mut self) {
-        let errno = unsafe { ffi::ibv_destroy_cq(self.cq) };
+        let errno = if !self.cq.is_null() {


Here, too, how can self.cq be NULL — that should make the constructor fail.

jonhoo · 2021-04-10T02:57:46Z

ibverbs/src/lib.rs

@@ -769,14 +804,57 @@ pub struct PreparedQueuePair<'res> {
 /// An identifier for the network endpoint of a `QueuePair`.
 ///
 /// Internally, this contains the `QueuePair`'s `qp_num`, as well as the context's `lid` and `gid`.
-#[derive(Copy, Clone)]
+#[derive(Clone)]


Why remove Copy from this type?

jonhoo · 2021-04-10T02:58:25Z

ibverbs/src/lib.rs

+/// A wrapper around `QueuePairEndpoint` with addintional `rkey` and `raddr` field. Intended to be sent to the peer.
+// TODO: write more thoughout documentation for fields.
+#[derive(Serialize, Deserialize, Clone)]
+pub struct EndpointMsg {


We can probably make this nicer with the changes that landed in #7.

Oh! That's some good timing

jonhoo · 2021-04-10T02:59:39Z

ibverbs/src/lib.rs

+        let errno = if !self.mr.is_null() {
+            unsafe { ffi::ibv_dereg_mr(self.mr) }
+        } else {
+            0
+        };


Suggested change

let errno = if !self.mr.is_null() {

unsafe { ffi::ibv_dereg_mr(self.mr) }

} else {

0

};

if self.mr.is_null() {

return;

}

let errno = unsafe { ffi::ibv_dereg_mr(self.mr) };

jonhoo · 2021-04-10T02:59:54Z

ibverbs/src/lib.rs


 impl<T> Drop for MemoryRegion<T> {
    fn drop(&mut self) {
-        let errno = unsafe { ffi::ibv_dereg_mr(self.mr) };
+        let errno = if !self.mr.is_null() {


And here as well, it feels like NULL should be handled by the constructor.

jonhoo · 2021-04-10T03:00:09Z

ibverbs/src/lib.rs

@@ -1019,7 +1160,7 @@ impl<'ctx> ProtectionDomain<'ctx> {
            ffi::ibv_reg_mr(
                self.pd,
                data.as_mut_ptr() as *mut _,
-                n * mem::size_of::<T>(),
+                (n * mem::size_of::<T>()) as u64,


This shouldn't be necessary on master.

jonhoo · 2021-04-10T03:00:37Z

ibverbs/src/lib.rs

+        let errno = if !self.pd.is_null() {
+            unsafe { ffi::ibv_dealloc_pd(self.pd) }
+        } else {
+            0
+        };


Suggested change

let errno = if !self.pd.is_null() {

unsafe { ffi::ibv_dealloc_pd(self.pd) }

} else {

0

};

if self.pd.is_null() {

return;

}

let errno = unsafe { ffi::ibv_dealloc_pd(self.pd) };

jonhoo · 2021-04-10T03:00:44Z

ibverbs/src/lib.rs

    fn drop(&mut self) {
-        let errno = unsafe { ffi::ibv_dealloc_pd(self.pd) };
+        let errno = if !self.pd.is_null() {


And same re NULL

jonhoo · 2021-04-10T03:01:17Z

ibverbs/src/lib.rs

    qp: *mut ffi::ibv_qp,
 }

-unsafe impl<'a> Send for QueuePair<'a> {}
-unsafe impl<'a> Sync for QueuePair<'a> {}
+impl Default for QueuePair {


What's the motivation for implementing Default for QueuePair?

jonhoo · 2021-04-10T03:02:16Z

ibverbs/src/lib.rs

@@ -1226,15 +1384,173 @@ impl<'res> QueuePair<'res> {
            Ok(())
        }
    }
+
+    /// Write N values from the specified MR to the remote memory.


This could use some more documentation. Especially with regards to why it is unsafe.

jonhoo · 2021-04-10T03:02:26Z

ibverbs/src/lib.rs

+        }
+    }
+
+    /// Write a single value from the top of the memory region


This could use some more documentation. Especially with regards to why it is unsafe.

jonhoo · 2021-04-10T03:02:37Z

ibverbs/src/lib.rs

+        }
+    }
+
+    /// Write a single value from the top of the memory region


This could use some more documentation. Especially with regards to why it is unsafe.

jonhoo · 2021-04-10T03:02:42Z

ibverbs/src/lib.rs

+        self.post_write_buf(mr, 1, addr, key, wr_id, sig)
+    }
+
+    /// Write a single value from the top of the memory region


This could use some more documentation. Especially with regards to why it is unsafe.

jonhoo · 2021-04-10T03:03:28Z

ibverbs/src/lib.rs

+
+    /// Write a single value from the top of the memory region
+    #[inline]
+    pub unsafe fn post_write_single<T>(


What's the reason for having this helper method, given that it's longer to type out than just calling the non-single variant with an extra argument of 1?

jonhoo · 2021-04-10T03:03:32Z

ibverbs/src/lib.rs

+        wr_id: u64,
+        sig: bool,
+    ) -> io::Result<()> {
+        self.post_read_buf(mr, 1, addr, key, wr_id, sig)


What's the reason for having this helper method, given that it's longer to type out than just calling the non-single variant with an extra argument of 1?

jonhoo · 2021-04-10T03:03:56Z

ibverbs/src/lib.rs

+        let errno = if !self.qp.is_null() {
+            unsafe { ffi::ibv_destroy_qp(self.qp) }
+        } else {
+            0
+        };


Suggested change

let errno = if !self.qp.is_null() {

unsafe { ffi::ibv_destroy_qp(self.qp) }

} else {

0

};

if self.qp.is_null() {

return;

}

let errno = unsafe { ffi::ibv_destroy_qp(self.qp) };

jonhoo · 2021-04-10T03:04:36Z

ibverbs/src/lib.rs

    fn drop(&mut self) {
        // TODO: ibv_destroy_qp() fails if the QP is attached to a multicast group.
-        let errno = unsafe { ffi::ibv_destroy_qp(self.qp) };
+        let errno = if !self.qp.is_null() {


Here I think the value can be NULL if we want QueuePair to implement Default, but I'd still like to know what the use-case for that is. If there isn't one, we should instead enforce in the constructor that QP is never NULL.

jonhoo · 2021-04-10T03:05:28Z

ibverbs/README.md

@@ -0,0 +1,67 @@
+# ibverbs


We probably still want a top-level README as well. Not sure how other projects go about this? Maybe symlink one to the other?

Let me look around how projects with workspaces handle this. IIRC they usually just have identical readme's on the root and main crate, but a symlink might be nicer.

jonhoo · 2021-04-10T03:06:06Z

ibverbs-sys/Cargo.toml

@@ -0,0 +1,21 @@
+[package]


ibverbs-sys should also get a short README that points to ibverbs.

rdelfin · 2021-04-10T18:57:32Z

By the looks of it, the biggest single thing to send out in a separate PR is the sys vs regular crate change. I'll send out a separate PR with just that first and leave this one open as a reference mostly

jonhoo · 2021-04-10T23:22:37Z

That sounds like a great plan — I think that is indeed a very neatly separable change.

rdelfin · 2021-11-04T22:23:41Z

I'm really sorry for taking so long on this. I've been a bit swamped with work (ironically using this library!) and forgot to get back to this.
@jonhoo am I reading correctly that a lot of the changes I put in here have already made it to master, but with correct lifetimes instead of arcs everywhere?

jonhoo · 2021-11-06T19:53:01Z

No worries at all — open-source is basically the act of trying to organize lots of people's volunteer time 😅

Am I reading correctly that a lot of the changes I put in here have already made it to master, but with correct lifetimes instead of arcs everywhere?

Hard for me to say. Some definitely have, but I don't know if all of them have. You'd have to walk through the diff and see unfortunately.

rdelfin · 2021-11-07T14:19:23Z

No worries, let me go through it and see what is missing. I did separate out the split creates too!

Neve Noomo and others added 30 commits February 22, 2020 17:36

Add device querying

1d4c147

Extend endpoint for DMA usage

7ce5c17

Add a message for passing to peer

bc8d2e1

Resolve pub issues

351246f

Lifetime changes

5391007

Changes to allocate context on heap

ad42de0

Example update

e72f63d

Make Context more like singleton

fb520dd

Update Drops

e34a37d

Add drop for PreparedQueuePair

105457c

Remove unnecessary Arc

5a08640

Fix drop for PreparedQueuePair

16dc772

Verbose panic

17f4447

Add DMA functions

b104f57

More verbose errors

253e470

Writing and reading memory regions from the memory

2b0ad92

Bugfix

c5b718a

Separated FFI and main module into sys and non-sys crate

ef4378f

Cleaned up warnings in lib.rs

51e7d5d

Properly got rid of FFI warnings

84f4046

Upgraded bindgen version

1e1d60c

Over-specify pointer cast type

46300ca

Respecified type

698039b

Removed vendored bindgen

f625d06

Reverted to not specifying type

7c6fa2b

Merge branch 'bazel-fixes'

aedd496

Updated rdmacore commit to v34

50535fd

Fixed casting issue in ibv_query_port

72622da

Updated rdma-core dependency to 34.0

9bbd9ac

Switched to using the cmake crate

06a7e6c

jonhoo reviewed Apr 10, 2021

View reviewed changes

jonhoo closed this in 6332a3a Jan 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Separated sys crate out and applied changes from nevenoomo #8

[RFC] Separated sys crate out and applied changes from nevenoomo #8

rdelfin commented Apr 9, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

rdelfin Apr 10, 2021

jonhoo Apr 10, 2021 •

edited

Loading

rdelfin Apr 10, 2021

jonhoo Apr 10, 2021

rdelfin Apr 14, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

rdelfin Apr 10, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021 •

edited

Loading

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

jonhoo Apr 10, 2021

rdelfin Apr 10, 2021

jonhoo Apr 10, 2021

rdelfin commented Apr 10, 2021

jonhoo commented Apr 10, 2021

rdelfin commented Nov 4, 2021

jonhoo commented Nov 6, 2021

rdelfin commented Nov 7, 2021

[RFC] Separated sys crate out and applied changes from nevenoomo #8

[RFC] Separated sys crate out and applied changes from nevenoomo #8

Conversation

rdelfin commented Apr 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonhoo Apr 10, 2021 • edited Loading

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

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

jonhoo Apr 10, 2021 • edited Loading

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

rdelfin commented Apr 10, 2021

jonhoo commented Apr 10, 2021

rdelfin commented Nov 4, 2021

jonhoo commented Nov 6, 2021

rdelfin commented Nov 7, 2021

jonhoo Apr 10, 2021 •

edited

Loading

jonhoo Apr 10, 2021 •

edited

Loading