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

[David Hildenbrand <david@redhat.com>]

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