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

[David Hildenbrand]

On 06.09.24 10: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@xxxxxxx>
> > Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxx>
>
> > [1] https://lore.kernel.org/all/cover.1724140601.git.baolin.wang@xxxxxxxxxxxxxxxxx/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?
>
> >   	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).

I think the HPAGE_PMD_ORDER thingy originally was introduced to detect 
code that is supposed to handle THPs, but really cannot handle them 
properly -- e.g., no arch support, not enabled in the kernel config.

My take is that we should start removing this HPAGE_* stuff.

--
Cheers,

David / dhildenb