Re: [PATCH 4/6] Have x86_64 use add_active_range() and free_area_init_nodes

[mel@skynet.ie (Mel Gorman)]

On (30/08/06 13:57), Keith Mannthey didst pronounce:
> On 8/21/06, Mel Gorman <mel@csn.ul.ie> wrote:
> >
> >Size zones and holes in an architecture independent manner for x86_64.
> 
> 
> Hey Mel,

Hi Keith.

>  I am having some trouble with the srat.c changes.  I keep running into
> "SRAT: Hotplug area has existing memory" so am am taking more throught
> look at this patch.
>  I am working on 2.6.18-rc4-mm3 x86_64.
> 

ok, great. How much physical memory is installed on the machine? I want to
determine if the "usable" entries in the e820 map contain physical memory
or not.

>   srat.c is doing some sanity checking against the e820 and hot-add
> memory ranges.  BIOS folk aren't to be trusted with the SRAT.  Calling
> remove_all_active_ranges before acpi_numa_init leaves nothing to fall
> back onto if the SRAT is bad.  (see bad_srat()). What should happen
> when we discard the srat info?
> 

When the SRAT is bad, the information is discarded and discovered by an
alternative method later in the boot process.

In this case, numa_initmem_init() is called after acpi_numa_init(). It
calls acpi_scan_nodes() which returns -1 because the SRAT is bad. Once
that happens, either k8_scan_nodes() will be called and the regions
discovered there or if that is not possible, it'll fall through and
e820_register_active_regions will be called without any node awareness.

>  i386 code may have similar fallback logic (haven't been there in a while)
> 

There is fallback logic in the i386 code as well.

> also
> 
> >diff -rup -X /usr/src/patchset-0.6/bin//dontdiff 
> >linux-2.6.18-rc4-mm2-103-x86_use_init_nodes/arch/x86_64/mm/srat.c 
> >linux-2.6.18-rc4-mm2-104-x86_64_use_init_nodes/arch/x86_64/mm/srat.c
> >--- linux-2.6.18-rc4-mm2-103-x86_use_init_nodes/arch/x86_64/mm/srat.c   
> >2006-08-21 09:23:50.000000000 +0100
> >+++ linux-2.6.18-rc4-mm2-104-x86_64_use_init_nodes/arch/x86_64/mm/srat.c   
> >2006-08-21 10:15:58.000000000 +0100
> >@@ -84,6 +84,7 @@ static __init void bad_srat(void)
> >                apicid_to_node[i] = NUMA_NO_NODE;
> >        for (i = 0; i < MAX_NUMNODES; i++)
> >                nodes_add[i].start = nodes[i].end = 0;
> >+       remove_all_active_ranges();
> > }
> 
> We go back to setup_arch with no active areas?
> 

Yes, and it'll be discovered using an alternative method later. There is
no point returning to setup_arch with known bad information about active
areas.

> > static __init inline int srat_disabled(void)
> >@@ -166,7 +167,7 @@ static int hotadd_enough_memory(struct b
> >
> >        if (mem < 0)
> >                return 0;
> >-       allowed = (end_pfn - e820_hole_size(0, end_pfn)) * PAGE_SIZE;
> >+       allowed = (end_pfn - absent_pages_in_range(0, end_pfn)) * 
> >PAGE_SIZE;
> >        allowed = (allowed / 100) * hotadd_percent;
> >        if (allocated + mem > allowed) {
> >                unsigned long range;
> >@@ -238,7 +239,7 @@ static int reserve_hotadd(int node, unsi
> >        }
> >
> >        /* This check might be a bit too strict, but I'm keeping it for 
> >        now. */
> >-       if (e820_hole_size(s_pfn, e_pfn) != e_pfn - s_pfn) {
> >+       if (absent_pages_in_range(s_pfn, e_pfn) != e_pfn - s_pfn) {
> >                printk(KERN_ERR "SRAT: Hotplug area has existing 
> >                memory\n");
> >                return -1;
> >        }
> We really do want to to compare against the e820 map at it contains
> the memory that is really present (this info was blown away before
> acpi_numa) 

The information used by absent_pages_in_range() should match what was
available to e820_hole_size().

> Anyway I fixed up to have the current chunk added
> (e820_register_active_regions) after calling this code so it logicaly
> makes sense but it still trip over the check.  
> I am not sure what you
> are printing out in you debug code but dosen't look like pfns or
> phys_addresses but maybe it can tell us why the check fails.
> 

My debug code for add_active_range() printing out pfns but I spotted one
case where absent_pages_in_range(I) does not do what one would expect.
Lets say the ranges with physical memory was 0->1000 and 2000-3000 (in
pfns).  absent_pages_in_range(0, 3000) would return 1000 as you'd expect but
absent_pages_in_range(5000-6000) would return 0! I have a patch that might
fix this at the end of the mail but I'm not sure it's the problem you are
hitting. In the bootlog, I see;

SRAT: Node 0 PXM 0 0-80000000
Entering add_active_range(0, 0, 152) 0 entries of 3200 used
Entering add_active_range(0, 256, 524165) 1 entries of 3200 used
SRAT: Node 0 PXM 0 0-470000000
Entering add_active_range(0, 0, 152) 2 entries of 3200 used
Entering add_active_range(0, 256, 524165) 2 entries of 3200 used
Entering add_active_range(0, 1048576, 4653056) 2 entries of 3200 used
SRAT: Node 0 PXM 0 0-1070000000
SRAT: Hotplug area has existing memory

The last part (0-1070000000) is checked as a hotplug area but it's clear
that memory exists in that range.  As reserve_hotadd() requires that the
whole range be a hole, I'm having trouble seeing how it ever successfully
reserved unless the ranges going into reserve_hotadd() are something other
than the pfn range for 0-1070000000). The patch later will print out the
range used by reserve_hotadd() so we can see.

> >@@ -329,6 +330,8 @@ acpi_numa_memory_affinity_init(struct ac
> >
> >        printk(KERN_INFO "SRAT: Node %u PXM %u %Lx-%Lx\n", node, pxm,
> >               nd->start, nd->end);
> >+       e820_register_active_regions(node, nd->start >> PAGE_SHIFT,
> >+                                               nd->end >> PAGE_SHIFT);
> 
> A node chunk in this section of code may be a hot-pluggable zone. With
> MEMORY_HOTPLUG_SPARSE we don't want to register these regions.
> 

The ranges should not get registered as active memory by
e820_register_active_regions() unless they are marked E820_RAM. My
understanding is that the regions for hotadd would be marked "reserved"
in the e820 map. Is that wrong?

> >        if (ma->flags.hot_pluggable && !reserve_hotadd(node, start, end) < 
> >        0) {
> >                /* Ignore hotadd region. Undo damage */
> 
>  I have but the e820_register_active_regions as a else to this
> statment the absent pages check fails.
> 

The patch below omits this change because I think
e820_register_active_regions() will still have got called by the time
you encounter a hotplug area.

> Also nodes_cover_memory and alot of these check were based against
> comparing the srat data against the e820.  Now all this code is
> comparing SRAT against SRAT....
> 

I don't see why. The SRAT table passes a range to
e820_register_active_regions() so should be comparing SRAT to e820

> I am willing to help here but we should compare the SRAT against to
> e820. Table v. Table.
> 
> What to you think should be done?
> 

Can you read through this patch and see does it address the problem in any
way? If it doesn't, can you send a complete bootlog so I can see what is
being sent to reserve_hotadd()? Thanks

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.18-rc4-mm3-clean/arch/x86_64/mm/srat.c linux-2.6.18-rc4-mm3-fix_x8664_hotadd/arch/x86_64/mm/srat.c
--- linux-2.6.18-rc4-mm3-clean/arch/x86_64/mm/srat.c	2006-08-29 16:25:10.000000000 +0100
+++ linux-2.6.18-rc4-mm3-fix_x8664_hotadd/arch/x86_64/mm/srat.c	2006-08-31 16:17:26.000000000 +0100
@@ -240,7 +240,8 @@ static int reserve_hotadd(int node, unsi
 
 	/* This check might be a bit too strict, but I'm keeping it for now. */
 	if (absent_pages_in_range(s_pfn, e_pfn) != e_pfn - s_pfn) {
-		printk(KERN_ERR "SRAT: Hotplug area has existing memory\n");
+		printk(KERN_ERR "SRAT: Hotplug area %lu -> %lu has existing memory\n",
+				s_pfn, e_pfn);
 		return -1;
 	}
 
diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.18-rc4-mm3-clean/mm/page_alloc.c linux-2.6.18-rc4-mm3-fix_x8664_hotadd/mm/page_alloc.c
--- linux-2.6.18-rc4-mm3-clean/mm/page_alloc.c	2006-08-29 16:25:31.000000000 +0100
+++ linux-2.6.18-rc4-mm3-fix_x8664_hotadd/mm/page_alloc.c	2006-08-31 14:52:38.000000000 +0100
@@ -2280,6 +2280,10 @@ unsigned long __init __absent_pages_in_r
 		prev_end_pfn = early_node_map[i].end_pfn;
 	}
 
+	/* If the range is outside of physical memory, return the range */
+	if (range_start_pfn > prev_end_pfn)
+		hole_pages = range_end_pfn - range_start_pfn;
+
 	return hole_pages;
 }