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

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

[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 <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 | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index 98a31bafc8a2..735073fd2926 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -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
 		}
 
 		/*
-- 
2.21.0

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

[Michal Hocko]

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.

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

> ---
>  drivers/base/node.c | 23 +++++++----------------
>  1 file changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 98a31bafc8a2..735073fd2926 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -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
>  		}
>  
>  		/*
> -- 
> 2.21.0

-- 
Michal Hocko
SUSE Labs

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

[David Hildenbrand]

> 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?

> 
> 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
> 
>> ---
>> drivers/base/node.c | 23 +++++++----------------
>> 1 file changed, 7 insertions(+), 16 deletions(-)
>> 
>> diff --git a/drivers/base/node.c b/drivers/base/node.c
>> index 98a31bafc8a2..735073fd2926 100644
>> --- a/drivers/base/node.c
>> +++ b/drivers/base/node.c
>> @@ -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
>>        }
>> 
>>        /*
>> -- 
>> 2.21.0
> 
> -- 
> Michal Hocko
> SUSE Labs
> 

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

[David Hildenbrand]

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

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

[Michal Hocko]

On Thu 28-11-19 12:23:08, David Hildenbrand wrote:
[...]
> >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>

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 <mhocko@suse.com>

Thanks!

> ---
>  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

-- 
Michal Hocko
SUSE Labs

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

[David Hildenbrand]

On 28.11.19 12:50, Michal Hocko wrote:
> On Thu 28-11-19 12:23:08, David Hildenbrand wrote:
> [...]
>> >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>
> 
> 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.

Will give it a try, though - thanks!

> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks!


-- 
Thanks,

David / dhildenb

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

[Michal Hocko]

On Thu 28-11-19 12:52:16, David Hildenbrand wrote:
> On 28.11.19 12:50, Michal Hocko wrote:
> > On Thu 28-11-19 12:23:08, David Hildenbrand wrote:
> > [...]
> >> >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>
> > 
> > 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.

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.

-- 
Michal Hocko
SUSE Labs

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

[David Hildenbrand]

On 28.11.19 13:01, Michal Hocko wrote:
> On Thu 28-11-19 12:52:16, David Hildenbrand wrote:
>> On 28.11.19 12:50, Michal Hocko wrote:
>>> On Thu 28-11-19 12:23:08, David Hildenbrand wrote:
>>> [...]
>>>> >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>
>>>
>>> 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.
> 
> 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

-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 28.11.19 13:19, David Hildenbrand wrote:
> On 28.11.19 13:01, Michal Hocko wrote:
>> On Thu 28-11-19 12:52:16, David Hildenbrand wrote:
>>> On 28.11.19 12:50, Michal Hocko wrote:
>>>> On Thu 28-11-19 12:23:08, David Hildenbrand wrote:
>>>> [...]
>>>>> >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>
>>>>
>>>> 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.
>>
>> 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
> 

So ... the code is fenced by MEMORY_HOTPLUG_SPARSE, which requires
"SPARSEMEM && MEMORY_HOTPLUG". MEMORY_HOTPLUG requires
ARCH_ENABLE_MEMORY_HOTPLUG

Architectures with ARCH_ENABLE_MEMORY_HOTPLUG (conditional):
arm64, ia64, powerpc, s390, sh, x86

Architectures with HAVE_MEMBLOCK_NODE_MAP (unconditional):
arm64, ia64, mips, powerpc, riscv, s390, sh, sparc, x86


Which implies that we will always have
mm/page_alloc.c:early_pfn_to_nid() and therefore no compile issues with

if (IS_ENABLED(CONFIG_DEFERRED_STRUCT_PAGE_INIT))
	return early_pfn_to_nid(pfn);


Will do more testing and send this as an official patch. Thanks!

-- 
Thanks,

David / dhildenb