On Thu, Apr 4, 2024 at 12:48 AM Ryan Roberts <ryan.roberts@xxxxxxx> wrote:
>
> On 03/04/2024 09:22, David Hildenbrand wrote:
> > On 03.04.24 05:55, Barry Song wrote:
> >> From: Barry Song <v-songbaohua@xxxxxxxx>
> >>
> >> 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@xxxxxxxx>
> >> ---
> >> -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".
>
