Add optional memory locking support#27
Conversation
Add backend capability initialization for memory stats and memory locking, with POSIX shared lock helpers and Windows VirtualLock/VirtualUnlock support. Expose --lock-memory/-l only when supported, keep help output aligned with backend capabilities, and reuse the memory summary printer for help and normal execution. Extend malloc-hook and CLI tests to cover chunk sizing and lock-memory behavior.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds memory locking capability to eatmemory with backend-aware feature detection. The refactored API introduces a ChangesMemory locking feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
kluster.ai PR Review SummaryThis PR introduces a memory locking feature to prevent the operating system from swapping allocated pages to disk. A new Key changes include:
The UI has also been updated to conditionally display help text and available memory stats based on what the current backend supports.
Powered by kluster.ai - Real-Time AI code review in your IDE |
![kluster-ai[bot]](https://avatars.githubusercontent.com/in/2988314?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@README.md`:
- Around line 92-97: Update the CLI flags table to include the short alias -l
alongside --lock-memory: modify the table row that currently shows
`--lock-memory` to list both `-l`, `--lock-memory` (or add a separate row for
`-l`) and keep the same description "Lock allocated pages in physical memory,
when supported." so the README flag table matches the CLI help output and
documents the short alias used by the program.
In `@src/main.c`:
- Around line 27-32: Always register the "lock-memory l" option regardless of
backend support (call ap_add_flag for "lock-memory l" in configure_cmd), but
keep the help text conditional; after argument parsing, check whether the
lock-memory flag was requested and if backend->memory_lock_supported is false
emit the EM_ERROR_MEMORY_LOCK_UNSUPPORTED error/exit code 22 (reject explicitly)
— update the post-parse validation logic (where you inspect parsed flags) to
detect the lock flag and fail when unsupported. Ensure you make the same change
in the other configure/parse site referenced (lines ~108-111).
In `@test/ci/simple/allocation.test.sh`:
- Around line 56-58: The "Lock small allocation" CI test currently asserts
successful locking by calling run_success with "-l 1K" and the "Done,
sleeping..." message which is flaky on systems with restrictive RLIMIT_MEMLOCK;
instead, change the test in allocation.test.sh to not assume locking succeeds:
either assert that the CLI exposes the lock flag/help (e.g., run_success or
run_cmd on the program with --help or -h and verify the -l/--lock option
appears) or gate the success-path assertion behind a runtime probe (use a quick
runtime check such as querying the current memlock limit via ulimit -l or a
small probe program and only expect the "Done, sleeping..." success when that
probe shows sufficient limit), leaving the actual lock-success behavior to the
malloc-hook tests; update references to the "Lock small allocation" test and
calls to run_success accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f13434dd-469d-4df8-89c4-adb90df493a2
📒 Files selected for processing (14)
README.mdsrc/eatmemory.aix.csrc/eatmemory.apple.csrc/eatmemory.csrc/eatmemory.hsrc/eatmemory.linux.csrc/eatmemory.posix-common.csrc/eatmemory.solaris.csrc/eatmemory.unknown.csrc/eatmemory.windows.csrc/errors.hsrc/main.ctest/ci/malloc/eatmemory_malloc_hook.test.ctest/ci/simple/allocation.test.sh
Greptile SummaryThis PR adds optional memory locking (
Confidence Score: 3/5The memory locking mechanics and cleanup paths are correct, but the migration away from per-call stats success tracking introduces a silent failure on every platform — a regression worth fixing before merge. The old design propagated runtime stat-call failure through a per-call supported field; the new design only checks a static init-time flag. When a backend's stat call fails at runtime, the function returns zeroed stats with no error indicator. Any %-based size argument silently resolves to 0 bytes instead of producing a parse error. src/eatmemory.c (string_to_bytes % path), src/eatmemory.aix.c, src/eatmemory.apple.c, src/eatmemory.linux.c, and src/eatmemory.windows.c all have the same runtime-failure-not-surfaced pattern. Important Files Changed
Reviews (1): Last reviewed commit: "Add optional memory locking support" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Document the -l alias in the README and make the cross-host CLI suite verify lock option exposure instead of requiring real page-lock success.
Rename backend initialization to eatmemory_get_backend_capabilities and make eatmemory_get_system_memory_stats return an explicit result enum. Reject percentage sizes when the runtime stats query fails, while keeping backend capabilities responsible for help rendering.
Keep the core allocation and cleanup functions named eat and digest while passing lock/progress options explicitly. Move lock attempts into the allocation loop and show progress for eating, verification, and freeing only on interactive terminals.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/eatmemory.linux.c`:
- Around line 15-29: In eatmemory_get_system_memory_stats, the sysconf check
treats free_pages > 0 as required, which wrongly marks a valid zero free-pages
state as failure; change the condition so pages and page_size remain checked for
> 0 but allow free_pages >= 0 (i.e., treat -1 as the only failure), then keep
computing stats->total and stats->free with pages_to_bytes_clamped and return
EM_STATS_OK when free_pages >= 0.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e1d13ff-dd73-4284-9bcb-fdf743c8b990
📒 Files selected for processing (13)
AGENTS.mdREADME.mdsrc/eatmemory.aix.csrc/eatmemory.apple.csrc/eatmemory.csrc/eatmemory.hsrc/eatmemory.linux.csrc/eatmemory.solaris.csrc/eatmemory.unknown.csrc/eatmemory.windows.csrc/main.ctest/ci/malloc/eatmemory_malloc_hook.test.ctest/ci/simple/allocation.test.sh
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
1 participant
Summary
--lock-memory/-l, shown only when the active backend supports page lockingmlock/munlocksupport and WindowsVirtualLock/VirtualUnlocksupportTests
git diff --checkbash test/ci/all.test.shCloses #12
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation