| 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
| ||
|
Date: Thu, 27 Jan 2022 13:41:16 +0100
From: David Hildenbrand <david@...hat.com>
To: Michal Hocko <mhocko@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: linux-mm@...ck.org, LKML <linux-kernel@...r.kernel.org>,
Alexey Makhalov <amakhalov@...are.com>,
Dennis Zhou <dennis@...nel.org>,
Eric Dumazet <eric.dumazet@...il.com>,
Oscar Salvador <osalvador@...e.de>, Tejun Heo <tj@...nel.org>,
Christoph Lameter <cl@...ux.com>,
Nico Pache <npache@...hat.com>,
Wei Yang <richard.weiyang@...il.com>,
Rafael Aquini <raquini@...hat.com>,
Michal Hocko <mhocko@...e.com>
Subject: Re: [PATCH 2/6] mm: handle uninitialized numa nodes gracefully
On 27.01.22 09:53, Michal Hocko wrote:
> From: Michal Hocko <mhocko@...e.com>
>
> We have had several reports [1][2][3] that page allocator blows up when
> an allocation from a possible node is requested. The underlying reason
> is that NODE_DATA for the specific node is not allocated.
>
> NUMA specific initialization is arch specific and it can vary a lot.
> E.g. x86 tries to initialize all nodes that have some cpu affinity (see
> init_cpu_to_node) but this can be insufficient because the node might be
> cpuless for example.
>
> One way to address this problem would be to check for !node_online nodes
> when trying to get a zonelist and silently fall back to another node.
> That is unfortunately adding a branch into allocator hot path and it
> doesn't handle any other potential NODE_DATA users.
>
> This patch takes a different approach (following a lead of [3]) and it
> pre allocates pgdat for all possible nodes in an arch indipendent code
> - free_area_init. All uninitialized nodes are treated as memoryless
> nodes. node_state of the node is not changed because that would lead to
> other side effects - e.g. sysfs representation of such a node and from
> past discussions [4] it is known that some tools might have problems
> digesting that.
>
> Newly allocated pgdat only gets a minimal initialization and the rest of
> the work is expected to be done by the memory hotplug - hotadd_new_pgdat
> (renamed to hotadd_init_pgdat).
>
> generic_alloc_nodedata is changed to use the memblock allocator because
> neither page nor slab allocators are available at the stage when all
> pgdats are allocated. Hotplug doesn't allocate pgdat anymore so we can
> use the early boot allocator. The only arch specific implementation is
> ia64 and that is changed to use the early allocator as well.
>
> Reported-by: Alexey Makhalov <amakhalov@...are.com>
> Tested-by: Alexey Makhalov <amakhalov@...are.com>
> Reported-by: Nico Pache <npache@...hat.com>
> Acked-by: Rafael Aquini <raquini@...hat.com>
> Tested-by: Rafael Aquini <raquini@...hat.com>
> Signed-off-by: Michal Hocko <mhocko@...e.com>
>
> [1] http://lkml.kernel.org/r/20211101201312.11589-1-amakhalov@vmware.com
> [2] http://lkml.kernel.org/r/20211207224013.880775-1-npache@redhat.com
> [3] http://lkml.kernel.org/r/20190114082416.30939-1-mhocko@kernel.org
> [4] http://lkml.kernel.org/r/20200428093836.27190-1-srikar@linux.vnet.ibm.com
> ---
> arch/ia64/mm/discontig.c | 4 ++--
> include/linux/memory_hotplug.h | 2 +-
> mm/memory_hotplug.c | 21 +++++++++------------
> mm/page_alloc.c | 34 +++++++++++++++++++++++++++++++---
> 4 files changed, 43 insertions(+), 18 deletions(-)
>
> diff --git a/arch/ia64/mm/discontig.c b/arch/ia64/mm/discontig.c
> index 8dc8a554f774..dd0cf4834eaa 100644
> --- a/arch/ia64/mm/discontig.c
> +++ b/arch/ia64/mm/discontig.c
> @@ -608,11 +608,11 @@ void __init paging_init(void)
> zero_page_memmap_ptr = virt_to_page(ia64_imva(empty_zero_page));
> }
>
> -pg_data_t *arch_alloc_nodedata(int nid)
> +pg_data_t * __init arch_alloc_nodedata(int nid)
> {
> unsigned long size = compute_pernodesize(nid);
>
> - return kzalloc(size, GFP_KERNEL);
> + return memblock_alloc(size, SMP_CACHE_BYTES);
I feel like we should have
long arch_pgdat_size(void) instead and have a generic allocation function.
But we can clean that up in the future.
> }
>
> void arch_free_nodedata(pg_data_t *pgdat)
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 4355983b364d..cdd66bfdf855 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -44,7 +44,7 @@ extern void arch_refresh_nodedata(int nid, pg_data_t *pgdat);
> */
> #define generic_alloc_nodedata(nid) \
> ({ \
> - kzalloc(sizeof(pg_data_t), GFP_KERNEL); \
> + memblock_alloc(sizeof(*pgdat), SMP_CACHE_BYTES); \
> })
> /*
> * This definition is just for error path in node hotadd.
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 2a9627dc784c..fc991831d296 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1162,19 +1162,21 @@ static void reset_node_present_pages(pg_data_t *pgdat)
> }
>
> /* we are OK calling __meminit stuff here - we have CONFIG_MEMORY_HOTPLUG */
> -static pg_data_t __ref *hotadd_new_pgdat(int nid)
> +static pg_data_t __ref *hotadd_init_pgdat(int nid)
> {
> struct pglist_data *pgdat;
>
> pgdat = NODE_DATA(nid);
> - if (!pgdat) {
> - pgdat = arch_alloc_nodedata(nid);
> - if (!pgdat)
> - return NULL;
>
> + /*
> + * NODE_DATA is preallocated (free_area_init) but its internal
> + * state is not allocated completely. Add missing pieces.
> + * Completely offline nodes stay around and they just need
> + * reintialization.
> + */
> + if (pgdat->per_cpu_nodestats == &boot_nodestats) {
> pgdat->per_cpu_nodestats =
> alloc_percpu(struct per_cpu_nodestat);
> - arch_refresh_nodedata(nid, pgdat);
> } else {
> int cpu;
> /*
> @@ -1193,8 +1195,6 @@ static pg_data_t __ref *hotadd_new_pgdat(int nid)
> }
> }
>
> - /* we can use NODE_DATA(nid) from here */
> - pgdat->node_id = nid;
> pgdat->node_start_pfn = 0;
>
> /* init node's zones as empty zones, we don't have any present pages.*/
> @@ -1246,7 +1246,7 @@ static int __try_online_node(int nid, bool set_node_online)
> if (node_online(nid))
> return 0;
>
> - pgdat = hotadd_new_pgdat(nid);
> + pgdat = hotadd_init_pgdat(nid);
> if (!pgdat) {
> pr_err("Cannot online node %d due to NULL pgdat\n", nid);
> ret = -ENOMEM;
> @@ -1445,9 +1445,6 @@ int __ref add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags)
>
> return ret;
> error:
> - /* rollback pgdat allocation and others */
> - if (new_node)
> - rollback_node_hotadd(nid);
As static rollback_node_hotadd() is unused in this patch, doesn't this
trigger a warning? IOW, maybe merge at least the rollback_node_hotadd()
removal into this patch. The arch_free_nodedata() removal can stay separate.
> if (IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK))
> memblock_remove(start, size);
> error_mem_hotplug_end:
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 3589febc6d31..1a05669044d3 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6402,7 +6402,11 @@ static void __build_all_zonelists(void *data)
> if (self && !node_online(self->node_id)) {
> build_zonelists(self);
> } else {
> - for_each_online_node(nid) {
> + /*
> + * All possible nodes have pgdat preallocated
... in free_area_init() ?
> + * free_area_init
> + */
> + for_each_node(nid) {
> pg_data_t *pgdat = NODE_DATA(nid);
>
> build_zonelists(pgdat);
> @@ -8096,8 +8100,32 @@ void __init free_area_init(unsigned long *max_zone_pfn)
> /* Initialise every node */
> mminit_verify_pageflags_layout();
> setup_nr_node_ids();
> - for_each_online_node(nid) {
> - pg_data_t *pgdat = NODE_DATA(nid);
> + for_each_node(nid) {
> + pg_data_t *pgdat;
> +
> + if (!node_online(nid)) {
> + pr_warn("Node %d uninitialized by the platform. Please report with boot dmesg.\n", nid);
> +
> + /* Allocator not initialized yet */
> + pgdat = arch_alloc_nodedata(nid);
> + if (!pgdat) {
> + pr_err("Cannot allocate %zuB for node %d.\n",
> + sizeof(*pgdat), nid);
> + continue;
> + }
> + arch_refresh_nodedata(nid, pgdat);
We could get rid of arch_refresh_nodedata() now and simply merge that
into arch_alloc_nodedata(). But depends on how we want to proceed with
arch_alloc_nodedata() eventually.
Acked-by: David Hildenbrand <david@...hat.com>
--
Thanks,
David / dhildenb
Powered by blists - more mailing lists