[v3,3/8] mm/hugetlb: unify migration callbacks
diff mbox series

Message ID 1592892828-1934-4-git-send-email-iamjoonsoo.kim@lge.com
State New
Headers show
Series
  • clean-up the migration target allocation functions
Related show

Commit Message

Joonsoo Kim June 23, 2020, 6:13 a.m. UTC
From: Joonsoo Kim <iamjoonsoo.kim@lge.com>

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 an argument, gfp_mask, on
alloc_huge_page_nodemask() and replace the callsite for
alloc_huge_page_node() with the call to
alloc_huge_page_nodemask(..., __GFP_THISNODE).

It's safe to remove a node id check in alloc_huge_page_node() since
there is no caller passing NUMA_NO_NODE as a node id.

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 include/linux/hugetlb.h | 11 +++--------
 mm/hugetlb.c            | 26 +++-----------------------
 mm/mempolicy.c          |  9 +++++----
 mm/migrate.c            |  5 +++--
 4 files changed, 14 insertions(+), 37 deletions(-)

Comments

Mike Kravetz June 24, 2020, 9:18 p.m. UTC | #1
On 6/22/20 11:13 PM, js1304@gmail.com wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> 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 an argument, gfp_mask, on
> alloc_huge_page_nodemask() and replace the callsite for
> alloc_huge_page_node() with the call to
> alloc_huge_page_nodemask(..., __GFP_THISNODE).
> 
> It's safe to remove a node id check in alloc_huge_page_node() since
> there is no caller passing NUMA_NO_NODE as a node id.
> 
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Thanks for consolidating these.

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
Michal Hocko June 25, 2020, 11:26 a.m. UTC | #2
On Tue 23-06-20 15:13:43, Joonsoo Kim wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> 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 an argument, gfp_mask, on
> alloc_huge_page_nodemask() and replace the callsite for
> alloc_huge_page_node() with the call to
> alloc_huge_page_nodemask(..., __GFP_THISNODE).
> 
> It's safe to remove a node id check in alloc_huge_page_node() since
> there is no caller passing NUMA_NO_NODE as a node id.

Yes this is indeed safe. alloc_huge_page_node used to be called from
other internal hugetlb allocation layer and that allowed NUMA_NO_NODE as
well. Now it is called only from the mempolicy migration callback and
that always specifies a node and want to stick with that node.

But I have to say I really dislike the gfp semantic because it is
different from any other allocation function I can think of. It
specifies what to be added rather than what should be used.

Removing the function is ok but please use the full gfp mask instead
or if that is impractical for some reason (wich shouldn't be the case
as htlb_alloc_mask should be trivial to make static inline) make it
explicit that this is not a gfp_mask but a gfp modifier and explicitly
state which modifiers are allowed.
 
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> ---
>  include/linux/hugetlb.h | 11 +++--------
>  mm/hugetlb.c            | 26 +++-----------------------
>  mm/mempolicy.c          |  9 +++++----
>  mm/migrate.c            |  5 +++--
>  4 files changed, 14 insertions(+), 37 deletions(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 50650d0..8a8b755 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -504,9 +504,8 @@ struct huge_bootmem_page {
>  
>  struct page *alloc_huge_page(struct vm_area_struct *vma,
>  				unsigned long addr, int avoid_reserve);
> -struct page *alloc_huge_page_node(struct hstate *h, int nid);
>  struct page *alloc_huge_page_nodemask(struct hstate *h, int preferred_nid,
> -				nodemask_t *nmask);
> +				nodemask_t *nmask, gfp_t gfp_mask);
>  struct page *alloc_huge_page_vma(struct hstate *h, struct vm_area_struct *vma,
>  				unsigned long address);
>  struct page *alloc_migrate_huge_page(struct hstate *h, gfp_t gfp_mask,
> @@ -759,13 +758,9 @@ static inline struct page *alloc_huge_page(struct vm_area_struct *vma,
>  	return NULL;
>  }
>  
> -static inline struct page *alloc_huge_page_node(struct hstate *h, int nid)
> -{
> -	return NULL;
> -}
> -
>  static inline struct page *
> -alloc_huge_page_nodemask(struct hstate *h, int preferred_nid, nodemask_t *nmask)
> +alloc_huge_page_nodemask(struct hstate *h, int preferred_nid,
> +			nodemask_t *nmask, gfp_t gfp_mask)
>  {
>  	return NULL;
>  }
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index d54bb7e..bd408f2 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1979,30 +1979,10 @@ struct page *alloc_buddy_huge_page_with_mpol(struct hstate *h,
>  }
>  
>  /* page migration callback function */
> -struct page *alloc_huge_page_node(struct hstate *h, int nid)
> -{
> -	gfp_t gfp_mask = htlb_alloc_mask(h);
> -	struct page *page = NULL;
> -
> -	if (nid != NUMA_NO_NODE)
> -		gfp_mask |= __GFP_THISNODE;
> -
> -	spin_lock(&hugetlb_lock);
> -	if (h->free_huge_pages - h->resv_huge_pages > 0)
> -		page = dequeue_huge_page_nodemask(h, gfp_mask, nid, NULL);
> -	spin_unlock(&hugetlb_lock);
> -
> -	if (!page)
> -		page = alloc_migrate_huge_page(h, gfp_mask, nid, NULL);
> -
> -	return page;
> -}
> -
> -/* page migration callback function */
>  struct page *alloc_huge_page_nodemask(struct hstate *h, int preferred_nid,
> -		nodemask_t *nmask)
> +		nodemask_t *nmask, gfp_t gfp_mask)
>  {
> -	gfp_t gfp_mask = htlb_alloc_mask(h);
> +	gfp_mask |= htlb_alloc_mask(h);
>  
>  	spin_lock(&hugetlb_lock);
>  	if (h->free_huge_pages - h->resv_huge_pages > 0) {
> @@ -2031,7 +2011,7 @@ struct page *alloc_huge_page_vma(struct hstate *h, struct vm_area_struct *vma,
>  
>  	gfp_mask = htlb_alloc_mask(h);
>  	node = huge_node(vma, address, gfp_mask, &mpol, &nodemask);
> -	page = alloc_huge_page_nodemask(h, node, nodemask);
> +	page = alloc_huge_page_nodemask(h, node, nodemask, 0);
>  	mpol_cond_put(mpol);
>  
>  	return page;
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index b9e85d4..f21cff5 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1068,10 +1068,11 @@ static int migrate_page_add(struct page *page, struct list_head *pagelist,
>  /* page allocation callback for NUMA node migration */
>  struct page *alloc_new_node_page(struct page *page, unsigned long node)
>  {
> -	if (PageHuge(page))
> -		return alloc_huge_page_node(page_hstate(compound_head(page)),
> -					node);
> -	else if (PageTransHuge(page)) {
> +	if (PageHuge(page)) {
> +		return alloc_huge_page_nodemask(
> +			page_hstate(compound_head(page)), node,
> +			NULL, __GFP_THISNODE);
> +	} else if (PageTransHuge(page)) {
>  		struct page *thp;
>  
>  		thp = alloc_pages_node(node,
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 6b5c75b..6ca9f0c 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1543,10 +1543,11 @@ struct page *new_page_nodemask(struct page *page,
>  	unsigned int order = 0;
>  	struct page *new_page = NULL;
>  
> -	if (PageHuge(page))
> +	if (PageHuge(page)) {
>  		return alloc_huge_page_nodemask(
>  				page_hstate(compound_head(page)),
> -				preferred_nid, nodemask);
> +				preferred_nid, nodemask, 0);
> +	}
>  
>  	if (PageTransHuge(page)) {
>  		gfp_mask |= GFP_TRANSHUGE;
> -- 
> 2.7.4
>
Joonsoo Kim June 26, 2020, 4:02 a.m. UTC | #3
2020년 6월 25일 (목) 오후 8:26, Michal Hocko <mhocko@kernel.org>님이 작성:
>
> On Tue 23-06-20 15:13:43, Joonsoo Kim wrote:
> > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> >
> > 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 an argument, gfp_mask, on
> > alloc_huge_page_nodemask() and replace the callsite for
> > alloc_huge_page_node() with the call to
> > alloc_huge_page_nodemask(..., __GFP_THISNODE).
> >
> > It's safe to remove a node id check in alloc_huge_page_node() since
> > there is no caller passing NUMA_NO_NODE as a node id.
>
> Yes this is indeed safe. alloc_huge_page_node used to be called from
> other internal hugetlb allocation layer and that allowed NUMA_NO_NODE as
> well. Now it is called only from the mempolicy migration callback and
> that always specifies a node and want to stick with that node.
>
> But I have to say I really dislike the gfp semantic because it is
> different from any other allocation function I can think of. It
> specifies what to be added rather than what should be used.
>
> Removing the function is ok but please use the full gfp mask instead
> or if that is impractical for some reason (wich shouldn't be the case
> as htlb_alloc_mask should be trivial to make static inline) make it
> explicit that this is not a gfp_mask but a gfp modifier and explicitly
> state which modifiers are allowed.

Okay. I will try to solve your concern. Concrete solution is not yet prepared
but perhaps I will use full gfp_mask by using htlb_alloc_mask() in caller sites.

Thanks.
Vlastimil Babka July 2, 2020, 4:13 p.m. UTC | #4
On 6/26/20 6:02 AM, Joonsoo Kim wrote:
> 2020년 6월 25일 (목) 오후 8:26, Michal Hocko <mhocko@kernel.org>님이 작성:
>>
>> On Tue 23-06-20 15:13:43, Joonsoo Kim wrote:
>> > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>> >
>> > 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 an argument, gfp_mask, on
>> > alloc_huge_page_nodemask() and replace the callsite for
>> > alloc_huge_page_node() with the call to
>> > alloc_huge_page_nodemask(..., __GFP_THISNODE).
>> >
>> > It's safe to remove a node id check in alloc_huge_page_node() since
>> > there is no caller passing NUMA_NO_NODE as a node id.
>>
>> Yes this is indeed safe. alloc_huge_page_node used to be called from
>> other internal hugetlb allocation layer and that allowed NUMA_NO_NODE as
>> well. Now it is called only from the mempolicy migration callback and
>> that always specifies a node and want to stick with that node.
>>
>> But I have to say I really dislike the gfp semantic because it is
>> different from any other allocation function I can think of. It
>> specifies what to be added rather than what should be used.
>>
>> Removing the function is ok but please use the full gfp mask instead
>> or if that is impractical for some reason (wich shouldn't be the case
>> as htlb_alloc_mask should be trivial to make static inline) make it
>> explicit that this is not a gfp_mask but a gfp modifier and explicitly
>> state which modifiers are allowed.
> 
> Okay. I will try to solve your concern. Concrete solution is not yet prepared
> but perhaps I will use full gfp_mask by using htlb_alloc_mask() in caller sites.

Yeah, that should be feasible. alloc_huge_page_vma() already does
htlb_alloc_mask(h). In alloc_new_node_page() and new_page_nodemask() it would be
consistent with the other cases handled there (THP and base).

> Thanks.
>
Joonsoo Kim July 3, 2020, 12:55 a.m. UTC | #5
2020년 7월 3일 (금) 오전 1:13, Vlastimil Babka <vbabka@suse.cz>님이 작성:
>
> On 6/26/20 6:02 AM, Joonsoo Kim wrote:
> > 2020년 6월 25일 (목) 오후 8:26, Michal Hocko <mhocko@kernel.org>님이 작성:
> >>
> >> On Tue 23-06-20 15:13:43, Joonsoo Kim wrote:
> >> > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> >> >
> >> > 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 an argument, gfp_mask, on
> >> > alloc_huge_page_nodemask() and replace the callsite for
> >> > alloc_huge_page_node() with the call to
> >> > alloc_huge_page_nodemask(..., __GFP_THISNODE).
> >> >
> >> > It's safe to remove a node id check in alloc_huge_page_node() since
> >> > there is no caller passing NUMA_NO_NODE as a node id.
> >>
> >> Yes this is indeed safe. alloc_huge_page_node used to be called from
> >> other internal hugetlb allocation layer and that allowed NUMA_NO_NODE as
> >> well. Now it is called only from the mempolicy migration callback and
> >> that always specifies a node and want to stick with that node.
> >>
> >> But I have to say I really dislike the gfp semantic because it is
> >> different from any other allocation function I can think of. It
> >> specifies what to be added rather than what should be used.
> >>
> >> Removing the function is ok but please use the full gfp mask instead
> >> or if that is impractical for some reason (wich shouldn't be the case
> >> as htlb_alloc_mask should be trivial to make static inline) make it
> >> explicit that this is not a gfp_mask but a gfp modifier and explicitly
> >> state which modifiers are allowed.
> >
> > Okay. I will try to solve your concern. Concrete solution is not yet prepared
> > but perhaps I will use full gfp_mask by using htlb_alloc_mask() in caller sites.
>
> Yeah, that should be feasible. alloc_huge_page_vma() already does
> htlb_alloc_mask(h). In alloc_new_node_page() and new_page_nodemask() it would be
> consistent with the other cases handled there (THP and base).

Okay. Will check it.

Thanks.

Patch
diff mbox series

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 50650d0..8a8b755 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -504,9 +504,8 @@  struct huge_bootmem_page {
 
 struct page *alloc_huge_page(struct vm_area_struct *vma,
 				unsigned long addr, int avoid_reserve);
-struct page *alloc_huge_page_node(struct hstate *h, int nid);
 struct page *alloc_huge_page_nodemask(struct hstate *h, int preferred_nid,
-				nodemask_t *nmask);
+				nodemask_t *nmask, gfp_t gfp_mask);
 struct page *alloc_huge_page_vma(struct hstate *h, struct vm_area_struct *vma,
 				unsigned long address);
 struct page *alloc_migrate_huge_page(struct hstate *h, gfp_t gfp_mask,
@@ -759,13 +758,9 @@  static inline struct page *alloc_huge_page(struct vm_area_struct *vma,
 	return NULL;
 }
 
-static inline struct page *alloc_huge_page_node(struct hstate *h, int nid)
-{
-	return NULL;
-}
-
 static inline struct page *
-alloc_huge_page_nodemask(struct hstate *h, int preferred_nid, nodemask_t *nmask)
+alloc_huge_page_nodemask(struct hstate *h, int preferred_nid,
+			nodemask_t *nmask, gfp_t gfp_mask)
 {
 	return NULL;
 }
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index d54bb7e..bd408f2 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1979,30 +1979,10 @@  struct page *alloc_buddy_huge_page_with_mpol(struct hstate *h,
 }
 
 /* page migration callback function */
-struct page *alloc_huge_page_node(struct hstate *h, int nid)
-{
-	gfp_t gfp_mask = htlb_alloc_mask(h);
-	struct page *page = NULL;
-
-	if (nid != NUMA_NO_NODE)
-		gfp_mask |= __GFP_THISNODE;
-
-	spin_lock(&hugetlb_lock);
-	if (h->free_huge_pages - h->resv_huge_pages > 0)
-		page = dequeue_huge_page_nodemask(h, gfp_mask, nid, NULL);
-	spin_unlock(&hugetlb_lock);
-
-	if (!page)
-		page = alloc_migrate_huge_page(h, gfp_mask, nid, NULL);
-
-	return page;
-}
-
-/* page migration callback function */
 struct page *alloc_huge_page_nodemask(struct hstate *h, int preferred_nid,
-		nodemask_t *nmask)
+		nodemask_t *nmask, gfp_t gfp_mask)
 {
-	gfp_t gfp_mask = htlb_alloc_mask(h);
+	gfp_mask |= htlb_alloc_mask(h);
 
 	spin_lock(&hugetlb_lock);
 	if (h->free_huge_pages - h->resv_huge_pages > 0) {
@@ -2031,7 +2011,7 @@  struct page *alloc_huge_page_vma(struct hstate *h, struct vm_area_struct *vma,
 
 	gfp_mask = htlb_alloc_mask(h);
 	node = huge_node(vma, address, gfp_mask, &mpol, &nodemask);
-	page = alloc_huge_page_nodemask(h, node, nodemask);
+	page = alloc_huge_page_nodemask(h, node, nodemask, 0);
 	mpol_cond_put(mpol);
 
 	return page;
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index b9e85d4..f21cff5 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1068,10 +1068,11 @@  static int migrate_page_add(struct page *page, struct list_head *pagelist,
 /* page allocation callback for NUMA node migration */
 struct page *alloc_new_node_page(struct page *page, unsigned long node)
 {
-	if (PageHuge(page))
-		return alloc_huge_page_node(page_hstate(compound_head(page)),
-					node);
-	else if (PageTransHuge(page)) {
+	if (PageHuge(page)) {
+		return alloc_huge_page_nodemask(
+			page_hstate(compound_head(page)), node,
+			NULL, __GFP_THISNODE);
+	} else if (PageTransHuge(page)) {
 		struct page *thp;
 
 		thp = alloc_pages_node(node,
diff --git a/mm/migrate.c b/mm/migrate.c
index 6b5c75b..6ca9f0c 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1543,10 +1543,11 @@  struct page *new_page_nodemask(struct page *page,
 	unsigned int order = 0;
 	struct page *new_page = NULL;
 
-	if (PageHuge(page))
+	if (PageHuge(page)) {
 		return alloc_huge_page_nodemask(
 				page_hstate(compound_head(page)),
-				preferred_nid, nodemask);
+				preferred_nid, nodemask, 0);
+	}
 
 	if (PageTransHuge(page)) {
 		gfp_mask |= GFP_TRANSHUGE;