Re: [discuss] 2.6.19-rc6: known regressions (v4)

[Mel Gorman]

On (23/11/06 11:09), Andrew Morton didst pronounce:
> On Thu, 23 Nov 2006 12:01:41 +0000
> mel@xxxxxxxxx (Mel Gorman) wrote:
> 
> > find_min_pfn_for_node() and find_min_pfn_with_active_regions() both depend
> > on a sorted early_node_map[]. However, sort_node_map() is being called after
> > fin_min_pfn_with_active_regions() in free_area_init_nodes(). In most cases,
> > this is ok, but on at least one x86_64, the SRAT table caused the E820 ranges
> > to be registered out of order. This gave the wrong values for the min PFN
> > range resulting in some pages not being initialised.
> > 
> > This patch sorts the early_node_map in find_min_pfn_for_node(). It has
> > been boot tested on x86, x86_64, ppc64 and ia64.
> > 
> > Signed-off-by: Mel Gorman <mel@xxxxxxxxx>
> > 
> > diff -rup linux-2.6.19-rc6-clean/mm/page_alloc.c linux-2.6.19-rc6-sort_in_find_min/mm/page_alloc.c
> > --- linux-2.6.19-rc6-clean/mm/page_alloc.c	2006-11-15 20:03:40.000000000 -0800
> > +++ linux-2.6.19-rc6-sort_in_find_min/mm/page_alloc.c	2006-11-23 02:23:57.000000000 -0800
> > @@ -2612,6 +2612,9 @@ unsigned long __init find_min_pfn_for_no
> >  {
> >  	int i;
> >  
> > +	/* Regions in the early_node_map can be in any order */
> > +	sort_node_map();
> > +
> >  	/* Assuming a sorted map, the first range found has the starting pfn */
> >  	for_each_active_range_index_in_nid(i, nid)
> >  		return early_node_map[i].start_pfn;
> > @@ -2680,9 +2683,6 @@ void __init free_area_init_nodes(unsigne
> >  			max(max_zone_pfn[i], arch_zone_lowest_possible_pfn[i]);
> >  	}
> >  
> > -	/* Regions in the early_node_map can be in any order */
> > -	sort_node_map();
> > -
> >  	/* Print out the zone ranges */
> >  	printk("Zone PFN ranges:\n");
> >  	for (i = 0; i < MAX_NR_ZONES; i++)
> 

yes, once per active node.
          
> Seems a bit ... ungainly?
>


It is, but this late in the cycle, I was going for the
obviously-correct-and-will-definitly-work solution.

It would be sufficient to call sort_node_map() in
find_min_pfn_with_active_regions() but I wasn't sure someone would call
find_min_pfn_for_node() at some future time causing another fun bug.

A slightly smarter, but not quite as obviously correct, patch is below if
you prefer it. It removes the assumption about early_node_map being sorted
for find_min_pfns and friends by always searching the whole map.  The map
is then only sorted once when it is required. Andre, I'd appreciate it if
you could give it a spin to be 100% sure it's ok. It passed a boot-test on
a few machines here.

===========

find_min_pfn_for_node() and find_min_pfn_with_active_regions() both
depend on a sorted  early_node_map[] to find the correct values. However,
sort_node_map() is being called after fin_min_pfn_with_active_regions()
in free_area_init_nodes(). In most cases, this is ok, but on an x86_64,
the SRAT table caused the E820 ranges to be registered out of order. This gave
the wrong values for the min PFN range resulting in some pages not being
initialised.

This patch works by always searching the whole early_node_map[] in
find_min_pfn_for_node().

Signed-off-by: Mel Gorman <mel@xxxxxxxxx>

diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.19-rc5-mm2-clean/mm/page_alloc.c linux-2.6.19-rc5-mm2-sort_in_find_min/mm/page_alloc.c
--- linux-2.6.19-rc5-mm2-clean/mm/page_alloc.c	2006-11-14 14:01:37.000000000 +0000
+++ linux-2.6.19-rc5-mm2-sort_in_find_min/mm/page_alloc.c	2006-11-23 20:37:18.000000000 +0000
@@ -2945,17 +2945,22 @@ static void __init sort_node_map(void)
 			cmp_node_active_region, NULL);
 }
 
-/* Find the lowest pfn for a node. This depends on a sorted early_node_map */
+/* Find the lowest pfn for a node */
 unsigned long __init find_min_pfn_for_node(unsigned long nid)
 {
 	int i;
+	unsigned long min_pfn = -1UL;
 
 	/* Assuming a sorted map, the first range found has the starting pfn */
 	for_each_active_range_index_in_nid(i, nid)
-		return early_node_map[i].start_pfn;
+		min_pfn = min(min_pfn, early_node_map[i].start_pfn);
 
-	printk(KERN_WARNING "Could not find start_pfn for node %lu\n", nid);
-	return 0;
+	if (min_pfn == -1UL) {
+		printk(KERN_WARNING "Could not find start_pfn for node %lu\n", nid);
+		return 0;
+	}
+	
+	return min_pfn;
 }
 
 /**
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/