Message-ID: <b68642ac-ca2a-4775-8a1f-02cf7681545e@linux.ibm.com>
Date: Thu, 25 Sep 2025 10:31:25 +0530
From: Donet Tom <donettom@...ux.ibm.com>
To: Christophe Leroy <christophe.leroy@...roup.eu>,
        Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...hat.com>,
        Oscar Salvador <osalvador@...e.de>
Cc: Ritesh Harjani <ritesh.list@...il.com>,
        Aboorva Devarajan <aboorvad@...ux.ibm.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        Madhavan Srinivasan <maddy@...ux.ibm.com>,
        linuxppc-dev@...ts.ozlabs.org, Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, x86@...nel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Danilo Krummrich <dakr@...nel.org>, Mike Rapoport <rppt@...nel.org>,
        Dave Jiang <dave.jiang@...el.com>
Subject: Re: [PATCH 1/2] drivers/base/node: merge register_one_node() and
 register_node() to a single function.


On 9/25/25 12:45 AM, Christophe Leroy wrote:
>
>
> Le 24/09/2025 à 20:40, Donet Tom a écrit :
>> register_one_node() and register_node() are small functions.
>> This patch merges them into a single function named register_node()
>> to improve code readability.
>
> This is unclear.
>
> What it really does is it folds register_node() into its only caller 
> which is register_one_node() and then it renames register_one_node() 
> into register_node().

Yes, you are right. I will add proper explanations in the next version.

>
>>
>> No functional changes are introduced.
>>
>> Signed-off-by: Donet Tom <donettom@...ux.ibm.com>
>> ---
>>   arch/powerpc/platforms/pseries/pci_dlpar.c |  2 +-
>>   arch/x86/mm/numa.c                         |  4 +-
>>   drivers/base/node.c                        | 52 +++++++++-------------
>>   include/linux/node.h                       |  4 +-
>>   mm/memory_hotplug.c                        |  4 +-
>>   mm/mm_init.c                               |  2 +-
>>   6 files changed, 28 insertions(+), 40 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c 
>> b/arch/powerpc/platforms/pseries/pci_dlpar.c
>> index aeb8633a3d00..8c77ec7980de 100644
>> --- a/arch/powerpc/platforms/pseries/pci_dlpar.c
>> +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
>> @@ -29,7 +29,7 @@ struct pci_controller *init_phb_dynamic(struct 
>> device_node *dn)
>>       nid = of_node_to_nid(dn);
>>       if (likely((nid) >= 0)) {
>>           if (!node_online(nid)) {
>> -            if (register_one_node(nid)) {
>> +            if (register_node(nid)) {
>>                   pr_err("PCI: Failed to register node %d\n", nid);
>>               } else {
>>                   update_numa_distance(dn);
>> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
>> index c24890c40138..7a97327140df 100644
>> --- a/arch/x86/mm/numa.c
>> +++ b/arch/x86/mm/numa.c
>> @@ -262,7 +262,7 @@ void __init init_gi_nodes(void)
>>        * bringup_nonboot_cpus
>>        *  cpu_up
>>        *   __try_online_node
>> -     *    register_one_node
>> +     *    register_node
>>        * because node_subsys is not initialized yet.
>>        * TODO remove dependency on node_online
>>        */
>> @@ -303,7 +303,7 @@ void __init init_cpu_to_node(void)
>>            * bringup_nonboot_cpus
>>            *  cpu_up
>>            *   __try_online_node
>> -         *    register_one_node
>> +         *    register_node
>>            * because node_subsys is not initialized yet.
>>            * TODO remove dependency on node_online
>>            */
>> diff --git a/drivers/base/node.c b/drivers/base/node.c
>> index 6b6e55a98b79..eab620e29c78 100644
>> --- a/drivers/base/node.c
>> +++ b/drivers/base/node.c
>> @@ -638,33 +638,6 @@ static void node_device_release(struct device *dev)
>>       kfree(to_node(dev));
>>   }
>>   -/*
>> - * register_node - Setup a sysfs device for a node.
>> - * @num - Node number to use when creating the device.
>> - *
>> - * Initialize and register the node device.
>> - */
>> -static int register_node(struct node *node, int num)
>> -{
>> -    int error;
>> -
>> -    node->dev.id = num;
>> -    node->dev.bus = &node_subsys;
>> -    node->dev.release = node_device_release;
>> -    node->dev.groups = node_dev_groups;
>> -    error = device_register(&node->dev);
>> -
>> -    if (error) {
>> -        put_device(&node->dev);
>> -    } else {
>> -        hugetlb_register_node(node);
>> -        compaction_register_node(node);
>> -        reclaim_register_node(node);
>> -    }
>> -
>> -    return error;
>> -}
>> -
>>   /**
>>    * unregister_node - unregister a node device
>>    * @node: node going away
>> @@ -869,7 +842,13 @@ void 
>> register_memory_blocks_under_node_hotplug(int nid, unsigned long 
>> start_pfn,
>>   }
>>   #endif /* CONFIG_MEMORY_HOTPLUG */
>>   -int register_one_node(int nid)
>> +/*
>> + * register_node - Setup a sysfs device for a node.
>> + * @nid - Node number to use when creating the device.
>> + *
>> + * Initialize and register the node device.
>> + */
>> +int register_node(int nid)
>>   {
>>       int error;
>>       int cpu;
>> @@ -880,14 +859,23 @@ int register_one_node(int nid)
>>           return -ENOMEM;
>>         INIT_LIST_HEAD(&node->access_list);
>> -    node_devices[nid] = node;
>>   -    error = register_node(node_devices[nid], nid);
>> +    node->dev.id = nid;
>> +    node->dev.bus = &node_subsys;
>> +    node->dev.release = node_device_release;
>> +    node->dev.groups = node_dev_groups;
>> +
>> +    error = device_register(&node->dev);
>>       if (error) {
>> -        node_devices[nid] = NULL;
>> +        put_device(&node->dev);
>>           return error;
>>       }
>>   +    node_devices[nid] = node;
>> +    hugetlb_register_node(node);
>> +    compaction_register_node(node);
>> +    reclaim_register_node(node);
>> +
>>       /* link cpu under this node */
>>       for_each_present_cpu(cpu) {
>>           if (cpu_to_node(cpu) == nid)
>> @@ -980,7 +968,7 @@ void __init node_dev_init(void)
>>        * to already created cpu devices.
>>        */
>>       for_each_online_node(i) {
>> -        ret =  register_one_node(i);
>> +        ret =  register_node(i);
>>           if (ret)
>>               panic("%s() failed to add node: %d\n", __func__, ret);
>>       }
>> diff --git a/include/linux/node.h b/include/linux/node.h
>> index 2c7529335b21..4dcf876cd0b4 100644
>> --- a/include/linux/node.h
>> +++ b/include/linux/node.h
>> @@ -168,7 +168,7 @@ static inline int 
>> hotplug_node_notifier(notifier_fn_t fn, int pri)
>>   #ifdef CONFIG_NUMA
>>   extern void node_dev_init(void);
>>   /* Core of the node registration - only memory hotplug should use 
>> this */
>> -extern int register_one_node(int nid);
>> +extern int register_node(int nid);
>
> extern keyword is pointless on functions prototypes.
>
> checkpatch.pl usually complains about that.
>
> I know previous prototype was extern and all surrounding also are, but 
> it is not because mistakes were done in the past that you have to 
> continue doing the same mistakes.
>

Thank you for the comment. I will address it in the next version.


>
>
>>   extern void unregister_one_node(int nid);
>>   extern int register_cpu_under_node(unsigned int cpu, unsigned int 
>> nid);
>>   extern int unregister_cpu_under_node(unsigned int cpu, unsigned int 
>> nid);
>> @@ -181,7 +181,7 @@ extern int 
>> register_memory_node_under_compute_node(unsigned int mem_nid,
>>   static inline void node_dev_init(void)
>>   {
>>   }
>> -static inline int register_one_node(int nid)
>> +static inline int register_node(int nid)
>>   {
>>       return 0;
>>   }
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 0be83039c3b5..6c050d867031 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1311,7 +1311,7 @@ static int __try_online_node(int nid, bool 
>> set_node_online)
>>         if (set_node_online) {
>>           node_set_online(nid);
>> -        ret = register_one_node(nid);
>> +        ret = register_node(nid);
>>           BUG_ON(ret);
>>       }
>>   out:
>> @@ -1542,7 +1542,7 @@ int add_memory_resource(int nid, struct 
>> resource *res, mhp_t mhp_flags)
>>           goto error_memblock_remove;
>>       if (ret) {
>>           node_set_online(nid);
>> -        ret = register_one_node(nid);
>> +        ret = register_node(nid);
>>           if (WARN_ON(ret)) {
>>               node_set_offline(nid);
>>               goto error_memblock_remove;
>> diff --git a/mm/mm_init.c b/mm/mm_init.c
>> index df614556741a..e1a19a3dadd7 100644
>> --- a/mm/mm_init.c
>> +++ b/mm/mm_init.c
>> @@ -1909,7 +1909,7 @@ void __init free_area_init(unsigned long 
>> *max_zone_pfn)
>>           free_area_init_node(nid);
>>             /*
>> -         * No sysfs hierarchy will be created via register_one_node()
>> +         * No sysfs hierarchy will be created via register_node()
>>            *for memory-less node because here it's not marked as 
>> N_MEMORY
>>            *and won't be set online later. The benefit is userspace
>>            *program won't be confused by sysfs files/directories of
>
>

Powered by blists - more mailing lists