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

[Oscar Salvador <osalvador@suse.de>]

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