Re: [PATCH v2] mm: migrate: fix missing update page_private to hugetlb_page_subpool

[Michal Hocko]

On Fri 21-05-21 10:27:47, Muchun Song wrote:
> Since commit d6995da31122 ("hugetlb: use page.private for hugetlb specific
> page flags") converts page.private for hugetlb specific page flags. We
> should use hugetlb_page_subpool() to get the subpool pointer instead of
> page_private().
> 
> This 'could' prevent the migration of hugetlb pages. page_private(hpage)
> is now used for hugetlb page specific flags.  At migration time, the
> only flag which could be set is HPageVmemmapOptimized.  This flag will
> only be set if the new vmemmap reduction feature is enabled.  In
> addition, !page_mapping() implies an anonymous mapping.  So, this will
> prevent migration of hugetb pages in anonymous mappings if the vmemmap
> reduction feature is enabled.
> 
> In addition, that if statement checked for the rare race condition of a
> page being migrated while in the process of being freed.  Since that
> check is now wrong, we could leak hugetlb subpool usage counts.
> 
> The commit forgot to update it in the page migration routine. So fix it.
> 
> Fixes: d6995da31122 ("hugetlb: use page.private for hugetlb specific page flags")
> Reported-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> Reported-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> Reviewed-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>

The patch looks good to me. I hope Fixes tag will not confuse stable
backporters as this is a non issue for any released kernel.

Acked-by: Michal Hocko <mhocko@xxxxxxxx>

> ---
> Changeslog in v2:
>  1. Update commit log suggested by Mike and Andrew.
>  2. Fix compiler error when !CONFIG_HUGETLB_PAGE reported by Randy.
> 
>  Thanks.
> 
>  include/linux/hugetlb.h | 5 +++++
>  mm/migrate.c            | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 9f192ce44088..e11a2e9aa514 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -915,6 +915,11 @@ static inline bool is_hugetlb_free_vmemmap_enabled(void)
>  #else	/* CONFIG_HUGETLB_PAGE */
>  struct hstate {};
>  
> +static inline struct hugepage_subpool *hugetlb_page_subpool(struct page *hpage)
> +{
> +	return NULL;
> +}
> +
>  static inline int isolate_or_dissolve_huge_page(struct page *page,
>  						struct list_head *list)
>  {
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 066b1e4e8bde..0692fc2638aa 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1292,7 +1292,7 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
>  	 * page_mapping() set, hugetlbfs specific move page routine will not
>  	 * be called and we could leak usage counts for subpools.
>  	 */
> -	if (page_private(hpage) && !page_mapping(hpage)) {
> +	if (hugetlb_page_subpool(hpage) && !page_mapping(hpage)) {
>  		rc = -EBUSY;
>  		goto out_unlock;
>  	}
> -- 
> 2.11.0

-- 
Michal Hocko
SUSE Labs