[PATCH] x86, mm: clean up setup_node_bootmem

[PATCH] x86, mm: clean up setup_node_bootmem

[Yinghai Lu]

only one user now, so change it to static

Also move validity checking into the fuction.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 arch/x86/include/asm/numa_64.h |    2 --
 arch/x86/mm/numa_64.c          |   10 +++-------
 2 files changed, 3 insertions(+), 9 deletions(-)

Index: linux-2.6/arch/x86/include/asm/numa_64.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/numa_64.h
+++ linux-2.6/arch/x86/include/asm/numa_64.h
@@ -13,8 +13,6 @@ struct bootnode {
 extern int numa_off;
 
 extern unsigned long numa_free_all_bootmem(void);
-extern void setup_node_bootmem(int nodeid, unsigned long start,
-			       unsigned long end);
 
 #ifdef CONFIG_NUMA
 /*
Index: linux-2.6/arch/x86/mm/numa_64.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/numa_64.c
+++ linux-2.6/arch/x86/mm/numa_64.c
@@ -237,21 +237,18 @@ int __init numa_add_memblk(int nid, u64
 }
 
 /* Initialize bootmem allocator for a node */
-void __init
+static void __init
 setup_node_bootmem(int nodeid, unsigned long start, unsigned long end)
 {
 	unsigned long start_pfn, last_pfn, nodedata_phys;
 	const int pgdat_size = roundup(sizeof(pg_data_t), PAGE_SIZE);
 	int nid;
 
-	if (!end)
-		return;
-
 	/*
 	 * Don't confuse VM with a node that doesn't have the
 	 * minimum amount of memory:
 	 */
-	if (end && (end - start) < NODE_MIN_SIZE)
+	if (end < (start +  NODE_MIN_SIZE))
 		return;
 
 	start = roundup(start, ZONE_ALIGN);
@@ -557,8 +554,7 @@ static int __init numa_register_memblks(
 			end = max(mi->blk[i].end, end);
 		}
 
-		if (start < end)
-			setup_node_bootmem(nid, start, end);
+		setup_node_bootmem(nid, start, end);
 	}
 
 	return 0;

Re: [PATCH] x86, mm: clean up setup_node_bootmem

[Tejun Heo]

On Wed, Mar 02, 2011 at 05:19:29PM -0800, Yinghai Lu wrote:
> only one user now, so change it to static

Can you please try to use proper sentences?  ie.

  There is only one user left now, so make it static.

> Also move validity checking into the fuction.
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
...
> Index: linux-2.6/arch/x86/mm/numa_64.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/mm/numa_64.c
> +++ linux-2.6/arch/x86/mm/numa_64.c
> @@ -237,21 +237,18 @@ int __init numa_add_memblk(int nid, u64
>  }
>  
>  /* Initialize bootmem allocator for a node */
> -void __init
> +static void __init
>  setup_node_bootmem(int nodeid, unsigned long start, unsigned long end)
>  {
>  	unsigned long start_pfn, last_pfn, nodedata_phys;
>  	const int pgdat_size = roundup(sizeof(pg_data_t), PAGE_SIZE);
>  	int nid;
>  
> -	if (!end)
> -		return;
> -
>  	/*
>  	 * Don't confuse VM with a node that doesn't have the
>  	 * minimum amount of memory:
>  	 */
> -	if (end && (end - start) < NODE_MIN_SIZE)
> +	if (end < (start +  NODE_MIN_SIZE))

Please drop the parentheses.

Thanks.

-- 
tejun

Re: [PATCH] x86, mm: clean up setup_node_bootmem

[David Rientjes]

On Wed, 2 Mar 2011, Yinghai Lu wrote:

> 
> only one user now, so change it to static
> 
> Also move validity checking into the fuction.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  arch/x86/include/asm/numa_64.h |    2 --
>  arch/x86/mm/numa_64.c          |   10 +++-------
>  2 files changed, 3 insertions(+), 9 deletions(-)
> 
> Index: linux-2.6/arch/x86/include/asm/numa_64.h
> ===================================================================
> --- linux-2.6.orig/arch/x86/include/asm/numa_64.h
> +++ linux-2.6/arch/x86/include/asm/numa_64.h
> @@ -13,8 +13,6 @@ struct bootnode {
>  extern int numa_off;
>  
>  extern unsigned long numa_free_all_bootmem(void);
> -extern void setup_node_bootmem(int nodeid, unsigned long start,
> -			       unsigned long end);
>  
>  #ifdef CONFIG_NUMA
>  /*
> Index: linux-2.6/arch/x86/mm/numa_64.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/mm/numa_64.c
> +++ linux-2.6/arch/x86/mm/numa_64.c
> @@ -237,21 +237,18 @@ int __init numa_add_memblk(int nid, u64
>  }
>  
>  /* Initialize bootmem allocator for a node */
> -void __init
> +static void __init
>  setup_node_bootmem(int nodeid, unsigned long start, unsigned long end)
>  {
>  	unsigned long start_pfn, last_pfn, nodedata_phys;
>  	const int pgdat_size = roundup(sizeof(pg_data_t), PAGE_SIZE);
>  	int nid;
>  
> -	if (!end)
> -		return;
> -
>  	/*
>  	 * Don't confuse VM with a node that doesn't have the
>  	 * minimum amount of memory:
>  	 */
> -	if (end && (end - start) < NODE_MIN_SIZE)
> +	if (end < (start +  NODE_MIN_SIZE))
>  		return;
>  
>  	start = roundup(start, ZONE_ALIGN);
> @@ -557,8 +554,7 @@ static int __init numa_register_memblks(
>  			end = max(mi->blk[i].end, end);
>  		}
>  
> -		if (start < end)
> -			setup_node_bootmem(nid, start, end);
> +		setup_node_bootmem(nid, start, end);
>  	}
>  
>  	return 0;
> 

Good catch on finding only the one caller of setup_node_bootmem().

I'd actually rather see the validity checking being done in 
numa_register_memblks(), though, because it's a bug for a node to be set 
in node_possible_map without having a valid 
[mi->blk[i].start, mi->blk[i].end) range.

So could this be

	BUG_ON(end < start);
	/*
	 * Don't confuse the VM with a node that doesn't have the minimum
	 * amount of memory.
	 */
	if (end < start + NODE_MIN_SIZE) {
		node_clear(nid, node_possible_map);
		continue;
	}
	setup_node_bootmem(nid, start, end);

instead?

Re: [PATCH] x86, mm: clean up setup_node_bootmem

[Yinghai Lu]

On 03/03/2011 12:49 PM, David Rientjes wrote:
> On Wed, 2 Mar 2011, Yinghai Lu wrote:
> 
>>
>> only one user now, so change it to static
>>
>> Also move validity checking into the fuction.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> ---
>>  arch/x86/include/asm/numa_64.h |    2 --
>>  arch/x86/mm/numa_64.c          |   10 +++-------
>>  2 files changed, 3 insertions(+), 9 deletions(-)
>>
>> Index: linux-2.6/arch/x86/include/asm/numa_64.h
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/include/asm/numa_64.h
>> +++ linux-2.6/arch/x86/include/asm/numa_64.h
>> @@ -13,8 +13,6 @@ struct bootnode {
>>  extern int numa_off;
>>  
>>  extern unsigned long numa_free_all_bootmem(void);
>> -extern void setup_node_bootmem(int nodeid, unsigned long start,
>> -			       unsigned long end);
>>  
>>  #ifdef CONFIG_NUMA
>>  /*
>> Index: linux-2.6/arch/x86/mm/numa_64.c
>> ===================================================================
>> --- linux-2.6.orig/arch/x86/mm/numa_64.c
>> +++ linux-2.6/arch/x86/mm/numa_64.c
>> @@ -237,21 +237,18 @@ int __init numa_add_memblk(int nid, u64
>>  }
>>  
>>  /* Initialize bootmem allocator for a node */
>> -void __init
>> +static void __init
>>  setup_node_bootmem(int nodeid, unsigned long start, unsigned long end)
>>  {
>>  	unsigned long start_pfn, last_pfn, nodedata_phys;
>>  	const int pgdat_size = roundup(sizeof(pg_data_t), PAGE_SIZE);
>>  	int nid;
>>  
>> -	if (!end)
>> -		return;
>> -
>>  	/*
>>  	 * Don't confuse VM with a node that doesn't have the
>>  	 * minimum amount of memory:
>>  	 */
>> -	if (end && (end - start) < NODE_MIN_SIZE)
>> +	if (end < (start +  NODE_MIN_SIZE))
>>  		return;
>>  
>>  	start = roundup(start, ZONE_ALIGN);
>> @@ -557,8 +554,7 @@ static int __init numa_register_memblks(
>>  			end = max(mi->blk[i].end, end);
>>  		}
>>  
>> -		if (start < end)
>> -			setup_node_bootmem(nid, start, end);
>> +		setup_node_bootmem(nid, start, end);
>>  	}
>>  
>>  	return 0;
>>
> 
> Good catch on finding only the one caller of setup_node_bootmem().
> 
> I'd actually rather see the validity checking being done in 
> numa_register_memblks(), though, because it's a bug for a node to be set 
> in node_possible_map without having a valid 
> [mi->blk[i].start, mi->blk[i].end) range.
> 
> So could this be
> 
> 	BUG_ON(end < start);

no, it could cause crash here

        /* Finally register nodes. */
        for_each_node_mask(nid, node_possible_map) {
                u64 start = (u64)max_pfn << PAGE_SHIFT;
                u64 end = 0;

                for (i = 0; i < mi->nr_blks; i++) {
                        if (nid != mi->blk[i].nid)
                                continue;
                        start = min(mi->blk[i].start, start);
                        end = max(mi->blk[i].end, end);
                }

could have some node without memory. and it could be set in node_possible_map, and it will have end = 0, and start max_pfn<<page_shift.


> 	/*
> 	 * Don't confuse the VM with a node that doesn't have the minimum
> 	 * amount of memory.
> 	 */
> 	if (end < start + NODE_MIN_SIZE) {
> 		node_clear(nid, node_possible_map);

why touch that possible_map ?
online_map is for that purpose that state node have ram.

> 		continue;
> 	}
> 	setup_node_bootmem(nid, start, end);
> 
> instead?

Thanks

Yinghai

Re: [PATCH] x86, mm: clean up setup_node_bootmem

[David Rientjes]

On Thu, 3 Mar 2011, Yinghai Lu wrote:

> >> Index: linux-2.6/arch/x86/mm/numa_64.c
> >> ===================================================================
> >> --- linux-2.6.orig/arch/x86/mm/numa_64.c
> >> +++ linux-2.6/arch/x86/mm/numa_64.c
> >> @@ -237,21 +237,18 @@ int __init numa_add_memblk(int nid, u64
> >>  }
> >>  
> >>  /* Initialize bootmem allocator for a node */
> >> -void __init
> >> +static void __init
> >>  setup_node_bootmem(int nodeid, unsigned long start, unsigned long end)
> >>  {
> >>  	unsigned long start_pfn, last_pfn, nodedata_phys;
> >>  	const int pgdat_size = roundup(sizeof(pg_data_t), PAGE_SIZE);
> >>  	int nid;
> >>  
> >> -	if (!end)
> >> -		return;
> >> -
> >>  	/*
> >>  	 * Don't confuse VM with a node that doesn't have the
> >>  	 * minimum amount of memory:
> >>  	 */
> >> -	if (end && (end - start) < NODE_MIN_SIZE)
> >> +	if (end < (start +  NODE_MIN_SIZE))
> >>  		return;
> >>  
> >>  	start = roundup(start, ZONE_ALIGN);
> >> @@ -557,8 +554,7 @@ static int __init numa_register_memblks(
> >>  			end = max(mi->blk[i].end, end);
> >>  		}
> >>  
> >> -		if (start < end)
> >> -			setup_node_bootmem(nid, start, end);
> >> +		setup_node_bootmem(nid, start, end);
> >>  	}
> >>  
> >>  	return 0;
> >>
> > 
> > Good catch on finding only the one caller of setup_node_bootmem().
> > 
> > I'd actually rather see the validity checking being done in 
> > numa_register_memblks(), though, because it's a bug for a node to be set 
> > in node_possible_map without having a valid 
> > [mi->blk[i].start, mi->blk[i].end) range.
> > 
> > So could this be
> > 
> > 	BUG_ON(end < start);
> 
> no, it could cause crash here
> 
>         /* Finally register nodes. */
>         for_each_node_mask(nid, node_possible_map) {
>                 u64 start = (u64)max_pfn << PAGE_SHIFT;
>                 u64 end = 0;
> 
>                 for (i = 0; i < mi->nr_blks; i++) {
>                         if (nid != mi->blk[i].nid)
>                                 continue;
>                         start = min(mi->blk[i].start, start);
>                         end = max(mi->blk[i].end, end);
>                 }
> 
> could have some node without memory. and it could be set in node_possible_map, and it will have end = 0, and start max_pfn<<page_shift.
> 

Ok, I didn't realize this was being used for memoryless nodes.

> > 	/*
> > 	 * Don't confuse the VM with a node that doesn't have the minimum
> > 	 * amount of memory.
> > 	 */
> > 	if (end < start + NODE_MIN_SIZE) {
> > 		node_clear(nid, node_possible_map);
> 
> why touch that possible_map ?
> online_map is for that purpose that state node have ram.
> 

Hmm, that's the purpose of N_NORMAL_MEMORY, not node_online_map.  I 
understand why we don't want to register bootmem for a node that has such 
little memory that we disregard it, but shouldn't we then consider it to 
no longer be possible?