Xilinx-CNS / tcpdirect

AMD TCPDirect ultra low latency kernel bypass TCP and UDP implementation for AMD Solarflare network adapters, to be used with corresponding versions of Onload®️ at https://github.com/Xilinx-CNS/onload. The stable branch is currently `v8_1`.
22 stars 17 forks source link

ON-12878: use the new cplane user API #22

Closed jfeather-amd closed 4 months ago

jfeather-amd commented 5 months ago

Starting with Xilinx-CNS/onload@b660ac5b259f33cddcb1c6bd6f81ef7910dc10a7, @rhughes-xilinx added a public cplane API to onload. This PR modifies tcpdirect to use this new public cplane API, and is ported from before the open-sourcing of tcpdirect. For internal reviewers, the following PRs may be useful reading as an introduction to this PR:

Many of the changes from the original PR are rebase conflicts, and some small bugs (mostly in the unit tests).

Testing Done

Perhaps worth noting, as I'm not sure if this behaviour was present before, but specifying a physical interface for transmit can be used to bypass logical interfaces like bonds and VLANs.

TODO

Some further modification to onload was required for me to get the test apps working (zfsend in this case), perhaps because of my specific workflow. I had to modify onload_install to also install the cplane libraries, otherwise tests would fail with ENOENT. The diff for this change is below, but I also intend to raise this as a PR in onload, unless someone points out an alternative making the below unecessary.

diff --git a/scripts/onload_install b/scripts/onload_install
index fa04b05cb2..e104b001c8 100755
--- a/scripts/onload_install
+++ b/scripts/onload_install
@@ -229,6 +229,8 @@ install_userland() {
               "$i_lib64/libonload.so"
     install_dlib "$u64"/lib/onload_ext/libonload_ext.so "$i_lib64"
     install_slib "$u64"/lib/onload_ext/libonload_ext.a "$i_lib64"
+    install_dlib "$u64"/lib/cplane/libefcp.so "$i_lib64"
+    install_slib "$u64"/lib/cplane/libcplane0.a "$i_lib64"
     if [ -n "$ppc_at" ]; then
       install_plib "${u64}_at/lib/transport/unix/libcitransport0.so" \
                    "$i_prefix/$ppc_at/lib64/libonload.so"
rhughes-xilinx commented 5 months ago

Can you look again at why we need libcplane0.a from onload? Surely the whole point of this change is to get rid of that

jfeather-amd commented 5 months ago

Can you look again at why we need libcplane0.a from onload? Surely the whole point of this change is to get rid of that

Is this regardng the TODO section onload diff? If so, I hadn't put much thought into the change, and just installed any libraries that contained the word "cplane". I would be very unsurprised if removing libcplane0.a from the install script had no effect on the final behaviour. The test apps only complains about the lack of libefcp.so.

jfeather-amd commented 4 months ago

After some deeper testing, including some performance testing (see ON-1440 internally), it looks like this patch is ready to go so I'll merge this now.