Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <afc8e20c-0317-afe8-ced5-320a575980ea@redhat.com>
Date:   Thu, 1 Sep 2022 10:50:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Content-Language: en-US
To:     Muchun Song <songmuchun@bytedance.com>, gregkh@linuxfoundation.org,
        rafael@kernel.org, mike.kravetz@oracle.com,
        akpm@linux-foundation.org, osalvador@suse.de, ying.huang@intel.com,
        aneesh.kumar@linux.ibm.com, rientjes@google.com
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org
References: <20220901083023.42319-1-songmuchun@bytedance.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH] mm: hugetlb: eliminate memory-less nodes handling
In-Reply-To: <20220901083023.42319-1-songmuchun@bytedance.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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