rdma: add a log callback ABI to the RC data plane - #2363
Open
potatogim wants to merge 6 commits into
Open
Conversation
The hipobj-rc-v2 data plane started with its session, queue pair, staging and timeout limits hardcoded at the rcserver.Init call site, so operators could not size the RC plane for their hardware the way they can for the cuObject backend. Add one flag per limit plus the READY admission slot count, all defaulting to the values the gateway passes today, and validate them through a new rdmamode.V2ValidationError consulted only when the RC data plane is enabled, mirroring the stale-value handling of the v1 settings. Counts are parsed as uint64 and range-checked against the uint32 narrowing at the DeviceOpts boundary, and the timeouts carry an upper bound that keeps the nowMs + timeout deadline arithmetic in the C core from wrapping.
Wire C-side diagnostics (session reap, READY data phase outcome, init failures) through a sink callback so the gateway can surface them next to its own logs instead of losing them in stderr noise. The sink is a plain C function pointer installed once after init and valid until destroy: the Go side registers a fixed cgo trampoline (closures cannot cross the boundary), copies the message immediately per the lifetime contract, and never runs under the session map lock. Error-level lines keep the existing stderr output; --debug enables the level-2 diagnostic stream.
The verbs loader and device enumeration failures returned RC_E_INTERNAL without any stderr trace, which made a VM or container without RDMA indistinguishable from a genuine library problem. Print the failing step so operators can tell the two apart at startup.
ibv_poll_cq, ibv_post_send, and ibv_post_recv stopped being exported library symbols in modern rdma-core: verbs.h ships them as static inline wrappers that dispatch through cq->context->ops. dlsym therefore returned null for them and the loader rejected perfectly usable libraries, failing RC server init with a bare RC_E_INTERNAL on hosts with rdma-core 61+. Open the first device briefly, read the three function pointers from its context ops table, and close it again. The check now only requires symbols that actually exist in the library, and the failure mode for an ops-less provider is explicit.
Collaborator
|
This won't automatically run in continuous integration without approval. A member of the Versity organization must allow it. |
The Prepare and ReadyTransfer wrappers embed string views built from Go heap strings inside request structs passed to C by pointer. The cgo pointer check rejects such requests when the string data is an unpinned Go heap pointer, so any live PREPARE or READY call with header-derived strings panicked at the call boundary and the route returned a 500. Constant strings passed the check because their data lives in read-only static storage, which is why standalone callers kept working while the gateway did not. Pin the string bytes with runtime.Pinner for the duration of the cgo call and drop the now redundant KeepAlive calls in those two wrappers. The other string-taking wrappers pass rc_str_in by value and are unaffected. Also add a deviceless cgo boundary regression test that calls the real Prepare wrapper with heap-backed interior-pointer strings and an invalid opcode, so C returns from argument validation before the server handle is touched.
The session id only existed as the sessions map key; the session record itself kept an empty id string, so the terminal reap record logged an empty id for every expired, cancelled, or destroyed session. Copy the id into the record at creation time so teardown logs identify the session they describe.
Contributor
Author
|
The MacOS s3cmd check failure looks unrelated to this change - the log stops at |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #2361.
Adds a log callback ABI to the RC data plane so gateway operators can see session-level diagnostics (reap events, READY data-phase outcomes, init failures) instead of only having raw stderr.
rc_server_set_log_sink()installs a plain function pointer after init that stays valid until destroy. The gateway registers a fixed cgo trampoline (closures cannot cross the boundary), copies each message immediately per the lifetime contract, and never calls the sink while holding the session map lock. Error-level lines keep their existing stderr output, and--debugenables the level-2 diagnostic stream.While verifying the init failure path I noticed a loader bug on rdma-core 61+ hosts: as far as I can tell,
ibv_poll_cq,ibv_post_send, andibv_post_recvare no longer exported as library symbols in modern rdma-core. verbs.h ships them as static inline wrappers that dispatch throughcq->context->ops, so the dlsym-based loader treated the installed library as incomplete and init failed with a bareRC_E_INTERNAL. The loader now resolves those three calls through the ops table of a briefly opened device, and deviceless and gid-hint-mismatch init failures now print which step failed.While verifying the sink end to end I ran into two more defects, both fixed here. First, the
PrepareandReadyTransferGo wrappers embedded string views built from heap strings inside request structs passed to C by pointer, so every live PREPARE or READY call with header-derived strings panicked at the cgo pointer-check boundary and the route returned a 500; the string bytes are now pinned withruntime.Pinnerfor the duration of the call. Second, the session id only lived as the sessions map key, so the reap record logged an empty id; the record now carries its own copy. A deviceless cgo regression test covers the panic path by calling the real wrapper with heap-backed strings and an opcode that returns from C argument validation before the server handle is touched.Verified on a host with rdma-core 61 and a passthrough mlx5_0 VF: archive rebuild, full vgwrdma link, stub and unit tests, the new cgo boundary test under
GODEBUG=cgocheck=1andGOEXPERIMENT=cgocheck2,--debugstartup reaching a listening gateway, a signedPOST /.hipobj-rc/preparereturning a complete session (id, QPN, PSN, staging address and rkey) instead of the previous 500, reap records reaching the gateway log through the callback with the session id present, and clean SIGTERM shutdown.