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

[David Hildenbrand <david@redhat.com>]

On 02.04.24 23:29, Barry Song wrote:
> On Wed, Apr 3, 2024 at 7:46 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 28.03.24 10:51, 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 alloc_success and alloc_fail
>>> counters.  Incorporating additional counters should now be
>>> straightforward as well.
>>>
>>> The initial two unsigned longs for each event are unused, given
>>> that order-0 and order-1 are not mTHP. Nonetheless, this refinement
>>> improves code clarity.
>>>
>>> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
>>> ---
>>>    -v2:
>>>    * move to sysfs and provide per-order counters; David, Ryan, Willy
>>>    -v1:
>>>    https://lore.kernel.org/linux-mm/20240326030103.50678-1-21cnbao@gmail.com/
>>>
>>>    include/linux/huge_mm.h | 17 +++++++++++++
>>>    mm/huge_memory.c        | 54 +++++++++++++++++++++++++++++++++++++++++
>>>    mm/memory.c             |  3 +++
>>>    3 files changed, 74 insertions(+)
>>>
>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>> index e896ca4760f6..27fa26a22a8f 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_event_item {
>>> +     THP_ALLOC_SUCCESS,
>>> +     THP_ALLOC_FAIL,
>>> +     NR_THP_EVENT_ITEMS
>>> +};

Why not simply "enum thp_event" ... "NR_THP_EVENT".

>>
>> I'm wondering if these should be ANON specific for now. We might want to
>> add others (shmem, file) in the future.
> 
> I've two ways to do that
> 1. rename to ANON_THP_ALLOC, so that I can have SHMEM_THP_ALLOC, FILE_THP_ALLOC
> in the future;
> 2. let THP_ALLOC cover all of shmem, file and anon.
> 
> following vmstat, actually 1 might be better as we have both THP_FAULT_ALLOC and
> THP_FILE_ALLOC for pmd-mapped THP.

Yes. Because anon was first, people just named it "THP". Then, file THP 
were added later. Some of that needs a cleanup.

> 
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>                  THP_FAULT_ALLOC,
>                  THP_FAULT_FALLBACK,
>                  THP_FAULT_FALLBACK_CHARGE,
>                  THP_COLLAPSE_ALLOC,
>                  THP_COLLAPSE_ALLOC_FAILED,
>                  THP_FILE_ALLOC,
>                  THP_FILE_FALLBACK,
>                  THP_FILE_FALLBACK_CHARGE,
>                  THP_FILE_MAPPED,
>                  THP_SPLIT_PAGE,
>                  THP_SPLIT_PAGE_FAILED,
>                  THP_DEFERRED_SPLIT_PAGE,
>                  THP_SPLIT_PMD,
>                  THP_SCAN_EXCEED_NONE_PTE,
>                  THP_SCAN_EXCEED_SWAP_PTE,
>                  THP_SCAN_EXCEED_SHARED_PTE,
> #ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
>                  THP_SPLIT_PUD,
> #endif
>                  THP_ZERO_PAGE_ALLOC,
>                  THP_ZERO_PAGE_ALLOC_FAILED,
>                  THP_SWPOUT,
>                  THP_SWPOUT_FALLBACK,
> #endif
> 
> And reading mm/shmem.c, obviously, shmem is using THP_FILE_ALLOC.

Right.

> 
> I will rename it to ANON_THP_ALLOC in v3, let me know if you disagree :-)

You should give people more time to respond before resending. Please try 
sending new versions only after the discussion on the old version 
finished. Otherwise it's going to be a mess (because I won't repost my 
feedback to v3 :P ).

THP_EVENT_ANON_ALLOC

might be better, so "THP_EVENT" would be your common prefix for "enum 
thp_event".

-- 
Cheers,

David / dhildenb