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

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

On Thu, Apr 4, 2024 at 12:48 AM Ryan Roberts <ryan.roberts@arm.com> wrote:
>
> On 03/04/2024 09:22, David Hildenbrand wrote:
> > On 03.04.24 05:55, 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 anon_alloc_success and
> >> anon_alloc_fail counters.  Incorporating additional counters
> >> should now be straightforward as well.
> >>
> >> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> >> ---
> >>   -v3:
> >>   * save some memory as order-0 and order-1 can't be THP, Ryan;
> >>   * rename to anon_alloc as right now we only support anon to address
> >>     David's comment;
> >>   * drop a redundant "else", Ryan
> >>
> >>   include/linux/huge_mm.h | 18 ++++++++++++++
> >>   mm/huge_memory.c        | 54 +++++++++++++++++++++++++++++++++++++++++
> >>   mm/memory.c             |  2 ++
> >>   3 files changed, 74 insertions(+)
> >>
> >> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> >> index e896ca4760f6..5e9af6be9537 100644
> >> --- a/include/linux/huge_mm.h
> >> +++ b/include/linux/huge_mm.h
> >> @@ -70,6 +70,7 @@ extern struct kobj_attribute shmem_enabled_attr;
> >>    * (which is a limitation of the THP implementation).
> >>    */
> >>   #define THP_ORDERS_ALL_ANON    ((BIT(PMD_ORDER + 1) - 1) & ~(BIT(0) | BIT(1)))
> >> +#define THP_MIN_ORDER        2
> >>     /*
> >>    * Mask of all large folio orders supported for file THP.
> >> @@ -264,6 +265,23 @@ unsigned long thp_vma_allowable_orders(struct
> >> vm_area_struct *vma,
> >>                         enforce_sysfs, orders);
> >>   }
> >>   +enum thp_event_item {
> >> +    THP_ANON_ALLOC_SUCCESS,
> >> +    THP_ANON_ALLOC_FAIL,
> >> +    NR_THP_EVENT_ITEMS
> >> +};
> >
> > Maybe use a prefix that resembles matches the enum name and is "obviously"
> > different to the ones in vm_event_item.h, like
> >
> > enum thp_event {
> >     THP_EVENT_ANON_ALLOC_SUCCESS,
> >     THP_EVENT_ANON_ALLOC_FAIL,
> >     __THP_EVENT_COUNT,
> > };
>
> FWIW, I'd personally replace "event" with "stat". For me "event" only ever
> increments, but "stat" can increment and decrement. An event is a type of stat.
>
> You are only adding events for now, but we have identified a need for inc/dec
> stats that will be added in future.

What about the below?

enum thp_stat {
   THP_EVENT_ANON_ALLOC,
   THP_EVENT_ANON_ALLOC_FALLBACK,
   THP_EVENT_SWPOUT,
   THP_EVENT_SWPOUT_FALLBACK,
   ...
   THP_NR_ANON_PAGES,
   THP_NR_FILE_PAGES,
   ...
   __THP_STAT_COUNT,
};

THP_EVENT can only increase; THP_NR_ANON/FILE_PAGES can increase or
decrease. Thus, their names have no "EVENT".

>