[PATCH] mm: hugetlb: eliminate memory-less nodes handling

[PATCH] mm: hugetlb: eliminate memory-less nodes handling

[Muchun Song]

The memory-notify-based approach aims to handle meory-less nodes, however, it just adds
the complexity of code as pointed by David in thread [1].  The handling of memory-less
nodes is introduced by commit 4faf8d950ec4 ("hugetlb: handle memory hot-plug events").
From its commit message, we cannot find any necessity of handling this case. So, we can
simply register/unregister sysfs entries in register_node/unregister_node to simlify the
code.

https://lore.kernel.org/linux-mm/60933ffc-b850-976c-78a0-0ee6e0ea9ef0@redhat.com/ [1]
Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 drivers/base/node.c  |  7 +++++--
 include/linux/node.h |  5 +++++
 mm/hugetlb.c         | 37 ++++++++++---------------------------
 3 files changed, 20 insertions(+), 29 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index ed391cb09999..cf115d5a9b8a 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -608,10 +608,12 @@ static int register_node(struct node *node, int num)
 	node->dev.groups = node_dev_groups;
 	error = device_register(&node->dev);
 
-	if (error)
+	if (error) {
 		put_device(&node->dev);
-	else
+	} else {
+		hugetlb_register_node(node);
 		compaction_register_node(node);
+	}
 
 	return error;
 }
@@ -625,6 +627,7 @@ static int register_node(struct node *node, int num)
  */
 void unregister_node(struct node *node)
 {
+	hugetlb_unregister_node(node);
 	compaction_unregister_node(node);
 	node_remove_accesses(node);
 	node_remove_caches(node);
diff --git a/include/linux/node.h b/include/linux/node.h
index 427a5975cf40..f5d41498c2bf 100644
--- a/include/linux/node.h
+++ b/include/linux/node.h
@@ -138,6 +138,11 @@ extern void unregister_memory_block_under_nodes(struct memory_block *mem_blk);
 extern int register_memory_node_under_compute_node(unsigned int mem_nid,
 						   unsigned int cpu_nid,
 						   unsigned access);
+
+#ifdef CONFIG_HUGETLBFS
+void hugetlb_register_node(struct node *node);
+void hugetlb_unregister_node(struct node *node);
+#endif
 #else
 static inline void node_dev_init(void)
 {
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index d0617d64d718..722e862bb6be 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3898,6 +3898,7 @@ static void __init hugetlb_sysfs_init(void)
 }
 
 #ifdef CONFIG_NUMA
+static bool hugetlb_initialized __ro_after_init;
 
 /*
  * node_hstate/s - associate per node hstate attributes, via their kobjects,
@@ -3953,7 +3954,7 @@ static struct hstate *kobj_to_node_hstate(struct kobject *kobj, int *nidp)
  * Unregister hstate attributes from a single node device.
  * No-op if no hstate attributes attached.
  */
-static void hugetlb_unregister_node(struct node *node)
+void hugetlb_unregister_node(struct node *node)
 {
 	struct hstate *h;
 	struct node_hstate *nhs = &node_hstates[node->dev.id];
@@ -3983,19 +3984,22 @@ static void hugetlb_unregister_node(struct node *node)
  * Register hstate attributes for a single node device.
  * No-op if attributes already registered.
  */
-static int hugetlb_register_node(struct node *node)
+void hugetlb_register_node(struct node *node)
 {
 	struct hstate *h;
 	struct node_hstate *nhs = &node_hstates[node->dev.id];
 	int err;
 
+	if (!hugetlb_initialized)
+		return;
+
 	if (nhs->hugepages_kobj)
-		return 0;		/* already allocated */
+		return;		/* already allocated */
 
 	nhs->hugepages_kobj = kobject_create_and_add("hugepages",
 							&node->dev.kobj);
 	if (!nhs->hugepages_kobj)
-		return -ENOMEM;
+		return;
 
 	for_each_hstate(h) {
 		err = hugetlb_sysfs_add_hstate(h, nhs->hugepages_kobj,
@@ -4005,28 +4009,9 @@ static int hugetlb_register_node(struct node *node)
 			pr_err("HugeTLB: Unable to add hstate %s for node %d\n",
 				h->name, node->dev.id);
 			hugetlb_unregister_node(node);
-			return -ENOMEM;
+			break;
 		}
 	}
-	return 0;
-}
-
-static int __meminit hugetlb_memory_callback(struct notifier_block *self,
-					     unsigned long action, void *arg)
-{
-	int ret = 0;
-	struct memory_notify *mnb = arg;
-	int nid = mnb->status_change_nid;
-
-	if (nid == NUMA_NO_NODE)
-		return NOTIFY_DONE;
-
-	if (action == MEM_GOING_ONLINE)
-		ret = hugetlb_register_node(node_devices[nid]);
-	else if (action == MEM_CANCEL_ONLINE || action == MEM_OFFLINE)
-		hugetlb_unregister_node(node_devices[nid]);
-
-	return notifier_from_errno(ret);
 }
 
 /*
@@ -4038,11 +4023,9 @@ static void __init hugetlb_register_all_nodes(void)
 {
 	int nid;
 
-	get_online_mems();
-	hotplug_memory_notifier(hugetlb_memory_callback, 0);
+	hugetlb_initialized = true;
 	for_each_node_state(nid, N_MEMORY)
 		hugetlb_register_node(node_devices[nid]);
-	put_online_mems();
 }
 #else	/* !CONFIG_NUMA */
 
-- 
2.11.0

Re: [PATCH] mm: hugetlb: eliminate memory-less nodes handling

[David Hildenbrand]

On 01.09.22 10:30, Muchun Song wrote:
> The memory-notify-based approach aims to handle meory-less nodes, however, it just adds
> the complexity of code as pointed by David in thread [1].  The handling of memory-less
> nodes is introduced by commit 4faf8d950ec4 ("hugetlb: handle memory hot-plug events").
> From its commit message, we cannot find any necessity of handling this case. So, we can
> simply register/unregister sysfs entries in register_node/unregister_node to simlify the
> code.
> 
> https://lore.kernel.org/linux-mm/60933ffc-b850-976c-78a0-0ee6e0ea9ef0@redhat.com/ [1]
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  drivers/base/node.c  |  7 +++++--
>  include/linux/node.h |  5 +++++
>  mm/hugetlb.c         | 37 ++++++++++---------------------------
>  3 files changed, 20 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index ed391cb09999..cf115d5a9b8a 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -608,10 +608,12 @@ static int register_node(struct node *node, int num)
>  	node->dev.groups = node_dev_groups;
>  	error = device_register(&node->dev);
>  
> -	if (error)
> +	if (error) {
>  		put_device(&node->dev);
> -	else
> +	} else {
> +		hugetlb_register_node(node);
>  		compaction_register_node(node);
> +	}

Good, so this matches what other code does.

>  
>  	return error;
>  }
> @@ -625,6 +627,7 @@ static int register_node(struct node *node, int num)
>   */
>  void unregister_node(struct node *node)
>  {
> +	hugetlb_unregister_node(node);
>  	compaction_unregister_node(node);
>  	node_remove_accesses(node);
>  	node_remove_caches(node);
> diff --git a/include/linux/node.h b/include/linux/node.h
> index 427a5975cf40..f5d41498c2bf 100644
> --- a/include/linux/node.h
> +++ b/include/linux/node.h
> @@ -138,6 +138,11 @@ extern void unregister_memory_block_under_nodes(struct memory_block *mem_blk);
>  extern int register_memory_node_under_compute_node(unsigned int mem_nid,
>  						   unsigned int cpu_nid,
>  						   unsigned access);
> +
> +#ifdef CONFIG_HUGETLBFS
> +void hugetlb_register_node(struct node *node);
> +void hugetlb_unregister_node(struct node *node);
> +#endif

compaction_register_node() resides in include/linux/compaction.h, so I
wonder if this should go into hugetlb.h (unless it causes trouble)

>  #else
>  static inline void node_dev_init(void)
>  {
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index d0617d64d718..722e862bb6be 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3898,6 +3898,7 @@ static void __init hugetlb_sysfs_init(void)
>  }
>  
>  #ifdef CONFIG_NUMA
> +static bool hugetlb_initialized __ro_after_init;

We set it out of hugetlb_register_all_nodes(), so it conceptually not
correct. We either need a better name here or set it from generic init code.

You could call it hugetlb_sysfs_initialized() and set that from
hugetlb_sysfs_init(), which is called just before
hugetlb_register_all_nodes().

[ shouldn't hugetlb_register_all_nodes() get called from
hugetlb_sysfs_init() ? it's all about sysfs as well ... ]

>  
>  /*
>   * node_hstate/s - associate per node hstate attributes, via their kobjects,
> @@ -3953,7 +3954,7 @@ static struct hstate *kobj_to_node_hstate(struct kobject *kobj, int *nidp)
>   * Unregister hstate attributes from a single node device.
>   * No-op if no hstate attributes attached.
>   */
> -static void hugetlb_unregister_node(struct node *node)
> +void hugetlb_unregister_node(struct node *node)
>  {
>  	struct hstate *h;
>  	struct node_hstate *nhs = &node_hstates[node->dev.id];
> @@ -3983,19 +3984,22 @@ static void hugetlb_unregister_node(struct node *node)
>   * Register hstate attributes for a single node device.
>   * No-op if attributes already registered.
>   */
> -static int hugetlb_register_node(struct node *node)
> +void hugetlb_register_node(struct node *node)
>  {
>  	struct hstate *h;
>  	struct node_hstate *nhs = &node_hstates[node->dev.id];
>  	int err;
>  
> +	if (!hugetlb_initialized)
> +		return;
> +
>  	if (nhs->hugepages_kobj)
> -		return 0;		/* already allocated */
> +		return;		/* already allocated */
>  
>  	nhs->hugepages_kobj = kobject_create_and_add("hugepages",
>  							&node->dev.kobj);
>  	if (!nhs->hugepages_kobj)
> -		return -ENOMEM;
> +		return;
>  
>  	for_each_hstate(h) {
>  		err = hugetlb_sysfs_add_hstate(h, nhs->hugepages_kobj,
> @@ -4005,28 +4009,9 @@ static int hugetlb_register_node(struct node *node)
>  			pr_err("HugeTLB: Unable to add hstate %s for node %d\n",
>  				h->name, node->dev.id);
>  			hugetlb_unregister_node(node);
> -			return -ENOMEM;
> +			break;
>  		}
>  	}
> -	return 0;
> -}
> -
> -static int __meminit hugetlb_memory_callback(struct notifier_block *self,
> -					     unsigned long action, void *arg)
> -{
> -	int ret = 0;
> -	struct memory_notify *mnb = arg;
> -	int nid = mnb->status_change_nid;
> -
> -	if (nid == NUMA_NO_NODE)
> -		return NOTIFY_DONE;
> -
> -	if (action == MEM_GOING_ONLINE)
> -		ret = hugetlb_register_node(node_devices[nid]);
> -	else if (action == MEM_CANCEL_ONLINE || action == MEM_OFFLINE)
> -		hugetlb_unregister_node(node_devices[nid]);
> -
> -	return notifier_from_errno(ret);
>  }
>  
>  /*
> @@ -4038,11 +4023,9 @@ static void __init hugetlb_register_all_nodes(void)
>  {
>  	int nid;
>  
> -	get_online_mems();
> -	hotplug_memory_notifier(hugetlb_memory_callback, 0);
> +	hugetlb_initialized = true;
>  	for_each_node_state(nid, N_MEMORY)
>  		hugetlb_register_node(node_devices[nid]);
> -	put_online_mems();
>  }
>  #else	/* !CONFIG_NUMA */
>  

Apart from the comments, looks good and clean to me. Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm: hugetlb: eliminate memory-less nodes handling

[Aneesh Kumar K V]

On 9/1/22 2:00 PM, Muchun Song wrote:
> The memory-notify-based approach aims to handle meory-less nodes, however, it just adds
> the complexity of code as pointed by David in thread [1].  The handling of memory-less
> nodes is introduced by commit 4faf8d950ec4 ("hugetlb: handle memory hot-plug events").
> From its commit message, we cannot find any necessity of handling this case. So, we can
> simply register/unregister sysfs entries in register_node/unregister_node to simlify the
> code.

Isn't that hotplug callback added because in hugetlb_register_all_nodes() we register
sysfs nodes only for N_MEMORY nodes? 


> 
> https://lore.kernel.org/linux-mm/60933ffc-b850-976c-78a0-0ee6e0ea9ef0@redhat.com/ [1]
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  drivers/base/node.c  |  7 +++++--
>  include/linux/node.h |  5 +++++
>  mm/hugetlb.c         | 37 ++++++++++---------------------------
>  3 files changed, 20 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index ed391cb09999..cf115d5a9b8a 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -608,10 +608,12 @@ static int register_node(struct node *node, int num)
>  	node->dev.groups = node_dev_groups;
>  	error = device_register(&node->dev);
>  
> -	if (error)
> +	if (error) {
>  		put_device(&node->dev);
> -	else
> +	} else {
> +		hugetlb_register_node(node);
>  		compaction_register_node(node);
> +	}
>  


I guess this will handle register of sysfs hugetlb files for new NUMA nodes
after hugetlb_initialized = true;

But what about N_CPU that can get memory added later. Do we need to update
hugetlb_register_all_nodes() to handle N_ONLINE? 


>  	return error;
>  }
> @@ -625,6 +627,7 @@ static int register_node(struct node *node, int num)
>   */
>  void unregister_node(struct node *node)
>  {
> +	hugetlb_unregister_node(node);
>  	compaction_unregister_node(node);
>  	node_remove_accesses(node);
>  	node_remove_caches(node);
> diff --git a/include/linux/node.h b/include/linux/node.h
> index 427a5975cf40..f5d41498c2bf 100644
> --- a/include/linux/node.h
> +++ b/include/linux/node.h
> @@ -138,6 +138,11 @@ extern void unregister_memory_block_under_nodes(struct memory_block *mem_blk);
>  extern int register_memory_node_under_compute_node(unsigned int mem_nid,
>  						   unsigned int cpu_nid,
>  						   unsigned access);
> +
> +#ifdef CONFIG_HUGETLBFS
> +void hugetlb_register_node(struct node *node);
> +void hugetlb_unregister_node(struct node *node);
> +#endif
>  #else
>  static inline void node_dev_init(void)
>  {
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index d0617d64d718..722e862bb6be 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3898,6 +3898,7 @@ static void __init hugetlb_sysfs_init(void)
>  }
>  
>  #ifdef CONFIG_NUMA
> +static bool hugetlb_initialized __ro_after_init;
>  
>  /*
>   * node_hstate/s - associate per node hstate attributes, via their kobjects,
> @@ -3953,7 +3954,7 @@ static struct hstate *kobj_to_node_hstate(struct kobject *kobj, int *nidp)
>   * Unregister hstate attributes from a single node device.
>   * No-op if no hstate attributes attached.
>   */
> -static void hugetlb_unregister_node(struct node *node)
> +void hugetlb_unregister_node(struct node *node)
>  {
>  	struct hstate *h;
>  	struct node_hstate *nhs = &node_hstates[node->dev.id];
> @@ -3983,19 +3984,22 @@ static void hugetlb_unregister_node(struct node *node)
>   * Register hstate attributes for a single node device.
>   * No-op if attributes already registered.
>   */
> -static int hugetlb_register_node(struct node *node)
> +void hugetlb_register_node(struct node *node)
>  {
>  	struct hstate *h;
>  	struct node_hstate *nhs = &node_hstates[node->dev.id];
>  	int err;
>  
> +	if (!hugetlb_initialized)
> +		return;
> +
>  	if (nhs->hugepages_kobj)
> -		return 0;		/* already allocated */
> +		return;		/* already allocated */
>  
>  	nhs->hugepages_kobj = kobject_create_and_add("hugepages",
>  							&node->dev.kobj);
>  	if (!nhs->hugepages_kobj)
> -		return -ENOMEM;
> +		return;
>  
>  	for_each_hstate(h) {
>  		err = hugetlb_sysfs_add_hstate(h, nhs->hugepages_kobj,
> @@ -4005,28 +4009,9 @@ static int hugetlb_register_node(struct node *node)
>  			pr_err("HugeTLB: Unable to add hstate %s for node %d\n",
>  				h->name, node->dev.id);
>  			hugetlb_unregister_node(node);
> -			return -ENOMEM;
> +			break;
>  		}
>  	}
> -	return 0;
> -}
> -
> -static int __meminit hugetlb_memory_callback(struct notifier_block *self,
> -					     unsigned long action, void *arg)
> -{
> -	int ret = 0;
> -	struct memory_notify *mnb = arg;
> -	int nid = mnb->status_change_nid;
> -
> -	if (nid == NUMA_NO_NODE)
> -		return NOTIFY_DONE;
> -
> -	if (action == MEM_GOING_ONLINE)
> -		ret = hugetlb_register_node(node_devices[nid]);
> -	else if (action == MEM_CANCEL_ONLINE || action == MEM_OFFLINE)
> -		hugetlb_unregister_node(node_devices[nid]);
> -
> -	return notifier_from_errno(ret);
>  }
>  
>  /*
> @@ -4038,11 +4023,9 @@ static void __init hugetlb_register_all_nodes(void)
>  {
>  	int nid;
>  
> -	get_online_mems();
> -	hotplug_memory_notifier(hugetlb_memory_callback, 0);
> +	hugetlb_initialized = true;
>  	for_each_node_state(nid, N_MEMORY)


Should this be for_each_online_node() ?

>  		hugetlb_register_node(node_devices[nid]);
> -	put_online_mems();
>  }
>  #else	/* !CONFIG_NUMA */
>  

Re: [PATCH] mm: hugetlb: eliminate memory-less nodes handling

[David Hildenbrand]

On 01.09.22 11:00, Aneesh Kumar K V wrote:
> On 9/1/22 2:00 PM, Muchun Song wrote:
>> The memory-notify-based approach aims to handle meory-less nodes, however, it just adds
>> the complexity of code as pointed by David in thread [1].  The handling of memory-less
>> nodes is introduced by commit 4faf8d950ec4 ("hugetlb: handle memory hot-plug events").
>> From its commit message, we cannot find any necessity of handling this case. So, we can
>> simply register/unregister sysfs entries in register_node/unregister_node to simlify the
>> code.
> 
> Isn't that hotplug callback added because in hugetlb_register_all_nodes() we register
> sysfs nodes only for N_MEMORY nodes? 
> 

Right, that needs adjustment as well.


-- 
Thanks,

David / dhildenb

Re: [PATCH] mm: hugetlb: eliminate memory-less nodes handling

[Muchun Song]

> On Sep 1, 2022, at 16:50, David Hildenbrand <david@redhat.com> wrote:
> 
> On 01.09.22 10:30, Muchun Song wrote:
>> The memory-notify-based approach aims to handle meory-less nodes, however, it just adds
>> the complexity of code as pointed by David in thread [1].  The handling of memory-less
>> nodes is introduced by commit 4faf8d950ec4 ("hugetlb: handle memory hot-plug events").
>> From its commit message, we cannot find any necessity of handling this case. So, we can
>> simply register/unregister sysfs entries in register_node/unregister_node to simlify the
>> code.
>> 
>> https://lore.kernel.org/linux-mm/60933ffc-b850-976c-78a0-0ee6e0ea9ef0@redhat.com/ [1]
>> Suggested-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>> ---
>> drivers/base/node.c  |  7 +++++--
>> include/linux/node.h |  5 +++++
>> mm/hugetlb.c         | 37 ++++++++++---------------------------
>> 3 files changed, 20 insertions(+), 29 deletions(-)
>> 
>> diff --git a/drivers/base/node.c b/drivers/base/node.c
>> index ed391cb09999..cf115d5a9b8a 100644
>> --- a/drivers/base/node.c
>> +++ b/drivers/base/node.c
>> @@ -608,10 +608,12 @@ static int register_node(struct node *node, int num)
>> 	node->dev.groups = node_dev_groups;
>> 	error = device_register(&node->dev);
>> 
>> -	if (error)
>> +	if (error) {
>> 		put_device(&node->dev);
>> -	else
>> +	} else {
>> +		hugetlb_register_node(node);
>> 		compaction_register_node(node);
>> +	}
> 
> Good, so this matches what other code does.
> 
>> 
>> 	return error;
>> }
>> @@ -625,6 +627,7 @@ static int register_node(struct node *node, int num)
>>  */
>> void unregister_node(struct node *node)
>> {
>> +	hugetlb_unregister_node(node);
>> 	compaction_unregister_node(node);
>> 	node_remove_accesses(node);
>> 	node_remove_caches(node);
>> diff --git a/include/linux/node.h b/include/linux/node.h
>> index 427a5975cf40..f5d41498c2bf 100644
>> --- a/include/linux/node.h
>> +++ b/include/linux/node.h
>> @@ -138,6 +138,11 @@ extern void unregister_memory_block_under_nodes(struct memory_block *mem_blk);
>> extern int register_memory_node_under_compute_node(unsigned int mem_nid,
>> 						   unsigned int cpu_nid,
>> 						   unsigned access);
>> +
>> +#ifdef CONFIG_HUGETLBFS
>> +void hugetlb_register_node(struct node *node);
>> +void hugetlb_unregister_node(struct node *node);
>> +#endif
> 
> compaction_register_node() resides in include/linux/compaction.h, so I
> wonder if this should go into hugetlb.h (unless it causes trouble)

I think yes. Will update in next version.

> 
>> #else
>> static inline void node_dev_init(void)
>> {
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index d0617d64d718..722e862bb6be 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -3898,6 +3898,7 @@ static void __init hugetlb_sysfs_init(void)
>> }
>> 
>> #ifdef CONFIG_NUMA
>> +static bool hugetlb_initialized __ro_after_init;
> 
> We set it out of hugetlb_register_all_nodes(), so it conceptually not
> correct. We either need a better name here or set it from generic init code.
> 
> You could call it hugetlb_sysfs_initialized() and set that from
> hugetlb_sysfs_init(), which is called just before
> hugetlb_register_all_nodes().

Make sense.

> 
> [ shouldn't hugetlb_register_all_nodes() get called from
> hugetlb_sysfs_init() ? it's all about sysfs as well ... ]

Yep, we can call hugetlb_register_all_nodes() in hugetlb_sysfs_init().

> 
>> 
>> /*
>>  * node_hstate/s - associate per node hstate attributes, via their kobjects,
>> @@ -3953,7 +3954,7 @@ static struct hstate *kobj_to_node_hstate(struct kobject *kobj, int *nidp)
>>  * Unregister hstate attributes from a single node device.
>>  * No-op if no hstate attributes attached.
>>  */
>> -static void hugetlb_unregister_node(struct node *node)
>> +void hugetlb_unregister_node(struct node *node)
>> {
>> 	struct hstate *h;
>> 	struct node_hstate *nhs = &node_hstates[node->dev.id];
>> @@ -3983,19 +3984,22 @@ static void hugetlb_unregister_node(struct node *node)
>>  * Register hstate attributes for a single node device.
>>  * No-op if attributes already registered.
>>  */
>> -static int hugetlb_register_node(struct node *node)
>> +void hugetlb_register_node(struct node *node)
>> {
>> 	struct hstate *h;
>> 	struct node_hstate *nhs = &node_hstates[node->dev.id];
>> 	int err;
>> 
>> +	if (!hugetlb_initialized)
>> +		return;
>> +
>> 	if (nhs->hugepages_kobj)
>> -		return 0;		/* already allocated */
>> +		return;		/* already allocated */
>> 
>> 	nhs->hugepages_kobj = kobject_create_and_add("hugepages",
>> 							&node->dev.kobj);
>> 	if (!nhs->hugepages_kobj)
>> -		return -ENOMEM;
>> +		return;
>> 
>> 	for_each_hstate(h) {
>> 		err = hugetlb_sysfs_add_hstate(h, nhs->hugepages_kobj,
>> @@ -4005,28 +4009,9 @@ static int hugetlb_register_node(struct node *node)
>> 			pr_err("HugeTLB: Unable to add hstate %s for node %d\n",
>> 				h->name, node->dev.id);
>> 			hugetlb_unregister_node(node);
>> -			return -ENOMEM;
>> +			break;
>> 		}
>> 	}
>> -	return 0;
>> -}
>> -
>> -static int __meminit hugetlb_memory_callback(struct notifier_block *self,
>> -					     unsigned long action, void *arg)
>> -{
>> -	int ret = 0;
>> -	struct memory_notify *mnb = arg;
>> -	int nid = mnb->status_change_nid;
>> -
>> -	if (nid == NUMA_NO_NODE)
>> -		return NOTIFY_DONE;
>> -
>> -	if (action == MEM_GOING_ONLINE)
>> -		ret = hugetlb_register_node(node_devices[nid]);
>> -	else if (action == MEM_CANCEL_ONLINE || action == MEM_OFFLINE)
>> -		hugetlb_unregister_node(node_devices[nid]);
>> -
>> -	return notifier_from_errno(ret);
>> }
>> 
>> /*
>> @@ -4038,11 +4023,9 @@ static void __init hugetlb_register_all_nodes(void)
>> {
>> 	int nid;
>> 
>> -	get_online_mems();
>> -	hotplug_memory_notifier(hugetlb_memory_callback, 0);
>> +	hugetlb_initialized = true;
>> 	for_each_node_state(nid, N_MEMORY)
>> 		hugetlb_register_node(node_devices[nid]);
>> -	put_online_mems();
>> }
>> #else	/* !CONFIG_NUMA */
>> 
> 
> Apart from the comments, looks good and clean to me. Thanks!

Thanks for your suggestions and review.

Muchun

> 
> -- 
> Thanks,
> 
> David / dhildenb

Re: [PATCH] mm: hugetlb: eliminate memory-less nodes handling

[Muchun Song]

> On Sep 1, 2022, at 17:00, Aneesh Kumar K V <aneesh.kumar@linux.ibm.com> wrote:
> 
> On 9/1/22 2:00 PM, Muchun Song wrote:
>> The memory-notify-based approach aims to handle meory-less nodes, however, it just adds
>> the complexity of code as pointed by David in thread [1].  The handling of memory-less
>> nodes is introduced by commit 4faf8d950ec4 ("hugetlb: handle memory hot-plug events").
>> From its commit message, we cannot find any necessity of handling this case. So, we can
>> simply register/unregister sysfs entries in register_node/unregister_node to simlify the
>> code.
> 
> Isn't that hotplug callback added because in hugetlb_register_all_nodes() we register
> sysfs nodes only for N_MEMORY nodes? 

I think you might right. I have looked at the commit 9a30523066cd which introduces the sysfs
creation. I saw it create the sysfs for every possible node.

	for (nid = 0; nid < nr_node_ids; nid++)
		hugetlb_register_node(node);

And then I checked the commit 9b5e5d0fdc91, which said it was a preparation for handling
memory-less nodes via memory hotplug.

> 
> 
>> 
>> https://lore.kernel.org/linux-mm/60933ffc-b850-976c-78a0-0ee6e0ea9ef0@redhat.com/ [1]
>> Suggested-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>> ---
>> drivers/base/node.c  |  7 +++++--
>> include/linux/node.h |  5 +++++
>> mm/hugetlb.c         | 37 ++++++++++---------------------------
>> 3 files changed, 20 insertions(+), 29 deletions(-)
>> 
>> diff --git a/drivers/base/node.c b/drivers/base/node.c
>> index ed391cb09999..cf115d5a9b8a 100644
>> --- a/drivers/base/node.c
>> +++ b/drivers/base/node.c
>> @@ -608,10 +608,12 @@ static int register_node(struct node *node, int num)
>> 	node->dev.groups = node_dev_groups;
>> 	error = device_register(&node->dev);
>> 
>> -	if (error)
>> +	if (error) {
>> 		put_device(&node->dev);
>> -	else
>> +	} else {
>> +		hugetlb_register_node(node);
>> 		compaction_register_node(node);
>> +	}
>> 
> 
> 
> I guess this will handle register of sysfs hugetlb files for new NUMA nodes
> after hugetlb_initialized = true;

Yes.

> 
> But what about N_CPU that can get memory added later. Do we need to update
> hugetlb_register_all_nodes() to handle N_ONLINE? 

I think we should.

> 
> 
>> 	return error;
>> }
>> @@ -625,6 +627,7 @@ static int register_node(struct node *node, int num)
>>  */
>> void unregister_node(struct node *node)
>> {
>> +	hugetlb_unregister_node(node);
>> 	compaction_unregister_node(node);
>> 	node_remove_accesses(node);
>> 	node_remove_caches(node);
>> diff --git a/include/linux/node.h b/include/linux/node.h
>> index 427a5975cf40..f5d41498c2bf 100644
>> --- a/include/linux/node.h
>> +++ b/include/linux/node.h
>> @@ -138,6 +138,11 @@ extern void unregister_memory_block_under_nodes(struct memory_block *mem_blk);
>> extern int register_memory_node_under_compute_node(unsigned int mem_nid,
>> 						   unsigned int cpu_nid,
>> 						   unsigned access);
>> +
>> +#ifdef CONFIG_HUGETLBFS
>> +void hugetlb_register_node(struct node *node);
>> +void hugetlb_unregister_node(struct node *node);
>> +#endif
>> #else
>> static inline void node_dev_init(void)
>> {
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index d0617d64d718..722e862bb6be 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -3898,6 +3898,7 @@ static void __init hugetlb_sysfs_init(void)
>> }
>> 
>> #ifdef CONFIG_NUMA
>> +static bool hugetlb_initialized __ro_after_init;
>> 
>> /*
>>  * node_hstate/s - associate per node hstate attributes, via their kobjects,
>> @@ -3953,7 +3954,7 @@ static struct hstate *kobj_to_node_hstate(struct kobject *kobj, int *nidp)
>>  * Unregister hstate attributes from a single node device.
>>  * No-op if no hstate attributes attached.
>>  */
>> -static void hugetlb_unregister_node(struct node *node)
>> +void hugetlb_unregister_node(struct node *node)
>> {
>> 	struct hstate *h;
>> 	struct node_hstate *nhs = &node_hstates[node->dev.id];
>> @@ -3983,19 +3984,22 @@ static void hugetlb_unregister_node(struct node *node)
>>  * Register hstate attributes for a single node device.
>>  * No-op if attributes already registered.
>>  */
>> -static int hugetlb_register_node(struct node *node)
>> +void hugetlb_register_node(struct node *node)
>> {
>> 	struct hstate *h;
>> 	struct node_hstate *nhs = &node_hstates[node->dev.id];
>> 	int err;
>> 
>> +	if (!hugetlb_initialized)
>> +		return;
>> +
>> 	if (nhs->hugepages_kobj)
>> -		return 0;		/* already allocated */
>> +		return;		/* already allocated */
>> 
>> 	nhs->hugepages_kobj = kobject_create_and_add("hugepages",
>> 							&node->dev.kobj);
>> 	if (!nhs->hugepages_kobj)
>> -		return -ENOMEM;
>> +		return;
>> 
>> 	for_each_hstate(h) {
>> 		err = hugetlb_sysfs_add_hstate(h, nhs->hugepages_kobj,
>> @@ -4005,28 +4009,9 @@ static int hugetlb_register_node(struct node *node)
>> 			pr_err("HugeTLB: Unable to add hstate %s for node %d\n",
>> 				h->name, node->dev.id);
>> 			hugetlb_unregister_node(node);
>> -			return -ENOMEM;
>> +			break;
>> 		}
>> 	}
>> -	return 0;
>> -}
>> -
>> -static int __meminit hugetlb_memory_callback(struct notifier_block *self,
>> -					     unsigned long action, void *arg)
>> -{
>> -	int ret = 0;
>> -	struct memory_notify *mnb = arg;
>> -	int nid = mnb->status_change_nid;
>> -
>> -	if (nid == NUMA_NO_NODE)
>> -		return NOTIFY_DONE;
>> -
>> -	if (action == MEM_GOING_ONLINE)
>> -		ret = hugetlb_register_node(node_devices[nid]);
>> -	else if (action == MEM_CANCEL_ONLINE || action == MEM_OFFLINE)
>> -		hugetlb_unregister_node(node_devices[nid]);
>> -
>> -	return notifier_from_errno(ret);
>> }
>> 
>> /*
>> @@ -4038,11 +4023,9 @@ static void __init hugetlb_register_all_nodes(void)
>> {
>> 	int nid;
>> 
>> -	get_online_mems();
>> -	hotplug_memory_notifier(hugetlb_memory_callback, 0);
>> +	hugetlb_initialized = true;
>> 	for_each_node_state(nid, N_MEMORY)
> 
> 
> Should this be for_each_online_node() ?

So, yes.

Thanks for your review.

Muchun.

> 
>> 		hugetlb_register_node(node_devices[nid]);
>> -	put_online_mems();
>> }
>> #else	/* !CONFIG_NUMA */

Re: [PATCH] mm: hugetlb: eliminate memory-less nodes handling

[kernel test robot]

Hi Muchun,

I love your patch! Yet something to improve:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Muchun-Song/mm-hugetlb-eliminate-memory-less-nodes-handling/20220901-163132
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
config: loongarch-buildonly-randconfig-r004-20220901 (https://download.01.org/0day-ci/archive/20220902/202209021255.1ChhyCsl-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/8e3d203cb06be81565d117b3f6d5e279a833174c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Muchun-Song/mm-hugetlb-eliminate-memory-less-nodes-handling/20220901-163132
        git checkout 8e3d203cb06be81565d117b3f6d5e279a833174c
        # save the config file
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 ARCH=loongarch 

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/base/node.c: In function 'register_node':
>> drivers/base/node.c:614:17: error: implicit declaration of function 'hugetlb_register_node'; did you mean 'unregister_node'? [-Werror=implicit-function-declaration]
     614 |                 hugetlb_register_node(node);
         |                 ^~~~~~~~~~~~~~~~~~~~~
         |                 unregister_node
   drivers/base/node.c: In function 'unregister_node':
>> drivers/base/node.c:630:9: error: implicit declaration of function 'hugetlb_unregister_node'; did you mean 'unregister_node'? [-Werror=implicit-function-declaration]
     630 |         hugetlb_unregister_node(node);
         |         ^~~~~~~~~~~~~~~~~~~~~~~
         |         unregister_node
   cc1: some warnings being treated as errors


vim +614 drivers/base/node.c

   594	
   595	/*
   596	 * register_node - Setup a sysfs device for a node.
   597	 * @num - Node number to use when creating the device.
   598	 *
   599	 * Initialize and register the node device.
   600	 */
   601	static int register_node(struct node *node, int num)
   602	{
   603		int error;
   604	
   605		node->dev.id = num;
   606		node->dev.bus = &node_subsys;
   607		node->dev.release = node_device_release;
   608		node->dev.groups = node_dev_groups;
   609		error = device_register(&node->dev);
   610	
   611		if (error) {
   612			put_device(&node->dev);
   613		} else {
 > 614			hugetlb_register_node(node);
   615			compaction_register_node(node);
   616		}
   617	
   618		return error;
   619	}
   620	
   621	/**
   622	 * unregister_node - unregister a node device
   623	 * @node: node going away
   624	 *
   625	 * Unregisters a node device @node.  All the devices on the node must be
   626	 * unregistered before calling this function.
   627	 */
   628	void unregister_node(struct node *node)
   629	{
 > 630		hugetlb_unregister_node(node);
   631		compaction_unregister_node(node);
   632		node_remove_accesses(node);
   633		node_remove_caches(node);
   634		device_unregister(&node->dev);
   635	}
   636	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp