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

[David Hildenbrand <david@redhat.com>]

On 05.04.24 06:01, Barry Song wrote:
> On Fri, Apr 5, 2024 at 3:57 PM Barry Song <21cnbao@gmail.com> wrote:
>>
>> On Fri, Apr 5, 2024 at 4:31 AM David Hildenbrand <david@redhat.com> wrote:
>>>
>>> On 04.04.24 09:21, Ryan Roberts wrote:
>>>> On 03/04/2024 22:00, Barry Song wrote:
>>>>> On Thu, Apr 4, 2024 at 12:48 AM Ryan Roberts <ryan.roberts@arm.com> 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.
>>>>>
>>>>> What about the below?
>>>>>
>>>>> enum thp_stat {
>>
>> It seems we still need to use enum thp_stat_item rather than thp_stat.
>> This follows
>> enum zone_stat_item
>> enum numa_stat_item
>> enum node_stat_item


Right, we can do that.

[...]

>> ok.
> 
> Hi David, Ryan,
> 
> I've named everything as follows. Please let me know if you have any further
> suggestions before I send the updated version :-)
> 
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index e896ca4760f6..cc13fa14aa32 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_stat_item {
> +       THP_STAT_ANON_ALLOC,
> +       THP_STAT_ANON_ALLOC_FALLBACK,
> +       __THP_STAT_COUNT
> +};
> +


Final bikeshedding: so we want to call all of that MTHP_, mthp_ etc, to 
make it clearer that this is per mTHP size?

-- 
Cheers,

David / dhildenb