Support for RDMA Atomics + Support for user-defined initiator and responder resources on connect/accept #51

TaranovK · 2020-03-31T19:06:55Z

Hi,

The following changes add:

RDMA Atomics
RDMAConnParam which allows to set atomic resources for initiator and responder

For RDMA atomics I followed implementation of NatRdma and made similar implementation of NatAtomic. Depending on opcode, the writeBack of NatIbvSendWR will append Rdma or Atomic data.
I have also implemented a simple test in com.ibm.disni.examples.AtomicServer and com.ibm.disni.examples.AtomicClient.

For the second change I added extra functions in JNI to connect and accept using all fields of RdmaConnParam. Therefore, the JNI disni library supports both old and new way of establishing connections. In addition, I fixed other parts of the code to support custom RdmaConnParam instead of the default one with zeros for initiator and responder depths.

Cheers,
Konstantin

…onnect/accept.

PepperJo

Thanks for the pull request! It generally looks good to me, however I don't like introducing a new connect and accept call (see inline comments).

PepperJo · 2020-04-02T06:49:47Z

libdisni/src/verbs/com_ibm_disni_verbs_impl_NativeDispatcher.cpp

+ * Method:    _connectV2
+ * Signature: (JJ)V
+ */
+JNIEXPORT void JNICALL Java_com_ibm_disni_verbs_impl_NativeDispatcher__1connectV2(


I don't like the idea of multiple connect implementations. Why not only keep V2 and expose query_device to Java. This solves multiple problems at once: 1) setting initiator/responder resource without knowing what the max are 2) two implementations of the same function

Makes sense. However, since the libdisni is shipped separately, it may force some developers to recompile their java code which uses the library.

That should be ok. Developers can always choose to use an old version of the library if needed.

PepperJo · 2020-04-02T06:50:05Z

libdisni/src/verbs/com_ibm_disni_verbs_impl_NativeDispatcher.cpp

+ * Method:    _acceptV2
+ * Signature: (JJ)V
+ */
+JNIEXPORT void JNICALL Java_com_ibm_disni_verbs_impl_NativeDispatcher__1acceptV2(


See comment to connectV2

src/main/java/com/ibm/disni/verbs/impl/RdmaCmNat.java

PepperJo · 2020-04-02T06:55:48Z

src/main/java/com/ibm/disni/verbs/impl/RdmaCmNat.java

-				connParam.getRnr_retry_count(), connParam.getPrivate_data(), connParam.getPrivate_data_len());
+
+		MemBuf memBuf = memAlloc.allocate(connParam.CSIZE);
+        connParam.writeBack(memBuf.getBuffer());


Wrong indentation

PepperJo · 2020-04-02T06:57:00Z

src/test/java/com/ibm/disni/benchmarks/RDMAvsTcpBenchmarkClient.java

@@ -76,6 +76,12 @@ public void runRDMA() throws Exception {
    //create a EndpointGroup. The RdmaActiveEndpointGroup contains CQ processing and delivers CQ event to the endpoint.dispatchCqEvent() method.
    endpointGroup = new RdmaActiveEndpointGroup<CustomClientEndpoint>(1000, false, 128, 4, 128);
    endpointGroup.init(this);
+
+    RdmaConnParam connParam = new RdmaConnParam();
+    connParam.setResponder_resources((byte) 16);


Use query_device to set initiator/responder resources (see connectV2 comment)

Sure. I did not check that Disni did not implement it. I usually check the params manually using ibv_devinfo -v. I can add it.

Can we use the new query_device function here? Otherwise this code will not work on device where responder or initiator depth is less than 16.

PepperJo · 2020-04-02T06:57:55Z

src/test/java/com/ibm/disni/examples/AtomicClient.java

+/*
+ * DiSNI: Direct Storage and Networking Interface
+ *
+ * Author: Patrick Stuedi <[email protected]>


Obviously, Patrick is not the author of this file so please remove him and add yourself. Also please remove the IBM copyright of this file (I assume you are not an IBM employee)

Sure. it was copy-pasted based implementation. Do I need to include his name though, as 99% of the code there is his?

I agree if that is the case keep his name.

PepperJo · 2020-04-02T06:58:18Z

src/test/java/com/ibm/disni/examples/AtomicServer.java

+/*
+ * DiSNI: Direct Storage and Networking Interface
+ *
+ * Author: Patrick Stuedi <[email protected]>


See above (change author, remove copyright)

PepperJo

Wrong byte format of RdmaConnParam

src/main/java/com/ibm/disni/verbs/RdmaConnParam.java

PepperJo · 2020-04-02T11:50:50Z

src/main/java/com/ibm/disni/verbs/RdmaConnParam.java

+		buffer.put(retry_count);
+		buffer.put(rnr_retry_count);
+		buffer.put(srq);
+		buffer.put((byte)0);


cq_num is u32 not u8, so this should be putInt(0)

here I do not agree. Actually, buffer.put((byte)0); is a padding. cq_num field is 8 byte aligned. To add cq_num the code should be: buffer.put(srq); buffer.put((byte)0); buffer.putInt(cq_num);
I did not add cq_num as it is ignored anyway and does not cause any problems.

Makes sense.

TaranovK · 2020-04-05T15:57:43Z

Hey, I have applied the suggested changes, but the one with removing old connect/accept.

PepperJo · 2020-04-06T06:37:20Z

libdisni/src/verbs/com_ibm_disni_verbs_impl_NativeDispatcher.cpp

+ * Signature: (J)I
+ */
+JNIEXPORT jint JNICALL
+Java_com_ibm_disni_verbs_impl_NativeDispatcher__1queryMaxResponderResources(


That is not what I had in mind. I would like to be able to get all device attributes in Java. Otherwise we will be adding new JNI functions for every member of the struct along the way.

Sure. it was just more work.

PepperJo · 2020-04-06T06:37:39Z

libdisni/src/verbs/com_ibm_disni_verbs_impl_NativeDispatcher.cpp

+ * Signature: (J)I
+ */
+JNIEXPORT jint JNICALL
+Java_com_ibm_disni_verbs_impl_NativeDispatcher__1queryMaxInitiatorDepth(JNIEnv *env,


See comment queryMaxResponderResources

PepperJo

Thanks for the changes. It generally looks good. I just have a few comments:
I still believe we should merge the two JNI connect and accept functions. We can keep the upper level DiSNi API consistent by setting max responder/initiator depth when no connection parameters are set.
Please also update the native library version we use to check for compatibility

src/main/java/com/ibm/disni/verbs/impl/RdmaVerbsNat.java

PepperJo · 2020-04-15T06:25:00Z

src/test/java/com/ibm/disni/benchmarks/RDMAvsTcpBenchmarkClient.java

@@ -76,6 +76,12 @@ public void runRDMA() throws Exception {
    //create a EndpointGroup. The RdmaActiveEndpointGroup contains CQ processing and delivers CQ event to the endpoint.dispatchCqEvent() method.
    endpointGroup = new RdmaActiveEndpointGroup<CustomClientEndpoint>(1000, false, 128, 4, 128);
    endpointGroup.init(this);
+
+    RdmaConnParam connParam = new RdmaConnParam();
+    connParam.setResponder_resources((byte) 16);


Can we use the new query_device function here? Otherwise this code will not work on device where responder or initiator depth is less than 16.

PepperJo · 2020-04-15T06:26:20Z

src/test/java/com/ibm/disni/benchmarks/ReadClient.java

@@ -58,6 +58,11 @@ public ReadClient(String host, int port, int size, int loop) throws IOException{
 	private void run() throws Exception {
 		System.out.println("ReadClient, size " + size + ", loop " + loop);

+		RdmaConnParam connParam = new RdmaConnParam();
+		connParam.setResponder_resources((byte) 0);
+		connParam.setInitiator_depth((byte) 16);


query_device (see other comment)

PepperJo · 2020-04-15T06:26:26Z

src/test/java/com/ibm/disni/benchmarks/ReadServer.java

@@ -57,6 +57,11 @@ public ReadServer(String host, int port, int size, int loop) throws IOException{
 	private void run() throws Exception {
 		System.out.println("ReadServer, size " + size + ", loop " + loop);

+		RdmaConnParam connParam = new RdmaConnParam();
+		connParam.setResponder_resources((byte) 16);


query_device (see other comment)

PepperJo · 2020-04-15T06:26:36Z

src/test/java/com/ibm/disni/examples/ReadClient.java

+
+		RdmaConnParam connParam = new RdmaConnParam();
+		connParam.setResponder_resources((byte) 0);
+		connParam.setInitiator_depth((byte) 16);


query_device (see other comment)

PepperJo · 2020-04-15T06:26:51Z

src/test/java/com/ibm/disni/examples/ReadServer.java

@@ -49,6 +49,12 @@ public void run() throws Exception {
 		//create a EndpointGroup. The RdmaActiveEndpointGroup contains CQ processing and delivers CQ event to the endpoint.dispatchCqEvent() method.
 		endpointGroup = new RdmaActiveEndpointGroup<CustomServerEndpoint>(1000, false, 128, 4, 128);
 		endpointGroup.init(this);
+
+	    RdmaConnParam connParam = new RdmaConnParam();
+	    connParam.setResponder_resources((byte) 16);


query_device (see other comment)

TaranovK · 2020-04-21T11:27:48Z

Ok. I pushed the changes.

Regarding having single accept vs. having 2 accepts in JNI. I do not think it is a good idea to keep only one call and lose back-compatibility. However, I respect your will to have a single call in the master. You are free to modify my changes and remove the second call before merging the pull request.

For my personal use I need to be back-compatible and I will simply keep using my version.

PepperJo · 2020-04-22T06:46:45Z

Can you please explain the use case where you need this backwards compatibility?
Do you need to run the same user library with older Java code, why not compile the old library seperatly for that code? What restricts you from updating the old code?
I'm just trying to understand if there are real usecases that I don't see where this is a problem.

TaranovK · 2020-04-22T08:02:22Z

The problem is that libdisini.so is not included in disni.jar and has to be installed separately.
Therefore, if I install the new Disni and new libdisni all my old java code will not work as an old connect function is not in the shared libdisini.so. Sure, I can have different LD_LIBRARY_PATH for old and new applications, but it sounds cumbersome.

PepperJo · 2020-04-22T08:23:10Z

If we would "include" libdisni.so in the disni.jar like proposed here: #17
Would that solve your problem with the library versions?

TaranovK · 2020-04-22T08:46:54Z

Yes, it would.

patrickstuedi · 2020-06-08T21:22:24Z

I haven't read all the comments, and I'm fine with whatever the outcome is related to the connect/accept changes. As for including libdisni.so in disni.jar, that's something we've discussed many times and my opinion remains the same, let's not include shared libraries with the jar. For the time being, increment the version of libdisni after your changes, and similarly make sure in NativeDispatcher you increment the version of libdisni that is required.

TaranovK · 2020-06-09T12:20:29Z

@patrickstuedi I want to make sure that I understood you correctly. Do you propose to have only the new accept/connect and lose back compatibility? Or are you fine with having both old and new accept/connect in the libdisni.so?

patrickstuedi · 2020-06-10T00:13:31Z

I still need to look at the accept/connect changes in more detail, I haven't so far. But keeping an old and new version at the same time doesn't sound too appealing. I'm not so worried about backward compatibility if the changes are required and can be combined with a version bump.

TaranovK added 2 commits March 31, 2020 19:24

Added support for user-defined responder and initiator resources on c…

14ee342

…onnect/accept.

added support for atomics and made a simple test

0e7f403

PepperJo suggested changes Apr 2, 2020

View reviewed changes

added support for atomics and made a simple test

402d8a5

TaranovK requested a review from PepperJo April 5, 2020 15:57

PepperJo suggested changes Apr 6, 2020

View reviewed changes

added fully functioning query device

f96a00c

TaranovK requested a review from PepperJo April 6, 2020 15:56

PepperJo suggested changes Apr 15, 2020

View reviewed changes

added setting max connparams for various endpoints

759e282

TaranovK requested a review from PepperJo April 21, 2020 11:28

bug in old disni connect/accept.

09a75a0

bumped the version

642ed59

Support for RDMA Atomics + Support for user-defined initiator and responder resources on connect/accept #51

Are you sure you want to change the base?

Support for RDMA Atomics + Support for user-defined initiator and responder resources on connect/accept #51

Conversation

TaranovK commented Mar 31, 2020

PepperJo left a 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

PepperJo left a 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

TaranovK commented Apr 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PepperJo left a comment • 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

TaranovK commented Apr 21, 2020

PepperJo commented Apr 22, 2020

TaranovK commented Apr 22, 2020 • edited Loading

PepperJo commented Apr 22, 2020

TaranovK commented Apr 22, 2020

patrickstuedi commented Jun 8, 2020

TaranovK commented Jun 9, 2020

patrickstuedi commented Jun 10, 2020

PepperJo left a comment •

edited

Loading

TaranovK commented Apr 22, 2020 •

edited

Loading