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

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

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