mm: hugetlb: fix hugetlb_cma_reserve() if CONFIG_NUMA isn't set
Subsystems (2)
PATCH 1/1 mm: hugetlb: fix hugetlb_cma_reserve() if CONFIG_NUMA isn't set
If CONFIG_NUMA isn't set, there is no need to ensure that the hugetlb cma area belongs to a specific numa node.
min/max_low_pfn can be used for limiting the maximum size of the hugetlb_cma area.
Also for_each_mem_pfn_range() is defined only if CONFIG_HAVE_MEMBLOCK_NODE_MAP is set, and on arm (unlike most other architectures) it depends on CONFIG_NUMA. This makes the build fail if CONFIG_NUMA isn't set.
--- mm/hugetlb.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
mm/hugetlb.c +8 -3
@@ -5439,16 +5439,21 @@ void __init hugetlb_cma_reserve(int order) reserved =0 ;for_each_node_state (nid,N_ONLINE ) { -unsigned long start_pfn, end_pfn;unsigned long min_pfn =0 , max_pfn =0 ; -int res, i; +int res; +#ifdef CONFIG_NUMA +unsigned long start_pfn, end_pfn; +int i;for_each_mem_pfn_range (i, nid, &start_pfn, &end_pfn,NULL ) {if (!min_pfn) min_pfn = start_pfn; max_pfn = end_pfn; } - +#else + min_pfn = min_low_pfn; + max_pfn = max_low_pfn; +#endif size =max (per_node, hugetlb_cma_size - reserved); size =round_up (size,PAGE_SIZE << order);
Signature · 1 line
2.24.1
Quoted 10 lines · Roman Gushchin
If
CONFIG_NUMAisn't set, there is no need to ensure that the hugetlb cma area belongs to a specific numa node.min/max_low_pfn can be used for limiting the maximum size of the
hugetlb_cmaarea.Also
for_each_mem_pfn_range()is defined only ifCONFIG_HAVE_MEMBLOCK_NODE_MAPis set, and on arm (unlike most other architectures) it depends onCONFIG_NUMA. This makes the build fail ifCONFIG_NUMAisn't set.
CONFIG_HAVE_MEMBLOCK_NODE_MAP has popped out as a problem several times already. Is there any real reason we cannot make it unconditional? Essentially make the functionality always enabled and drop the config? The code below is ugly as hell. Just look at it. You have for_each_node_state without any ifdefery but the having ifdef CONFIG_NUMA. That just doesn't make any sense.
Quoted 37 lines
Signed-off-by Roman Gushchin <[email protected]>Reported-by Andreas Schaufler <[email protected]>--- mm/hugetlb.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)mm/hugetlb.c +8
-3@@ -5439,16 +5439,21 @@ void __init hugetlb_cma_reserve(int order) reserved =0 ;for_each_node_state (nid,N_ONLINE ) { -unsigned long start_pfn, end_pfn;unsigned long min_pfn =0 , max_pfn =0 ; -int res, i; +int res; +#ifdef CONFIG_NUMA +unsigned long start_pfn, end_pfn; +int i;for_each_mem_pfn_range (i, nid, &start_pfn, &end_pfn,NULL ) {if (!min_pfn) min_pfn = start_pfn; max_pfn = end_pfn; } - +#else + min_pfn = min_low_pfn; + max_pfn = max_low_pfn; +#endif size =max (per_node, hugetlb_cma_size - reserved); size =round_up (size,PAGE_SIZE << order);Signature · 1 line
2.24.1
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 7 lines · Michal Hocko
Quoted 6 lines · Roman Gushchin
If
CONFIG_NUMAisn't set, there is no need to ensure that the hugetlb cma area belongs to a specific numa node.min/max_low_pfn can be used for limiting the maximum size of the
hugetlb_cmaarea.
Quoted 8 lines
Quoted 4 lines
Also
for_each_mem_pfn_range()is defined only ifCONFIG_HAVE_MEMBLOCK_NODE_MAPis set, and on arm (unlike most other architectures) it depends onCONFIG_NUMA. This makes the build fail ifCONFIG_NUMAisn't set.
CONFIG_HAVE_MEMBLOCK_NODE_MAPhas popped out as a problem several times already. Is there any real reason we cannot make it unconditional? Essentially make the functionality always enabled and drop the config?
It depends on CONFIG_NUMA only on arm, and I really don't know if there is a good justification for it. It not, that will be a much simpler fix.
Quoted 3 lines
The code below is ugly as hell. Just look at it. You have
for_each_node_statewithout any ifdefery but the having ifdefCONFIG_NUMA. That just doesn't make any sense.
I don't think it makes no sense:
it tries to reserve a cma area on each node (need for_each_node_state()), and it uses the for_each_mem_pfn_range() to get a min and max pfn for each node. With !CONFIG_NUMA the first part is reduced to one iteration and the second part is not required at all.
I agree that for_each_mem_pfn_range() here looks quite ugly, but I don't know of a better way to get min/max pfns for a node so early in the boot process. If somebody has any ideas here, I'll appreciate a lot.
I know Rik plans some further improvements here, so the goal for now is to fix the build. If you think that enabling CONFIG_HAVE_MEMBLOCK_NODE_MAP unconditionally is a way to go, I'm fine with it too.
Rik also posted a different fix for the build problem, but from what I've seen it didn't fix it completely. I'm fine with either option here.
Thanks!
Quoted 43 lines
Quoted 38 lines
Signed-off-by Roman Gushchin <[email protected]>Reported-by Andreas Schaufler <[email protected]>--- mm/hugetlb.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)mm/hugetlb.c +8
-3@@ -5439,16 +5439,21 @@ void __init hugetlb_cma_reserve(int order) reserved =0 ;for_each_node_state (nid,N_ONLINE ) { -unsigned long start_pfn, end_pfn;unsigned long min_pfn =0 , max_pfn =0 ; -int res, i; +int res; +#ifdef CONFIG_NUMA +unsigned long start_pfn, end_pfn; +int i;for_each_mem_pfn_range (i, nid, &start_pfn, &end_pfn,NULL ) {if (!min_pfn) min_pfn = start_pfn; max_pfn = end_pfn; } - +#else + min_pfn = min_low_pfn; + max_pfn = max_low_pfn; +#endif size =max (per_node, hugetlb_cma_size - reserved); size =round_up (size,PAGE_SIZE << order);Signature · 1 line
2.24.1Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 13 lines · Roman Gushchin
Quoted 12 lines · Michal Hocko
Quoted 10 lines · Roman Gushchin
If
CONFIG_NUMAisn't set, there is no need to ensure that the hugetlb cma area belongs to a specific numa node.min/max_low_pfn can be used for limiting the maximum size of the
hugetlb_cmaarea.Also
for_each_mem_pfn_range()is defined only ifCONFIG_HAVE_MEMBLOCK_NODE_MAPis set, and on arm (unlike most other architectures) it depends onCONFIG_NUMA. This makes the build fail ifCONFIG_NUMAisn't set.
Quoted 7 lines
Quoted 3 lines
CONFIG_HAVE_MEMBLOCK_NODE_MAPhas popped out as a problem several times already. Is there any real reason we cannot make it unconditional? Essentially make the functionality always enabled and drop the config?It depends on
CONFIG_NUMAonly on arm, and I really don't know if there is a good justification for it. It not, that will be a much simpler fix.
I have checked the history and the dependency is there since NUMA was introduced in arm64. So it would be great to double check with arch maintainers.
Quoted 9 lines
Quoted 3 lines
The code below is ugly as hell. Just look at it. You have
for_each_node_statewithout any ifdefery but the having ifdefCONFIG_NUMA. That just doesn't make any sense.I don't think it makes no sense:
it tries to reserve a cma area on each node (needfor_each_node_state()), and it uses thefor_each_mem_pfn_range()to get a min and max pfn for each node. With !CONFIG_NUMA the first part is reduced to one iteration and the second part is not required at all.
Sure the resulting code logic makes sense. I meant that it doesn't make much sense wrt readability. There is a loop over all existing numa nodes to have ifdef for NUMA inside the loop. See?
Quoted 3 lines
I agree that
for_each_mem_pfn_range()here looks quite ugly, but I don't know of a better way to get min/max pfns for a node so early in the boot process. If somebody has any ideas here, I'll appreciate a lot.
The loop is ok. Maybe we have other memblock API that would be better but I am not really aware of it from top of my head. I would stick with it. It just sucks that this API depends on HAVE_MEMBLOCK_NODE_MAP and that it is not generally available. This is what I am complaining about. Just look what kind of dirty hack it made you to create ;)
Quoted 3 lines
I know Rik plans some further improvements here, so the goal for now is to fix the build. If you think that enabling
CONFIG_HAVE_MEMBLOCK_NODE_MAPunconditionally is a way to go, I'm fine with it too.
This is not the first time HAVE_MEMBLOCK_NODE_MAP has been problematic. I might be missing something but I really do not get why do we really need it these days. As for !NUMA, I suspect we can make it generate the right thing when !NUMA.
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 7 lines · Michal Hocko
This is not the first time
HAVE_MEMBLOCK_NODE_MAPhas been problematic.
I might be missing something but I really do not get why do we really need it these days. As for !NUMA, I suspect we can make it generate the
right thing when !NUMA.
We're working on a different fix now.
It looks like cma_declare_contiguous calls memblock_phys_alloc_range, which calls memblock_alloc_range_nid, which takes a NUMA node as one of its arguments.
Aslan is looking at simply adding a cma_declare_contiguous_nid, which also takes a NUMA node ID as an argument. At that point we can simply leave CMA free to allocate from anywhere in each NUMA node, which by default already happens from the top down.
That should be the nicer long term fix to this issue.
Signature · 1 line
All Rights Reversed.
Quoted 15 lines · Rik van Riel
Quoted 7 lines · Michal Hocko
This is not the first time
HAVE_MEMBLOCK_NODE_MAPhas been problematic.
I might be missing something but I really do not get why do we really need it these days. As for !NUMA, I suspect we can make it generate the
right thing when !NUMA.We're working on a different fix now.
It looks like
cma_declare_contiguouscallsmemblock_phys_alloc_range, which callsmemblock_alloc_range_nid, which takes a NUMA node as one of its arguments.
Quoted 6 lines
Aslan is looking at simply adding a
cma_declare_contiguous_nid, which also takes a NUMA node ID as an argument. At that point we can simply leave CMA free to allocate from anywhere in each NUMA node, which by default already happens from the top down.That should be the nicer long term fix to this issue.
Yes, that sounds like a better solution. Not that I would be much happier to have HAVE_MEMBLOCK_NODE_MAP off the table as well ;) Ohh well, it will have to remain on my todo list for some longer.
Signature · 2 lines
Michal Hocko SUSE Labs