Re: [PATCH v1] drivers/base/node.c: get rid of get_nid_for_pfn()

[David Hildenbrand <david@redhat.com>]

On 28.11.19 11:25, David Hildenbrand wrote:
> 
> 
>> Am 28.11.2019 um 11:20 schrieb Michal Hocko <mhocko@kernel.org>:
>>
>> 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@linuxfoundation.org>
>>> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>>> Cc: Michal Hocko <mhocko@kernel.org>
>>> Cc: Oscar Salvador <osalvador@suse.de>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>
>> 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@redhat.com>
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@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 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