drivers/base/node.c: get rid of 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(). Let's just inline that code and get rid of get_nid_for_pfn().

Cc Greg Kroah-Hartman <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Oscar Salvador <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 drivers/base/node.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

drivers/base/node.c +7 -16

index 98a31bafc8a2..735073fd2926 100644
@@ -744,17 +744,6 @@ 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)
-{
-	if (!pfn_valid_within(pfn))
-		return -1;
-#ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
-	if (system_state < SYSTEM_RUNNING)
-		return early_pfn_to_nid(pfn);
-#endif
-	return pfn_to_nid(pfn);
-}
-
 /* register memory section under specified node if it spans that node */
 static int register_mem_sect_under_node(struct memory_block *mem_blk,
 					 void *arg)

@@ -766,8 +755,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

@@ -784,11 +771,15 @@ static int register_mem_sect_under_node(struct memory_block *mem_blk,
 		 * block belong to the same node.
 		 */
 		if (system_state == SYSTEM_BOOTING) {
-			page_nid = get_nid_for_pfn(pfn);
-			if (page_nid < 0)
+			if (!pfn_valid_within(pfn))
 				continue;
-			if (page_nid != nid)
+#ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
+			if (early_pfn_to_nid(pfn) != nid)
 				continue;
+#else
+			if (pfn_to_nid(pfn) != nid)
+				continue;
+#endif
 		}
 
 		/*

Signature · 1 line

2.21.0

Am 28.11.2019 um 11:20 schrieb Michal Hocko <[email protected]>:

Quoted 11 lines · David Hildenbrand

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 <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Oscar Salvador <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

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.

If anything get_nid_for_pfn deserves a comment why CONFIG_DEFERRED_STRUCT_PAGE_INIT calls for special case as early_pfn_to_nid is not bound to that config (it is defined when CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID || CONFIG_HAVE_MEMBLOCK_NODE_MAP

Quoted 56 lines

---
drivers/base/node.c | 23 +++++++----------------

1 file changed, 7 insertions(+), 16 deletions(-)

drivers/base/node.c +7 -16

index 98a31bafc8a2..735073fd2926 100644
@@ -744,17 +744,6 @@ 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)
-{
-    if (!pfn_valid_within(pfn))
-        return -1;
-#ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
-    if (system_state < SYSTEM_RUNNING)
-        return early_pfn_to_nid(pfn);
-#endif
-    return pfn_to_nid(pfn);
-}
-
 /* register memory section under specified node if it spans that node */
 static int register_mem_sect_under_node(struct memory_block *mem_blk,
                    void *arg)

@@ -766,8 +755,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

@@ -784,11 +771,15 @@ static int register_mem_sect_under_node(struct memory_block *mem_blk,
        * block belong to the same node.
        */
       if (system_state == SYSTEM_BOOTING) {
-            page_nid = get_nid_for_pfn(pfn);
-            if (page_nid < 0)
+            if (!pfn_valid_within(pfn))
               continue;
-            if (page_nid != nid)
+#ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
+            if (early_pfn_to_nid(pfn) != nid)
               continue;
+#else
+            if (pfn_to_nid(pfn) != nid)
+                continue;
+#endif
       }
 
       /*

Signature · 1 line

2.21.0

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 17 lines

Am 28.11.2019 um 11:20 schrieb Michal Hocko <[email protected]>:

Quoted 11 lines · David Hildenbrand

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 <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Oscar Salvador <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

Quoted 3 lines

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?

Quoted 1 line

From fc13fd540a1702592e389e821f6266098e41e2bd Mon Sep 17 00:00:00 2001

From: David Hildenbrand <[email protected]>
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 <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Oscar Salvador <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 drivers/base/node.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

drivers/base/node.c +9 -13

index 98a31bafc8a2..d525e30581de 100644
@@ -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

Signature · 9 lines

2.21.0




-- 
Thanks,

David / dhildenb

On Thu 28-11-19 12:23:08, David Hildenbrand wrote:
[...]

Quoted 19 lines

Quoted 1 line

From fc13fd540a1702592e389e821f6266098e41e2bd Mon Sep 17 00:00:00 2001

From: David Hildenbrand <[email protected]>
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).

Quoted 6 lines

Cc Greg Kroah-Hartman <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Oscar Salvador <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

Yes this looks much better! I am not sure this will pass all weird config combinations because IS_ENABLED will not hide early_pfn_to_nid from the early compiler stages so it might complain. But if this passes 0day compile scrutiny then this is much much better. If not then we just have to use ifdef which is a minor thing.

Acked-by Michal Hocko <[email protected]>

Thanks!

Quoted 28 lines · Michal Hocko

On Thu 28-11-19 12:23:08, David Hildenbrand wrote:
[...]

Quoted 19 lines

Quoted 1 line

From fc13fd540a1702592e389e821f6266098e41e2bd Mon Sep 17 00:00:00 2001

From: David Hildenbrand <[email protected]>
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).

Quoted 6 lines

Cc Greg Kroah-Hartman <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Oscar Salvador <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

Quoted 5 lines

Yes this looks much better! I am not sure this will pass all weird config combinations because IS_ENABLED will not hide early_pfn_to_nid from the early compiler stages so it might complain. But if this passes 0day compile scrutiny then this is much much better. If not then we just have to use ifdef which is a minor thing.

The compiler should optimize out

if (0)
code

and therefore never link to early_pfn_to_nid.

Quoted 40 lines · David Hildenbrand

Quoted 28 lines · Michal Hocko

On Thu 28-11-19 12:23:08, David Hildenbrand wrote:
[...]

Quoted 19 lines

Quoted 1 line

From fc13fd540a1702592e389e821f6266098e41e2bd Mon Sep 17 00:00:00 2001

From: David Hildenbrand <[email protected]>
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).

Quoted 6 lines

Cc Greg Kroah-Hartman <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Oscar Salvador <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

Quoted 5 lines

Yes this looks much better! I am not sure this will pass all weird config combinations because IS_ENABLED will not hide early_pfn_to_nid from the early compiler stages so it might complain. But if this passes 0day compile scrutiny then this is much much better. If not then we just have to use ifdef which is a minor thing.

The compiler should optimize out

if (0)
code

Quoted 1 line

and therefore never link to early_pfn_to_nid.

You are right, but there is a catch. The optimization phase is much later than the syntactic check so if the code doesn't make sense for the syntactic point of view then it will complain. This is a notable difference to #ifdef which just removes the whole block in the preprocessor phase.

Quoted 41 lines · Michal Hocko

Quoted 40 lines · David Hildenbrand

Quoted 28 lines · Michal Hocko

On Thu 28-11-19 12:23:08, David Hildenbrand wrote:
[...]

Quoted 19 lines

Quoted 1 line

From fc13fd540a1702592e389e821f6266098e41e2bd Mon Sep 17 00:00:00 2001

From: David Hildenbrand <[email protected]>
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).

Quoted 6 lines

Cc Greg Kroah-Hartman <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Oscar Salvador <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

Quoted 5 lines

Yes this looks much better! I am not sure this will pass all weird config combinations because IS_ENABLED will not hide early_pfn_to_nid from the early compiler stages so it might complain. But if this passes 0day compile scrutiny then this is much much better. If not then we just have to use ifdef which is a minor thing.

The compiler should optimize out

if (0)
code

Quoted 8 lines

Quoted 1 line

and therefore never link to early_pfn_to_nid.

You are right, but there is a catch. The optimization phase is much later than the syntactic check so if the code doesn't make sense for the syntactic point of view then it will complain. This is a notable difference to #ifdef which just removes the whole block in the preprocessor phase.

We should always have a declaration of early_pfn_to_nid(). The interesting part AFAIKS is include/linux/mmzone.h:

#if !defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID) && \
!defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP)
static inline unsigned long early_pfn_to_nid(unsigned long pfn)

{
	BUILD_BUG_ON(IS_ENABLED(CONFIG_NUMA));
	return 0;
}

#endif

so we would have

if (IS_ENABLED(...))

	BUILD_BUG_ON(IS_ENABLED(CONFIG_NUMA));

Let's see how that will turn out :) Will do some test builds (CONFIG_NUMA, !CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID and !CONFIG_HAVE_MEMBLOCK_NODE_MAP) ... if possible