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: <a4275591-46bc-009b-c67d-8a474a57d2d6@redhat.com>
Date:   Thu, 1 Sep 2022 08:57:46 +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 <muchun.song@linux.dev>
Cc:     Muchun Song <songmuchun@bytedance.com>,
        Greg KH <gregkh@linuxfoundation.org>, rafael@kernel.org,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Oscar Salvador <osalvador@suse.de>, andi@firstfloor.org,
        linux-kernel@vger.kernel.org, Linux MM <linux-mm@kvack.org>,
        David Rientjes <rientjes@google.com>
References: <20220819080029.12241-1-songmuchun@bytedance.com>
 <60933ffc-b850-976c-78a0-0ee6e0ea9ef0@redhat.com>
 <685F3238-D727-4C1F-9BA4-36651ABCD91A@linux.dev>
 <E4D5CD53-99A9-479E-AF4C-6653852D8A98@linux.dev>
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: <E4D5CD53-99A9-479E-AF4C-6653852D8A98@linux.dev>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 01.09.22 08:35, Muchun Song wrote:
> 
> 
>> On Aug 24, 2022, at 11:23, Muchun Song <muchun.song@linux.dev> wrote:
>>
>>
>>
>>> On Aug 23, 2022, at 18:21, David Hildenbrand <david@redhat.com> wrote:
>>>
>>> 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.
>>
>> Yep, a minor cleanup BTW.
>>
>>>
>>>> */
>>>> #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?
> 
> Hi David,
> 

Hi,

thanks for playing with the idea.

> After some tries, I think it may not reduce the complexity. node_dev_init()
> is called at early stage before hugetlb_register_all_nodes(). So we need to
> add a mechanism to detect if the hugetlb subsystem finishes initialization
> in node_dev_init() so that it can determine to help hugetlb create /sysfs
> files, the mechanism is similar with the changes in drivers/base/node.c of
> commit 9a30523066cd ("hugetlb: add per node hstate attributes”). This approach

If I'm not wrong, all you need is a single call from node_dev_init()
into hugetlb code.

There, you create the hugetlb sysfs if hugetlb was already initialized,
otherwise it's a NOP as you initialize when hugetlb gets initialized.

When initializing hugetlb, you go over all added nodes and create
hugetlb sysfs.

Testing/remembering if hugetlb was initialized should be easy, no?

What's the complicated part I am missing?

> may add more code than the memory-notify-based approach like this patch
> implemented. And it also add the code coupling between node.c and hugetlb.c.
> So I tend to use memory hotplug notifier. What’s your opinion?

We have hugetlb special casing all over the place, it's an integral MM
part -- not some random driver where we'd really want decoupling.

So I don't see why the decouling would be beneficial here and how using
the memory notifier is any better then a simple callback.


But again, I did not look into the details of the necessary implementation.

Thanks!


-- 
Thanks,

David / dhildenb