> On Aug 24, 2022, at 11:23, Muchun Song <muchun.song@xxxxxxxxx> wrote:
>
>
>
>> On Aug 23, 2022, at 18:21, David Hildenbrand <david@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
>>
>>
>> [...]
>>
>>> @@ -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,
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
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?
Thanks,
Muchun
>
> I have found the commit introduced this mechanism, see commit:
>
> 4faf8d950ec4 ("hugetlb: handle memory hot-plug events")
>
> From the commit message, I think it is a suggestion from David Rientjes.
> I didn’t see any reasons why we need it. So Cc David Rientjes (Maybe
> he knew more context). The committer Lee and the reviewer Andi’s email
> is invalid (don’t Cc them)
>
>>
>> Why can't we simply register/unregister sysfs entries in
>> register_node/unregister_node and call it a day?
>>
>
> At least, I agree with you. Before I change to this way, let’s wait for
> some potential comments from David Rientjes.
>
>
> Thanks.
>
>> TBH, we should just have sysfs entries for memory-less nodes and not
>> care about such (corner) cases.
>>
>>
>> --
>> Thanks,
>>
>> David / dhildenb
