On Tue, 16 Jul 2024 14:13:44 +0300
Mike Rapoport <rppt@xxxxxxxxxx> wrote:
> From: "Mike Rapoport (Microsoft)" <rppt@xxxxxxxxxx>
>
> Introduce numa_memblks_init() and move some code around to avoid several
> global variables in numa_memblks.
Hi Mike,
Adding the effectively always on memblock_force_top_down
deserves a comment on why. I assume because you are going to do
something with it later?
There also seems to be more going on in here such as the change to
get_pfn_range_for_nid() Perhaps break this up so each
change can have an explanation.
>
> Signed-off-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>
> ---
> arch/x86/mm/numa.c | 53 ++++---------------------
> include/linux/numa_memblks.h | 9 +----
> mm/numa_memblks.c | 77 +++++++++++++++++++++++++++---------
> 3 files changed, 68 insertions(+), 71 deletions(-)
>
> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index 3848e68d771a..16bc703c9272 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -115,30 +115,19 @@ void __init setup_node_to_cpumask_map(void)
> pr_debug("Node to cpumask map for %u nodes\n", nr_node_ids);
> }
>
> -static int __init numa_register_memblks(struct numa_meminfo *mi)
> +static int __init numa_register_nodes(void)
> {
> - int i, nid, err;
> -
> - err = numa_register_meminfo(mi);
> - if (err)
> - return err;
> + int nid;
>
> if (!memblock_validate_numa_coverage(SZ_1M))
> return -EINVAL;
>
> /* Finally register nodes. */
> for_each_node_mask(nid, node_possible_map) {
> - u64 start = PFN_PHYS(max_pfn);
> - u64 end = 0;
> -
> - for (i = 0; i < mi->nr_blks; i++) {
> - if (nid != mi->blk[i].nid)
> - continue;
> - start = min(mi->blk[i].start, start);
> - end = max(mi->blk[i].end, end);
> - }
> + unsigned long start_pfn, end_pfn;
>
> - if (start >= end)
> + get_pfn_range_for_nid(nid, &start_pfn, &end_pfn);
It's not immediately obvious to me that this code is equivalent so I'd
prefer it in a separate patch with some description of why
it is a valid change.
> + if (start_pfn >= end_pfn)
> continue;
>
> alloc_node_data(nid);
> @@ -178,39 +167,11 @@ static int __init numa_init(int (*init_func)(void))
> for (i = 0; i < MAX_LOCAL_APIC; i++)
> set_apicid_to_node(i, NUMA_NO_NODE);
>
> - nodes_clear(numa_nodes_parsed);
> - nodes_clear(node_possible_map);
> - nodes_clear(node_online_map);
> - memset(&numa_meminfo, 0, sizeof(numa_meminfo));
> - WARN_ON(memblock_set_node(0, ULLONG_MAX, &memblock.memory,
> - NUMA_NO_NODE));
> - WARN_ON(memblock_set_node(0, ULLONG_MAX, &memblock.reserved,
> - NUMA_NO_NODE));
> - /* In case that parsing SRAT failed. */
> - WARN_ON(memblock_clear_hotplug(0, ULLONG_MAX));
> - numa_reset_distance();
> -
> - ret = init_func();
> - if (ret < 0)
> - return ret;
> -
> - /*
> - * We reset memblock back to the top-down direction
> - * here because if we configured ACPI_NUMA, we have
> - * parsed SRAT in init_func(). It is ok to have the
> - * reset here even if we did't configure ACPI_NUMA
> - * or acpi numa init fails and fallbacks to dummy
> - * numa init.
> - */
> - memblock_set_bottom_up(false);
> -
> - ret = numa_cleanup_meminfo(&numa_meminfo);
> + ret = numa_memblks_init(init_func, /* memblock_force_top_down */ true);
The comment in parameter list seems unnecessary.
Maybe add a comment above the call instead if need to call that out?
> if (ret < 0)
> return ret;
>
> - numa_emulation(&numa_meminfo, numa_distance_cnt);
> -
> - ret = numa_register_memblks(&numa_meminfo);
> + ret = numa_register_nodes();
> if (ret < 0)
> return ret;
>
> diff --git a/mm/numa_memblks.c b/mm/numa_memblks.c
> index e0039549aaac..640f3a3ce0ee 100644
> --- a/mm/numa_memblks.c
> +++ b/mm/numa_memblks.c
> @@ -7,13 +7,27 @@
> #include <linux/numa.h>
> #include <linux/numa_memblks.h>
>
> +/*
> + * Set nodes, which have memory in @mi, in *@nodemask.
> + */
> +static void __init numa_nodemask_from_meminfo(nodemask_t *nodemask,
> + const struct numa_meminfo *mi)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(mi->blk); i++)
> + if (mi->blk[i].start != mi->blk[i].end &&
> + mi->blk[i].nid != NUMA_NO_NODE)
> + node_set(mi->blk[i].nid, *nodemask);
> +}
The code move doesn't have an obvious purpose. Maybe call that
out in the patch description if it is needed for a future patch.
Or do it in two goes so first just adds the static, 2nd shuffles
the code.
>
> /**
> * numa_reset_distance - Reset NUMA distance table
> @@ -287,20 +301,6 @@ int __init numa_cleanup_meminfo(struct numa_meminfo *mi)
> return 0;
> }
>
> -/*
> - * Set nodes, which have memory in @mi, in *@nodemask.
> - */
> -void __init numa_nodemask_from_meminfo(nodemask_t *nodemask,
> - const struct numa_meminfo *mi)
> -{
> - int i;
> -
> - for (i = 0; i < ARRAY_SIZE(mi->blk); i++)
> - if (mi->blk[i].start != mi->blk[i].end &&
> - mi->blk[i].nid != NUMA_NO_NODE)
> - node_set(mi->blk[i].nid, *nodemask);
> -}
> -
> /*
> * Mark all currently memblock-reserved physical memory (which covers the
> * kernel's own memory ranges) as hot-unswappable.
> @@ -368,7 +368,7 @@ static void __init numa_clear_kernel_node_hotplug(void)
> }
> }
>
> -int __init numa_register_meminfo(struct numa_meminfo *mi)
> +static int __init numa_register_meminfo(struct numa_meminfo *mi)
> {
> int i;
>
> @@ -412,6 +412,47 @@ int __init numa_register_meminfo(struct numa_meminfo *mi)
> return 0;
> }
>
> +int __init numa_memblks_init(int (*init_func)(void),
> + bool memblock_force_top_down)
> +{
> + int ret;
> +
> + nodes_clear(numa_nodes_parsed);
> + nodes_clear(node_possible_map);
> + nodes_clear(node_online_map);
> + memset(&numa_meminfo, 0, sizeof(numa_meminfo));
> + WARN_ON(memblock_set_node(0, ULLONG_MAX, &memblock.memory,
> + NUMA_NO_NODE));
> + WARN_ON(memblock_set_node(0, ULLONG_MAX, &memblock.reserved,
> + NUMA_NO_NODE));
> + /* In case that parsing SRAT failed. */
> + WARN_ON(memblock_clear_hotplug(0, ULLONG_MAX));
> + numa_reset_distance();
> +
> + ret = init_func();
> + if (ret < 0)
> + return ret;
> +
> + /*
> + * We reset memblock back to the top-down direction
> + * here because if we configured ACPI_NUMA, we have
> + * parsed SRAT in init_func(). It is ok to have the
> + * reset here even if we did't configure ACPI_NUMA
> + * or acpi numa init fails and fallbacks to dummy
> + * numa init.
> + */
> + if (memblock_force_top_down)
> + memblock_set_bottom_up(false);
> +
> + ret = numa_cleanup_meminfo(&numa_meminfo);
> + if (ret < 0)
> + return ret;
> +
> + numa_emulation(&numa_meminfo, numa_distance_cnt);
> +
> + return numa_register_meminfo(&numa_meminfo);
> +}
> +
> static int __init cmp_memblk(const void *a, const void *b)
> {
> const struct numa_memblk *ma = *(const struct numa_memblk **)a;
