| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZpuuYnMCd8RRZEcH@kernel.org>
Date: Sat, 20 Jul 2024 15:32:34 +0300
From: Mike Rapoport <rppt@...nel.org>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: linux-kernel@...r.kernel.org,
Alexander Gordeev <agordeev@...ux.ibm.com>,
Andreas Larsson <andreas@...sler.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Arnd Bergmann <arnd@...db.de>, Borislav Petkov <bp@...en8.de>,
Catalin Marinas <catalin.marinas@....com>,
Christophe Leroy <christophe.leroy@...roup.eu>,
Dan Williams <dan.j.williams@...el.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
David Hildenbrand <david@...hat.com>,
"David S. Miller" <davem@...emloft.net>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Heiko Carstens <hca@...ux.ibm.com>,
Huacai Chen <chenhuacai@...nel.org>, Ingo Molnar <mingo@...hat.com>,
Jiaxun Yang <jiaxun.yang@...goat.com>,
John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>,
Michael Ellerman <mpe@...erman.id.au>,
Palmer Dabbelt <palmer@...belt.com>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Rob Herring <robh@...nel.org>,
Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
Thomas Gleixner <tglx@...utronix.de>,
Vasily Gorbik <gor@...ux.ibm.com>, Will Deacon <will@...nel.org>,
linux-arm-kernel@...ts.infradead.org, loongarch@...ts.linux.dev,
linux-mips@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
linux-riscv@...ts.infradead.org, linux-s390@...r.kernel.org,
linux-sh@...r.kernel.org, sparclinux@...r.kernel.org,
linux-acpi@...r.kernel.org, linux-cxl@...r.kernel.org,
nvdimm@...ts.linux.dev, devicetree@...r.kernel.org,
linux-arch@...r.kernel.org, linux-mm@...ck.org, x86@...nel.org
Subject: Re: [PATCH 15/17] mm: make numa_memblks more self-contained
On Fri, Jul 19, 2024 at 07:07:12PM +0100, Jonathan Cameron wrote:
> On Tue, 16 Jul 2024 14:13:44 +0300
> Mike Rapoport <rppt@...nel.org> wrote:
>
> > From: "Mike Rapoport (Microsoft)" <rppt@...nel.org>
> >
> > 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?
Yes, arch_numa sets it to false. I'll add a note in the changelog.
> 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.
Ok.
> >
> > Signed-off-by: Mike Rapoport (Microsoft) <rppt@...nel.org>
> > ---
> > 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.
Will do.
> > + 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?
I'll drop it for now.
> > 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 *@...emask.
> > + */
> > +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.
Before the move numa_nodemask_from_meminfo() was global so it was ok to
define it after its callers.
I'll split this into a separate commit.
--
Sincerely yours,
Mike.
Powered by blists - more mailing lists