Re: [PATCH x86/mm UPDATED] x86-64, NUMA: Fix distance table handling

[Yinghai Lu <yinghai@kernel.org>]

On 03/02/2011 02:25 AM, Tejun Heo wrote:
>>From d968be2ff381c667bfd09795f82248558902a1ae Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@kernel.org>
> Date: Wed, 2 Mar 2011 11:22:14 +0100
> 
> NUMA distance table handling has the following problems.
> 
> * numa_reset_distance() uses numa_distance * sizeof(numa_distance[0])
>   as the table size when it should be using the square of
>   numa_distance.
> 
> * The same size miscalculation when allocation space for phys_dist in
>   numa_emulation().
> 
> * In numa_emulation(), phys_dist must be reserved; otherwise, the new
>   emulated distance table may overlap it.
> 
> Fix them and, while at it, take numa_distance_cnt resetting in
> numa_reset_distance() out of the if block to simplify the code a bit.
> 
> David Rientjes reported incorrect handling of distance table during
> emulation and Yinghai identified the above problems and wrote the
> original patch to fix the problems.  This patch is based on Yinghai's
> patch.
> 
> -v2: Ingo was unhappy with 80-column limit induced linebreaks.  Let
>      lines run over 80-column.
> 
> Reported-by: David Rientjes <rientjes@google.com>
> Patch-originally-from: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/mm/numa_64.c        |    8 +++-----
>  arch/x86/mm/numa_emulation.c |   14 ++++++++------
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/mm/numa_64.c b/arch/x86/mm/numa_64.c
> index 7757d22..541746f 100644
> --- a/arch/x86/mm/numa_64.c
> +++ b/arch/x86/mm/numa_64.c
> @@ -390,14 +390,12 @@ static void __init numa_nodemask_from_meminfo(nodemask_t *nodemask,
>   */
>  void __init numa_reset_distance(void)
>  {
> -	size_t size;
> +	size_t size = numa_distance_cnt * numa_distance_cnt * sizeof(numa_distance[0]);
>  
> -	if (numa_distance_cnt) {
> -		size = numa_distance_cnt * sizeof(numa_distance[0]);
> +	if (numa_distance_cnt)
>  		memblock_x86_free_range(__pa(numa_distance),
>  					__pa(numa_distance) + size);
> -		numa_distance_cnt = 0;
> -	}
> +	numa_distance_cnt = 0;
>  	numa_distance = NULL;
>  }

my original part:

@@ -393,7 +393,7 @@ void __init numa_reset_distance(void)
        size_t size;
 
        if (numa_distance_cnt) {
-               size = numa_distance_cnt * sizeof(numa_distance[0]);
+               size = numa_distance_cnt * numa_distance_cnt * sizeof(numa_distance[0]);
                memblock_x86_free_range(__pa(numa_distance),
                                        __pa(numa_distance) + size);
                numa_distance_cnt = 0;

So can you tell me why you need to make those change?
	move out assigning or numa_distance_cnt and size of the the IF

>  
> diff --git a/arch/x86/mm/numa_emulation.c b/arch/x86/mm/numa_emulation.c
> index 607a2e8..0afa25d 100644
> --- a/arch/x86/mm/numa_emulation.c
> +++ b/arch/x86/mm/numa_emulation.c
> @@ -300,6 +300,7 @@ void __init numa_emulation(struct numa_meminfo *numa_meminfo, int numa_dist_cnt)
>  	static struct numa_meminfo pi __initdata;
>  	const u64 max_addr = max_pfn << PAGE_SHIFT;
>  	u8 *phys_dist = NULL;
> +	size_t phys_size = numa_dist_cnt * numa_dist_cnt * sizeof(phys_dist[0]);
>  	int i, j, ret;
>  
>  	if (!emu_cmdline)
> @@ -336,21 +337,18 @@ void __init numa_emulation(struct numa_meminfo *numa_meminfo, int numa_dist_cnt)
>  		goto no_emu;
>  	}
>  
> -	/*
> -	 * Copy the original distance table.  It's temporary so no need to
> -	 * reserve it.
> -	 */
> +	/* copy the physical distance table */
>  	if (numa_dist_cnt) {
> -		size_t size = numa_dist_cnt * sizeof(phys_dist[0]);
>  		u64 phys;
>  
>  		phys = memblock_find_in_range(0,
>  					      (u64)max_pfn_mapped << PAGE_SHIFT,
> -					      size, PAGE_SIZE);
> +					      phys_size, PAGE_SIZE);
>  		if (phys == MEMBLOCK_ERROR) {
>  			pr_warning("NUMA: Warning: can't allocate copy of distance table, disabling emulation\n");
>  			goto no_emu;
>  		}
> +		memblock_x86_reserve_range(phys, phys + phys_size, "TMP NUMA DIST");
>  		phys_dist = __va(phys);
>  
>  		for (i = 0; i < numa_dist_cnt; i++)
> @@ -398,6 +396,10 @@ void __init numa_emulation(struct numa_meminfo *numa_meminfo, int numa_dist_cnt)
>  			numa_set_distance(i, j, dist);
>  		}
>  	}
> +
> +	/* free the copied physical distance table */
> +	if (phys_dist)
> +		memblock_x86_free_range(__pa(phys_dist), __pa(phys_dist) + phys_size);
>  	return;
>  
>  no_emu:

you missed 

@@ -383,21 +386,40 @@ void __init numa_emulation(struct numa_m
 
 	/* transform distance table */
 	numa_reset_distance();
-	for (i = 0; i < MAX_NUMNODES; i++) {
-		for (j = 0; j < MAX_NUMNODES; j++) {
-			int physi = emu_nid_to_phys[i];
-			int physj = emu_nid_to_phys[j];
-			int dist;
-
-			if (physi >= numa_dist_cnt || physj >= numa_dist_cnt)
-				dist = physi == physj ?
-					LOCAL_DISTANCE : REMOTE_DISTANCE;
-			else
+	/* allocate numa_distance at first, it will set new numa_dist_cnt */
+	new_nr = numa_alloc_distance();
+	if (new_nr < 0)
+		goto free_temp_phys;
+
+	/*
+	 * only set it when we have old phys_dist,
+	 * numa_alloc_distance already set default values
+	 */
+	if (phys_dist)
+		for (i = 0; i < new_nr; i++) {
+			for (j = 0; j < new_nr; j++) {
+				int physi = emu_nid_to_phys[i];
+				int physj = emu_nid_to_phys[j];
+				int dist;
+
+				/* really need this check ? */
+				if (physi >= numa_dist_cnt ||
+				    physj >= numa_dist_cnt)
+					continue;
+
 				dist = phys_dist[physi * numa_dist_cnt + physj];
 
-			numa_set_distance(i, j, dist);
+				numa_set_distance(i, j, dist);
+			}
 		}
-	}
+

the change include:
1. you only need to go over new_nr*new_nr instead huge MAX_NUMNODES * MAX_NUMNODES
2. you do NOT need to go over it if you don't have phys_dist assigned before.
   numa_alloc_distance already have that default set.
3. do need to check if phys_dist is assigned before referring phys_dist.

so please just use my original patch.

Thanks

Yinghai