On 04/07/2024 19:33, Andrew Morton wrote:
On Thu, 4 Jul 2024 10:10:50 +0100 Ryan Roberts ryan.roberts@arm.com 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
...
-static inline bool hugepage_flags_enabled(void) +static inline bool hugepage_pmd_enabled(void) { /*
* We cover both the anon and the file-backed case here; we must return* true if globally enabled, even when all anon sizes are set to never.* So we don't need to look at huge_anon_orders_inherit.
* We cover both the anon and the file-backed case here; file-backed* hugepages, when configured in, are determined by the global control.* Anon pmd-sized hugepages are determined by the pmd-size control.
- return hugepage_global_enabled() ||
READ_ONCE(huge_anon_orders_always) ||READ_ONCE(huge_anon_orders_madvise);
- 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());}
That's rather a mouthful. Is this nicer?
Sure, I'll take your version into v3.
static inline bool hugepage_pmd_enabled(void) { /* * We cover both the anon and the file-backed case here; file-backed * hugepages, when configured in, are determined by the global control. * Anon pmd-sized hugepages are determined by the pmd-size control. */ if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && hugepage_global_enabled()) return true; if (test_bit(PMD_ORDER, &huge_anon_orders_always)) return true; if (test_bit(PMD_ORDER, &huge_anon_orders_madvise)) return true; if (test_bit(PMD_ORDER, &huge_anon_orders_inherit) && hugepage_global_enabled()) return true; return false; }
Also, that's a pretty large function to be inlined. It could be a non-inline function static to khugepaged.c. But I suppose that's a separate patch.
Yeah fair point. I'll respin it now as a static in khugepaged.c.