[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <58cf63c1-25e5-4958-96cb-a9d65390ca3e@arm.com>
Date: Fri, 6 Sep 2024 09:55:57 +0100
From: Ryan Roberts <ryan.roberts@....com>
To: Baolin Wang <baolin.wang@...ux.alibaba.com>, akpm@...ux-foundation.org,
hughd@...gle.com
Cc: willy@...radead.org, david@...hat.com, 21cnbao@...il.com,
linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: shmem: fix khugepaged activation policy for shmem
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@....com>
> Signed-off-by: Baolin Wang <baolin.wang@...ux.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@....com>
>
> [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?
> 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).
> + return true;
> + if (test_bit(HPAGE_PMD_ORDER, &huge_shmem_orders_madvise))
> + return true;
> + if (test_bit(HPAGE_PMD_ORDER, &huge_shmem_orders_within_size))
> + return true;
> + if (test_bit(HPAGE_PMD_ORDER, &huge_shmem_orders_inherit) &&
> + shmem_huge != SHMEM_HUGE_NEVER)
> + return true;
> +
> + return false;
> +}
> +
> 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)
> @@ -5036,7 +5053,7 @@ static ssize_t shmem_enabled_store(struct kobject *kobj,
> struct kobj_attribute *attr, const char *buf, size_t count)
> {
> char tmp[16];
> - int huge;
> + int huge, err;
>
> if (count + 1 > sizeof(tmp))
> return -EINVAL;
> @@ -5060,7 +5077,9 @@ static ssize_t shmem_enabled_store(struct kobject *kobj,
> shmem_huge = huge;
> if (shmem_huge > SHMEM_HUGE_DENY)
> SHMEM_SB(shm_mnt->mnt_sb)->huge = shmem_huge;
> - return count;
> +
> + err = start_stop_khugepaged();
> + return err ? err : count;
> }
>
> struct kobj_attribute shmem_enabled_attr = __ATTR_RW(shmem_enabled);
> @@ -5137,6 +5156,12 @@ static ssize_t thpsize_shmem_enabled_store(struct kobject *kobj,
> ret = -EINVAL;
> }
>
> + if (ret > 0) {
> + int err = start_stop_khugepaged();
> +
> + if (err)
> + ret = err;
> + }
> return ret;
> }
>
Powered by blists - more mailing lists