Re: [PATCH v3 1/3] driver/base: Optimize memory block registration to reduce boot time

[Donet Tom <donettom@linux.ibm.com>]

On 5/8/25 2:48 PM, David Hildenbrand wrote:
> On 05.05.25 15:24, Mike Rapoport wrote:
>> On Mon, May 05, 2025 at 10:18:43AM +0200, David Hildenbrand wrote:
>>> On 05.05.25 09:53, Mike Rapoport wrote:
>>>> On Mon, May 05, 2025 at 09:38:43AM +0200, David Hildenbrand wrote:
>>>>> On 05.05.25 09:28, Oscar Salvador wrote:
>>>>>> On Mon, May 05, 2025 at 09:16:48AM +0200, David Hildenbrand wrote:
>>>>>>> memory hotplug code never calls register_one_node(), unless I am 
>>>>>>> missing
>>>>>>> something.
>>>>>>>
>>>>>>> During add_memory_resource(), we call __try_online_node(nid, 
>>>>>>> false), meaning
>>>>>>> we skip register_one_node().
>>>>>>>
>>>>>>> The only caller of __try_online_node(nid, true) is 
>>>>>>> try_online_node(), called
>>>>>>> from CPU hotplug code, and I *guess* that is not required.
>>>>>>
>>>>>> Well, I guess this is because we need to link the cpus to the node.
>>>>>> register_one_node() has two jobs: 1) register cpus belonging to 
>>>>>> the node
>>>>>> and 2) register memory-blocks belonging to the node (if any).
>>>>>
>>>>> Ah, via __register_one_node() ...
>>>>>
>>>>> I would assume that an offline node
>>>>>
>>>>> (1) has no memory
>>>>> (2) has no CPUs
>>>>>
>>>>> When we *hotplug* either memory or CPUs, and we first online the 
>>>>> node, there
>>>>> is nothing to register. Because if there would be something, the 
>>>>> node would
>>>>> already be online.
>>>>>
>>>>> In particular, try_offline_node() will only offline a node if
>>>>>
>>>>> (A) No present pages: No pages are spanned anymore. This includes
>>>>>       offline memory blocks.
>>>>> (B) No present CPUs.
>>>>>
>>>>> But maybe there is some case that I am missing ...
>>>>
>>>> I actually hoped you and Oscar know how that stuff works :)
>>>
>>> Well, I know how the memory side works, but the CPU side is giving 
>>> me a hard
>>> time :)
>>>
>>>>
>>>> I tried to figure what is going on there and it all looks really 
>>>> convoluted.
>>>
>>> Jap ...
>>>
>>>>
>>>> So, on boot we have
>>>>     cpu_up() ->
>>>>         try_online_node() ->
>>>>                bails out because all nodes are online (at least on
>>>>             x86 AFAIU, see 1ca75fa7f19d ("arch/x86/mm/numa: Do
>>>>                           not initialize nodes twice"))
>>>>     node_dev_init()i ->
>>>>         register_one_node() ->
>>>>             this one can use __register_one_node() and loop
>>>>             over memblock regions.
>>>>
>>>> And for the hotplug/unplug path, it seems that
>>>> register_memory_blocks_under_node(MEMINIT_EARLY) is superfluous, 
>>>> because if
>>>> a node had memory it wouldn't get offlined, and if we are 
>>>> hotplugging an
>>>> node with memory and cpus, memory hotplug anyway calls
>>>> register_memory_blocks_under_node_hotplug().
>>>>
>>>> So, IMHO, register_one_node() should not call
>>>> register_memory_blocks_under_node() at all, but again, I might have 
>>>> missed
>>>> something :)
>>>
>>> Hm, but someone has to create these links for the memory blocks.
>>
>> My understanding that the links for the memory blocks during hotplug 
>> are created in
>>
>> add_memory_resource()
>>    register_memory_blocks_under_node()
>
> Yes, during hotplug it's exactly that, after registering the node + 
> setting it online.
>
>>
>> So register_one_node() only calls register_memory_blocks_under_node() 
>> when
>> there are no actual memory resources under that node, isn't it?
>
> Except in early boot. That's why register_one_node() has:
>
>     register_memory_blocks_under_node(nid, start_pfn, end_pfn,
>                       MEMINIT_EARLY);
>
>                         ^ early :)
> And that is triggered by
>
>     node_dev_init()->register_one_node()
>
>
>>
>> Then we can drop the call to register_memory_blocks_under_node() from
>> register_one_node() and add creation of memory blocks to 
>> node_dev_init(),
>> i.e.
>>
>> node_dev_init()
>>    for_each_node(nid)
>>      __register_one_node(nid)
>>        for_each_mem_region()
>>          /* create memory block if node matches */
>
> Yes exactly, that makes sense.

Hi Andrew and Mike

Based on the discussion so far, it is clear that the patch will work in all cases,
including when CONFIG_ARCH_KEEP_MEMBLOCK  is disabled. Just checking —
would you prefer to take this version, or should I send a v4?

Thanks
Donet