2020-05-18 01:24:25

by Joonsoo Kim

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

From: Joonsoo Kim <[email protected]>

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 <[email protected]>
---
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;
};

#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;

--
2.7.4

2020-05-21 00:50:33

by Roman Gushchin

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

On Mon, May 18, 2020 at 10:20:50AM +0900, [email protected] wrote:
> From: Joonsoo Kim <[email protected]>
>
> 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 <[email protected]>
> ---
> 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);

Maybe drop _nodemask suffix in the remaining function?

> 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;
> };

Can you, please, add some comments what each field exactly mean?

Thanks!

>
> #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;
>
> --
> 2.7.4
>

2020-05-21 01:25:36

by Joonsoo Kim

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

2020년 5월 21일 (목) 오전 9:46, Roman Gushchin <[email protected]>님이 작성:
>
> On Mon, May 18, 2020 at 10:20:50AM +0900, [email protected] wrote:
> > From: Joonsoo Kim <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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);
>
> Maybe drop _nodemask suffix in the remaining function?

We cannot remove the suffix since there is already the same named function,
alloc_huge_page().

> > 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;
> > };
>
> Can you, please, add some comments what each field exactly mean?

Will do.

Thanks.

2020-05-21 20:58:26

by Mike Kravetz

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

On 5/17/20 6:20 PM, [email protected] wrote:
> From: Joonsoo Kim <[email protected]>
>
> 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 <[email protected]>
> ---
> 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;
>
>

2020-05-22 07:43:40

by Joonsoo Kim

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

2020년 5월 22일 (금) 오전 5:54, Mike Kravetz <[email protected]>님이 작성:
>
> On 5/17/20 6:20 PM, [email protected] wrote:
> > From: Joonsoo Kim <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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.

Okay. I will check it.

> 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.

Agreed.

Thanks.