On Thu, Mar 11, 2021 at 4:53 PM Michal Hocko [off-list ref] wrote:

On Thu 11-03-21 16:45:51, Muchun Song wrote:

quoted

On Thu, Mar 11, 2021 at 10:58 AM Muchun Song [off-list ref] wrote:

quoted

On Wed, Mar 10, 2021 at 10:14 PM Michal Hocko [off-list ref] wrote:

quoted

[I am sorry for a late review]

Thanks for your review.

quoted

On Mon 08-03-21 18:27:59, Muchun Song wrote:

quoted

Move bootmem info registration common API to individual bootmem_info.c.
And we will use {get,put}_page_bootmem() to initialize the page for the
vmemmap pages or free the vmemmap pages to buddy in the later patch.
So move them out of CONFIG_MEMORY_HOTPLUG_SPARSE. This is just code
movement without any functional change.

Signed-off-by: Muchun Song <redacted>
Acked-by: Mike Kravetz <redacted>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
Reviewed-by: David Hildenbrand <redacted>
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
Tested-by: Chen Huang <redacted>
Tested-by: Bodeddula Balasubramaniam <redacted>

Separation from memory_hotplug.c is definitely a right step. I am
wondering about the config dependency though
[...]

quoted

diff --git a/mm/Makefile b/mm/Makefile
index 72227b24a616..daabf86d7da8 100644
--- a/mm/Makefile
+++ b/mm/Makefile

@@ -83,6 +83,7 @@ obj-$(CONFIG_SLUB) += slub.o
 obj-$(CONFIG_KASAN)  += kasan/
 obj-$(CONFIG_KFENCE) += kfence/
 obj-$(CONFIG_FAILSLAB) += failslab.o
+obj-$(CONFIG_HAVE_BOOTMEM_INFO_NODE) += bootmem_info.o

I would have expected this would depend on CONFIG_SPARSE.
BOOTMEM_INFO_NODE is really an odd thing to depend on here. There is
some functionality which requires the node info but that can be gated
specifically. Or what is the thinking behind?

I have tried this. And I find that it is better to depend on
BOOTMEM_INFO_NODE instead of SPARSEMEM.

If we enable SPARSEMEM but disable HAVE_BOOTMEM_INFO_NODE,
the bootmem_info.c also is compiled. Actually, we do not
need those functions on other architectures. And these
functions are also related to bootmem info. So it may be
more reasonable to depend on BOOTMEM_INFO_NODE.
Just my thoughts.

If BOOTMEM_INFO_NODE is disbabled then bootmem_info.c would be
effectivelly only {get,put}_page_bootmem, no?

{get,put}_page_bootmem also would be effective. I found that
get_page_bootmem is only used in the scope of the
CONFIG_BOOTMEM_INFO_NODE. So I move them
to the bootmem_info.c.

Thanks.

--
Michal Hocko
SUSE Labs