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

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

On Fri, Apr 5, 2024 at 8:18 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>
> On 05/04/2024 05:01, Barry Song wrote:
> > On Fri, Apr 5, 2024 at 3:57 PM Barry Song <21cnbao@gmail.com> wrote:
> >>
> >> On Fri, Apr 5, 2024 at 4:31 AM David Hildenbrand <david@redhat.com> wrote:
> >>>
> >>> On 04.04.24 09:21, Ryan Roberts wrote:
> >>>> On 03/04/2024 22:00, Barry Song wrote:
> >>>>> 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 {
> >>
> >> It seems we still need to use enum thp_stat_item rather than thp_stat.
> >> This follows
> >> enum zone_stat_item
> >> enum numa_stat_item
> >> enum node_stat_item
> >>
> >> And most importantly, the below looks much better
> >>
> >>        enum thp_stat_item {
> >>               THP_STAT_ANON_ALLOC,
> >>               THP_STAT_ANON_ALLOC_FALLBACK,
> >>               __THP_STAT_COUNT
> >>        };
> >>
> >>        struct thp_state {
> >>               unsigned long state[PMD_ORDER + 1][__THP_STAT_COUNT];
> >>        };
> >>
> >>        DECLARE_PER_CPU(struct thp_state, thp_states);
> >>
> >> than
> >>
> >>        enum thp_stat {
> >>               THP_STAT_ANON_ALLOC,
> >>               THP_STAT_ANON_ALLOC_FALLBACK,
> >>               __THP_STAT_COUNT
> >>        };
> >>
> >>        struct thp_state {
> >>               unsigned long state[PMD_ORDER + 1][__THP_STAT_COUNT];
> >>        };
> >>
> >>>>>     THP_EVENT_ANON_ALLOC,
> >>>>>     THP_EVENT_ANON_ALLOC_FALLBACK,
> >>>>>     THP_EVENT_SWPOUT,
> >>>>>     THP_EVENT_SWPOUT_FALLBACK,
> >>>>>     ...
> >>>>>     THP_NR_ANON_PAGES,
> >>>>>     THP_NR_FILE_PAGES,
> >>>>
> >>>> I find this ambiguous; Is it the number of THPs or the number of base pages?
> >>>>
> >>>> I think David made the point about incorporating the enum name into the labels
> >>>> too, so that there can be no namespace confusion. How about:
> >>>>
> >>>> <enum>_<type>_<name>
> >>>>
> >>>> So:
> >>>>
> >>>> THP_STAT_EV_ANON_ALLOC
> >>>> THP_STAT_EV_ANON_ALLOC_FALLBACK
> >>>> THP_STAT_EV_ANON_PARTIAL
> >>>> THP_STAT_EV_SWPOUT
> >>>> THP_STAT_EV_SWPOUT_FALLBACK
> >>>> ...
> >>>> THP_STAT_NR_ANON
> >>>> THP_STAT_NR_FILE
> >>>> ...
> >>>> __THP_STAT_COUNT
> >>>
> >>> I'd even drop the "EV". "NR_ANON" vs "ANON_ALLOC" etc. is expressive enough.
> >>
> >> ok.
> >
> > Hi David, Ryan,
> >
> > I've named everything as follows. Please let me know if you have any further
> > suggestions before I send the updated version :-)
>
> Please treat all my comments below as optional - I'm just stating my preference!
>
> >
> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index e896ca4760f6..cc13fa14aa32 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_stat_item {
> > +       THP_STAT_ANON_ALLOC,
> > +       THP_STAT_ANON_ALLOC_FALLBACK,
> > +       __THP_STAT_COUNT
> > +};
> > +
> > +struct thp_state {
> > +       unsigned long state[PMD_ORDER + 1][__THP_STAT_COUNT];
>
> Why using "state" here? I think "stats" is more appropriate? (as in short for
> "statistics") i.e. `struct thp_stats` and `unsigned long stats[][]`.

Sounds good.

>
> > +};
> > +
> > +DECLARE_PER_CPU(struct thp_state, thp_states);
> > +
> > +static inline void count_thp_state(int order, enum thp_stat_item item)
> > +{
> > +       this_cpu_inc(thp_states.state[order][item]);
> > +}
> > +
> >  #define transparent_hugepage_use_zero_page()                           \
> >         (transparent_hugepage_flags &                                   \
> >          (1<<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG))
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 9d4b2fbf6872..e704b4408181 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -526,6 +526,46 @@ static const struct kobj_type thpsize_ktype = {
> >         .sysfs_ops = &kobj_sysfs_ops,
> >  };
> >
> > +DEFINE_PER_CPU(struct thp_state, thp_states) = {{{0}}};
>
> Does this need to be explicitly zeroed? Won't that be the default initial state,
> just like for regular globals? Perhaps PER_CPU is special?

I really don't know. uninitialized data is usually put in bss section so kernel
will memset them to 0. but uninitialized per-cpu data is still in .data..percpu
section and I can't find where memset is done in this area as this section
can also contain initialized per-cpu data. Will compilers fill zero for
uninitialized per-cpu data automatically so kernel just thinks they are
initialized 0 data?

anyway, I copied the code from vmstat
DEFINE_PER_CPU(struct vm_event_state, vm_event_states) = {{0}};

>
> > +
> > +static unsigned long sum_thp_states(int order, enum thp_stat_item item)
>
> Again, I'd call it sum_thp_stats().

sounds good.

>
> > +{
> > +       unsigned long sum = 0;
> > +       int cpu;
> > +
> > +       for_each_online_cpu(cpu) {
> > +               struct thp_state *this = &per_cpu(thp_states, cpu);
> > +
> > +               sum += this->state[order][item];
> > +       }
> > +
> > +       return sum;
> > +}
> > +
> > +#define THP_STATE_ATTR(_name, _index)                                  \
>
> And THP_STATS_ATTR(); they are going to live in the "stats" directory after all.

sounds good.

>
> > +static ssize_t _name##_show(struct kobject *kobj,                      \
> > +                       struct kobj_attribute *attr, char *buf)         \
> > +{                                                                      \
> > +       int order = to_thpsize(kobj)->order;                            \
> > +                                                                       \
> > +       return sysfs_emit(buf, "%lu\n", sum_thp_states(order, _index)); \
> > +}                                                                      \
> > +static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
> > +
> > +THP_STATE_ATTR(anon_alloc, THP_STAT_ANON_ALLOC);
> > +THP_STATE_ATTR(anon_alloc_fallback, THP_STAT_ANON_ALLOC_FALLBACK);
> >
> >>
> >>>
> >>> --
> >>> Cheers,
> >>>
> >>> David / dhildenb
> >
> > Thanks
> > Barry
>