by David Hildenbrand

[permalink] [raw]

Subject: Re: [PATCH] mm: fix panic in __alloc_pages

>>> In add_memory_resource() we hotplug the new node if required and set it
>>> online. Memory might get onlined later, via online_pages().
>>
>> You are correct. In case of memory hot add, it is true. But in case of adding
>> CPU with memoryless node, try_node_online() will be called only during CPU
>> onlining, see cpu_up().
>>
>> Is there any reason why try_online_node() resides in cpu_up() and not in add_cpu()?
>> I think it would be correct to online node during the CPU hot add to align with
>> memory hot add.
>
> I am not familiar with cpu hotplug, but this doesn't seem to be anything
> new so how come this became problem only now?

So IIUC, the issue is that we have a node

a) That has no memory
b) That is offline

This node will get onlined when onlining the CPU as Alexey says. Yet we
have some code that stumbles over the node and goes ahead trying to use
the pgdat -- that code is broken.

If we take a look at build_zonelists() we indeed skip any
!node_online(node). Any other code should do the same. If the node is
not online, it shall be ignored because we might not even have a pgdat
yet -- see hotadd_new_pgdat(). Without node_online(), the pgdat might be
stale or non-existant.

The node onlining logic when onlining a CPU sounds bogus as well: Let's
take a look at try_offline_node(). It checks that:
1) That no memory is *present*
2) That no CPU is *present*

We should online the node when adding the CPU ("present"), not when
onlining the CPU.

--
Thanks,

David / dhildenb

2021-11-02 09:41:48

On 02.11.21 15:12, Michal Hocko wrote:
> On Tue 02-11-21 14:41:25, David Hildenbrand wrote:
>> On 02.11.21 14:25, Michal Hocko wrote:
> [...]
>>> Btw. do you plan to send a patch for pcp allocator to use cpu_to_mem?
>>
>> You mean s/cpu_to_node/cpu_to_mem/ or also handling offline nids?
>
> just cpu_to_mem
>
>> cpu_to_mem() corresponds to cpu_to_node() unless on ia64+ppc IIUC, so it
>> won't help for this very report.
>
> Weird, x86 allows memory less nodes as well. But you are right
> there is nothing selecting HAVE_MEMORYLESS_NODES neither do I see any
> arch specific implementation. I have to say that I have forgot all those
> nasty details... Sigh
>

I assume HAVE_MEMORYLESS_NODES is just an optimization to set a
preferred memory node for memoryless nodes. It doesn't imply that we
cannot have memoryless nodes otherwise.

I suspect just as so often, the config option name doesn't express what
it really does.

--
Thanks,

David / dhildenb

2021-11-08 11:06:40

by Alexey Makhalov

[permalink] [raw]

Subject: Re: [PATCH] mm: fix panic in __alloc_pages

I’m going to send patch v2, with node_online check moved to caller (pcpu_alloc_pages() function)
as was suggested by David. It seems as it is only one place which passes present but offlined
node to alloc-pages_node(). Moving node online check to the caller keeps hot path (alloc_pages)
simple and performant.

> diff --git a/mm/percpu-vm.c b/mm/percpu-vm.c
> index 2054c9213c43..c21ff5bb91dc 100644
> --- a/mm/percpu-vm.c
> +++ b/mm/percpu-vm.c
> @@ -84,15 +84,19 @@ static int pcpu_alloc_pages(struct pcpu_chunk *chunk,
> gfp_t gfp)
> {
> unsigned int cpu, tcpu;
> - int i;
> + int i, nid;
>
> gfp |= __GFP_HIGHMEM;
>
> for_each_possible_cpu(cpu) {
> + nid = cpu_to_node(cpu);
> +
> + if (nid == NUMA_NO_NODE || !node_online(nid))
> + nid = numa_mem_id();
> for (i = page_start; i < page_end; i++) {
> struct page **pagep = &pages[pcpu_page_idx(cpu, i)];
>
> - *pagep = alloc_pages_node(cpu_to_node(cpu), gfp, 0);
> + *pagep = alloc_pages_node(nid, gfp, 0);
> if (!*pagep)
> goto err;
> }
>

Thanks,
—Alexey

Attachments:

signature.asc (849.00 B)
Message signed with OpenPGP