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

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

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.