-
Notifications
You must be signed in to change notification settings - Fork 21
fix(mm): handle concurrent page faults on same page #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a critical concurrency bug in the memory management module that caused multi-threaded applications to crash when concurrent page faults occurred on the same lazy-loaded page. The fix ensures that already-mapped pages with correct permissions are counted as successfully populated, preventing spurious "No pages populated" errors.
- Addresses race condition where Thread B faults on a page already mapped by Thread A
- Updates both COW and file-backed memory backends to count pre-mapped pages
- Prevents
populate()from incorrectly returning 0 for already-correctly-mapped pages
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| modules/axmm/src/backend/cow.rs | Added check to count already-mapped pages with correct permissions in COW backend's populate function |
| modules/axmm/src/backend/file.rs | Added check to count already-mapped pages with correct permissions in file backend's populate function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Mivik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the docs of BackendOps::populate accordingly
DONE |
Mivik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Tested and it seems to work.
Problem
I encountered this problem when adapting to ROS2.
Multi-threaded applications crash with "No pages populated" error when
threads concurrently access the same lazy-loaded page in COW mappings.
Scenario:
populate()returns 0 (treated as failure) → segmentation faultRoot Cause
In both
cow.rsandfile.rs, thepopulate()functions only countpages that need work (COW copy, permission upgrade, or new allocation),
but return 0 when a page is already correctly mapped by another thread.
Solution
In both backends, count already-mapped pages with correct permissions as
successful population.