| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 2 Sep 2022 11:42:13 +0800
From: Muchun Song <muchun.song@...ux.dev>
To: Aneesh Kumar K V <aneesh.kumar@...ux.ibm.com>
Cc: Muchun Song <songmuchun@...edance.com>,
Greg KH <gregkh@...uxfoundation.org>, rafael@...nel.org,
mike.kravetz@...cle.com, akpm@...ux-foundation.org,
osalvador@...e.de, david@...hat.com, ying.huang@...el.com,
rientjes@...gle.com, linux-kernel@...r.kernel.org,
linux-mm@...ck.org
Subject: Re: [PATCH] mm: hugetlb: eliminate memory-less nodes handling
> On Sep 1, 2022, at 17:00, Aneesh Kumar K V <aneesh.kumar@...ux.ibm.com> wrote:
>
> On 9/1/22 2:00 PM, 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.
>
> Isn't that hotplug callback added because in hugetlb_register_all_nodes() we register
> sysfs nodes only for N_MEMORY nodes?
I think you might right. I have looked at the commit 9a30523066cd which introduces the sysfs
creation. I saw it create the sysfs for every possible node.
for (nid = 0; nid < nr_node_ids; nid++)
hugetlb_register_node(node);
And then I checked the commit 9b5e5d0fdc91, which said it was a preparation for handling
memory-less nodes via memory hotplug.
>
>
>>
>> https://lore.kernel.org/linux-mm/60933ffc-b850-976c-78a0-0ee6e0ea9ef0@redhat.com/ [1]
>> Suggested-by: David Hildenbrand <david@...hat.com>
>> Signed-off-by: Muchun Song <songmuchun@...edance.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);
>> + }
>>
>
>
> I guess this will handle register of sysfs hugetlb files for new NUMA nodes
> after hugetlb_initialized = true;
Yes.
>
> But what about N_CPU that can get memory added later. Do we need to update
> hugetlb_register_all_nodes() to handle N_ONLINE?
I think we should.
>
>
>> 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
>> #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;
>>
>> /*
>> * 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)
>
>
> Should this be for_each_online_node() ?
So, yes.
Thanks for your review.
Muchun.
>
>> hugetlb_register_node(node_devices[nid]);
>> - put_online_mems();
>> }
>> #else /* !CONFIG_NUMA */
Powered by blists - more mailing lists