Fix comments and documentation

.github/workflows/hipobject-hardware-test-two-vm-gpu-direct.yml

@@ -915,6 +915,10 @@ jobs:
           `IBV_WC_REM_ACCESS_ERR` (`status=10 vendor_err=0x9`) at 1 MiB, well
           under the 16 MiB `IONIC_MESH_MAX_MSG` cap.
 
+          In about 1 run in 5, it fails differently: the PUT succeeds
+          (`PUT ok`), then the GET hangs until the client's timeout ends it
+          (exit status 124), with nothing in the server's journal.
+
           Ruled out by measurement on a two-guest lab that passes 8/8 with the
           same binaries: rocjitsu build, QEMU build, guest memory, vCPU count,
           accelerator, `ip_discovery.bin`, the message-size cap and BAR

CHANGELOG.md

@@ -213,6 +213,13 @@
     which finish normally. hipObject releases the RDMA resources and
     buffer registrations when the last of them returns, and
     `hipObjInit()` can initialize it again before then.
+  - `hipObjInit()` reads hipObject's environment variables, so it
+    mustn't run while another thread changes the environment, for
+    example with `setenv()`, `putenv()`, or `unsetenv()`.
+  - A callback must return on the thread that called it, since hipObject
+    tells which transfers a callback starts by the thread it runs on.
+    This matters for a callback that runs in a fiber or coroutine that
+    can move to another thread.
 - `hipObjGetVersionString()` no longer writes to a shared buffer on every
   call, which was a data race when two threads called it at once.
 - The library links CMake's `Threads::Threads`, which adds `-pthread`
@@ -353,12 +360,13 @@
   `hipObjBufDeregister()` and `hipObjShutdown()` freed the staging buffer
   while the copy could still use it, and later transfers could use the
   staging buffer at the same time as the copy. hipObject now keeps the
-  staging buffer until the copy finishes. Until then, `hipObjGet()`,
+  staging buffer until the copy finishes. Until then, calls that start
+  after the copy times out can't use the buffer: `hipObjGet()`,
   `hipObjPut()`, `hipObjBufSync()`, and `hipObjGetRdmaToken()` return
-  the new `hipObjBusy` error for the buffer, and `hipObjBufDeregister()`
+  the new `hipObjBusy` error for it, and `hipObjBufDeregister()`
   returns `hipObjBusy` and leaves it registered; call it again once the
-  copy has finished. If the copy never finishes, the staging
-  buffer is never freed.
+  copy has finished. Calls that had already started aren't stopped. If
+  the copy never finishes, the staging buffer is never freed.
 - The queue pair `hipObjInit()` creates didn't allow remote reads, which
   the server needs to read a `hipObjPut()` buffer. hipObject used the wrong
   values for the libibverbs access flags, so the queue pair allowed remote
@@ -454,3 +462,10 @@
 
 ### Known issues
 
+- The client and the server don't negotiate a path MTU. hipObject sets
+  its connections' path MTU to 4096 bytes, so both ends' ports, and the
+  network between them, must carry 4096-byte RDMA packets, which with
+  RoCE takes jumbo frames. If the MTUs don't match, transfers hang until
+  the server times out. `docs/interop.rst` now describes the
+  requirement.
+

docs/building.rst

@@ -466,7 +466,12 @@ Additional system packages (Ubuntu 24.04):
 
 .. code-block:: bash
 
-   sudo apt install libssl-dev zlib1g-dev libcurl4-openssl-dev
+   sudo apt install libssl-dev zlib1g-dev libcurl4-openssl-dev \
+     nlohmann-json3-dev libpugixml-dev libcurlpp-dev
+
+The build needs every package but ``libcurlpp-dev``. Without it,
+the build downloads curlpp from GitHub and builds it. The build
+always downloads minio-cpp and inih.
 
 See ``integrations/minio-cpp/TESTING.md`` for lab validation
 against MinIO AIStor over RDMA.

docs/interop.rst

@@ -97,6 +97,14 @@ cuObject-equipped storage gateways in production deployments.
 
 .. _adapter: https://github.com/versity/versitygw/tree/main/cuwrapper
 
+The client and the server don't negotiate a path MTU, and the token
+doesn't carry one, so both ends' MTUs must match. hipObject sets its
+connections' path MTU to 4096 bytes, so configure the client's and the
+server's ports, and the network between them, for 4096-byte RDMA
+packets. With RoCE, that takes jumbo frames, usually an Ethernet MTU of
+9000. If the MTUs don't match, transfers hang until the server times
+out, without an error on either end.
+
 Testing paths
 -------------
 

include/hipobj.h

@@ -49,7 +49,9 @@ extern "C" {
  * while the other changes something that both use, such as the list of
  * registered buffers, with two exceptions: a transfer can wait for an
  * RDMA connection (see below), and calls to hipObjInit() and
- * hipObjShutdown() run one at a time.
+ * hipObjShutdown() run one at a time. hipObjInit() reads hipObject's
+ * environment variables, so it mustn't run while another thread changes
+ * the environment, for example with setenv(), putenv(), or unsetenv().
  *
  * **Transfers.** hipObjGet() and hipObjPut() calls run at the same time
  * as each other, and as every other call. Each transfer uses an RDMA
@@ -65,7 +67,10 @@ extern "C" {
  * connection, since the one it would wait for could be the one the
  * callback's own transfer holds: if none is free, it returns hipObjBusy.
  * A callback that waits for another thread's transfer can wait forever
- * for the same reason, and hipObject can't detect it.
+ * for the same reason, and hipObject can't detect it. hipObject tells
+ * which transfers a callback starts by the thread it runs on, so a
+ * callback must return on the thread that called it. This matters if it
+ * runs in a fiber or coroutine that can move to another thread.
  *
  * **Buffers.** hipObjBufDeregister() returns hipObjBusy, and leaves the
  * buffer registered, while another call is using the buffer: a transfer,
@@ -272,6 +277,10 @@ typedef struct {
  * functions return hipObjNotInitialized until it has
  * succeeded.
  *
+ * It reads hipObject's environment variables, so
+ * don't change the environment on another thread
+ * while it runs (see @ref threads).
+ *
  * Returns hipObjInvalidValue if config is NULL, flags
  * isn't 0, or gpuDevice is below -1 or names a GPU that
  * doesn't exist; hipObjAlreadyInitialized if hipObject
@@ -390,8 +399,9 @@ HIPOBJ_API hipObjError_t hipObjBufDeregister(void *devPtr);
  * HIPOBJ_STAGE_TIMEOUT_MS environment variable), this
  * returns hipObjInternalError and leaves the copy
  * running. It can still write the buffer after this
- * returns, and until it finishes, the buffer can't be
- * used or deregistered (see hipObjBufSync()).
+ * returns, and until it finishes, calls that start
+ * after this returns can't use or deregister the
+ * buffer (see hipObjBufSync()).
  *
  * @param handle  S3 object handle (from application)
  * @param devPtr  Registered GPU or host buffer
@@ -432,8 +442,9 @@ HIPOBJ_API hipObjError_t hipObjGet(hipObjHandle_t handle, void *devPtr, size_t s
  * HIPOBJ_STAGE_TIMEOUT_MS environment variable), this
  * returns hipObjInternalError and leaves the copy
  * running. It can still read the buffer after this
- * returns, and until it finishes, the buffer can't be
- * used or deregistered (see hipObjBufSync()).
+ * returns, and until it finishes, calls that start
+ * after this returns can't use or deregister the
+ * buffer (see hipObjBufSync()).
  *
  * @param handle  S3 object handle (from application)
  * @param devPtr  Registered GPU or host buffer
@@ -468,10 +479,12 @@ HIPOBJ_API hipObjError_t hipObjPut(hipObjHandle_t handle, const void *devPtr, si
  * doesn't finish within the staging timeout (the HIPOBJ_STAGE_TIMEOUT_MS
  * environment variable), this returns hipObjInternalError and leaves the
  * copy running. It can still read or write the buffer after this returns.
- * Until it finishes, hipObject keeps the staging buffer it uses, and the
- * buffer can't be used: hipObjGet(), hipObjPut(), hipObjBufSync(), and
- * hipObjGetRdmaToken() return hipObjBusy for it, and
- * hipObjBufDeregister() returns hipObjBusy and leaves it registered.
+ * Until it finishes, hipObject keeps the staging buffer it uses, and
+ * calls that start after the copy times out can't use the buffer:
+ * hipObjGet(), hipObjPut(), hipObjBufSync(), and hipObjGetRdmaToken()
+ * return hipObjBusy for it, and hipObjBufDeregister() returns hipObjBusy
+ * and leaves it registered. Calls that had already started aren't
+ * stopped.
  * Once the copy has finished, the buffer can be used and deregistered
  * again. If it never finishes, the staging buffer is never freed.
  *

src/buffer.cpp

@@ -266,10 +266,13 @@ BufferMap::deregisterBuffer(void *devPtr)
         if (it == entries_.end()) {
             return DeregisterResult::NotRegistered;
         }
-        /* Other copies are only made by find(), under the lock this holds
-         * exclusively, so none can appear while it's held. One that's
-         * released while this checks can only make the entry look busy
-         * when it's not, never the reverse. */
+        /* A copy can only be made from one that's already held. find()
+         * copies the map's own under the lock this holds exclusively,
+         * and the other copies, such as the one AbandonedCopies::add()
+         * keeps, are made from one that find() returned. So while this
+         * holds the lock, a count of 1, the map's own copy, can't go
+         * up. A copy that's released while this checks can only make
+         * the entry look busy when it's not, never the reverse. */
         if (it->second.use_count() > 1) {
             return DeregisterResult::Busy;
         }

src/rdma/connection-pool.cpp

@@ -5,6 +5,7 @@
 
 #include "connection-pool.h"
 
+#include <cassert>
 #include <cstddef>
 #include <cstdio>
 #include <memory>
@@ -50,11 +51,14 @@ ConnectionPool::Lease::Lease(ConnectionPool &pool, size_t index) noexcept : pool
 }
 
 /* A lease that's been moved from was never counted by the move, so it
- * isn't uncounted either */
+ * isn't uncounted either. The count can only be 0 here if the lease is
+ * destroyed on a thread that didn't acquire it, which the header forbids
+ * by requiring callbacks to return on the thread that called them. */
 ConnectionPool::Lease::~Lease()
 {
     if (pool_ != nullptr) {
         pool_->release(index_, connected_);
+        assert(t_leasesHeld > 0);
         --t_leasesHeld;
     }
 }