Re: [RFC] mm/vmstat: Add events for HugeTLB migration

[Anshuman Khandual]

On 09/25/2020 03:19 PM, Oscar Salvador wrote:
> On Fri, Sep 25, 2020 at 02:42:29PM +0530, Anshuman Khandual wrote:
>> Add following new vmstat events which will track HugeTLB page migration.
>>
>> 1. HUGETLB_MIGRATION_SUCCESS
>> 2. HUGETLB_MIGRATION_FAILURE
>>
>> It follows the existing semantics to accommodate HugeTLB subpages in total
>> page migration statistics. While here, this updates current trace event
>> "mm_migrate_pages" to accommodate now available HugeTLB based statistics.
>>
>> Cc: Daniel Jordan <daniel.m.jordan@xxxxxxxxxx>
>> Cc: Zi Yan <ziy@xxxxxxxxxx>
>> Cc: John Hubbard <jhubbard@xxxxxxxxxx>
>> Cc: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: linux-mm@xxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> 
> Was this inspired by some usecase/debugging or just to follow THP's example?

Currently HugeTLB migration events get accommodated in PGMIGRATE_SUCCESS and
PGMIGRATE_FAIL event stats as normal single page instances. Previously this
might have just seemed okay as HugeTLB page could be viewed as a single page
entity, even though it was not fully accurate as PGMIGRATE_[SUCCESS|FAILURE]
tracked statistics in terms of normal base pages.

But tracking HugeTLB pages as single pages does not make sense any more, now
that THP pages are accounted for properly. This would complete the revamped
page migration accounting where PGMIGRATE_[SUCCESS|FAILURE] will track entire
page migration events in terms of normal base pages and THP_*/HUGETLB_* will
track specialized events when applicable.

> 
>>  	int retry = 1;
>>  	int thp_retry = 1;
>> +	int hugetlb_retry = 1;
>>  	int nr_failed = 0;
>>  	int nr_succeeded = 0;
>>  	int nr_thp_succeeded = 0;
>>  	int nr_thp_failed = 0;
>>  	int nr_thp_split = 0;
>> +	int nr_hugetlb_succeeded = 0;
>> +	int nr_hugetlb_failed = 0;
>>  	int pass = 0;
>>  	bool is_thp = false;
>> +	bool is_hugetlb = false;
>>  	struct page *page;
>>  	struct page *page2;
>>  	int swapwrite = current->flags & PF_SWAPWRITE;
>> @@ -1433,6 +1437,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>  	for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
> 
> Should you not have put hugetlb_retry within the loop as well?
> Otherwise we might not rety for hugetlb pages now?
> 

Right, will fix it.