Re: [PATCH v2] mm: add per-order mTHP alloc_success and alloc_fail counters

[Barry Song <21cnbao@gmail.com>]

On Wed, Apr 3, 2024 at 9:13 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 02.04.24 23:29, Barry Song wrote:
> > On Wed, Apr 3, 2024 at 7:46 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 28.03.24 10:51, Barry Song wrote:
> >>> From: Barry Song <v-songbaohua@oppo.com>
> >>>
> >>> Profiling a system blindly with mTHP has become challenging due
> >>> to the lack of visibility into its operations. Presenting the
> >>> success rate of mTHP allocations appears to be pressing need.
> >>>
> >>> Recently, I've been experiencing significant difficulty debugging
> >>> performance improvements and regressions without these figures.
> >>> It's crucial for us to understand the true effectiveness of
> >>> mTHP in real-world scenarios, especially in systems with
> >>> fragmented memory.
> >>>
> >>> This patch sets up the framework for per-order mTHP counters,
> >>> starting with the introduction of alloc_success and alloc_fail
> >>> counters.  Incorporating additional counters should now be
> >>> straightforward as well.
> >>>
> >>> The initial two unsigned longs for each event are unused, given
> >>> that order-0 and order-1 are not mTHP. Nonetheless, this refinement
> >>> improves code clarity.
> >>>
> >>> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> >>> ---
> >>>    -v2:
> >>>    * move to sysfs and provide per-order counters; David, Ryan, Willy
> >>>    -v1:
> >>>    https://lore.kernel.org/linux-mm/20240326030103.50678-1-21cnbao@gmail.com/
> >>>
> >>>    include/linux/huge_mm.h | 17 +++++++++++++
> >>>    mm/huge_memory.c        | 54 +++++++++++++++++++++++++++++++++++++++++
> >>>    mm/memory.c             |  3 +++
> >>>    3 files changed, 74 insertions(+)
> >>>
> >>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> >>> index e896ca4760f6..27fa26a22a8f 100644
> >>> --- a/include/linux/huge_mm.h
> >>> +++ b/include/linux/huge_mm.h
> >>> @@ -264,6 +264,23 @@ unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
> >>>                                          enforce_sysfs, orders);
> >>>    }
> >>>
> >>> +enum thp_event_item {
> >>> +     THP_ALLOC_SUCCESS,
> >>> +     THP_ALLOC_FAIL,
> >>> +     NR_THP_EVENT_ITEMS
> >>> +};
>
> Why not simply "enum thp_event" ... "NR_THP_EVENT".

ok.

>
> >>
> >> I'm wondering if these should be ANON specific for now. We might want to
> >> add others (shmem, file) in the future.
> >
> > I've two ways to do that
> > 1. rename to ANON_THP_ALLOC, so that I can have SHMEM_THP_ALLOC, FILE_THP_ALLOC
> > in the future;
> > 2. let THP_ALLOC cover all of shmem, file and anon.
> >
> > following vmstat, actually 1 might be better as we have both THP_FAULT_ALLOC and
> > THP_FILE_ALLOC for pmd-mapped THP.
>
> Yes. Because anon was first, people just named it "THP". Then, file THP
> were added later. Some of that needs a cleanup.

agreed.

>
> >
> > #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >                  THP_FAULT_ALLOC,
> >                  THP_FAULT_FALLBACK,
> >                  THP_FAULT_FALLBACK_CHARGE,
> >                  THP_COLLAPSE_ALLOC,
> >                  THP_COLLAPSE_ALLOC_FAILED,
> >                  THP_FILE_ALLOC,
> >                  THP_FILE_FALLBACK,
> >                  THP_FILE_FALLBACK_CHARGE,
> >                  THP_FILE_MAPPED,
> >                  THP_SPLIT_PAGE,
> >                  THP_SPLIT_PAGE_FAILED,
> >                  THP_DEFERRED_SPLIT_PAGE,
> >                  THP_SPLIT_PMD,
> >                  THP_SCAN_EXCEED_NONE_PTE,
> >                  THP_SCAN_EXCEED_SWAP_PTE,
> >                  THP_SCAN_EXCEED_SHARED_PTE,
> > #ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
> >                  THP_SPLIT_PUD,
> > #endif
> >                  THP_ZERO_PAGE_ALLOC,
> >                  THP_ZERO_PAGE_ALLOC_FAILED,
> >                  THP_SWPOUT,
> >                  THP_SWPOUT_FALLBACK,
> > #endif
> >
> > And reading mm/shmem.c, obviously, shmem is using THP_FILE_ALLOC.
>
> Right.
>
> >
> > I will rename it to ANON_THP_ALLOC in v3, let me know if you disagree :-)
>
> You should give people more time to respond before resending. Please try
> sending new versions only after the discussion on the old version
> finished. Otherwise it's going to be a mess (because I won't repost my
> feedback to v3 :P ).

agreed :-)

>
> THP_EVENT_ANON_ALLOC
>
> might be better, so "THP_EVENT" would be your common prefix for "enum
> thp_event".
>

Should be ok. On the other hand, currently, there are numerous occurrences of
events or stats within mm, but we haven't incorporated "EVENT" or "STAT"
into the naming conventions.

> --
> Cheers,
>
> David / dhildenb
>

Thanks
Barry