On Wed, Apr 3, 2024 at 9:13 PM David Hildenbrand <david@xxxxxxxxxx> wrote: > > On 02.04.24 23:29, Barry Song wrote: > > On Wed, Apr 3, 2024 at 7:46 AM David Hildenbrand <david@xxxxxxxxxx> wrote: > >> > >> On 28.03.24 10:51, Barry Song wrote: > >>> From: Barry Song <v-songbaohua@xxxxxxxx> > >>> > >>> 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@xxxxxxxx> > >>> --- > >>> -v2: > >>> * move to sysfs and provide per-order counters; David, Ryan, Willy > >>> -v1: > >>> https://lore.kernel.org/linux-mm/20240326030103.50678-1-21cnbao@xxxxxxxxx/ > >>> > >>> 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". ok. > > >> > >> 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. agreed. > > > > > #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 ). agreed :-) > > THP_EVENT_ANON_ALLOC > > might be better, so "THP_EVENT" would be your common prefix for "enum > thp_event". > Should be ok. On the other hand, currently, there are numerous occurrences of events or stats within mm, but we haven't incorporated "EVENT" or "STAT" into the naming conventions. > -- > Cheers, > > David / dhildenb > Thanks Barry
