Re: [PATCH 04/11] mm/hugetlb: unify hugetlb migration callback function

[Mike Kravetz]

On 5/17/20 6:20 PM, js1304@xxxxxxxxx wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
> 
> There is no difference between two migration callback functions,
> alloc_huge_page_node() and alloc_huge_page_nodemask(), except
> __GFP_THISNODE handling. This patch adds one more field on to
> the alloc_control and handles this exception.
> 
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
> ---
>  include/linux/hugetlb.h |  8 --------
>  mm/hugetlb.c            | 23 ++---------------------
>  mm/internal.h           |  1 +
>  mm/mempolicy.c          |  3 ++-
>  4 files changed, 5 insertions(+), 30 deletions(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 6da217e..4892ed3 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -505,8 +505,6 @@ struct huge_bootmem_page {
>  
>  struct page *alloc_migrate_huge_page(struct hstate *h,
>  				struct alloc_control *ac);
> -struct page *alloc_huge_page_node(struct hstate *h,
> -				struct alloc_control *ac);
>  struct page *alloc_huge_page_nodemask(struct hstate *h,
>  				struct alloc_control *ac);
>  struct page *alloc_huge_page_vma(struct hstate *h, struct vm_area_struct *vma,
> @@ -755,12 +753,6 @@ static inline void huge_ptep_modify_prot_commit(struct vm_area_struct *vma,
>  struct hstate {};
>  
>  static inline struct page *
> -alloc_huge_page_node(struct hstate *h, struct alloc_control *ac)
> -{
> -	return NULL;
> -}
> -
> -static inline struct page *
>  alloc_huge_page_nodemask(struct hstate *h, struct alloc_control *ac)
>  {
>  	return NULL;
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 859dba4..60b0983 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1981,31 +1981,12 @@ struct page *alloc_buddy_huge_page_with_mpol(struct hstate *h,
>  }
>  
>  /* page migration callback function */
> -struct page *alloc_huge_page_node(struct hstate *h,
> -				struct alloc_control *ac)
> -{
> -	struct page *page = NULL;
> -
> -	ac->gfp_mask |= htlb_alloc_mask(h);
> -	if (ac->nid != NUMA_NO_NODE)
> -		ac->gfp_mask |= __GFP_THISNODE;
> -
> -	spin_lock(&hugetlb_lock);
> -	if (h->free_huge_pages - h->resv_huge_pages > 0)
> -		page = dequeue_huge_page_nodemask(h, ac);
> -	spin_unlock(&hugetlb_lock);
> -
> -	if (!page)
> -		page = alloc_migrate_huge_page(h, ac);
> -
> -	return page;
> -}
> -
> -/* page migration callback function */
>  struct page *alloc_huge_page_nodemask(struct hstate *h,
>  				struct alloc_control *ac)
>  {
>  	ac->gfp_mask |= htlb_alloc_mask(h);
> +	if (ac->thisnode && ac->nid != NUMA_NO_NODE)
> +		ac->gfp_mask |= __GFP_THISNODE;
>  
>  	spin_lock(&hugetlb_lock);
>  	if (h->free_huge_pages - h->resv_huge_pages > 0) {
> diff --git a/mm/internal.h b/mm/internal.h
> index 75b3f8e..574722d0 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -618,6 +618,7 @@ struct alloc_control {
>  	int nid;
>  	nodemask_t *nmask;
>  	gfp_t gfp_mask;
> +	bool thisnode;

I wonder if the new field is necessary?

IIUC, it simply prevents the check for NUMA_NO_NODE and possible setting
of __GFP_THISNODE in previous alloc_huge_page_nodemask() calling sequences.
However, it appears that node (preferred_nid) is always set to something
other than NUMA_NO_NODE in those callers.

It obviously makes sense to add the field to guarantee no changes to
functionality while making the conversions.  However, it it is not really
necessary then it may cause confusion later.

-- 
Mike Kravetz

>  };
>  
>  #endif	/* __MM_INTERNAL_H */
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 06f60a5..629feaa 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1073,9 +1073,10 @@ struct page *alloc_new_node_page(struct page *page, unsigned long node)
>  		struct alloc_control ac = {
>  			.nid = node,
>  			.nmask = NULL,
> +			.thisnode = true,
>  		};
>  
> -		return alloc_huge_page_node(h, &ac);
> +		return alloc_huge_page_nodemask(h, &ac);
>  	} else if (PageTransHuge(page)) {
>  		struct page *thp;
>  
>