On 28.11.19 11:25, David Hildenbrand wrote:
>
>
>> Am 28.11.2019 um 11:20 schrieb Michal Hocko <mhocko@xxxxxxxxxx>:
>>
>> On Wed 27-11-19 18:41:26, David Hildenbrand wrote:
>>> Since commit d84f2f5a7552 ("drivers/base/node.c: simplify
>>> unregister_memory_block_under_nodes()") we only have a single user of
>>> get_nid_for_pfn(). Let's just inline that code and get rid of
>>> get_nid_for_pfn().
>>>
>>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>> Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
>>> Cc: Michal Hocko <mhocko@xxxxxxxxxx>
>>> Cc: Oscar Salvador <osalvador@xxxxxxx>
>>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>>> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>>
>> I am not really sure this is an improvement. The code is ugly as hell
>> and open coding it just makes register_mem_sect_under_node harder to
>> read.
>
> The issue I see is that this is a dangerous wrapper for pfn_to_nid() that is absolutely not obvious. The old second user on the memory removal path was completely buggy. IMHO nobody should be reusing that function. But it looks like a general „safe wrapper to get a nid“ - it‘s not.
>
> How can we make that more obvious instead?
>
What about something like this (untested):
>From fc13fd540a1702592e389e821f6266098e41e2bd Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@xxxxxxxxxx>
Date: Wed, 27 Nov 2019 16:18:42 +0100
Subject: [PATCH] drivers/base/node.c: optimize get_nid_for_pfn()
Since commit d84f2f5a7552 ("drivers/base/node.c: simplify
unregister_memory_block_under_nodes()") we only have a single user of
get_nid_for_pfn(). The remaining user calls this function when booting -
where all added memory is online.
Make it clearer that this function should only be used during boot (
e.g., calling it on offline memory would be bad) by renaming the
function to something meaningful, optimize out the ifdef and the additional
system_state check, and add a comment why CONFIG_DEFERRED_STRUCT_PAGE_INIT
handling is in place at all.
Also, optimize the call site. There is no need to check against
page_nid < 0 - it will never match the nid (nid >= 0).
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
Cc: Michal Hocko <mhocko@xxxxxxxxxx>
Cc: Oscar Salvador <osalvador@xxxxxxx>
Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
---
drivers/base/node.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/drivers/base/node.c b/drivers/base/node.c
index 98a31bafc8a2..d525e30581de 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -744,14 +744,16 @@ int unregister_cpu_under_node(unsigned int cpu, unsigned int nid)
}
#ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
-static int __ref get_nid_for_pfn(unsigned long pfn)
+static int __ref boot_pfn_to_nid(unsigned long pfn)
{
if (!pfn_valid_within(pfn))
return -1;
-#ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
- if (system_state < SYSTEM_RUNNING)
+ /*
+ * With deferred struct page initialization, the memmap will contain
+ * garbage - we have to rely on the early nid.
+ */
+ if (IS_ENABLED(CONFIG_DEFERRED_STRUCT_PAGE_INIT))
return early_pfn_to_nid(pfn);
-#endif
return pfn_to_nid(pfn);
}
@@ -766,8 +768,6 @@ static int register_mem_sect_under_node(struct memory_block *mem_blk,
unsigned long pfn;
for (pfn = start_pfn; pfn <= end_pfn; pfn++) {
- int page_nid;
-
/*
* memory block could have several absent sections from start.
* skip pfn range from absent section
@@ -783,13 +783,9 @@ static int register_mem_sect_under_node(struct memory_block *mem_blk,
* case, during hotplug we know that all pages in the memory
* block belong to the same node.
*/
- if (system_state == SYSTEM_BOOTING) {
- page_nid = get_nid_for_pfn(pfn);
- if (page_nid < 0)
- continue;
- if (page_nid != nid)
- continue;
- }
+ if (system_state == SYSTEM_BOOTING &&
+ boot_pfn_to_nid(pfn) != nid)
+ continue;
/*
* If this memory block spans multiple nodes, we only indicate
--
2.21.0
--
Thanks,
David / dhildenb
