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

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

Hi Hannes Reinecke

On 7/1/25 5:11 PM, 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.

Since this fixes the issue, should we consider adding a Fixes: tag?

> 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;
>   }
>   #endif
>   
> @@ -892,7 +891,7 @@ static void remove_memory_block(struct memory_block *memory)
>    * Called under device_hotplug_lock.
>    */
>   int create_memory_block_devices(unsigned long start, unsigned long size,
> -				struct vmem_altmap *altmap,
> +				int nid, struct vmem_altmap *altmap,
>   				struct memory_group *group)
>   {
>   	const unsigned long start_block_id = pfn_to_block_id(PFN_DOWN(start));
> @@ -906,7 +905,7 @@ int create_memory_block_devices(unsigned long start, unsigned long size,
>   		return -EINVAL;
>   
>   	for (block_id = start_block_id; block_id != end_block_id; block_id++) {
> -		ret = add_memory_block(block_id, NUMA_NO_NODE, MEM_OFFLINE, altmap, group);
> +		ret = add_memory_block(block_id, nid, MEM_OFFLINE, altmap, group);
>   		if (ret)
>   			break;
>   	}
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index 5ec4e6d209b9..d7c3a4856031 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -161,7 +161,7 @@ static inline unsigned long memory_block_advised_max_size(void)
>   extern int register_memory_notifier(struct notifier_block *nb);
>   extern void unregister_memory_notifier(struct notifier_block *nb);
>   int create_memory_block_devices(unsigned long start, unsigned long size,
> -				struct vmem_altmap *altmap,
> +				int nid, struct vmem_altmap *altmap,
>   				struct memory_group *group);
>   void remove_memory_block_devices(unsigned long start, unsigned long size);
>   extern void memory_dev_init(void);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index b1caedbade5b..204bd6f19d8d 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1478,7 +1478,7 @@ static int create_altmaps_and_memory_blocks(int nid, struct memory_group *group,
>   		}
>   
>   		/* create memory block devices after memory was added */
> -		ret = create_memory_block_devices(cur_start, memblock_size,
> +		ret = create_memory_block_devices(cur_start, memblock_size, nid,
>   						  params.altmap, group);
>   		if (ret) {
>   			arch_remove_memory(cur_start, memblock_size, NULL);
> @@ -1540,8 +1540,16 @@ int add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags)
>   
>   	ret = __try_online_node(nid, false);
>   	if (ret < 0)
> -		goto error;
> -	new_node = ret;
> +		goto error_memblock_remove;
> +	if (ret) {
> +		node_set_online(nid);
> +		ret = __register_one_node(nid);
> +		if (WARN_ON(ret)) {
> +			node_set_offline(nid);
> +			goto error_memblock_remove;
> +		}
> +		new_node = true;
> +	}
>   
>   	/*
>   	 * Self hosted memmap array
> @@ -1557,24 +1565,13 @@ int add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags)
>   			goto error;
>   
>   		/* create memory block devices after memory was added */
> -		ret = create_memory_block_devices(start, size, NULL, group);
> +		ret = create_memory_block_devices(start, size, nid, NULL, group);
>   		if (ret) {
>   			arch_remove_memory(start, size, params.altmap);
>   			goto error;
>   		}
>   	}
>   
> -	if (new_node) {
> -		/* If sysfs file of new node can't be created, cpu on the node
> -		 * can't be hot-added. There is no rollback way now.
> -		 * So, check by BUG_ON() to catch it reluctantly..
> -		 * We online node here. We can't roll back from here.
> -		 */
> -		node_set_online(nid);
> -		ret = __register_one_node(nid);
> -		BUG_ON(ret);
> -	}
> -
>   	register_memory_blocks_under_node(nid, PFN_DOWN(start),
>   					  PFN_UP(start + size - 1),
>   					  MEMINIT_HOTPLUG);
> @@ -1599,6 +1596,11 @@ int add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags)
>   
>   	return ret;
>   error:
> +	if (new_node) {
> +		node_set_offline(nid);
> +		unregister_one_node(nid);
> +	}
> +error_memblock_remove:
>   	if (IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK))
>   		memblock_remove(start, size);
>   error_mem_hotplug_end: