On 02.07.24 18:33, Ryan Roberts wrote:
On 02/07/2024 16:38, David Hildenbrand wrote:
On 02.07.24 17:29, Ryan Roberts wrote:
On 02/07/2024 15:57, David Hildenbrand wrote:
On 02.07.24 16:46, Ryan Roberts wrote:
Since the introduction of mTHP, the docuementation has stated that
khugepaged would be enabled when any mTHP size is enabled, and disabled
when all mTHP sizes are disabled. There are 2 problems with this; 1.
this is not what was implemented by the code and 2. this is not the
desirable behavior.
Desirable behavior is for khugepaged to be enabled when any PMD-sized
THP is enabled, anon or file. (Note that file THP is still controlled by
the top-level control so we must always consider that, as well as the
PMD-size mTHP control for anon). khugepaged only supports collapsing to
PMD-sized THP so there is no value in enabling it when PMD-sized THP is
disabled. So let's change the code and documentation to reflect this
policy.
Further, per-size enabled control modification events were not
previously forwarded to khugepaged to give it an opportunity to start or
stop. Consequently the following was resulting in khugepaged eroneously
not being activated:
echo never > /sys/kernel/mm/transparent_hugepage/enabled
echo always > /sys/kernel/mm/transparent_hugepage/hugepages-2048kB/enabled
Signed-off-by: Ryan Roberts <ryan.roberts@xxxxxxx>
Fixes: 3485b88390b0 ("mm: thp: introduce multi-size THP sysfs interface")
Closes:
https://lore.kernel.org/linux-mm/7a0bbe69-1e3d-4263-b206-da007791a5c4@xxxxxxxxxx/
Cc: stable@xxxxxxxxxxxxxxx
---
Hi All,
Applies on top of today's mm-unstable (9bb8753acdd8). No regressions
observed in
mm selftests.
When fixing this I also noticed that khugepaged doesn't get (and never has
been)
activated/deactivated by `shmem_enabled=`. I'm not sure if khugepaged knows how
to collapse shmem - perhaps it should be activated in this case?
Call me confused.
khugepaged_scan_mm_slot() and madvise_collapse() only all
hpage_collapse_scan_file() with ... IS_ENABLED(CONFIG_SHMEM) ?
Looks like khugepaged_scan_mm_slot() was converted from:
if (shmem_file(vma->vm_file)) {
to:
if (IS_ENABLED(CONFIG_SHMEM) && vma->vm_file) {
CONFIG_READ_ONLY_THP_FOR_FS depends on CONFIG_SHMEM in Kconfig, so I think this is all correct/safe. Although I'm not really sure what the need for the dependency is.
Right. It's likely some historical artifact ...
[...]
I'll do a v2 with this addition if you agree?
static inline bool hugepage_pmd_enabled(void)
{
/*
* We cover both the anon and the file-backed case here; for
* file-backed, we must return true if globally enabled, regardless of
* the anon pmd size control status.
*/
return (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && hugepage_global_enabled()) ||
test_bit(PMD_ORDER, &huge_anon_orders_always) ||
test_bit(PMD_ORDER, &huge_anon_orders_madvise) ||
(test_bit(PMD_ORDER, &huge_anon_orders_inherit) && hugepage_global_enabled());
}
I think this looks good :)
But back to my original question, I think hugepage_pmd_enabled() should also be
explicitly checking the appropriate shmem_enabled controls and ORing in the
result? Otherwise in a situation where only shmem is THP enabled (and file/anon
THP is disabled) khugepaged won't run.
I think so.
I'll do this part as a separate change since it's fixing a separate bug.
Makes sense.
The thp_vma_allowable_order() check tests if we are allowed to collapse a
PMD_ORDER in that VMA.
I don't follow the relevance of this statement.
On whatever VMA we indicate true, we will try to collapse in khugepaged.
Regarding the question, what khugepaged will try to collapse.
Oh I see. Yes agreed. But we don't have a VMA when enabling/disabling khugepaged. We just want to know "are there vmas for which khugepaged would attempt to collapse to PMD size?"
Yes, only to give you a pointer on which VMAs khugepaged will be willing
to work with.
--
Cheers,
David / dhildenb
