-
Notifications
You must be signed in to change notification settings - Fork 175
feat(starboard): Add prctl wrapper for VMA naming #7308
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
base: main
Are you sure you want to change the base?
Conversation
This change introduces a wrapper for the `prctl` system call to support naming Virtual Memory Areas (VMAs) using `PR_SET_VMA_ANON_NAME`. When the underlying kernel does not support this feature, the wrapper provides a fallback mechanism that writes the VMA information to a temporary file (`/tmp/cobalt_vma_tags_<pid>.txt`). This allows for memory debugging and attribution even on systems without the latest kernel features. A new NPLB test, `PosixPrctlTest.SetVmaAnonName`, has been added to verify both the direct `prctl` call and the fallback mechanism. The `prctl` wrapper is now used in various parts of the codebase, including the partition allocator and persistent memory allocator, under the `IS_STARBOARD` build flag. Bug: 443798603
c1addf6 to
1759e97
Compare
| #define PR_PAC_GET_ENABLED_KEYS 61 | ||
|
|
||
| #if !defined(PR_SET_VMA) | ||
| #define PR_SET_VMA 0x53564d41 |
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.
This seems like a magic number - what's the origin of it ? Does upstream musl have this defined ?
EDIT: Actually i checked, it's not there. I'd recommend making a pull request to musl to add it: https://github.com/kraj/musl/blob/kraj/master/include/sys/prctl.h
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.
I get this number from "linux/prctl.h", and it should be consistent to all Linux kernel systems.
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.
@kaidokert I created this PR to update upstream code: kraj/musl#9, though I'm not sure how is the reviewing process going, and I can not add reviewers.
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.
They replied - they need patches emailed to mailing list. Different contribution workflow
|
Friendly ping.. |
|
/generate-commit-message |
🤖 Gemini Suggested Commit Message |
|
We would like to get this in so that smaps works on evergreen-rdk platform : https://issuetracker.google.com/454095852 RDK - Memory leak during endurance tests on 26.eap talked to @johnedocampo - john already added multiple functions under prctl - so vma(being added in this PR) would be an addition to that . John also mentioned some changes will be needed on this PR to make it work. @sherryzy when you are back - it'll be good to co-ordinate with John and get this in. |
| // Kernel does not support PR_SET_VMA_ANON_NAME. Fallback to writing to a | ||
| // file. | ||
| char file_path[256]; | ||
| snprintf(file_path, sizeof(file_path), "/tmp/cobalt_vma_tags_%d.txt", |
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.
I find a few things at issue with this:
- The base name of the file should be a const string so that it doesn't have to be repeated in the different places where it's needed.
- The path should not be hardcoded to /tmp because tempdir may not be /tmp on all devices,
- IMHO this logic should live above Starboard in the Cobalt layer wrapper.
- The file should also not be just stored in tmpdir because tmpdir system-wide readable for all processes, and we don't know if the umask is set to block others from reading the file, and this information should probably no tbe exposed to every process on the device. A good alternative is probably
mkstemps(we can add the implementation from musl).
This change introduces a wrapper for the
prctlsystem call to support naming Virtual Memory Areas (VMAs) usingPR_SET_VMA_ANON_NAME.When the underlying kernel does not support this feature, the wrapper provides a fallback mechanism that writes the VMA information to a temporary file (
/tmp/cobalt_vma_tags_<pid>.txt). This allows for memory debugging and attribution even on systems without the latest kernel features.A new NPLB test,
PosixPrctlTest.SetVmaAnonName, has been added to verify both the directprctlcall and the fallback mechanism.The
prctlwrapper is now used in various parts of the codebase, including the partition allocator and persistent memory allocator, under theIS_STARBOARDbuild flag.Bug: 443798603, 444471844