| Message ID | 1592892828-1934-4-git-send-email-iamjoonsoo.kim@lge.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clean-up the migration target allocation functions | expand |
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>
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 >
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.
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. >
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.
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;