Re: [PATCH RESEND] mm: hugetlb: simplify per-node sysfs creation and removal
From: Muchun Song
Date: Tue Aug 23 2022 - 23:26:55 EST
> 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?
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