feat: add host PID broker client and CUDA context accounting - #251
feat: add host PID broker client and CUDA context accounting#251iemAnshuman wants to merge 55 commits into
Conversation
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: iemAnshuman The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds trusted Host PID broker discovery, hardened fallback locking, PCI-based CUDA-to-NVML mapping, per-device primary-context accounting, fork handling, rollout documentation, and focused tests. ChangesHost PID discovery and broker protocol
Fallback locking and post-init coordination
Device mapping and accounting
Validation and build integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CUDAProcess
participant postInit
participant BrokerClient
participant FallbackLock
participant NVML
CUDAProcess->>postInit: initialize process
postInit->>BrokerClient: request host PID
BrokerClient-->>postInit: return PID or failure
postInit->>FallbackLock: acquire fallback locks when required
FallbackLock-->>postInit: return discovery synchronization
postInit->>NVML: map devices and query process memory
NVML-->>postInit: return validated device data
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
70c85e4 to
973a989
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
docs/hostpid-broker-client.md (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHyphenate the compound modifiers.
📝 Proposed fixes
-4. The directory is visible through a read only mount. +4. The directory is visible through a read-only mount.-...an index list and a GPU UUID use the same driver supplied identity. +...an index list and a GPU UUID use the same driver-supplied identity.-The separate post init lock migration from PR 248 has a different mixed binary constraint... +The separate post-init lock migration from PR 248 has a different mixed-binary constraint...Consider also linking PR 248 at Line 59 so readers can find the referenced migration.
Also applies to: 41-41, 59-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/hostpid-broker-client.md` at line 25, Hyphenate compound modifiers in the affected documentation statements, including “read-only mount” and the corresponding occurrences. At the referenced PR mention, link PR 248 so readers can navigate to the migration details.Source: Linters/SAST tools
test/test_hostpid_broker.c (1)
160-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBound the copy into
sun_path.
socket_pathis aPATH_MAXbuffer, andaddress.sun_pathholds 108 bytes. The current template produces a short path, so no overflow occurs today. Add an explicit length check so a longer template cannot overflow the destination.♻️ Proposed refactor
address.sun_family = AF_UNIX; - strcpy(address.sun_path, socket_path); + assert(strlen(socket_path) < sizeof(address.sun_path)); + snprintf(address.sun_path, sizeof(address.sun_path), "%s", socket_path);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_hostpid_broker.c` around lines 160 - 165, Bound the socket_path copy in the address setup around address.sun_path so paths longer than the destination capacity are rejected or safely truncated before strcpy is reached. Preserve the existing AF_UNIX initialization and address_length calculation for valid paths, using the available sun_path capacity rather than assuming the PATH_MAX source buffer fits.Source: Linters/SAST tools
src/hostpid_broker.c (2)
342-352: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueSet the socket flags atomically.
socket()creates the descriptor, and the twofcntlcalls set the flags afterwards. If another thread forks in that window, the descriptor leaks into the child. Linux accepts the flags in thetypeargument.♻️ Proposed refactor
- fd = socket(AF_UNIX, SOCK_STREAM, 0); + fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, 0); if (fd < 0) { return -1; } - if (fcntl(fd, F_SETFD, FD_CLOEXEC) != 0 || - fcntl(fd, F_SETFL, O_NONBLOCK) != 0) { - saved_errno = errno; - close(fd); - errno = saved_errno; - return -1; - }Note that
SOCK_CLOEXECandSOCK_NONBLOCKare Linux specific. Keep thefcntlpath if the project must build on macOS, where the__APPLE__branch at Line 358 suggests portability intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hostpid_broker.c` around lines 342 - 352, Update the socket creation in the surrounding broker function to request SOCK_CLOEXEC and SOCK_NONBLOCK atomically through the socket type on Linux, removing the post-creation fcntl setup there. Preserve the existing fcntl fallback for the __APPLE__ portability path and retain current errno cleanup behavior on failure.
245-281: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidate the full ancestor chain, not just the immediate directory.
hostpid_broker_validate_trustcallslstaton the socket and on its immediate parent only. For the fixed path/tmp/vgpulock/hostpid/broker.sock, the components/tmpand/tmp/vgpulockare not validated. The kernel resolves those components, including symlinks. The check is also path based, so the path can change between validation and theconnectat Line 362.The read-only mount requirement and the
SO_PEERCREDUID 0 check limit the impact. Consider hardening the traversal: open each component withopenat(..., O_PATH | O_NOFOLLOW)from/, applyfstatandfstatvfsto the resulting descriptors, and then connect. This removes the symlink and rename window.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hostpid_broker.c` around lines 245 - 281, Harden hostpid_broker_validate_trust and the subsequent connect flow by traversing every socket-path component from the root with openat using O_PATH|O_NOFOLLOW, validating each descriptor with fstat and applying the read-only check via fstatvfs. Retain ownership, directory/socket type, and permission checks across the full ancestor chain, then connect using the validated endpoint or otherwise prevent path changes between validation and connect.src/cuda/CMakeLists.txt (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated
hook.centry.
hook.cis listed twice. CMake removes the duplicate, so the build still works. Delete the second entry while this line is being edited.♻️ Proposed cleanup
-add_library(cuda_mod OBJECT context.c context_accounting.c device.c hook.c event.c hook.c memory.c stream.c graph.c) +add_library(cuda_mod OBJECT context.c context_accounting.c device.c hook.c event.c memory.c stream.c graph.c)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cuda/CMakeLists.txt` at line 1, Update the source list in the cuda_mod add_library declaration to remove the duplicated hook.c entry, keeping the remaining source files unchanged.test/test_context_accounting.c (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
assertis removed whenNDEBUGis defined.Every check in this file uses
assert. If the test target builds withNDEBUG, for example underCMAKE_BUILD_TYPE=Release, the compiler removes all checks andmainreturns0without testing anything. Add#undef NDEBUGbefore#include <assert.h>, or force the test target to build withoutNDEBUG.♻️ Proposed change
+#undef NDEBUG `#include` <assert.h>Run the following script to check the build type used for the test target:
#!/bin/bash # Inspect the test CMake configuration and any NDEBUG handling. fd -H 'CMakeLists.txt' | xargs rg -n 'test_context_accounting|NDEBUG|CMAKE_BUILD_TYPE|CMAKE_C_FLAGS'Also applies to: 198-210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_context_accounting.c` at line 7, Ensure assertions remain active in test_context_accounting by undefining NDEBUG immediately before including assert.h, or configure the test target to compile without NDEBUG. Prefer the localized test-source change so every assert check remains effective regardless of the build type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/postinit-lock-upgrade.md`:
- Around line 26-30: Update the cache-safety procedure in the post-init lock
upgrade instructions to check active memory mappings for every effective
CUDA_DEVICE_MEMORY_SHARED_CACHE path, including generated usage.cache files and
the fallback, rather than checking only open descriptors. Do not unlink any
cache until all processes retaining mappings have exited; alternatively, require
that every process using each cache has exited before removal.
- Around line 9-15: Update the postinit lock-upgrade documentation to describe
the broker-success path from postInit() separately from the NVML fallback:
identify the validation and synchronization performed after
set_task_pid_from_broker() returns NVML_SUCCESS, and state whether host PID
discovery can overlap with the fallback. Ensure both paths are documented as
following the same cache-lock contract, or specify the required implementation
change if the trusted broker-success path is not synchronized.
In `@src/cuda/context.c`:
- Around line 85-122: Move the lazy primary-context memory measurement in
cuDevicePrimaryCtxRetain outside context_accounting_lock, or protect it with a
per-device measurement lock and in-progress flag so unrelated devices and fork
preparation are not blocked. Preserve the existing measurement and accounting
updates while ensuring only one thread performs the wait/retry sequence for a
device.
- Around line 123-133: Update the primary-context accounting path around
primary_context_record_accounted_retain so an ENODATA failure from an unmeasured
retain does not convert a successful CUDA retain into CUDA_ERROR_OUT_OF_MEMORY.
Choose and implement an explicit policy: either perform a longer bounded
measurement before failing closed, or fall back to primary_context_record_retain
and log that the retain was unmeasured; preserve normal accounting for measured
retains and other errors.
In `@src/multiprocess/multiprocess_memory_limit.c`:
- Around line 1492-1497: Synchronize the host PID transition and monitor resets
in the code surrounding the host PID assignment: update atomic slot->hostpid and
clear every slot->monitorused entry under the shared-region lock or one
equivalent generation transition. Ensure utilization watchers cannot observe the
new PID until all monitor values have been reset.
In `@src/multiprocess/multiprocess_utilization_watcher.c`:
- Line 310: Initialize all visible userutil entries to -1 before each
get_used_gpu_utilization() call, ensuring devices without reverse mappings
retain the sentinel value. Keep the existing userutil[dev] >= 0 guard so only
valid utilization samples affect token adjustments in the device loop.
In `@src/utils.c`:
- Around line 263-273: Update the measured-memory read in the loop processing
the merged pids so it uses pids_on_device[i].usedGpuMemory rather than
tmp_pids_on_device[i].usedGpuMemory. Keep the existing availability, size
validation, and context_size handling unchanged.
- Around line 306-316: Update the function around parse_cuda_visible_env() to
capture and use its parsed entry count for cuda_to_nvml_map_count instead of
assigning the cuDeviceGetCount() result. Preserve the existing CUDA call only
where needed for validation, and ensure the stored count matches the populated
cuda_to_nvml_map_array entries.
- Around line 318-375: In src/utils.c lines 318-375, define and document the
contract for map_cuda_devices_to_nvml_by_pci, including whether
CUDA_DEVICE_MAX_COUNT is a valid sentinel and ensuring every consumer, including
nvml_to_cuda_map(), is bounded by cuda_to_nvml_map_count. In src/utils.c lines
103-134, preserve the env-derived mapping before invoking
map_cuda_devices_to_nvml_by_pci and restore it whenever the broker path fails
after the PCI mapping succeeds, so set_task_pid() uses the expected mapping.
- Around line 162-181: Before the realloc size calculation in the process-query
flow, validate driver-provided count against an overflow-safe maximum and return
an appropriate error when it exceeds the bound. Confirm the NVML macro target
used by nvmlDeviceGetComputeRunningProcesses, then make the allocation type and
call entry point use the matching process-info struct version, updating the
visible nvmlProcessInfo_v1_t usage if the build resolves to v3.
In `@test/test_hostpid_broker.c`:
- Around line 7-24: Add the appropriate header or POSIX feature-test
configuration before the includes in the test source so PATH_MAX is declared
under strict C99 builds. Keep the existing PATH_MAX uses in the test unchanged,
and avoid introducing a local replacement constant.
---
Nitpick comments:
In `@docs/hostpid-broker-client.md`:
- Line 25: Hyphenate compound modifiers in the affected documentation
statements, including “read-only mount” and the corresponding occurrences. At
the referenced PR mention, link PR 248 so readers can navigate to the migration
details.
In `@src/cuda/CMakeLists.txt`:
- Line 1: Update the source list in the cuda_mod add_library declaration to
remove the duplicated hook.c entry, keeping the remaining source files
unchanged.
In `@src/hostpid_broker.c`:
- Around line 342-352: Update the socket creation in the surrounding broker
function to request SOCK_CLOEXEC and SOCK_NONBLOCK atomically through the socket
type on Linux, removing the post-creation fcntl setup there. Preserve the
existing fcntl fallback for the __APPLE__ portability path and retain current
errno cleanup behavior on failure.
- Around line 245-281: Harden hostpid_broker_validate_trust and the subsequent
connect flow by traversing every socket-path component from the root with openat
using O_PATH|O_NOFOLLOW, validating each descriptor with fstat and applying the
read-only check via fstatvfs. Retain ownership, directory/socket type, and
permission checks across the full ancestor chain, then connect using the
validated endpoint or otherwise prevent path changes between validation and
connect.
In `@test/test_context_accounting.c`:
- Line 7: Ensure assertions remain active in test_context_accounting by
undefining NDEBUG immediately before including assert.h, or configure the test
target to compile without NDEBUG. Prefer the localized test-source change so
every assert check remains effective regardless of the build type.
In `@test/test_hostpid_broker.c`:
- Around line 160-165: Bound the socket_path copy in the address setup around
address.sun_path so paths longer than the destination capacity are rejected or
safely truncated before strcpy is reached. Preserve the existing AF_UNIX
initialization and address_length calculation for valid paths, using the
available sun_path capacity rather than assuming the PATH_MAX source buffer
fits.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e53c1994-6610-4dde-aca5-2b37183c1b45
📒 Files selected for processing (21)
docs/hostpid-broker-client.mddocs/postinit-lock-upgrade.mdsrc/CMakeLists.txtsrc/allocator/allocator.csrc/cuda/CMakeLists.txtsrc/cuda/context.csrc/cuda/context_accounting.csrc/cuda/context_accounting.hsrc/cuda/memory.csrc/hostpid_broker.csrc/include/hostpid_broker.hsrc/include/libvgpu.hsrc/libvgpu.csrc/multiprocess/multiprocess_memory_limit.csrc/multiprocess/multiprocess_memory_limit.hsrc/multiprocess/multiprocess_utilization_watcher.csrc/nvml/hook.csrc/utils.ctest/CMakeLists.txttest/test_context_accounting.ctest/test_hostpid_broker.c
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/hostpid-broker-client.md (1)
57-57: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCorrect the nested-retain charge semantics.
The first retain adds one context charge. Nested retains only increase the retain count. The final successful release removes the charge. Preserve retry behavior for failed additions and removals.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/hostpid-broker-client.md` at line 57, Update the accounting behavior described around the retain/release state so only the first retain adds a CUDA context charge; nested retains must increment the retain count without adding another charge. Keep the final successful release responsible for removing the single charge, while preserving retry handling for failed additions and removals.
🧹 Nitpick comments (3)
src/multiprocess/multiprocess_memory_limit.c (1)
1086-1109: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRestore the timeout
errnoon the unlock-failure path.At line 1090 the code saves
errno(ETIMEDOUTfrompostinit_deadline_remaining). On the successful-unlock path at line 1102 it restores that value. On the failed-unlock path at lines 1098-1099 it returns without restoring, so the caller observes theerrnothatpostinit_file_lock_untilleft behind. Callers such astest_same_process_live_holder_timeoutandsrc/libvgpu.cbranch on the timeouterrno.♻️ Proposed change
if (!postinit_file_lock_until(F_UNLCK, NULL)) { /* * Do not release the process-local guard while the * process-wide record lock may still be held. This leaves * the process failed closed instead of allowing overlap. */ postinit_lock_held = 1; + errno = saved_errno; return 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/multiprocess/multiprocess_memory_limit.c` around lines 1086 - 1109, Update the failed-unlock branch in the deadline handling of the postinit lock acquisition function to restore the saved timeout errno before returning, just as the successful-unlock path does. Preserve postinit_lock_held and the failed-closed behavior while ensuring callers observe the original timeout errno when postinit_file_lock_until fails.test/test_hostpid_fallback_race.c (1)
250-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the cache files that were already created when
create_cache_filesfails.
create_cache_filesreturns-1as soon as oneopencall fails, and it leaves the files it already created on disk.run_casethen callsrmdir(directory)at line 295, which fails because the directory is not empty. Each failure leaks a temporary directory under/tmp.♻️ Proposed change
if (create_cache_files(directory, cache_count, cache_paths) != 0) { + remove_cache_files(cache_paths, cache_count); rmdir(directory); return -1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_hostpid_fallback_race.c` around lines 250 - 297, Update run_case’s create_cache_files failure path to call remove_cache_files with the number of files successfully created (track or otherwise derive that count), then remove the temporary directory. Ensure partial cache files are cleaned up before rmdir so failed setup does not leak the directory.test/test_hostpid_fallback_lock.c (1)
45-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable probe helpers.
The repository invokes
test_hostpid_fallback_lockwithout probe arguments. Removeprobe_lock,probe_default_lock, and their argument branches unless an external harness requires them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_hostpid_fallback_lock.c` around lines 45 - 110, Remove the unused probe helpers probe_lock and probe_default_lock, along with the command-line argument branches that invoke them, from test_hostpid_fallback_lock. Keep only the no-argument execution path used by the repository, unless a confirmed external harness requires these probes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/hostpid-broker-client.md`:
- Line 41: Update the decision-table sentence describing postInit() so a
successful broker result selects the broker path only when
LIBVGPU_HOSTPID_BROKER is enabled; otherwise retain the cache-local fallback
behavior. Do not describe successful broker selection while the gate is disabled
unless explicitly labeling it as test-only.
- Line 35: Update the deadline description in the hostpid broker client
documentation to explicitly state that the monotonic transaction deadline is 500
ms and applies across connection, request write, and response read, including
when the response trickles in.
In `@test/CMakeLists.txt`:
- Around line 89-96: Increase the CTest timeout assigned by set_tests_properties
for hostpid_fallback_lock from 10 seconds to a value providing the same margin
as hostpid_fallback_race’s 30-second alarm and 35-second timeout, so the
sequential bounded sub-tests complete reliably under CI load.
In `@test/test_hostpid_fallback_lock.c`:
- Around line 464-513: Increase the holder’s post-release sleep duration in
test_deadline_not_renewed, specifically the hold timespec used before
hostpid_fallback_lock_release, so it exceeds the waiter’s 80 ms timeout by a
substantially wider margin. Leave the synchronization and child_timeout behavior
unchanged.
In `@test/test_postinit_owner_death.c`:
- Around line 505-552: Reset ready[0] to -1 immediately after closing it in the
cleanup path of test/test_postinit_owner_death.c lines 505-552, preventing fail
cleanup from closing it again. In test/test_hostpid_fallback_lock.c lines
577-608, initialize ready as {-1, -1} and return early if pipe(ready) fails
before forking or using the descriptors.
- Around line 655-659: Update the cleanup block in test_postinit_owner_death to
release the postinit lock when cache_acquired is true by calling
unlock_postinit() before other cleanup actions, ensuring unexpected success from
lock_postinit_deadline does not retain the lock for subsequent tests.
---
Outside diff comments:
In `@docs/hostpid-broker-client.md`:
- Line 57: Update the accounting behavior described around the retain/release
state so only the first retain adds a CUDA context charge; nested retains must
increment the retain count without adding another charge. Keep the final
successful release responsible for removing the single charge, while preserving
retry handling for failed additions and removals.
---
Nitpick comments:
In `@src/multiprocess/multiprocess_memory_limit.c`:
- Around line 1086-1109: Update the failed-unlock branch in the deadline
handling of the postinit lock acquisition function to restore the saved timeout
errno before returning, just as the successful-unlock path does. Preserve
postinit_lock_held and the failed-closed behavior while ensuring callers observe
the original timeout errno when postinit_file_lock_until fails.
In `@test/test_hostpid_fallback_lock.c`:
- Around line 45-110: Remove the unused probe helpers probe_lock and
probe_default_lock, along with the command-line argument branches that invoke
them, from test_hostpid_fallback_lock. Keep only the no-argument execution path
used by the repository, unless a confirmed external harness requires these
probes.
In `@test/test_hostpid_fallback_race.c`:
- Around line 250-297: Update run_case’s create_cache_files failure path to call
remove_cache_files with the number of files successfully created (track or
otherwise derive that count), then remove the temporary directory. Ensure
partial cache files are cleaned up before rmdir so failed setup does not leak
the directory.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c94164c5-9d80-4a8c-8c05-8da82c7e0fc5
📒 Files selected for processing (13)
docs/hostpid-broker-client.mdsrc/CMakeLists.txtsrc/hostpid_fallback_lock.csrc/include/hostpid_fallback_lock.hsrc/include/utils.hsrc/libvgpu.csrc/multiprocess/multiprocess_memory_limit.csrc/multiprocess/multiprocess_memory_limit.hsrc/utils.ctest/CMakeLists.txttest/test_hostpid_fallback_lock.ctest/test_hostpid_fallback_race.ctest/test_postinit_owner_death.c
💤 Files with no reviewable changes (2)
- src/include/utils.h
- src/utils.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CMakeLists.txt
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
|
resolve conflicts |
…tion-local Signed-off-by: iemAnshuman <asquare567@gmail.com> # Conflicts: # src/multiprocess/multiprocess_memory_limit.c # test/CMakeLists.txt
conflicts are resolved. If this PR is too large for review, I can split it into smaller prs. |
|
Thanks for the work. Please split this into smaller, focused PRs. It would also be best to provide a complete HAMi end-to-end test covering the real workload path, not only |
makes sense. converting to draft and splitting into context accounting, fallback lock, broker client, then the integration. |
3 participants
Related issue: Project-HAMi/HAMi#1662
Design discussion: Project-HAMi/HAMi#2244
Companion server PR: Project-HAMi/HAMi#2417
Current companion server head:
146ee027b8b0ac44c8f8b3d48581f2cf69a485e9What this changes
This adds an optional host PID broker client to HAMi-core. When
LIBVGPU_HOSTPID_BROKER=1,postInit()asks the HAMi device plugin for the caller's host PID over a Unix socket. A valid response avoids the temporary CUDA primary context probe used by the NVML discovery path.The client accepts only
/tmp/vgpulock/hostpid/broker.sock. It verifies the broker directory, read only mount, socket owner, server UID, protocol fields, deadline, and returned PID before accepting a response.The feature is disabled by default. A new HAMi-core build without the companion server continues through the NVML fallback.
Fallback correctness
The fallback must serialize NVML discovery across independent cache files because the NVML snapshots are node wide. The correction takes one node wide lock on the trusted broker mount before the existing cache record lock. It never accepts a guessed PID.
If the broker gate is enabled but the trusted mount is missing or unsafe, host PID discovery fails clearly. It does not return to cache local discovery, which could accept a concurrently starting peer PID.
The final path hardening also checks every path component, the supported filesystem policy, permission changes while a waiter is blocked, object replacement, and one absolute deadline shared by the node and cache locks.
CUDA context accounting
Removing the temporary probe also removes its context size measurement. The broker path measures memory when the application first retains the real primary context.
Accounting maps visible CUDA ordinals to physical NVML devices by PCI identity. It keeps retain and charge state for each visible device, clears inherited state after
fork(), charges nested retains once, and removes the charge after the final successful release.Validation
Current client head
4421b0970c172e343c7fb59491952b78ea9d86ebpasses the GitHub library build, hook consistency, cpplint, DCO, CodeRabbit, dependency, license, and security checks.Exact public heads
8d59bf9and935a6decompleted all 16 balanced eight GPUcuInit()cells in job181726. Mean API wall time changed from 56.373 to 22.501 seconds at N=128 and from 130.662 to 50.288 seconds at N=300. Every worker completed. This measurescuInit()only.Jobs
181845and182609show the remaining driver cost. Without admission, the broker condition took 155.152 seconds at N=128 and 792.573 seconds at N=240. The N=300 cell reached its 903.11 second deadline. All 300 broker queries completed with zero broker failures, so the timeout was after PID discovery.Fallback job
182607tested revisionb0df70awith one and eight GPU layouts at N=128 and with N=300. Every candidate cell recorded zero peer and zero external PID mappings. The candidate failed discovery clearly for 71 of 128, 116 of 128, and 290 of 300 workers in those three cells. These are correctness results, not completion or performance claims.The fallback lock tests cover owner death, live holder timeout, independent caches,
fork(),exec(), and overlap. A 300 waiter owner death run completed with no premature or failed waiter.The Linux root path hardening run completed 28 recorded checks with zero failures. Its archive SHA256 is
941d0a7c3de6e4602a8c57f3e71b8c21ad3acc60cef127d07599ef8ed6f7783a. The archive covers the hardening source through607d7e83c282f8c494b39a0c2d92e16a83bdf965. Later review and lint commits at the current public head are outside that archive.The source and evidence package passes its checksum, semantic, archive, and adversarial verifier suites. It reports 46 verified artifacts, 0 pending artifacts, and 0 failures. The package ledger SHA256 is
294637ae0abbe69115c9f900442029fa0ccb335ec08ad3e8982d17f8dea1448a.The interim validation report records the source boundaries, A100 results, fallback result, Linux archive, and remaining Kubernetes checks.
Remaining work
Real kubelet allocation with the broker enabled and disabled, DaemonSet rollout and process restart, rollback, and degradation when the socket is unavailable remain untested on an isolated NVIDIA Kubernetes node.
Additional container runtimes and other GPU and driver versions remain untested. The A100 result above predates the later fallback hardening commits at the current public head.
The broker removes PID discovery from the serialized path, but it does not remove primary context contention. Driver admission is an optional separate follow up and remains disabled by default.
User facing change
This adds the
LIBVGPU_HOSTPID_BROKER=1opt in. Default behavior remains unchanged.AI assistance
I used some AI assistance for this.