[PATCH 0/2] mm/memory_hotplug: fixup crash during uevent handling

[PATCH 0/2] mm/memory_hotplug: fixup crash during uevent handling

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.de>

Hi all,

we have some udev rules trying to read the sysfs attribute 'valid_zones' during
an memory 'add' event, causing a crash in zone_for_pfn_range(). Debugging found
that mem->nid was set to NUMA_NO_NODE, which crashed in NODE_DATA(nid).
Further analysis revealed that we're running into a race with udev event
processing: add_memory_resource() has this function calls:

1) __try_online_node()
2) arch_add_memory()
3) create_memory_block_devices()
  -> calls device_register() -> memory 'add' event
4) node_set_online()/__register_one_node()
  -> calls device_register() -> node 'add' event
5) register_memory_blocks_under_node()
  -> sets mem->nid

Which, to the uninitated, is ... weird ...

Why do we try to online the node in 1), but only register
the node in 4) _after_ we have created the memory blocks in 3) ?
And why do we set the 'nid' value in 5), when the uevent
(which might need to see the correct 'nid' value) is sent out
in 3) ?
There must be a reason, I'm sure ...

So here's a small patchset to fixup uevent ordering.
The first patch adds a 'nid' parameter to add_memory_blocks()
(to avoid mem->nid being initialized with NUMA_NO_NODE), and
the second patch reshuffles the code in add_memory_resource()
to fully initialize the node prior to calling
create_memory_block_devices() so that the node is valid at
that time and uevent processing will see correct values in sysfs.

As usual, comments and reviews are welcome.

Hannes Reinecke (2):
  drivers/base/memory: add node id parameter to add_memory_block()
  mm/memory_hotplug: activate node before adding new memory blocks

 drivers/base/memory.c  | 32 ++++++++++++--------------------
 include/linux/memory.h |  2 +-
 mm/memory_hotplug.c    | 32 +++++++++++++++++---------------
 3 files changed, 30 insertions(+), 36 deletions(-)

-- 
2.43.0

[PATCH 1/2] drivers/base/memory: add node id parameter to add_memory_block()

[Hannes Reinecke]

Add a 'nid' parameter to add_memory_block() to initialize the memory
block with the correct node id.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/base/memory.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index ed3e69dc785c..2b951e5f8a27 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -822,7 +822,7 @@ void memory_block_add_nid(struct memory_block *mem, int nid,
 }
 #endif
 
-static int add_memory_block(unsigned long block_id, unsigned long state,
+static int add_memory_block(unsigned long block_id, int nid, unsigned long state,
 			    struct vmem_altmap *altmap,
 			    struct memory_group *group)
 {
@@ -840,7 +840,7 @@ static int add_memory_block(unsigned long block_id, unsigned long state,
 
 	mem->start_section_nr = block_id * sections_per_block;
 	mem->state = state;
-	mem->nid = NUMA_NO_NODE;
+	mem->nid = nid;
 	mem->altmap = altmap;
 	INIT_LIST_HEAD(&mem->group_next);
 
@@ -867,13 +867,6 @@ static int add_memory_block(unsigned long block_id, unsigned long state,
 	return 0;
 }
 
-static int add_hotplug_memory_block(unsigned long block_id,
-				    struct vmem_altmap *altmap,
-				    struct memory_group *group)
-{
-	return add_memory_block(block_id, MEM_OFFLINE, altmap, group);
-}
-
 static void remove_memory_block(struct memory_block *memory)
 {
 	if (WARN_ON_ONCE(memory->dev.bus != &memory_subsys))
@@ -913,7 +906,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_hotplug_memory_block(block_id, altmap, group);
+		ret = add_memory_block(block_id, NUMA_NO_NODE, MEM_OFFLINE, altmap, group);
 		if (ret)
 			break;
 	}
@@ -1018,7 +1011,7 @@ void __init memory_dev_init(void)
 			continue;
 
 		block_id = memory_block_id(nr);
-		ret = add_memory_block(block_id, MEM_ONLINE, NULL, NULL);
+		ret = add_memory_block(block_id, NUMA_NO_NODE, MEM_ONLINE, NULL, NULL);
 		if (ret) {
 			panic("%s() failed to add memory block: %d\n",
 			      __func__, ret);
-- 
2.43.0

Re: [PATCH 1/2] drivers/base/memory: add node id parameter to add_memory_block()

[David Hildenbrand]

On 01.07.25 13:41, Hannes Reinecke wrote:
> Add a 'nid' parameter to add_memory_block() to initialize the memory
> block with the correct node id.

The patch doesn't do the latter, though. Probably best to make the 
clearer here. "This is a preparation for ...".

Also, I assume patch #2 needs a Fixes:

> 
> Signed-off-by: Hannes Reinecke <hare@kernel.org>
> ---

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] drivers/base/memory: add node id parameter to add_memory_block()

[Oscar Salvador]

On Tue, Jul 01, 2025 at 01:41:53PM +0200, Hannes Reinecke wrote:
> Add a 'nid' parameter to add_memory_block() to initialize the memory
> block with the correct node id.
> 
> Signed-off-by: Hannes Reinecke <hare@kernel.org>

Acked-by: Oscar Salvador <osalvador@suse.de>

As David mentioned, changelog should only mention the adding part, as
this patch only passes NUMA_NO_NODE.



-- 
Oscar Salvador
SUSE Labs

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

[Hannes Reinecke]

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.

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:
-- 
2.43.0

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

[David Hildenbrand]

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);


-- 
Cheers,

David / dhildenb

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

[Hannes Reinecke]

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

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

[Oscar Salvador]

On Tue, Jul 01, 2025 at 01:41:54PM +0200, Hannes Reinecke wrote:
> @@ -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;
> +	}

Do we have any constraints wrt. having the node onlined before calling in
__register_one_node() ?
This is a sensitive path (convulated :-) heh), but unless I'm missing anything,
it should be ok to call node_set_online after, right?

If so, the above could be simplified.

 

-- 
Oscar Salvador
SUSE Labs

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

[Oscar Salvador]

On Tue, Jul 01, 2025 at 04:02:08PM +0200, Oscar Salvador wrote:
> On Tue, Jul 01, 2025 at 01:41:54PM +0200, Hannes Reinecke wrote:
> > @@ -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;
> > +	}
> 
> Do we have any constraints wrt. having the node onlined before calling in
> __register_one_node() ?
> This is a sensitive path (convulated :-) heh), but unless I'm missing anything,
> it should be ok to call node_set_online after, right?

Ok, can you rebase this on top of mm-new?
Because of you do, you'll see that we no longer call __register_one_node,
as this was replaced with register_one_node.

 commit 6d166dec7451cab8782a08b85910685d930750e9
 Author: Donet Tom <donettom@linux.ibm.com>
 Date:   Wed May 28 12:18:04 2025 -0500

    drivers/base/node: rename __register_one_node() to register_one_node()

Now, with that in mind, we could do:

 diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 62d45752f9f4..9e0ade195a5f 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1537,10 +1537,9 @@ int add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags)
                        goto error_mem_hotplug_end;
        }
 
-       ret = __try_online_node(nid, false);
+       ret = __try_online_node(nid, true);
        if (ret < 0)
                goto error;
-       new_node = ret;
 
        /*
         * Self hosted memmap array
@@ -1563,17 +1562,6 @@ int add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags)
                }
        }
 
-       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_hotplug(nid, PFN_DOWN(start),
                                          PFN_UP(start + size - 1));


Note that this would be incomplete, as we have to know whether we fail before
we onlined the node, but the thing is that this chunk in __try_online_node():

<-----
 pgdat = hotadd_init_pgdat(nid);
 if (!pgdat) {
         pr_err("Cannot online node %d due to NULL pgdat\n", nid);
         ret = -ENOMEM;
         goto out;
 }
----->

Cannot be longer be triggered since:

 commit 09f49dca570a917a8c6bccd7e8c61f5141534e3a
 Author: Michal Hocko <mhocko@suse.com>
 Date:   Tue Mar 22 14:46:54 2022 -0700
 
     mm: handle uninitialized numa nodes gracefully

as we initialize all __possible__ numa nodes.

With that in mind, you'll have it easier as you can drop that, and then
the function becomes

Ret values
 0: node already onlined
 1: node onlined
 -err: register_one_node failed.

 

-- 
Oscar Salvador
SUSE Labs

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

[Oscar Salvador]

On Tue, Jul 01, 2025 at 08:52:50PM +0200, Oscar Salvador wrote:
> Ok, can you rebase this on top of mm-new?

mm-unstable will do.

> 

-- 
Oscar Salvador
SUSE Labs

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

[David Hildenbrand]

On 01.07.25 20:55, Oscar Salvador wrote:
> On Tue, Jul 01, 2025 at 08:52:50PM +0200, Oscar Salvador wrote:
>> Ok, can you rebase this on top of mm-new?
> 
> mm-unstable will do.

Hmm, if it's a fix that should go upstream asap, we should rather rebase 
the work that is in mm-unstable on top of the fix.

-- 
Cheers,

David / dhildenb

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

[Oscar Salvador]

On Tue, Jul 01, 2025 at 09:23:33PM +0200, David Hildenbrand wrote:
> Hmm, if it's a fix that should go upstream asap, we should rather rebase the
> work that is in mm-unstable on top of the fix.

Yap, I got carried away.
We can simplifiy it later.

 

-- 
Oscar Salvador
SUSE Labs

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

[Donet Tom]

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:

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

[David Hildenbrand]

On 02.07.25 08:25, Donet Tom wrote:
> 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?

Yeah (I mentioned this to patch #1 only). And we should probably CC stable.

I would assume the issue is fairly old.

-- 
Cheers,

David / dhildenb

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

[Hannes Reinecke]

On 7/2/25 08:36, David Hildenbrand wrote:
> On 02.07.25 08:25, Donet Tom wrote:
>> 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?
> 
> Yeah (I mentioned this to patch #1 only). And we should probably CC stable.
> 
> I would assume the issue is fairly old.
> 
And indeed, as old as to be basically meaningless.
The most likely candidate I've found is
b9ff036082cd ("mm/memory_hotplug.c: make add_memory_resource use 
__try_online_node")
(Sorry, Oscar, for picking on you :-)
Before that the history got obscured by later changes.
Problem is that we have two different issues here; the one
is the mis-order of uevents vs sysfs attributes, which, from
what I can see, is a day-one issue.
The other is a crash when accessing sysfs attributes, which I guess
crept in with
d84f2f5a7552 ("drivers/base/node.c: simplify 
unregister_memory_block_under_nodes()")
but it's hard to be sure.
And in either case, any of these is soo old as would affect pretty
much everyone.
(Oh fsck. _Someone_ will have to do the backports ...)

Anyway.

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

Re: [PATCH 0/2] mm/memory_hotplug: fixup crash during uevent handling

[David Hildenbrand]

On 01.07.25 13:41, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> Hi all,

Hi,

> 
> we have some udev rules trying to read the sysfs attribute 'valid_zones' during
> an memory 'add' event, causing a crash in zone_for_pfn_range(). Debugging found
> that mem->nid was set to NUMA_NO_NODE, which crashed in NODE_DATA(nid).
> Further analysis revealed that we're running into a race with udev event
> processing: add_memory_resource() has this function calls:
> 
> 1) __try_online_node()
> 2) arch_add_memory()
> 3) create_memory_block_devices()
>    -> calls device_register() -> memory 'add' event
> 4) node_set_online()/__register_one_node()
>    -> calls device_register() -> node 'add' event
> 5) register_memory_blocks_under_node()
>    -> sets mem->nid
> 
> Which, to the uninitated, is ... weird ...
> 
> Why do we try to online the node in 1), but only register
> the node in 4) _after_ we have created the memory blocks in 3) ?
> And why do we set the 'nid' value in 5), when the uevent
> (which might need to see the correct 'nid' value) is sent out
> in 3) ?
> There must be a reason, I'm sure ...

Probably just they way things developed historically.

We have some weird things to handle when we have early memory blocks 
that span multiple nodes. memory_block_add_nid() is all about that, and
maintaining mem->zone accordingly.

Probably should just be cleaned up.

-- 
Cheers,

David / dhildenb