fix: recover the post-init lock after process death

[iemAnshuman]

Problem

If a process is killed while holding sem_postinit, the semaphore stays locked. On a node with four A100 GPUs, none of the 4 waiters and none of the 16 waiters acquired it. Every waiter reached the retry limit after waiting between 327.4 and 328.0 seconds.

When this happens in postInit(), the code sets pidfound = 0. Later CUDA kernel launches then skip SM rate limiting.

Raw logs, commands, and checksums

Change

This change uses a POSIX record lock on one byte of the existing shared cache file. The lock protects host PID detection in postInit(). It is released when the owner
process exits, including after SIGKILL.

The lock uses a fixed byte outside the mapped shared region. A static assertion makes the build fail if shared_region_t grows to that byte. sem_postinit remains in shared_region_t, so its size and field positions do not change.

POSIX record locks belong to a process rather than one thread. A local guard inside each process keeps its threads in order. The fork() handler clears this local state in the child.

An earlier version used F_SETLKW. On the tested NFSv4 mount, some blocked calls took 30 or 60 seconds to wake after the owner died. Other waiters had already acquired and released the lock.

The final version uses repeated F_SETLK calls that do not block. Each waiter pauses for a slightly different time after a busy result. The pause starts at about 10 milliseconds and grows to at most 1 second. This avoids relying on delayed NFS wake events and reduces the chance that many waiters retry at the same time.

The code retries when fcntl() returns EACCES or EAGAIN. There is no time limit for normal contention. Other errors are logged and returned. Normal contention can no longer reach the old 330 second path that disables host PID detection and SM rate limiting.

Test

Added a CTest that uses the production shared region code and does not need a GPU. It does the following:

1. It queues two waiters behind a live owner and confirms that neither enters early.

2. It sends SIGKILL to the original owner and confirms that one waiter acquires the lock.

3. It sends SIGKILL to that waiter and confirms that the second waiter acquires and releases the lock.

4. It confirms that threads in the same process never enter at the same time.

5. It acquires the lock again to confirm that it remains usable.

Linux validation is complete:

1. The full Linux and CUDA build passed.

2. CTest passed once through the build and in 10 more direct runs.

3. The edited source produced no compiler warnings.

4. Focused Clang checks showed no findings on the changed lines.

5. On the NFSv4 cache path, all waiters recovered in 3 of 3 runs with 4 waiters, 10 of 10 runs with 16 waiters, and 3 of 3 runs with 64 waiters.

6. Stress runs recovered 300 of 300 waiters in 0.4509 seconds and 512 of 512 waiters in 1.1021 seconds.

7. A traced run with 16 waiters recorded 101 F_SETLK calls and no F_SETLKW calls at the selected lock byte.

8. The whitespace and checksum checks passed.

Upgrade requirement

All processes that share one cache file must use the same locking method. A binary that only uses the semaphore does not take the record lock. If old and new processes
run together, both could enter the protected section at the same time.

Drain the node or restart the affected processes together. Do not use a rolling upgrade while old and new processes share the cache.

AI disclosure

I used Chatgpt while analyzing the lock protocol, and creating the test. I reviewed the final diff and take responsibility for it.

Refs Project-HAMi/HAMi#1662 and #243.

Summary by CodeRabbit

• New Features

  • Improved coordination during concurrent initialization across processes and threads.
  • Initialization locks now recover safely when a process terminates unexpectedly.
  • Lock reuse and cleanup are handled more reliably after interruptions.
  • Lock acquisition now reports unexpected failures more accurately.

• Tests

  • Added regression coverage for process termination, recovery, serialization, and lock reuse.
  • Enabled the new tests through the standard build and test workflow.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4a464047-84ad-4ff7-9696-dd1c5874ee0b

📥 Commits

Reviewing files that changed from the base of the PR and between 2b77f89 and baec149.

📒 Files selected for processing (2)

• src/libvgpu.c
• src/multiprocess/multiprocess_memory_limit.c

🚧 Files skipped from review as they are similar to previous changes (1)

• src/libvgpu.c

---

📝 Walkthrough

Walkthrough

The post-initialization lock now uses a POSIX record lock with local process and thread guards. Child reinitialization clears lock state. New GPU-free CTest coverage verifies owner-death recovery and thread serialization.

Changes

Post-init locking

Layer / File(s)	Summary
Record-lock state and helpers src/multiprocess/multiprocess_memory_limit.c	Adds a fixed lock offset, POSIX record-lock helpers, an atomic process guard, and thread-local ownership state.
Post-init lifecycle integration src/multiprocess/multiprocess_memory_limit.c, src/multiprocess/multiprocess_memory_limit.h, src/libvgpu.c	Replaces semaphore timeout handling with record-lock acquisition and ownership checks. Child reinitialization clears lock state. Comments and logging describe lock errors.
Owner-death and thread-serialization validation test/test_postinit_owner_death.c, test/CMakeLists.txt, CMakeLists.txt	Adds process and thread regression tests. Builds and registers the GPU-free test with CTest and enables testing globally.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant postInit
  participant lock_postinit
  participant LocalGuard
  participant RecordLock
  postInit->>lock_postinit: acquire post-init lock
  lock_postinit->>LocalGuard: serialize threads in the process
  LocalGuard->>RecordLock: acquire fixed-offset fcntl lock
  RecordLock-->>lock_postinit: return lock status
  lock_postinit-->>postInit: continue or report lock error
  postInit->>lock_postinit: release post-init lock
  lock_postinit->>RecordLock: release fixed-offset lock

Loading

Possibly related PRs

• Project-HAMi/HAMi-core#247: Both PRs address post-initialization lock contention, but this PR changes locking behavior while that PR adds a cuInit() contention benchmark.

Suggested labels: enhancement

Suggested reviewers: chaunceyjiang, archlitchi

Poem

A rabbit guards the lock tonight,
With record hops both firm and right.
If owners vanish, waiters spring,
Threads take turns in orderly rings.
CTest thumps its happy feet.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: recovery of the post-init lock after process death.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@src/libvgpu.c`:
- Around line 897-907: Update the lock failure branch in the postinit handling
around lock_postinit() to assign NVML_ERROR_UNKNOWN instead of
NVML_ERROR_TIMEOUT, while preserving the warning log and successful
set_task_pid()/unlock_postinit() flow.

In `@test/test_postinit_owner_death.c`:
- Around line 361-406: Add the declaration header for log_utils_init by
including src/include/log_utils.h alongside the existing includes in
test_postinit_owner_death.c. Ensure this include is present before main calls
log_utils_init(), without changing the test logic.

🪄 Autofix (Beta)

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: 91d1b4d9-d34b-43c2-a712-58015882370c

📥 Commits

Reviewing files that changed from the base of the PR and between 52f33fc and 2b77f89.

📒 Files selected for processing (6)

• CMakeLists.txt
• src/libvgpu.c
• src/multiprocess/multiprocess_memory_limit.c
• src/multiprocess/multiprocess_memory_limit.h
• test/CMakeLists.txt
• test/test_postinit_owner_death.c

[maverick123123]

Tested this on a 8×RTX 3090 node (driver 580.126.20, CUDA 13.3 Docker).

Test results:

Test	Result
ctest -R postinit_owner_death	✅ passed
64-waiter stress test × 3 runs (owner SIGKILL → recovery)	✅ all waiters recovered, no overlap
vLLM v0.25.1 + Qwen3-0.6B model load	✅ normal
vLLM inference	✅ output correct

The stress test verified that after the lock holder is killed, all 64 concurrent waiters recover correctly and the lock remains reusable post-recovery. The vLLM inference test confirmed no regression in real GPU workload scenarios.

[archlitchi]

/lgtm

[hami-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: archlitchi, iemAnshuman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [archlitchi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Raja-89]

Hi @iemAnshuman , I did some follow-up measurements on the record-lock acquisition strategy in this PR, specifically the choice of non-blocking F_SETLK + exponential backoff/jitter instead of F_SETLKW.

First, to be clear: the owner-death correctness fix itself looks sound in the case I tested. I am not reporting a failure to recover the lock. The question here is narrower: whether polling is the right default acquisition strategy for the normal local shared-region cache path.

The implementation currently explains the choice as:

Use nonblocking requests instead of F_SETLKW. Some NFS lock managers defer grants to blocked requests for tens of seconds after the owner exits even though fresh requests can acquire the released lock immediately.

That made me curious about the cost of this workaround on a normal local filesystem, especially because the shared-region/postInit path is already important to the high-density initialization behavior discussed in Project-HAMi/HAMi#1662.

1. Contention experiment

I compared two otherwise equivalent POSIX record-lock loops:

1. blocking acquisition with fcntl(..., F_SETLKW, ...)
2. non-blocking fcntl(..., F_SETLK, ...) with the same style of exponential backoff and jitter used by the merged implementation:
   • initial retry: 10 ms
   • exponential increase
   • maximum retry interval: 1 s

Each worker holds the lock for 300 ms to model a serialized expensive postInit operation. All workers start concurrently.

Results:

workers	F_SETLKW wall	F_SETLK + polling wall	polling overhead
8	2.409 s	4.102 s	+1.693 s / ~70%
128	38.427 s	41.895 s	+3.468 s / ~9.0%
300	90.071 s	94.685 s	+4.614 s / ~5.1%

Both modes completed with zero failures.

For the 300-worker run, the latency distribution was:

F_SETLKW

• samples: 300
• median wait: 44,885.143 ms
• p95 wait: 85,259.973 ms
• p99 wait: 88,861.559 ms
• max wait: 89,761.914 ms
• p99 total: 89,161.635 ms
• max total: 90,061.994 ms

F_SETLK + polling

• samples: 300
• median wait: 45,507.885 ms
• p95 wait: 87,397.894 ms
• p99 wait: 92,042.136 ms
• max wait: 94,377.223 ms
• p99 total: 92,343.682 ms
• max total: 94,677.266 ms

So at high N the unavoidable serialized 300 ms critical section dominates, but polling still adds measurable tail/wall latency. At smaller N the proportional difference is considerably larger.

I don't think these numbers by themselves establish a serious regression; they mainly show that the polling policy has a real cost on the local-filesystem case.

2. Owner-death recovery latency

I then tested the specific case this PR is intended to fix.

The owner acquires the record lock and is killed while holding it. A waiter is already contending for the same lock. I measured how long the waiter takes to acquire after owner death.

100 trials per mode:

F_SETLKW

• successful recoveries: 100/100
• min: 4.810 ms
• median: 5.438 ms
• p95: 12.709 ms
• p99: 22.459 ms
• max: 24.057 ms

F_SETLK + polling/backoff

• successful recoveries: 100/100
• min: 5.438 ms
• median: 25.294 ms
• p95: 138.109 ms
• p99: 155.644 ms
• max: 158.074 ms

So both mechanisms recovered correctly in all of these trials, but the polling implementation had substantially higher owner-death recovery latency, particularly in the tail:

• median: ~25.3 ms vs ~5.4 ms
• p99: ~155.6 ms vs ~22.5 ms

This behavior makes sense: after the kernel releases the dead owner's record lock, an F_SETLKW waiter can become runnable as part of the kernel-managed lock wait, whereas a process currently sleeping in the retry/backoff loop cannot attempt acquisition until that sleep finishes.

3. Existing regression test

I also looked through test/test_postinit_owner_death.c.

The test does a useful correctness check:

• initial process acquires the postInit lock
• two waiters start
• initial owner is SIGKILLed
• exactly one waiter must acquire
• that recovery owner is SIGKILLed too
• the final waiter must recover
• the surviving waiter unlocks
• the lock must remain reusable afterward

It also separately verifies same-process thread serialization, which is important because POSIX record locks are process-associated.

One thing the test currently doesn't exercise is recovery latency.

TEST_TIMEOUT_MS is 5000 ms and recovery is effectively checked with:

wait_for_counter(&state->process_acquired, ..., TEST_TIMEOUT_MS)

Therefore a waiter recovering in 20 ms, 150 ms, 1 second, or several seconds would all satisfy the same correctness assertion.

The test cache is also created from:

/tmp/hami-postinit-ownerdeath.XXXXXX

so the regression test itself is currently exercising a local /tmp-style cache rather than the NFS scenario motivating the polling behavior.

4. Question about the NFS tradeoff

I have not tested the NFS behavior described in the source comment yet, so I don't want to claim that F_SETLKW should simply replace the current implementation everywhere.

If some NFS lock managers really keep blocked F_SETLKW requests deferred for tens of seconds after owner death while new non-blocking requests can acquire immediately, the polling implementation clearly has a reason to exist for that environment.

What I'm wondering is whether that behavior needs to determine the default strategy for the normal local shared-region cache as well.

Would one of these approaches make sense?

1. Use F_SETLKW for known-local filesystems and retain F_SETLK + retry for filesystems where the lock-manager behavior requires it.

2. Keep the current implementation but reduce/cap the maximum polling interval for the local/default case.

3. Keep the implementation unchanged, but add an owner-death latency benchmark/regression measurement so future changes can quantify this tradeoff.

4. If NFS-backed shared-region cache files are an explicitly supported configuration, add an NFS test reproducing the delayed F_SETLKW grant described in the comment. That would give us a concrete basis for choosing between the two mechanisms.

I'm particularly interested in (4), because right now I can reproduce the local-filesystem cost of polling, but I have not independently reproduced the NFS failure mode that polling is protecting against.

If there is a specific NFS server/client configuration where the delayed-grant behavior was observed, I'm happy to reproduce the same setup and run both acquisition strategies against it.

For now my conclusion is only:

• record locks correctly solve the owner-death problem in these tests;
• both acquisition strategies recovered 100/100 times locally;
• F_SETLK + backoff has measurable contention overhead locally;
• the difference is much clearer for owner-death recovery latency (p99 ~155.6 ms vs ~22.5 ms in my 100-trial test);
• the existing correctness regression test's 5 s deadline does not expose that latency difference;
• I have not yet tested the NFS case that motivates avoiding F_SETLKW.

Happy to share the benchmark source/raw CSVs or adapt this into a repository benchmark if that would be useful.

[iemAnshuman]

@Raja-89 thanks for measuring this. Your /tmp benchmark is a fair lock strategy comparison on a local filesystem. It shows that the current polling policy adds delay. I also agree that the CTest checks correctness, but does not measure recovery time.

I checked the validation behind baec149. The first version used F_SETLKW. I tested it on volga:/uno/work/aagrawal, mounted as NFSv4.2 with local_lock=none. The corrected harness placed all 16 waiters on the target lock before killing the owner.

Every waiter eventually recovered in the three longer runs. However, the last waiter completed after about 30 seconds in one run and about 60 seconds in two runs.
Process snapshots showed the delayed waiters sleeping inside the blocking fcntl call after other waiters had already acquired and released the lock.

With the final F_SETLK polling code on the same NFS path, all 16 waiters recovered in 10 of 10 runs. This NFS result is why I changed the first version.

The two tests answer different questions. Your test measures the cost of polling on a local filesystem. Mine measures delayed wake behavior after owner death on one NFS setup. Together, they show that one policy may not be right for both filesystems.

I would test option 1 next. We could use F_SETLKW on known local filesystems and keep fresh F_SETLK attempts for NFS and unknown filesystems. Both methods use the same record lock byte, so they remain compatible. Before changing the code, I want to run the same harness on both paths and define which filesystems should use each method.

I would leave the global retry cap unchanged until we measure the NFS request rate with a smaller cap. I also agree that we should measure recovery latency in a separate benchmark. A strict CTest limit may be unreliable on shared CI runners.

Please share the benchmark source and raw CSV files. I can publish the NFS harness and traces, then run the same benchmark on both /tmp and the NFSv4.2 path.

[Raja-89]

@iemAnshuman thanks for the NFS results and for explaining the reason behind switching from F_SETLKW to fresh F_SETLK attempts.

I've published the /tmp owner-death benchmark source and the raw 100-run CSVs for both strategies in #250:

https://github.com/Raja-89/HAMi-core/tree/evidence/owner-death-benchmark/evidence/owner-death

The CSVs preserve the original per-run measurements; I only converted the original text output into mode,recovery_us,recovery_ms columns. The README keeps the scope limited to the local /tmp filesystem.

I agree that the /tmp result shouldn't be generalized to NFS. Your NFSv4.2 results make the filesystem-aware split worth testing next: F_SETLKW for known local filesystems while retaining fresh F_SETLK attempts for NFS and unknown filesystems.

I'll keep the retry-cap question separate until we have the NFS request-rate measurements. Once you publish the NFS harness/traces, I can use the same setup for further comparison.