search

zrlio / disni

DiSNI: Direct Storage and Networking Interface
Apache License 2.0
186 stars 66 forks source link

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

Open TaranovK opened 4 years ago

TaranovK commented 4 years ago

Hi,

The following changes add:

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

TaranovK commented 4 years ago

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

TaranovK commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

If we would "include" libdisni.so in the disni.jar like proposed here: https://github.com/zrlio/disni/pull/17 Would that solve your problem with the library versions?

TaranovK commented 4 years ago

Yes, it would.

patrickstuedi commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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.