vmm: enable THP for anon shmem#7646
Conversation
Champ-Goblem
force-pushed
the
patch/enable-thp-for-shmem
branch
from
359a97f to
7712b85
Compare
There was a problem hiding this comment.
If hugetlbfs is already being used, does it make sense to also call MADV_HUGEPAGE?
There was a problem hiding this comment.
I have pushed a change which only tries THP when hugepages flag is disabled
There was a problem hiding this comment.
FWIW - thp defaults to true so this is probably being done right now away.
rbradford
left a comment
rbradford
left a comment
There was a problem hiding this comment.
Good catch - I want to improve our THP documentation and flow.
Champ-Goblem
force-pushed
the
patch/enable-thp-for-shmem
branch
from
7712b85 to
b53ac72
Compare
rbradford
left a comment
rbradford
left a comment
There was a problem hiding this comment.
I think my suggestion will make the meaning of thp vs enable_thp clearer.
| thp: bool, | ||
| ) -> Result<MmapRegion<AtomicBitmap>, Error> { | ||
| let mut mmap_flags = libc::MAP_NORESERVE; | ||
| let mut enable_thp = false; |
There was a problem hiding this comment.
What about let mut enable_thp = thp;
| // because the MAP_PRIVATE will trigger CoW against the backing file with | ||
| // the VFIO pinning | ||
| mmap_flags |= libc::MAP_SHARED; | ||
| enable_thp = !hugepages; |
| } | ||
|
|
||
| if region.file_offset().is_none() && thp { | ||
| if thp && (region.file_offset().is_none() || enable_thp) { |
There was a problem hiding this comment.
if enable_thp && region.file_offset().is_none()
There was a problem hiding this comment.
I think this would essentially be equivalent to the original if logic, because in the case of anon shmem, file_offset is Some, so THP would end up not being enabled. enable_thp || region.file_offset().is_none() would end up trying to enable THP even if the user did not pass thp=true?
So I think using your original suggested change or relabeling enable_thp to something like try_thp seems more appropriate?
There was a problem hiding this comment.
You're right. Let's call the variable thp_madvise and set it where appropriate - that's nice and explicit. Can you also move the info! statement outside that check - I don't think it should be thp gated.
There was a problem hiding this comment.
I have pushed a new commit that incorporates the above suggestions. Let me know if that looks good or if I missed anything. Thanks!
Champ-Goblem
force-pushed
the
patch/enable-thp-for-shmem
branch
from
b53ac72 to
4059160
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Champ-Goblem
force-pushed
the
patch/enable-thp-for-shmem
branch
from
4059160 to
482f1b3
Compare
The kernel allows madvise on shared memory if /sys/kernel/mm/transparent_hugepage/shmem_enabled is set. Always try and configure THP via madvise when the user requests THP be enabled. If this fails, only a warning log is emitted and THP won't be enabled. Signed-off-by: Champ-Goblem <cameron@northflank.com>
Champ-Goblem
force-pushed
the
patch/enable-thp-for-shmem
branch
from
482f1b3 to
ebc5a16
Compare
4 participants
The kernel allows madvise on shared memory if /sys/kernel/mm/transparent_hugepage/shmem_enabled is set. Try and configure THP via madvise when anonymous shared memory is used. If this fails, only a warning log is emitted and THP won't be enabled.