On Thursday 13 March 2008 02:22:40 Andrew Morton wrote:
quoted text
> On Wed, 12 Mar 2008 18:11:41 -0700 "Yinghai Lu" <yhlu.kernel@gmail.com> wrote:
> 
> > >  <looks at it>
> > >
> > >  Sorry, but I find the changelog very hard to amke sense of.  I presently
> > >  have:
> > >
> > >
> > >   So call it when numa is enabled, we don't know which node have that
> > >   range.  and make it more robust.
> > >
> > >   Try to trim it to get valid sidx, and eidx.
> > >
> > >  Could you please expand on this?
> > 
> > please check following...
> > 
> 
> Heaps better, thanks ;)  Below is what I now have.
> 
> (cc's people)
> 
> Guys, could you please review this?  Maybe test it a bit?
> 
> Thanks.
> 
> 
> From: "Yinghai Lu" <yhlu.kernel@gmail.com>
> 
> With numa enabled, some callers could have a range o fmemory on one node but
> try to free that on other node.  This can cause some pages to be freed
> wrongly.

Concrete examples?

If that happens it's really just a problem that the bootmem API
is wrong. I was always annoyed by the hardcoded NODE_DATA(0)s in
free_bootmem. 

I would suggest if that happens you just fix free_bootmem to search
for the correct node instead of hardcoding 0 and then eliminate
free_bootmem_node() everywhere and replace it with free_bootmem()
quoted text
> 
> For example: when we try to allocate 128g boot ram early for gart/swiotlb, and
> free that range later so gart/swiotlb can get some range afterwards.

I'm confused by the example. AFAIK there is no memory freeing in either
gart nor swiotlb. At least there wasn't until very recently.

quoted text
> 
> With this patch, we don't need to care which node holds the range, just loop
> to call free_bootmem_node for all online nodes.
> 
> This patch make free_bootmem_core() more robust by trimming the sidx and eidx
> according the ram range that the node has.

I think you should just kill free_bootmem_node() and replace it everywhere
with your improved free_bootmem()


quoted text
> @@ -125,6 +125,7 @@ static int __init reserve_bootmem_core(b
>  	BUG_ON(!size);
>  	BUG_ON(PFN_DOWN(addr) >= bdata->node_low_pfn);
>  	BUG_ON(PFN_UP(addr + size) > bdata->node_low_pfn);
> +	BUG_ON(addr < bdata->node_boot_start);

That seems unrelated?

quoted text
>  
>  	sidx = PFN_DOWN(addr - bdata->node_boot_start);
>  	eidx = PFN_UP(addr + size - bdata->node_boot_start);
> @@ -156,21 +157,31 @@ static void __init free_bootmem_core(boo
>  	unsigned long sidx, eidx;
>  	unsigned long i;
>  
> +	BUG_ON(!size);
> +
> +	/* out range */
> +	if (addr + size < bdata->node_boot_start ||
> +		PFN_DOWN(addr) > bdata->node_low_pfn)
> +		return;

I don't really like this silent return without error value.
There should be a BUG() or something for someone passing addresses
outside any node. This check should be probably in the caller.

quoted text
>  	/*
>  	 * round down end of usable mem, partially free pages are
>  	 * considered reserved.
>  	 */
> -	BUG_ON(!size);
> -	BUG_ON(PFN_DOWN(addr + size) > bdata->node_low_pfn);
>  
> -	if (addr < bdata->last_success)
> +	if (addr >= bdata->node_boot_start && addr < bdata->last_success)
>  		bdata->last_success = addr;
>  
>  	/*
> -	 * Round up the beginning of the address.
> +	 * Round up to index to the range.
>  	 */
> -	sidx = PFN_UP(addr) - PFN_DOWN(bdata->node_boot_start);
> +	if (PFN_UP(addr) > PFN_DOWN(bdata->node_boot_start))
> +		sidx = PFN_UP(addr) - PFN_DOWN(bdata->node_boot_start);
> +	else
> +		sidx = 0;
> +
>  	eidx = PFN_DOWN(addr + size - bdata->node_boot_start);
> +	if (eidx > bdata->node_low_pfn - PFN_DOWN(bdata->node_boot_start))
> +		eidx = bdata->node_low_pfn - PFN_DOWN(bdata->node_boot_start);

I'm not sure for what these other changes are needed?  Just adding the
initial range check should be enough.

If you want to fix something else unrelated please do separate patches.

-Andi

--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/