Re: [PATCH v1 1/2] mm: Cleanup count_mthp_stat() definition

[Lance Yang]

On Thu, Jul 11, 2024 at 3:29 PM Ryan Roberts <ryan.roberts@xxxxxxx> wrote:
>
> Let's move count_mthp_stat() so that it's always defined, even when THP
> is disabled. Previously uses of the function in files such as shmem.c,
> which are compiled even when THP is disabled, required ugly THP
> ifdeferry. With this cleanup, we can remove those ifdefs and the
> function resolves to a nop when THP is disabled.
>
> I shortly plan to call count_mthp_stat() from more THP-invariant source
> files.
>
> Signed-off-by: Ryan Roberts <ryan.roberts@xxxxxxx>

LGTM. Feel free to add:

Reviewed-by: Lance Yang <ioworker0@xxxxxxxxx>


Thanks,
Lance

> ---
>  include/linux/huge_mm.h | 70 ++++++++++++++++++++---------------------
>  mm/memory.c             |  2 --
>  mm/shmem.c              |  6 ----
>  3 files changed, 35 insertions(+), 43 deletions(-)
>
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index cff002be83eb..cb93b9009ce4 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -108,6 +108,41 @@ extern struct kobj_attribute thpsize_shmem_enabled_attr;
>  #define HPAGE_PUD_MASK (~(HPAGE_PUD_SIZE - 1))
>  #define HPAGE_PUD_SIZE ((1UL) << HPAGE_PUD_SHIFT)
>
> +enum mthp_stat_item {
> +       MTHP_STAT_ANON_FAULT_ALLOC,
> +       MTHP_STAT_ANON_FAULT_FALLBACK,
> +       MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE,
> +       MTHP_STAT_SWPOUT,
> +       MTHP_STAT_SWPOUT_FALLBACK,
> +       MTHP_STAT_SHMEM_ALLOC,
> +       MTHP_STAT_SHMEM_FALLBACK,
> +       MTHP_STAT_SHMEM_FALLBACK_CHARGE,
> +       MTHP_STAT_SPLIT,
> +       MTHP_STAT_SPLIT_FAILED,
> +       MTHP_STAT_SPLIT_DEFERRED,
> +       __MTHP_STAT_COUNT
> +};
> +
> +#if defined(CONFIG_TRANSPARENT_HUGEPAGE) && defined(CONFIG_SYSFS)
> +struct mthp_stat {
> +       unsigned long stats[ilog2(MAX_PTRS_PER_PTE) + 1][__MTHP_STAT_COUNT];
> +};
> +
> +DECLARE_PER_CPU(struct mthp_stat, mthp_stats);
> +
> +static inline void count_mthp_stat(int order, enum mthp_stat_item item)
> +{
> +       if (order <= 0 || order > PMD_ORDER)
> +               return;
> +
> +       this_cpu_inc(mthp_stats.stats[order][item]);
> +}
> +#else
> +static inline void count_mthp_stat(int order, enum mthp_stat_item item)
> +{
> +}
> +#endif
> +
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>
>  extern unsigned long transparent_hugepage_flags;
> @@ -263,41 +298,6 @@ struct thpsize {
>
>  #define to_thpsize(kobj) container_of(kobj, struct thpsize, kobj)
>
> -enum mthp_stat_item {
> -       MTHP_STAT_ANON_FAULT_ALLOC,
> -       MTHP_STAT_ANON_FAULT_FALLBACK,
> -       MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE,
> -       MTHP_STAT_SWPOUT,
> -       MTHP_STAT_SWPOUT_FALLBACK,
> -       MTHP_STAT_SHMEM_ALLOC,
> -       MTHP_STAT_SHMEM_FALLBACK,
> -       MTHP_STAT_SHMEM_FALLBACK_CHARGE,
> -       MTHP_STAT_SPLIT,
> -       MTHP_STAT_SPLIT_FAILED,
> -       MTHP_STAT_SPLIT_DEFERRED,
> -       __MTHP_STAT_COUNT
> -};
> -
> -struct mthp_stat {
> -       unsigned long stats[ilog2(MAX_PTRS_PER_PTE) + 1][__MTHP_STAT_COUNT];
> -};
> -
> -#ifdef CONFIG_SYSFS
> -DECLARE_PER_CPU(struct mthp_stat, mthp_stats);
> -
> -static inline void count_mthp_stat(int order, enum mthp_stat_item item)
> -{
> -       if (order <= 0 || order > PMD_ORDER)
> -               return;
> -
> -       this_cpu_inc(mthp_stats.stats[order][item]);
> -}
> -#else
> -static inline void count_mthp_stat(int order, enum mthp_stat_item item)
> -{
> -}
> -#endif
> -
>  #define transparent_hugepage_use_zero_page()                           \
>         (transparent_hugepage_flags &                                   \
>          (1<<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG))
> diff --git a/mm/memory.c b/mm/memory.c
> index 802d0d8a40f9..a50fdefb8f0b 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4597,9 +4597,7 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
>
>         folio_ref_add(folio, nr_pages - 1);
>         add_mm_counter(vma->vm_mm, MM_ANONPAGES, nr_pages);
> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>         count_mthp_stat(folio_order(folio), MTHP_STAT_ANON_FAULT_ALLOC);
> -#endif
>         folio_add_new_anon_rmap(folio, vma, addr, RMAP_EXCLUSIVE);
>         folio_add_lru_vma(folio, vma);
>  setpte:
> diff --git a/mm/shmem.c b/mm/shmem.c
> index f24dfbd387ba..fce1343f44e6 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1776,9 +1776,7 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
>
>                         if (pages == HPAGE_PMD_NR)
>                                 count_vm_event(THP_FILE_FALLBACK);
> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>                         count_mthp_stat(order, MTHP_STAT_SHMEM_FALLBACK);
> -#endif
>                         order = next_order(&suitable_orders, order);
>                 }
>         } else {
> @@ -1803,10 +1801,8 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
>                                 count_vm_event(THP_FILE_FALLBACK);
>                                 count_vm_event(THP_FILE_FALLBACK_CHARGE);
>                         }
> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>                         count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK);
>                         count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK_CHARGE);
> -#endif
>                 }
>                 goto unlock;
>         }
> @@ -2180,9 +2176,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
>                 if (!IS_ERR(folio)) {
>                         if (folio_test_pmd_mappable(folio))
>                                 count_vm_event(THP_FILE_ALLOC);
> -#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>                         count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_ALLOC);
> -#endif
>                         goto alloced;
>                 }
>                 if (PTR_ERR(folio) == -EEXIST)
> --
> 2.43.0
>