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

[Ryan Roberts <ryan.roberts@arm.com>]

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 {
>    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'm not sure if we need to prefix SWPOUT with ANON? (might we additionally want
SHMEM in future?

I'm aware I'm bikeshedding at this point... sorry :)


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