symmetric_heap: fix huge pages mmap and silent fallback - #1227
Conversation
Addresses two bugs in mmap_alloc() when SHMEM_SYMMETRIC_HEAP_USE_HUGE_PAGES=1: 1. (HIGH) MAP_ANON|MAP_PRIVATE was used even when a valid hugetlbfs fd was open. On Linux, MAP_ANONYMOUS causes the kernel to ignore the fd, so huge pages were never actually used. Fix: use MAP_SHARED when fd > 0 so the mapping is backed by the hugetlbfs file. 2. (LOW) When find_hugepage_dir() found no matching hugetlbfs mount, the code silently fell back to regular pages with no diagnostic. Fix: emit RAISE_WARN_MSG so the user knows their request was not honored. Also move the Linux-specific variables (file_name, fd, directory) and the mmap dispatch inside the #ifdef __linux__ / #else / #endif block so all hugepage logic is cleanly scoped to Linux, with a plain anonymous mmap in the #else for all other platforms.
ftruncate() must be called on the hugetlbfs file descriptor before mmap() to set the file size. Without it, mmap() against a hugetlbfs fd can fail with EINVAL or produce unexpected behavior. Also add MAP_HUGETLB alongside MAP_SHARED to make the intent explicit and match the usage confirmed working on the target system. On ftruncate failure, unlink the file before falling back to regular anonymous pages to avoid leaving stale files in /dev/hugepages.
…oc warning Guard all three DISABLE_NONFETCH_AMO blocks in shmem_comm.h with USE_OFI to prevent compile errors (FI_ATOMIC_WRITE undeclared) and runtime errors (transport_none: No path to peer) in non-OFI builds. Also improve the mmap_alloc file open failure warning to include strerror(errno) so the cause (e.g. Permission denied) is visible.
markbrown314
left a comment
markbrown314
left a comment
There was a problem hiding this comment.
The cleanup code needs a little bit of work.
| strerror(errno)); | ||
| unlink(file_name); | ||
| close(fd); | ||
| free(directory); |
There was a problem hiding this comment.
The directory and file_name can be leaked if fd is zero. This should be moved to a cleanup section.
There is also a bug in this code outside of this PR. If stdin is not open it is possible that you can get an fd of 0. fd should be initialized to a negative number.
markbrown314
left a comment
markbrown314
left a comment
There was a problem hiding this comment.
Some issues with cleanups and error checks.
|
|
||
| #ifdef __linux__ | ||
| char *file_name = NULL; | ||
| int fd = 0; |
There was a problem hiding this comment.
Don't initialize to zero. Initialize to -1. It is possible (although unlikely to get a fd of zero after open if stdin is not open).
| } | ||
| } | ||
|
|
||
| if (fd) { |
There was a problem hiding this comment.
This should be a negative check. You would need to fix the code above in the fd < 0 error case that resets fd to 0 and just leave it negative.
| if (file_name) | ||
| unlink(file_name); | ||
| close(fd); | ||
| free(directory); |
There was a problem hiding this comment.
directory and file_name can leak.
| ret = mmap(requested_base, bytes, PROT_READ | PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0); | ||
| } | ||
| #else | ||
| ret = mmap(requested_base, bytes, PROT_READ | PROT_WRITE, |
There was a problem hiding this comment.
nitpick: there are a lot of mmaps in this code block. This could have been simplified by just using a flags variable. (set it to some defaults and fd set to -1 and if you want to use hugetlbfs set fd to reference hugepagefs file and set the flag to MAP_SHARED.
| bytes, strerror(errno), shmem_internal_my_pe); | ||
| ret = NULL; | ||
| } | ||
| if (fd) { |
There was a problem hiding this comment.
This part is fine other than the bogus fd != 0 check. It does not leak if you cleanup here.
2 participants
Addresses two bugs in mmap_alloc() when SHMEM_SYMMETRIC_HEAP_USE_HUGE_PAGES=1:
(HIGH) MAP_ANON|MAP_PRIVATE was used even when a valid hugetlbfs fd was open. On Linux, MAP_ANONYMOUS causes the kernel to ignore the fd, so huge pages were never actually used. Fix: use MAP_SHARED when fd > 0 so the mapping is backed by the hugetlbfs file.
(LOW) When find_hugepage_dir() found no matching hugetlbfs mount, the code silently fell back to regular pages with no diagnostic. Fix: emit RAISE_WARN_MSG so the user knows their request was not honored.
Also moved the Linux-specific variables (file_name, fd, directory) and the mmap dispatch inside the '#ifdef linux' block so all huge page logic is cleanly scoped to Linux, with a plain anonymous mmap in the #else for all other platforms.
Addresses Issue #1207