| Message ID | 20200707031156.29932-1-song.bao.hua@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm/hugetlb: avoid hardcoding while checking if cma is enable | expand |
On Tue, Jul 07, 2020 at 03:11:56PM +1200, Barry Song wrote: > hugetlb_cma[0] can be NULL due to various reasons, for example, node0 has > no memory. so NULL hugetlb_cma[0] doesn't necessarily mean cma is not > enabled. gigantic pages might have been reserved on other nodes. > > Fixes: cf11e85fc08c ("mm: hugetlb: optionally allocate gigantic hugepages using cma") > Cc: Roman Gushchin <guro@fb.com> > Cc: Mike Kravetz <mike.kravetz@oracle.com> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> > --- > -v2: add hugetlb_cma_enabled() helper to improve readability according to Roman > > mm/hugetlb.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 57ece74e3aae..d5e98ed86bb9 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -2546,6 +2546,20 @@ static void __init gather_bootmem_prealloc(void) > } > } > > +bool __init hugetlb_cma_enabled(void) > +{ > + if (IS_ENABLED(CONFIG_CMA)) { > + int node; > + > + for_each_online_node(node) { > + if (hugetlb_cma[node]) > + return true; > + } > + } > + > + return false; > +} > + Can you, please, change it to a more canonical #ifdef CONFIG_CMA bool __init hugetlb_cma_enabled(void) { int node; for_each_online_node(node) if (hugetlb_cma[node]) return true; return false; } #else bool __init hugetlb_cma_enabled(void) { return false; } #endif or maybe just bool __init hugetlb_cma_enabled(void) { #ifdef CONFIG_CMA int node; for_each_online_node(node) if (hugetlb_cma[node]) return true; #endif return false; } ? Please, feel free to add Acked-by: Roman Gushchin <guro@fb.com> after that. Thank you!
On Mon, Jul 06, 2020 at 08:36:31PM -0700, Roman Gushchin wrote: > On Tue, Jul 07, 2020 at 03:11:56PM +1200, Barry Song wrote: > > hugetlb_cma[0] can be NULL due to various reasons, for example, node0 has > > no memory. so NULL hugetlb_cma[0] doesn't necessarily mean cma is not > > enabled. gigantic pages might have been reserved on other nodes. > > > > Fixes: cf11e85fc08c ("mm: hugetlb: optionally allocate gigantic hugepages using cma") > > Cc: Roman Gushchin <guro@fb.com> > > Cc: Mike Kravetz <mike.kravetz@oracle.com> > > Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> > > --- > > -v2: add hugetlb_cma_enabled() helper to improve readability according to Roman > > > > mm/hugetlb.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > index 57ece74e3aae..d5e98ed86bb9 100644 > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > > @@ -2546,6 +2546,20 @@ static void __init gather_bootmem_prealloc(void) > > } > > } > > > > +bool __init hugetlb_cma_enabled(void) > > +{ > > + if (IS_ENABLED(CONFIG_CMA)) { > > + int node; > > + > > + for_each_online_node(node) { > > + if (hugetlb_cma[node]) > > + return true; > > + } > > + } > > + > > + return false; > > +} > > + > > Can you, please, change it to a more canonical > > #ifdef CONFIG_CMA > bool __init hugetlb_cma_enabled(void) > { > int node; > > for_each_online_node(node) > if (hugetlb_cma[node]) > return true; > > return false; > } > #else > bool __init hugetlb_cma_enabled(void) > { > return false; > } > #endif > > or maybe just > > bool __init hugetlb_cma_enabled(void) > { > #ifdef CONFIG_CMA > int node; > > for_each_online_node(node) > if (hugetlb_cma[node]) > return true; > #endif > return false; > } This one please. > ? > > Please, feel free to add > Acked-by: Roman Gushchin <guro@fb.com> after that. > > Thank you! >
> -----Original Message----- > From: Mike Rapoport [mailto:rppt@kernel.org] > Sent: Tuesday, July 7, 2020 7:38 PM > To: Roman Gushchin <guro@fb.com> > Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; > akpm@linux-foundation.org; linux-mm@kvack.org; > linux-kernel@vger.kernel.org; Linuxarm <linuxarm@huawei.com>; Mike > Kravetz <mike.kravetz@oracle.com>; Jonathan Cameron > <jonathan.cameron@huawei.com> > Subject: Re: [PATCH v2] mm/hugetlb: avoid hardcoding while checking if cma > is enable > > On Mon, Jul 06, 2020 at 08:36:31PM -0700, Roman Gushchin wrote: > > On Tue, Jul 07, 2020 at 03:11:56PM +1200, Barry Song wrote: > > > hugetlb_cma[0] can be NULL due to various reasons, for example, node0 > has > > > no memory. so NULL hugetlb_cma[0] doesn't necessarily mean cma is not > > > enabled. gigantic pages might have been reserved on other nodes. > > > > > > Fixes: cf11e85fc08c ("mm: hugetlb: optionally allocate gigantic hugepages > using cma") > > > Cc: Roman Gushchin <guro@fb.com> > > > Cc: Mike Kravetz <mike.kravetz@oracle.com> > > > Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > > > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> > > > --- > > > -v2: add hugetlb_cma_enabled() helper to improve readability according > to Roman > > > > > > mm/hugetlb.c | 16 +++++++++++++++- > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > > index 57ece74e3aae..d5e98ed86bb9 100644 > > > --- a/mm/hugetlb.c > > > +++ b/mm/hugetlb.c > > > @@ -2546,6 +2546,20 @@ static void __init > gather_bootmem_prealloc(void) > > > } > > > } > > > > > > +bool __init hugetlb_cma_enabled(void) > > > +{ > > > + if (IS_ENABLED(CONFIG_CMA)) { > > > + int node; > > > + > > > + for_each_online_node(node) { > > > + if (hugetlb_cma[node]) > > > + return true; > > > + } > > > + } > > > + > > > + return false; > > > +} > > > + > > > > Can you, please, change it to a more canonical > > > > #ifdef CONFIG_CMA > > bool __init hugetlb_cma_enabled(void) > > { > > int node; > > > > for_each_online_node(node) > > if (hugetlb_cma[node]) > > return true; > > > > return false; > > } > > #else > > bool __init hugetlb_cma_enabled(void) > > { > > return false; > > } > > #endif > > > > or maybe just > > > > bool __init hugetlb_cma_enabled(void) > > { > > #ifdef CONFIG_CMA > > int node; > > > > for_each_online_node(node) > > if (hugetlb_cma[node]) > > return true; > > #endif > > return false; > > } > > This one please. Yep. Patch v3 is just the one you prefer: https://marc.info/?l=linux-mm&m=159409465228477&w=2 > > > ? > > > > Please, feel free to add > > Acked-by: Roman Gushchin <guro@fb.com> after that. > > > > Thank you! > > > > -- > Sincerely yours, > Mike. Thanks Barry
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 57ece74e3aae..d5e98ed86bb9 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -2546,6 +2546,20 @@ static void __init gather_bootmem_prealloc(void) } } +bool __init hugetlb_cma_enabled(void) +{ + if (IS_ENABLED(CONFIG_CMA)) { + int node; + + for_each_online_node(node) { + if (hugetlb_cma[node]) + return true; + } + } + + return false; +} + static void __init hugetlb_hstate_alloc_pages(struct hstate *h) { unsigned long i; @@ -2571,7 +2585,7 @@ static void __init hugetlb_hstate_alloc_pages(struct hstate *h) for (i = 0; i < h->max_huge_pages; ++i) { if (hstate_is_gigantic(h)) { - if (IS_ENABLED(CONFIG_CMA) && hugetlb_cma[0]) { + if (hugetlb_cma_enabled()) { pr_warn_once("HugeTLB: hugetlb_cma is enabled, skip boot time allocation\n"); break; }
hugetlb_cma[0] can be NULL due to various reasons, for example, node0 has no memory. so NULL hugetlb_cma[0] doesn't necessarily mean cma is not enabled. gigantic pages might have been reserved on other nodes. Fixes: cf11e85fc08c ("mm: hugetlb: optionally allocate gigantic hugepages using cma") Cc: Roman Gushchin <guro@fb.com> Cc: Mike Kravetz <mike.kravetz@oracle.com> Cc: Jonathan Cameron <jonathan.cameron@huawei.com> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> --- -v2: add hugetlb_cma_enabled() helper to improve readability according to Roman mm/hugetlb.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)