Re: [PATCH] mm: shmem: fix khugepaged activation policy for shmem

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2024/9/6 16:55, Ryan Roberts wrote:
> On 06/09/2024 06:28, Baolin Wang wrote:
>> Shmem has a separate interface (different from anonymous pages) to control
>> huge page allocation, that means shmem THP can be enabled while anonymous
>> THP is disabled. However, in this case, khugepaged will not start to collapse
>> shmem THP, which is unreasonable.
>>
>> To fix this issue, we should call start_stop_khugepaged() to activate or
>> deactivate the khugepaged thread when setting shmem mTHP interfaces.
>> Moreover, add a new helper shmem_hpage_pmd_enabled() to help to check
>> whether shmem THP is enabled, which will determine if khugepaged should
>> be activated.
>>
>> Reported-by: Ryan Roberts <ryan.roberts@arm.com>
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> ---
>> Hi Ryan,
>>
>> I remember we discussed the shmem khugepaged activation issue before, but
>> I haven’t seen any follow-up patches to fix it. Recently, I am trying to
>> fix the shmem mTHP collapse issue in the series [1], and I also addressed
>> this activation issue. Please correct me if you have a better idea. Thanks.
> 
> Thanks for for sorting this - it looks like a good approach to me! Just a couple
> of nits. Regardless:
> 
> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>

Thanks for reviewing.

> 
>>
>> [1] https://lore.kernel.org/all/cover.1724140601.git.baolin.wang@linux.alibaba.com/T/#u
>> ---
>>   include/linux/shmem_fs.h |  6 ++++++
>>   mm/khugepaged.c          |  2 ++
>>   mm/shmem.c               | 29 +++++++++++++++++++++++++++--
>>   3 files changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
>> index 515a9a6a3c6f..ee6635052383 100644
>> --- a/include/linux/shmem_fs.h
>> +++ b/include/linux/shmem_fs.h
>> @@ -114,6 +114,7 @@ int shmem_unuse(unsigned int type);
>>   unsigned long shmem_allowable_huge_orders(struct inode *inode,
>>   				struct vm_area_struct *vma, pgoff_t index,
>>   				loff_t write_end, bool shmem_huge_force);
>> +bool shmem_hpage_pmd_enabled(void);
>>   #else
>>   static inline unsigned long shmem_allowable_huge_orders(struct inode *inode,
>>   				struct vm_area_struct *vma, pgoff_t index,
>> @@ -121,6 +122,11 @@ static inline unsigned long shmem_allowable_huge_orders(struct inode *inode,
>>   {
>>   	return 0;
>>   }
>> +
>> +static inline bool shmem_hpage_pmd_enabled(void)
>> +{
>> +	return false;
>> +}
>>   #endif
>>   
>>   #ifdef CONFIG_SHMEM
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index f9c39898eaff..caf10096d4d1 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -430,6 +430,8 @@ static bool hugepage_pmd_enabled(void)
>>   	if (test_bit(PMD_ORDER, &huge_anon_orders_inherit) &&
>>   	    hugepage_global_enabled())
>>   		return true;
>> +	if (shmem_hpage_pmd_enabled())
>> +		return true;
> 
> nit: There is a comment at the top of this function, perhaps that could be
> extended to cover shmem too?

Sure. How about the following changes?

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 945c0f2aff81..b0ac46ae561b 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -416,9 +416,11 @@ static inline int 
hpage_collapse_test_exit_or_disable(struct mm_struct *mm)
  static bool hugepage_pmd_enabled(void)
  {
         /*
-        * We cover both the anon and the file-backed case here; file-backed
+        * We cover the anon, shmem 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.
+        * Shmem pmd-sized hugepages are also determined by its pmd-size 
control,
+        * except when the global shmem_huge is set to SHMEM_HUGE_DENY.
          */
         if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) &&
             hugepage_global_enabled())

>>   	return false;
>>   }
>>   
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 74f093d88c78..d7c342ae2b37 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -1653,6 +1653,23 @@ static gfp_t limit_gfp_mask(gfp_t huge_gfp, gfp_t limit_gfp)
>>   }
>>   
>>   #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> +bool shmem_hpage_pmd_enabled(void)
>> +{
>> +	if (shmem_huge == SHMEM_HUGE_DENY)
>> +		return false;
>> +	if (test_bit(HPAGE_PMD_ORDER, &huge_shmem_orders_always))
> 
> question: When is it correct to use HPAGE_PMD_ORDER vs PMD_ORDER? I tend to use
> PMD_ORDER (in hugepage_pmd_enabled() for example).

In shmem, the HPAGE_PMD_* related macros are all controlled by 
CONFIG_TRANSPARENT_HUGEPAGE, and at this point, HPAGE_PMD_ORDER and 
PMD_ORDER are equal. I would like to keep consistency in the shmem code 
by using the HPAGE_PMD_* related macros.