Re: [PATCH 2/3] mm/memory_hotplug: activate node before adding new memory blocks

[Hannes Reinecke <hare@suse.de>]

On 7/3/25 13:29, David Hildenbrand wrote:
> On 02.07.25 09:39, Hannes Reinecke wrote:
>> The sysfs attributes for memory blocks require the node ID to be
>> set and initialized, so move the node activation before adding
>> new memory blocks. This also has the nice side effect that the
>> BUG_ON() can be converted into a WARN_ON() as we now can handle
>> registration errors.
>>
>> Fixes: b9ff036082cd ("mm/memory_hotplug.c: make add_memory_resource 
>> use __try_online_node")
> 
> We should CC stable.
> 
Yeah, that's probably not a bad idea.

>> Signed-off-by: Hannes Reinecke <hare@kernel.org>
>> ---
>>   drivers/base/memory.c  | 19 +++++++++----------
>>   include/linux/memory.h |  2 +-
>>   mm/memory_hotplug.c    | 32 +++++++++++++++++---------------
>>   3 files changed, 27 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index 2b951e5f8a27..d24a90e0ea96 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -810,15 +810,14 @@ void memory_block_add_nid(struct memory_block 
>> *mem, int nid,
>>               mem->zone = early_node_zone_for_memory_block(mem, nid);
>>           else
>>               mem->zone = NULL;
>> +        /*
>> +         * If this memory block spans multiple nodes, we only indicate
>> +         * the last processed node. If we span multiple nodes (not 
>> applicable
>> +         * to hotplugged memory), zone == NULL will prohibit memory 
>> offlining
>> +         * and consequently unplug.
>> +         */
>> +        mem->nid = nid;
>>       }
>> -
>> -    /*
>> -     * If this memory block spans multiple nodes, we only indicate
>> -     * the last processed node. If we span multiple nodes (not 
>> applicable
>> -     * to hotplugged memory), zone == NULL will prohibit memory 
>> offlining
>> -     * and consequently unplug.
>> -     */
>> -    mem->nid = nid;
> 
> I would not perform this change in this patch. It's not required here, 
> right? We're simply re-setting the nid. Patch #3 can clean that up.
> 

Well. You tell me :-)

Thing is, we already have a 'nid' value as argument to
add_memory_resource().
So I would _think_ that it should be the same value which we end up
with in memory_block_add_nid().
But if that's the case we could've set it during memblock
initialization, which we don't. So I thought there must be a reason
for that, and the only reason I could think of is that 'nid' is not
known during memblock initialization, ie that the 'nid' value in
add_memory_resource() is a different 'nid' value from the one used in
memory_block_add_nid().
If so then we cannot leave out.
If not, then indeed, we can leave it out.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich