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

[Hannes Reinecke <hare@suse.de>]

On 7/1/25 14:09, David Hildenbrand wrote:
> On 01.07.25 13:41, 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.
> 
> I think this should work.
> 
>>
>> 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;
> 
> 
> In stead of that, I suggest we do something like this now, because the 
> function
> no longer makes any sense for hotplugged memory:
> 
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 5c6c1d6bb59f1..fb501df920cec 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -769,21 +769,22 @@ static struct zone 
> *early_node_zone_for_memory_block(struct memory_block *mem,
> 
>   #ifdef CONFIG_NUMA
>   /**
> - * memory_block_add_nid() - Indicate that system RAM falling into this 
> memory
> - *                         block device (partially) belongs to the 
> given node.
> + * memory_block_add_nid_early() - Indicate that early system RAM 
> falling into
> + *                               this memory block device (partially) 
> belongs
> + *                               to the given node.
>    * @mem: The memory block device.
>    * @nid: The node id.
> - * @context: The memory initialization context.
>    *
> - * Indicate that system RAM falling into this memory block (partially) 
> belongs
> - * to the given node. If the context indicates ("early") that we are 
> adding the
> - * node during node device subsystem initialization, this will also 
> properly
> - * set/adjust mem->zone based on the zone ranges of the given node.
> + * Indicate that early system RAM falling into this memory block 
> (partially)
> + * belongs to the given node. This will also properly set/adjust mem- 
>  >zone based
> + * on the zone ranges of the given node.
> + *
> + * Memory hotplug handles this on memory block creation, where we can 
> only have
> + * a single nid span a memory block.
>    */
> -void memory_block_add_nid(struct memory_block *mem, int nid,
> -                         enum meminit_context context)
> +void memory_block_add_nid_early(struct memory_block *mem, int nid)
>   {
> -       if (context == MEMINIT_EARLY && mem->nid != nid) {
> +       if (mem->nid != nid) {
>                  /*
>                   * For early memory we have to determine the zone when 
> setting
>                   * the node id and handle multiple nodes spanning a single
> @@ -836,7 +837,7 @@ static int add_memory_block(unsigned long block_id, 
> unsigned long state,
>                  /*
>                   * MEM_ONLINE at this point implies early memory. With 
> NUMA,
>                   * we'll determine the zone when setting the node id via
> -                * memory_block_add_nid(). Memory hotplug updated the zone
> +                * memory_block_add_nid_early(). Memory hotplug updated 
> the zone
>                   * manually when memory onlining/offlining succeeds.
>                   */
>                  mem->zone = early_node_zone_for_memory_block(mem, 
> NUMA_NO_NODE);
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index bef84f01712f3..6cfda015fdea6 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -786,7 +786,8 @@ static void do_register_memory_block_under_node(int 
> nid,
>   {
>          int ret;
> 
> -       memory_block_add_nid(mem_blk, nid, context);
> +       if (context == MEMINIT_EARLY)
> +               memory_block_add_nid_early(mem_blk, nid);
> 
>          ret = sysfs_create_link_nowarn(&node_devices[nid]->dev.kobj,
>                                         &mem_blk->dev.kobj,
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index 40eb70ccb09d5..bc805205ed258 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -202,8 +202,7 @@ static inline unsigned long 
> phys_to_block_id(unsigned long phys)
>   }
> 
>   #ifdef CONFIG_NUMA
> -void memory_block_add_nid(struct memory_block *mem, int nid,
> -                         enum meminit_context context);
> +void memory_block_add_nid_early(struct memory_block *mem, int nid);
>   #endif /* CONFIG_NUMA */
>   int memory_block_advise_max_size(unsigned long size);
>   unsigned long memory_block_advised_max_size(void);
> 
> 
Sure. I'll add it as a separate patch.

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