diff --git a/.github/workflows/hipobject-hardware-test-two-vm-gpu-direct.yml b/.github/workflows/hipobject-hardware-test-two-vm-gpu-direct.yml index b36d0039..e0df4ea2 100644 --- a/.github/workflows/hipobject-hardware-test-two-vm-gpu-direct.yml +++ b/.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 diff --git a/CHANGELOG.md b/CHANGELOG.md index e03b70e8..f8bca99b 100644 --- a/CHANGELOG.md +++ b/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. + diff --git a/docs/building.rst b/docs/building.rst index 6b384fb0..48480cee 100644 --- a/docs/building.rst +++ b/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. diff --git a/docs/interop.rst b/docs/interop.rst index be034ad5..52a7d4a7 100644 --- a/docs/interop.rst +++ b/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 ------------- diff --git a/include/hipobj.h b/include/hipobj.h index 9415541b..619149e8 100644 --- a/include/hipobj.h +++ b/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. * diff --git a/src/buffer.cpp b/src/buffer.cpp index 5f192886..4a52f840 100644 --- a/src/buffer.cpp +++ b/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; } diff --git a/src/rdma/connection-pool.cpp b/src/rdma/connection-pool.cpp index 6c6d8aa7..a98acf2f 100644 --- a/src/rdma/connection-pool.cpp +++ b/src/rdma/connection-pool.cpp @@ -5,6 +5,7 @@ #include "connection-pool.h" +#include #include #include #include @@ -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; } }