feat: pass shared-memory fd through RegistSpan - #288
Conversation
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
PR #288 — feat: pass shared-memory fd through RegistSpan
This PR adds an fd field to RegistSpan, exposes it via pybind, and propagates the shared-memory metadata (fd, size, client base pointer) into TairMempoolSdkConfig for zero-copy transfers. The overall approach is clean and follows the existing UpdateMooncakeSdkConfig pattern.
Issues Found
1. TairMempoolSdkConfig::operator== doesn't compare new fields (sdk_config.h)
The existing operator== only delegates to the base class. After this PR, shm_fd_ and shm_size_ are private members that are silently ignored in equality checks, meaning two configs with different shared-memory parameters compare as equal. Other derived config classes (e.g., MooncakeSdkConfig, Hf3fsSdkConfig) include their derived fields in operator==. At minimum, shm_fd_ and shm_size_ should be compared (the pointer client_base_ can be skipped per the MooncakeSdkConfig convention).
2. Missing trailing newline (sdk_wrapper.cc)
The file sdk_wrapper.cc ends without a trailing newline after the closing }, which triggers compiler warnings.
Minor Observations
-
The comment on line 341 says
regist_span is optionalfor TairMempool. In practice, the call site inInitalways passesinit_params.regist_spanwhich is a value type (not a pointer that can be null), so thenullptrcheck is defensive but never triggers. The comment is not wrong but could be slightly misleading to future readers. -
The
fd >= 0guard inUpdateTairMempoolSdkConfigcorrectly gates the shm propagation, which is good defensive design.
🤖 Generated by Qoder
| void set_client_base(void *base) { client_base_ = base; } | ||
|
|
||
| private: | ||
| int shm_fd_{-1}; |
There was a problem hiding this comment.
The operator== on line 157 still only delegates to SdkBackendConfig::operator== and doesn't compare the new shm_fd_, shm_size_, or client_base_ fields. Following the MooncakeSdkConfig pattern (which compares local_buffer_size_ but skips the pointer), this should be updated to at least compare shm_fd_ and shm_size_:
bool operator==(const TairMempoolSdkConfig &other) const {
return SdkBackendConfig::operator==(other) && shm_fd_ == other.shm_fd_ &&
shm_size_ == other.shm_size_;
}Two configs with different shared-memory parameters would currently compare as equal, which is inconsistent with how other derived SDK config classes work.
🤖 Generated by Qoder
| return ER_OK; | ||
| } | ||
|
|
||
| } // namespace kv_cache_manager No newline at end of file |
There was a problem hiding this comment.
Missing trailing newline at end of file. Most compilers emit a warning for this (no newline at end of file). Please add a newline after the closing brace.
🤖 Generated by Qoder
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a872fe7be6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| struct RegistSpan { | ||
| void *base{nullptr}; | ||
| size_t size{0}; | ||
| int fd{-1}; |
There was a problem hiding this comment.
Because common.h is exported with the client shared library, adding this member changes the size/layout of a public struct that callers allocate and pass by pointer. In a mixed-version client upgrade where an existing C++ worker keeps the old 16-byte RegistSpan layout but loads the new .so, UpdateTairMempoolSdkConfig can read past that object and interpret adjacent memory as a nonnegative shared-memory fd. Please make the fd opt-in through a versioned API/side channel, or document and coordinate this as a breaking client rebuild.
Useful? React with 👍 / 👎.
a872fe7 to
6227216
Compare
charpty
left a comment
charpty
left a comment
There was a problem hiding this comment.
Requesting changes for two additional correctness issues in the zero-copy handoff. The existing unresolved ABI-compatibility thread on RegistSpan also remains blocking. Please add focused tests for Python object lifetime and partial/invalid shared-memory tuples.
| .def_property("base", &kvcm::RegistSpan::base_as_uint64, &kvcm::RegistSpan::set_base_as_uint64) | ||
| .def_readwrite("size", &kvcm::RegistSpan::size); | ||
| .def_readwrite("size", &kvcm::RegistSpan::size) | ||
| .def_readwrite("fd", &kvcm::RegistSpan::fd); |
There was a problem hiding this comment.
[P1] Keep the span and backing mapping alive. InitParams.regist_span is still exposed as a raw RegistSpan * (def_readwrite below), so assigning a temporary Python RegistSpan does not give InitParams ownership. This change makes the Tair path dereference the pointer to read fd; after the Python span (or its backing mmap/buffer) is collected, initialization can read a dangling object and accidentally opt into zero-copy with garbage metadata. Prefer making InitParams own the span by value/shared_ptr; at minimum use a custom setter with an explicit keep-alive policy and retain the backing mapping for the client's lifetime. Please add a test that constructs the params in a helper, forces gc.collect(), and then initializes.
| // For TairMempool, regist_span is optional (only needed for shared memory registration) | ||
| return ER_OK; | ||
| } | ||
| if (span->fd >= 0) { |
There was a problem hiding this comment.
[P2] Validate the shared-memory tuple atomically before enabling zero-copy. fd >= 0 currently propagates base == nullptr and/or size == 0, so a partially initialized RegistSpan is accepted here and only fails later after PACE initialization. Require fd >= 0, non-null base, positive size, and a non-overflowing address range together (and preferably validate the fd/backing length with fstat); otherwise return ER_INVALID_PARAMS. The legacy path should remain the explicit all-default tuple.
Add file descriptor support to RegistSpan for shared memory registration, expose it via pybind, and propagate shm_fd/shm_size/client_base through TairMempoolSdkConfig in SdkWrapper.
6227216 to
b6a20e2
Compare
2 participants
Add an fd field to RegistSpan and expose it through pybind. Propagate the registered span into TairMempoolSdkConfig so the SDK can use the shared-memory fd, size, and client base for zero-copy transfers. The earlier direct-IO Bazel source fix is already covered by the current main branch glob. Local build and tests were not run per project policy.