Re: [PATCH v3 3/5] hugetlb: only set HPageMigratable for migratable hstates

[Mike Kravetz]

On 1/27/21 2:35 AM, Michal Hocko wrote:
> On Fri 22-01-21 11:52:29, Mike Kravetz wrote:
>> The HP_Migratable flag indicates a page is a candidate for migration.
>> Only set the flag if the page's hstate supports migration.  This allows
>> the migration paths to detect non-migratable pages earlier.  If migration
>> is not supported for the hstate, HP_Migratable will not be set, the page
>> will not be isolated and no attempt will be made to migrate.  We should
>> never get to unmap_and_move_huge_page for a page where migration is not
>> supported, so throw a warning if we do.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
>> ---
>>  fs/hugetlbfs/inode.c    | 2 +-
>>  include/linux/hugetlb.h | 9 +++++++++
>>  mm/hugetlb.c            | 8 ++++----
>>  mm/migrate.c            | 9 ++++-----
>>  4 files changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>> index e1d7ed2a53a9..93f7b8d3c5fd 100644
>> --- a/fs/hugetlbfs/inode.c
>> +++ b/fs/hugetlbfs/inode.c
>> @@ -735,7 +735,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>>  
>>  		mutex_unlock(&hugetlb_fault_mutex_table[hash]);
>>  
>> -		SetHPageMigratable(page);
>> +		SetHPageMigratableIfSupported(page);
>>  		/*
>>  		 * unlock_page because locked by add_to_page_cache()
>>  		 * put_page() due to reference from alloc_huge_page()
>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>> index 58be44a915d1..cd1960541f2a 100644
>> --- a/include/linux/hugetlb.h
>> +++ b/include/linux/hugetlb.h
>> @@ -740,6 +740,15 @@ static inline bool hugepage_migration_supported(struct hstate *h)
>>  	return arch_hugetlb_migration_supported(h);
>>  }
>>  
>> +/*
>> + * Only set HPageMigratable if migration supported for page
>> + */
>> +static inline void SetHPageMigratableIfSupported(struct page *page)
> 
> This is really mouthful...
> 
>> +{
>> +	if (hugepage_migration_supported(page_hstate(page)))
>> +		SetHPageMigratable(page);
> 
> and it is really a trivial wrapper. I do understand why you want to
> prevent from the code duplication and potentially a missing check but
> this all is just an internal hugetlb code. Even if the flag is set on
> non-migrateable hugetlb page then this will not be fatal. The migration
> can fail even on those pages for which migration is supported right?
> 
> So I am not really sure this is an improvement in the end. But up to you
> I do not really have a strong opinion here.

Yes, this patch is somewhat optional.  It should be a minor improvement
in cases where we are dealing with hpages in a non-migratable hstate.
Although, I do not believe this is the common case.

The real reason for even looking into this was a comment by Oscar.  With
the name change to HPageMigratable, it implies that the page is migratable.
However, this is not the case if the page's hstate does not support migration.
So, if we check the hstate when setting the flag we can eliminate those
cases where the page is certainly not migratable.

I don't really love this patch.  It has minimal functional value.

Oscar, what do you think about dropping this?
-- 
Mike Kravetz