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

[David Hildenbrand <david@redhat.com>]

On 03.04.24 13:48, Ryan Roberts 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.
> 

Fully agree!

-- 
Cheers,

David / dhildenb