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

[David Hildenbrand <david@redhat.com>]

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,
};

-- 
Cheers,

David / dhildenb