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: <60933ffc-b850-976c-78a0-0ee6e0ea9ef0@redhat.com>
Date:   Tue, 23 Aug 2022 12:21:34 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.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, andi@firstfloor.org
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        muchun.song@linux.dev
References: <20220819080029.12241-1-songmuchun@bytedance.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH RESEND] mm: hugetlb: simplify per-node sysfs creation and
 removal
In-Reply-To: <20220819080029.12241-1-songmuchun@bytedance.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 19.08.22 10:00, Muchun Song wrote:
> The following commit offload per-node sysfs creation and removal to a kworker and
> did not say why it is needed.  And it also said "I don't know that this is
> absolutely required".  It seems like the author was not sure as well.  Since it
> only complicates the code, this patch will revert the changes to simplify the code.
> 
>   39da08cb074c ("hugetlb: offload per node attribute registrations")
> 
> We could use memory hotplug notifier to do per-node sysfs creation and removal
> instead of inserting those operations to node registration and unregistration.
> Then, it can reduce the code coupling between node.c and hugetlb.c.  Also, it can
> simplify the code.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>


[...]

> @@ -683,7 +626,6 @@ static int register_node(struct node *node, int num)
>  void unregister_node(struct node *node)
>  {
>  	compaction_unregister_node(node);
> -	hugetlb_unregister_node(node);		/* no-op, if memoryless node */
>  	node_remove_accesses(node);
>  	node_remove_caches(node);
>  	device_unregister(&node->dev);
> @@ -905,74 +847,8 @@ void register_memory_blocks_under_node(int nid, unsigned long start_pfn,
>  			   (void *)&nid, func);
>  	return;
>  }

[...]

>  	/*
>  	 * Create all node devices, which will properly link the node
>  	 * to applicable memory block devices and already created cpu devices.
> diff --git a/include/linux/node.h b/include/linux/node.h
> index 40d641a8bfb0..ea817b507f54 100644
> --- a/include/linux/node.h
> +++ b/include/linux/node.h
> @@ -2,15 +2,15 @@
>  /*
>   * include/linux/node.h - generic node definition
>   *
> - * This is mainly for topological representation. We define the 
> - * basic 'struct node' here, which can be embedded in per-arch 
> + * This is mainly for topological representation. We define the
> + * basic 'struct node' here, which can be embedded in per-arch
>   * definitions of processors.
>   *
>   * Basic handling of the devices is done in drivers/base/node.c
> - * and system devices are handled in drivers/base/sys.c. 
> + * and system devices are handled in drivers/base/sys.c.
>   *
>   * Nodes are exported via driverfs in the class/node/devices/
> - * directory. 
> + * directory.

Unrelated changes.

>   */
>  #ifndef _LINUX_NODE_H_
>  #define _LINUX_NODE_H_
> @@ -18,7 +18,6 @@
>  #include <linux/device.h>
>  #include <linux/cpumask.h>
>  #include <linux/list.h>
> -#include <linux/workqueue.h>
>  
>  /**
>   * struct node_hmem_attrs - heterogeneous memory performance attributes
> @@ -84,10 +83,6 @@ static inline void node_set_perf_attrs(unsigned int nid,
>  struct node {
>  	struct device	dev;
>  	struct list_head access_list;
> -
> -#if defined(CONFIG_MEMORY_HOTPLUG) && defined(CONFIG_HUGETLBFS)
> -	struct work_struct	node_work;
> -#endif
>  #ifdef CONFIG_HMEM_REPORTING
>  	struct list_head cache_attrs;
>  	struct device *cache_dev;
> @@ -96,7 +91,6 @@ struct node {
>  
>  struct memory_block;
>  extern struct node *node_devices[];
> -typedef  void (*node_registration_func_t)(struct node *);
>  
>  #if defined(CONFIG_MEMORY_HOTPLUG) && defined(CONFIG_NUMA)
>  void register_memory_blocks_under_node(int nid, unsigned long start_pfn,
> @@ -144,11 +138,6 @@ 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
> -extern void register_hugetlbfs_with_node(node_registration_func_t doregister,
> -					 node_registration_func_t unregister);
> -#endif
>  #else
>  static inline void node_dev_init(void)
>  {
> @@ -176,11 +165,6 @@ static inline int unregister_cpu_under_node(unsigned int cpu, unsigned int nid)
>  static inline void unregister_memory_block_under_nodes(struct memory_block *mem_blk)
>  {
>  }
> -
> -static inline void register_hugetlbfs_with_node(node_registration_func_t reg,
> -						node_registration_func_t unreg)
> -{
> -}
>  #endif
>  
>  #define to_node(device) container_of(device, struct node, dev)
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 536a52c29035..9a72499486c1 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -33,6 +33,7 @@
>  #include <linux/migrate.h>
>  #include <linux/nospec.h>
>  #include <linux/delayacct.h>
> +#include <linux/memory.h>
>  
>  #include <asm/page.h>
>  #include <asm/pgalloc.h>
> @@ -3967,19 +3968,19 @@ static void hugetlb_unregister_node(struct node *node)
>   * Register hstate attributes for a single node device.
>   * No-op if attributes already registered.
>   */
> -static void hugetlb_register_node(struct node *node)
> +static int hugetlb_register_node(struct node *node)
>  {
>  	struct hstate *h;
>  	struct node_hstate *nhs = &node_hstates[node->dev.id];
>  	int err;
>  
>  	if (nhs->hugepages_kobj)
> -		return;		/* already allocated */
> +		return 0;		/* already allocated */
>  
>  	nhs->hugepages_kobj = kobject_create_and_add("hugepages",
>  							&node->dev.kobj);
>  	if (!nhs->hugepages_kobj)
> -		return;
> +		return -ENOMEM;
>  
>  	for_each_hstate(h) {
>  		err = hugetlb_sysfs_add_hstate(h, nhs->hugepages_kobj,
> @@ -3989,9 +3990,28 @@ static void 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);
> -			break;
> +			return -ENOMEM;
>  		}
>  	}
> +	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);
>  }
>  
>  /*
> @@ -4003,18 +4023,11 @@ static void __init hugetlb_register_all_nodes(void)
>  {
>  	int nid;
>  
> -	for_each_node_state(nid, N_MEMORY) {
> -		struct node *node = node_devices[nid];
> -		if (node->dev.id == nid)
> -			hugetlb_register_node(node);
> -	}
> -
> -	/*
> -	 * Let the node device driver know we're here so it can
> -	 * [un]register hstate attributes on node hotplug.
> -	 */
> -	register_hugetlbfs_with_node(hugetlb_register_node,
> -				     hugetlb_unregister_node);
> +	get_online_mems();
> +	hotplug_memory_notifier(hugetlb_memory_callback, 0);
> +	for_each_node_state(nid, N_MEMORY)
> +		hugetlb_register_node(node_devices[nid]);
> +	put_online_mems();
>  }
>  #else	/* !CONFIG_NUMA */

Do we really *need* the memory hotplug notifier and the added complexity
due for handling memory-less nodes?

Why can't we simply register/unregister sysfs entries in
register_node/unregister_node and call it a day?

TBH, we should just have sysfs entries for memory-less nodes and not
care about such (corner) cases.


-- 
Thanks,

David / dhildenb