Re: problem with numa reserve bootmem

[Michael Ellerman <michael@ellerman.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3148 bytes --]

On Tue, 2009-02-10 at 19:17 -0800, Geoff Levand wrote:
> Hi Jon,
> 
> Jon Tollefson wrote:
> > This patch takes out the reserved region loop from inside
> > the loop that goes over each node.  It looks up the active region containing
> > the start of the reserved region.  If it extends past that active region then
> > it adjusts the size and gets the next active region containing it.
> > 
> >  numa.c |  108 ++++++++++++++++++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 80 insertions(+), 28 deletions(-)
> 
> I had some problems with this numa change (commit 8f64e1f2d1e09267ac926e15090fd505c1c0cbcb)
> missing an lmb reserved region.
> 
> There have been some changes to this code since this patch was committed,
> but the general problem still exists.
> 
> With the PS3 platform, the boot wrapper program puts the device tree
> above the boot wrapper's _end symbol.  So with this there is a small
> reserved bootmem section for the DT of about 0x270 bytes
> (reserved.region[0x1]):
> 
> lmb_dump_all:
>     memory.cnt            = 0x1
>     memory.size           = 0x8000000
>     memory.region[0x0].base       = 0x0
>                       .size     = 0x8000000
>     reserved.cnt          = 0x2
>     reserved.size         = 0x8000000
>     reserved.region[0x0].base       = 0x0
>                       .size     = 0xcc8000
>     reserved.region[0x1].base       = 0xce0300
>                       .size     = 0x270
> 
> > +	/* Mark reserved regions */
> > +	for (i = 0; i < lmb.reserved.cnt; i++) {
> > +		unsigned long physbase = lmb.reserved.region[i].base;
> > +		unsigned long size = lmb.reserved.region[i].size;
> > +		unsigned long start_pfn = physbase >> PAGE_SHIFT;
> > +		unsigned long end_pfn = ((physbase + size) >> PAGE_SHIFT);
> 
> With reserved.region[0x1] start_pfn and end_pfn are equal (0xce0) here.
> 
> > +		struct node_active_region node_ar;
> > +
> > +		get_node_active_region(start_pfn, &node_ar);
> > +		while (start_pfn < end_pfn) {
> 
> And this while (start_pfn < end_pfn) test fails,
> 
> > +			/*
> > +			 * if reserved region extends past active region
> > +			 * then trim size to active region
> > +			 */
> > +			if (end_pfn > node_ar.end_pfn)
> > +				size = (node_ar.end_pfn << PAGE_SHIFT)
> > +					- (start_pfn << PAGE_SHIFT);
> > +			dbg("reserve_bootmem %lx %lx nid=%d\n", physbase, size,
> > +				node_ar.nid);
> > +			reserve_bootmem_node(NODE_DATA(node_ar.nid), physbase,
> > +						size, BOOTMEM_DEFAULT);
> 
> And so this reserve_bootmem_node() is never called for the small region.
> 
> I'm not sure if the problem is the calculation of the end_pfn, or if we
> need to test for equality in the while: (start_pfn <= end_pfn).  Please
> let me know what you think.  I'll look at it some more tomorrow.

Dave, you had a patch for this I think?

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]