Re: 2.6.17-mm1

[Mel Gorman <mel@csn.ul.ie>]

On Fri, 23 Jun 2006, Franck Bui-Huu wrote:

> Mel Gorman wrote:
> > On (23/06/06 17:06), Franck Bui-Huu didst pronounce:
> > > what do you think about this use of ARCH_PFN_OFFSET ?
> >
> > I believe that arches calling free_area_init_node() directly, but using
> > CONFIG_FLAT_NODE_MEM_MAP will have problems with this. There is no
> > guarantee that pgdat->node_start_pfn will have any relation to
> > ARCH_PFN_OFFSET.
>
> I agree we shouldn't make this assumption.
>
> what about this ?
>
> -- >8 --
> [PATCH] flatmem: tiny optimisation when memory do not start at 0
>
> The FLATMEM memory model assumes that memory is in one
> contigious area based at pfn 0. Some archictectures, like
> ARM, use ARCH_PFN_OFFSET to relax this requirement but it
> involves one more operation during page/pfn conversion.
>
> Instead this patch modifies the address of mem_map to make
> mem_map[ARCH_PFN_OFFSET] pointing at NODE_DATA(0)->node_mem_map.
> The result is a slight gain on kernel code size.
>
> It also relaxes this requirement in two more places:
> 	free_area_init(...)
> 	init_bootmem(...)
>
> ---
> include/asm-generic/memory_model.h |    6 +++---
> mm/bootmem.c                       |    4 ++--
> mm/page_alloc.c                    |   18 ++++++++++++------
> 3 files changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/include/asm-generic/memory_model.h b/include/asm-generic/memory_model.h
> index 0cfb086..2faa12f 100644
> --- a/include/asm-generic/memory_model.h
> +++ b/include/asm-generic/memory_model.h
> @@ -34,9 +34,9 @@ #else
>  */
> #if defined(CONFIG_FLATMEM)
>
> -#define pfn_to_page(pfn)	(mem_map + ((pfn) - ARCH_PFN_OFFSET))
> -#define page_to_pfn(page)	((unsigned long)((page) - mem_map) + \
> -				 ARCH_PFN_OFFSET)
> +#define pfn_to_page(pfn)	(mem_map + (pfn))
> +#define page_to_pfn(page)	((unsigned long)((page) - mem_map))
> +
> #elif defined(CONFIG_DISCONTIGMEM)
>
> #define pfn_to_page(pfn)			\
> diff --git a/mm/bootmem.c b/mm/bootmem.c
> index d213fed..fd28eed 100644
> --- a/mm/bootmem.c
> +++ b/mm/bootmem.c
> @@ -377,11 +377,11 @@ unsigned long __init free_all_bootmem_no
> 	return(free_all_bootmem_core(pgdat));
> }
>
> -unsigned long __init init_bootmem (unsigned long start, unsigned long pages)
> +unsigned long __init init_bootmem(unsigned long start, unsigned long pages)
> {
> 	max_low_pfn = pages;
> 	min_low_pfn = start;
> -	return(init_bootmem_core(NODE_DATA(0), start, 0, pages));
> +	return init_bootmem_core(NODE_DATA(0), start, ARCH_PFN_OFFSET, pages);
> }

Not all arches will use init_bootmem(). Arm for example uses 
init_bootmem_node(). ARCH_PFN_OFFSET is only meant to affect mem_map, not 
any node attribute. I suspect (but don't know for a fact) that if we try 
and change node attributes with ARCH_PFN_OFFSET, we'll shoot ourselves in 
the foot.

> #ifndef CONFIG_HAVE_ARCH_BOOTMEM_NODE
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 253a450..3e97bc3 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2118,15 +2118,16 @@ static void __init free_area_init_core(s
>
> static void __init alloc_node_mem_map(struct pglist_data *pgdat)
> {
> +#ifdef CONFIG_FLAT_NODE_MEM_MAP
> +	struct page *map = pgdat->node_mem_map;
> +
> 	/* Skip empty nodes */
> 	if (!pgdat->node_spanned_pages)
> 		return;
>
> -#ifdef CONFIG_FLAT_NODE_MEM_MAP
> 	/* ia64 gets its own node_mem_map, before this, without bootmem */
> -	if (!pgdat->node_mem_map) {
> +	if (!map) {
> 		unsigned long size, start, end;
> -		struct page *map;
>
> 		/*
> 		 * The zone's endpoints aren't required to be MAX_ORDER
> @@ -2147,7 +2148,13 @@ #ifdef CONFIG_FLATMEM
> 	 * With no DISCONTIG, the global mem_map is just set as node 0's
> 	 */
> 	if (pgdat == NODE_DATA(0))
> -		mem_map = NODE_DATA(0)->node_mem_map;
> +		/*
> +		 * mem_map is assumed to be based at pfn 0 such that
> +		 * 'pfn = page* - mem_map' is true. Adjust map
> +		 * relative to node_mem_map to maintain this
> +		 * relationship.
> +		 */
> +		mem_map = map - ARCH_PFN_OFFSET;

This part is fine.

> #endif
> #endif /* CONFIG_FLAT_NODE_MEM_MAP */
> }
> @@ -2174,8 +2181,7 @@ #endif
>
> void __init free_area_init(unsigned long *zones_size)
> {
> -	free_area_init_node(0, NODE_DATA(0), zones_size,
> -			__pa(PAGE_OFFSET) >> PAGE_SHIFT, NULL);
> +	free_area_init_node(0, NODE_DATA(0), zones_size, ARCH_PFN_OFFSET, NULL);
> }

Same comment applies as for init_bootmem(). I can't be sure it's a good 
idea.

--
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab
-
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/