[PATCH 1/1] Only use ARCH_PFN_OFFSET once during boot

[PATCH 1/1] Only use ARCH_PFN_OFFSET once during boot

[Mel Gorman]

The FLATMEM memory model assumes that memory is one contiguous region based
at PFN 0 and uses the NODE_DATA(0)->node_mem_map as the global mem_map. As
NODE_DATA(0)->node_mem_map may not be PFN 0, architectures optionally define
ARCH_PFN_OFFSET as the offset between NODE_DATA(0)->node_mem_map and PFN 0.
This offset is used every time page_to_pfn() or pfn_to_page() is called
resulting in a small amount of code bloat and overhead.

This patch changes ARCH_PFN_OFFSET so that it is used only once during
memory initialisation when working out where mem_map is. This gives
a very small reduction in zImage size for architectures using
ARCH_PFN_OFFSET. The patch also adds a small amount of documentation
on what ARCH_PFN_OFFSET is for.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>

 include/asm-generic/memory_model.h |    5 ++---
 mm/page_alloc.c                    |    8 ++++++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.17-mm6-clean/include/asm-generic/memory_model.h linux-2.6.17-mm6-archpfnoffset_optimise/include/asm-generic/memory_model.h
--- linux-2.6.17-mm6-clean/include/asm-generic/memory_model.h	2006-07-05 14:31:17.000000000 +0100
+++ linux-2.6.17-mm6-archpfnoffset_optimise/include/asm-generic/memory_model.h	2006-07-05 14:36:04.000000000 +0100
@@ -28,9 +28,8 @@
  */
 #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 -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.17-mm6-clean/mm/page_alloc.c linux-2.6.17-mm6-archpfnoffset_optimise/mm/page_alloc.c
--- linux-2.6.17-mm6-clean/mm/page_alloc.c	2006-07-05 14:31:18.000000000 +0100
+++ linux-2.6.17-mm6-archpfnoffset_optimise/mm/page_alloc.c	2006-07-05 17:01:01.000000000 +0100
@@ -2157,10 +2157,14 @@ static void __init alloc_node_mem_map(st
 	}
 #ifdef CONFIG_FLATMEM
 	/*
-	 * With no DISCONTIG, the global mem_map is just set as node 0's
+	 * With FLATMEM, the global mem_map is just set as node 0's. The
+	 * FLATMEM memory model assumes that memory is in one contiguous area
+	 * starting at PFN 0. Architectures that do not start NODE 0 at PFN 0
+	 * must define ARCH_PFN_OFFSET as the offset between
+	 * NODE_DATA(0)->node_mem_map and PFN 0.
 	 */
 	if (pgdat == NODE_DATA(0))
-		mem_map = NODE_DATA(0)->node_mem_map;
+		mem_map = NODE_DATA(0)->node_mem_map - ARCH_PFN_OFFSET;
 #endif
 #endif /* CONFIG_FLAT_NODE_MEM_MAP */
 }
 

Re: [PATCH 1/1] Only use ARCH_PFN_OFFSET once during boot

[Franck Bui-Huu]

Hi Mel !

Mel Gorman wrote:
> The FLATMEM memory model assumes that memory is one contiguous region based
> at PFN 0 and uses the NODE_DATA(0)->node_mem_map as the global mem_map. As

[snip]

> 
> diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.17-mm6-clean/include/asm-generic/memory_model.h linux-2.6.17-mm6-archpfnoffset_optimise/include/asm-generic/memory_model.h
> --- linux-2.6.17-mm6-clean/include/asm-generic/memory_model.h	2006-07-05 14:31:17.000000000 +0100
> +++ linux-2.6.17-mm6-archpfnoffset_optimise/include/asm-generic/memory_model.h	2006-07-05 14:36:04.000000000 +0100
> @@ -28,9 +28,8 @@
>   */
>  #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)
>  

ok for that part.

>  #define __pfn_to_page(pfn)			\
> diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.17-mm6-clean/mm/page_alloc.c linux-2.6.17-mm6-archpfnoffset_optimise/mm/page_alloc.c
> --- linux-2.6.17-mm6-clean/mm/page_alloc.c	2006-07-05 14:31:18.000000000 +0100
> +++ linux-2.6.17-mm6-archpfnoffset_optimise/mm/page_alloc.c	2006-07-05 17:01:01.000000000 +0100
> @@ -2157,10 +2157,14 @@ static void __init alloc_node_mem_map(st
>  	}
>  #ifdef CONFIG_FLATMEM
>  	/*
> -	 * With no DISCONTIG, the global mem_map is just set as node 0's
> +	 * With FLATMEM, the global mem_map is just set as node 0's. The
> +	 * FLATMEM memory model assumes that memory is in one contiguous area
> +	 * starting at PFN 0. Architectures that do not start NODE 0 at PFN 0
> +	 * must define ARCH_PFN_OFFSET as the offset between
> +	 * NODE_DATA(0)->node_mem_map and PFN 0.
>  	 */
>  	if (pgdat == NODE_DATA(0))
> -		mem_map = NODE_DATA(0)->node_mem_map;
> +		mem_map = NODE_DATA(0)->node_mem_map - ARCH_PFN_OFFSET;

is mem_map always aligned on MAX_ORDER ?

>  #endif
>  #endif /* CONFIG_FLAT_NODE_MEM_MAP */
>  }
>  

I'm not sure of that part. We basically make incoherent the use of
free_area_init_node()'s fourth parameter by doing this change (for
FLATMEM model of course).

When using free_area_init() which is defined as follow:

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);
}

we will end up to have 2 definitions for the mem start:

	- __pa(PAGE_OFFSET) >> PAGE_SHIFT
	- ARCH_PFN_OFFSET

The former will be used to calculate the size of mem_map and the
latter will be used to calculate the offset between
NODE_DATA(0)->node_mem_map and PFN 0. I don't think that will result
in any problem since:

	ARCH_PFN_OFFSET == __pa(PAGE_OFFSET) >> PAGE_SHIFT

That just makes the code is harder to follow.

But if some platforms were doing something like (which is unlikely)

	[...]
	free_area_init_node(0, NODE_DATA(0), zone_size, FOO_PFN_OFFSET, NULL);
	[...]

and ARCH_PFN_OFFSET != FOO_PFN_OFFSET then they may have some troubles.

What about this patch for page_alloc.c ? I think it makes more obvious
what is ARCH_PFN_OFFSET. And if someone doesn't want to use
ARCH_PFN_OFFSET, he still can use:

	free_area_init_node(0, NODE_DATA(0), zone_size, FOO_PFN_OFFSET, NULL);

		Franck

-- >8 --

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 253a450..9daee06 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2147,7 +2147,7 @@ #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 = NODE_DATA(0)->node_mem_map - pgdat->node_start_pfn;
 #endif
 #endif /* CONFIG_FLAT_NODE_MEM_MAP */
 }
@@ -2172,10 +2172,13 @@ struct pglist_data contig_page_data = { 
 EXPORT_SYMBOL(contig_page_data);
 #endif
 
+/*
+ * This function is used only by FLATMEM. In that case the 
+ * start of physical mem is always given by ARCH_PFN_OFFSET.
+ */
 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);
 }
 
 #ifdef CONFIG_PROC_FS

Re: [PATCH 1/1] Only use ARCH_PFN_OFFSET once during boot

[Mel Gorman]

On Thu, 6 Jul 2006, Franck Bui-Huu wrote:

> Hi Mel !
>

Hi.

> Mel Gorman wrote:
>> The FLATMEM memory model assumes that memory is one contiguous region based
>> at PFN 0 and uses the NODE_DATA(0)->node_mem_map as the global mem_map. As
>
> [snip]
>
>>
>> diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.17-mm6-clean/include/asm-generic/memory_model.h linux-2.6.17-mm6-archpfnoffset_optimise/include/asm-generic/memory_model.h
>> --- linux-2.6.17-mm6-clean/include/asm-generic/memory_model.h	2006-07-05 14:31:17.000000000 +0100
>> +++ linux-2.6.17-mm6-archpfnoffset_optimise/include/asm-generic/memory_model.h	2006-07-05 14:36:04.000000000 +0100
>> @@ -28,9 +28,8 @@
>>   */
>>  #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)
>>
>
> ok for that part.
>

Grand.

>>  #define __pfn_to_page(pfn)			\
>> diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.17-mm6-clean/mm/page_alloc.c linux-2.6.17-mm6-archpfnoffset_optimise/mm/page_alloc.c
>> --- linux-2.6.17-mm6-clean/mm/page_alloc.c	2006-07-05 14:31:18.000000000 +0100
>> +++ linux-2.6.17-mm6-archpfnoffset_optimise/mm/page_alloc.c	2006-07-05 17:01:01.000000000 +0100
>> @@ -2157,10 +2157,14 @@ static void __init alloc_node_mem_map(st
>>  	}
>>  #ifdef CONFIG_FLATMEM
>>  	/*
>> -	 * With no DISCONTIG, the global mem_map is just set as node 0's
>> +	 * With FLATMEM, the global mem_map is just set as node 0's. The
>> +	 * FLATMEM memory model assumes that memory is in one contiguous area
>> +	 * starting at PFN 0. Architectures that do not start NODE 0 at PFN 0
>> +	 * must define ARCH_PFN_OFFSET as the offset between
>> +	 * NODE_DATA(0)->node_mem_map and PFN 0.
>>  	 */
>>  	if (pgdat == NODE_DATA(0))
>> -		mem_map = NODE_DATA(0)->node_mem_map;
>> +		mem_map = NODE_DATA(0)->node_mem_map - ARCH_PFN_OFFSET;
>
> is mem_map always aligned on MAX_ORDER ?
>

The start of mem_map is at PFN 0 so it's always aligned. node_mem_map is 
MAX_ORDER_NR_PAGES aligned in alloc_node_mem_map().

Bear in mind that the only difference between the patched and unpatched 
kernel is that the unpatched kernel offsets NODE_DATA(0)->node_mem_map by 
NODE_DATA(0)->node_mem_map at every call to __pfn_to_page() and 
__page_to_pfn() and the patched kernel offsets just once. I'm not looking 
to alter anything fundamental here.

>>  #endif
>>  #endif /* CONFIG_FLAT_NODE_MEM_MAP */
>>  }
>>
>
> I'm not sure of that part. We basically make incoherent the use of
> free_area_init_node()'s fourth parameter by doing this change (for
> FLATMEM model of course).
>
> When using free_area_init() which is defined as follow:
>
> 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);
> }
>
> we will end up to have 2 definitions for the mem start:
>
> 	- __pa(PAGE_OFFSET) >> PAGE_SHIFT
> 	- ARCH_PFN_OFFSET
>
> The former will be used to calculate the size of mem_map

The size of the node_mem_map is determined by zones_size. mem_map does not 
have a size as such because it's not necessarily a fully populated array. 
The location of mem_map is determined by the location of 
NODE_DATA(0)->node_mem_map and ARCH_PFN_OFFSET. Neither the former nor the 
latter affect the size of mem_map in other words.

> and the
> latter will be used to calculate the offset between
> NODE_DATA(0)->node_mem_map and PFN 0. I don't think that will result
> in any problem since:
>
> 	ARCH_PFN_OFFSET == __pa(PAGE_OFFSET) >> PAGE_SHIFT
>
> That just makes the code is harder to follow.
>
> But if some platforms were doing something like (which is unlikely)
>
> 	[...]
> 	free_area_init_node(0, NODE_DATA(0), zone_size, FOO_PFN_OFFSET, NULL);
> 	[...]
>
> and ARCH_PFN_OFFSET != FOO_PFN_OFFSET then they may have some troubles.
>

I'm not aware of an architecture that would run into such a problem but 
that is not saying a lot.

> What about this patch for page_alloc.c ? I think it makes more obvious
> what is ARCH_PFN_OFFSET. And if someone doesn't want to use
> ARCH_PFN_OFFSET, he still can use:
>
> 	free_area_init_node(0, NODE_DATA(0), zone_size, FOO_PFN_OFFSET, NULL);
>


I think my patch does the job of moving ARCH_PFN_OFFSET out of the hot 
path in a less risky fashion. However, if you are sure that callers to 
free_area_init() and ARCH_PFN_OFFSET are ok after your patch, I'd be happy 
to go with it. If you're not sure, I reckon my patch would be the way to 
go.

> 		Franck
>
> -- >8 --
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 253a450..9daee06 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2147,7 +2147,7 @@ #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 = NODE_DATA(0)->node_mem_map - pgdat->node_start_pfn;
> #endif
> #endif /* CONFIG_FLAT_NODE_MEM_MAP */
> }
> @@ -2172,10 +2172,13 @@ struct pglist_data contig_page_data = {
> EXPORT_SYMBOL(contig_page_data);
> #endif
>
> +/*
> + * This function is used only by FLATMEM. In that case the
> + * start of physical mem is always given by ARCH_PFN_OFFSET.
> + */
> 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);
> }
>
> #ifdef CONFIG_PROC_FS
>

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: [PATCH 1/1] Only use ARCH_PFN_OFFSET once during boot

[Franck Bui-Huu]

2006/7/6, Mel Gorman <mel@csn.ul.ie>:
>
> I think my patch does the job of moving ARCH_PFN_OFFSET out of the hot
> path in a less risky fashion. However, if you are sure that callers to
> free_area_init() and ARCH_PFN_OFFSET are ok after your patch, I'd be happy
> to go with it. If you're not sure, I reckon my patch would be the way to
> go.
>

Ok I try to explain better what I have in mind. Your patch changes the
behaviour of free_area_init_node() in the sense that it doesn't work
as expected if its fourth parameter is different from ARCH_PFN_OFFSET,
it even becomes boggus IMHO. And I think it's valid to use it when
FLATMEM model is selected.

I don't know if there is a platform which uses FLATMEM model and do
not setup ARCH_PFN_OFFSET when its memory do not start at 0. But I
don't think we should assume that if FLATMEM model is selected then
all uses of free_area_init_node() imply ARCH_PFN_OFFSET whatever the
value of the fourth parameter. I would say it's a risky implementation
of free_area_init_node() and prone boggus uses of this function.

One example comes in mind, though I don't know if it's possible. Let's
say a platform can't determine where exactly its memory start. It has
to determine this start at boot time, the BIOS may pass it for
example. So in that case you can't use ARCH_PFN_OFFSET and you have to
use free_area_init_node() with a variable as fourth parameter.

-- 
               Franck

Re: [PATCH 1/1] Only use ARCH_PFN_OFFSET once during boot

[Mel Gorman]

On Thu, 6 Jul 2006, Franck Bui-Huu wrote:

> 2006/7/6, Mel Gorman <mel@csn.ul.ie>:
>> 
>> I think my patch does the job of moving ARCH_PFN_OFFSET out of the hot
>> path in a less risky fashion. However, if you are sure that callers to
>> free_area_init() and ARCH_PFN_OFFSET are ok after your patch, I'd be happy
>> to go with it. If you're not sure, I reckon my patch would be the way to
>> go.
>> 
>
> Ok I try to explain better what I have in mind. Your patch changes the
> behaviour of free_area_init_node() in the sense that it doesn't work
> as expected if its fourth parameter is different from ARCH_PFN_OFFSET,
> it even becomes boggus IMHO. And I think it's valid to use it when
> FLATMEM model is selected.

I'm missing something silly here.

Before the patch, we have the following
  o Call free_area_initSOMETHING()
  o Set mem_map to NODE_DATA(0)->node_mem_map
  o At each call to page_to_pfn() or pfn_to_page(), offset mem_map by 
    ARCH_PFN_OFFSET

After the patch, we have

  o Call free_area_initSOMETHING()
  o Set mem_map to NODE_DATA(0)->node_mem_map - ARCH_PFN_OFFSET
  o At each call to page_to_pfn() or pfn_to_page(), use mem_map without 
    any additional offset

I don't see how free_area_init_node() changed except for callers 
using mem_map directly.

....

using mem_map directly. uh uh

Both of our patches are broken.

page_to_pfn() and pfn_to_page() both need ARCH_PFN_OFFSET to get PFNs,
that's fine. However, I forgot that another assumption of the FLATMEM memory
model is that mem_map[0] is the first valid struct page in the system. A
number of architectures walk mem_map[] directly (cris and frv are examples)
without offsetting based on this assumption.

This means that any patch that moves mem_map without altering every direct
user of the mem_map[] array will break further down the line. If nothing else,
this shows that ARCH_PFN_OFFSET could have done with a comment :) .

> <rest of mail snipped>


diff -rup -X /usr/src/patchset-0.6/bin//dontdiff linux-2.6.17-mm6-clean/include/asm-generic/memory_model.h linux-2.6.17-mm6-documentarchpfn/include/asm-generic/memory_model.h
--- linux-2.6.17-mm6-clean/include/asm-generic/memory_model.h	2006-07-05 14:31:17.000000000 +0100
+++ linux-2.6.17-mm6-documentarchpfn/include/asm-generic/memory_model.h	2006-07-06 20:48:46.000000000 +0100
@@ -6,6 +6,16 @@
 
 #if defined(CONFIG_FLATMEM)
 
+/*
+ * The FLATMEM memory model assumes that memory is one contiguous block of
+ * memory starting at PFN 0 with the first valid struct page at mem_map[0].
+ * mem_map is initialised to point to NODE_DATA(0)->node_mem_map.
+ *
+ * Architectures that do not start memory at PFN 0 are required to
+ * define ARCH_PFN_OFFSET so that __page_to_pfn(&mem_map[0]) == 0 and
+ * __pfn_to_page(0) == &mem_map[0]
+ *
+ */
 #ifndef ARCH_PFN_OFFSET
 #define ARCH_PFN_OFFSET		(0UL)
 #endif

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: [PATCH 1/1] Only use ARCH_PFN_OFFSET once during boot

[Franck Bui-Huu]

Mel Gorman wrote:
> On Thu, 6 Jul 2006, Franck Bui-Huu wrote:
> 
>> 2006/7/6, Mel Gorman <mel@csn.ul.ie>:
>>> I think my patch does the job of moving ARCH_PFN_OFFSET out of the hot
>>> path in a less risky fashion. However, if you are sure that callers to
>>> free_area_init() and ARCH_PFN_OFFSET are ok after your patch, I'd be happy
>>> to go with it. If you're not sure, I reckon my patch would be the way to
>>> go.
>>>
>> Ok I try to explain better what I have in mind. Your patch changes the
>> behaviour of free_area_init_node() in the sense that it doesn't work
>> as expected if its fourth parameter is different from ARCH_PFN_OFFSET,
>> it even becomes boggus IMHO. And I think it's valid to use it when
>> FLATMEM model is selected.
> 
> I'm missing something silly here.
> 
> Before the patch, we have the following
>   o Call free_area_initSOMETHING()
>   o Set mem_map to NODE_DATA(0)->node_mem_map
>   o At each call to page_to_pfn() or pfn_to_page(), offset mem_map by 
>     ARCH_PFN_OFFSET
> 
> After the patch, we have
> 
>   o Call free_area_initSOMETHING()
>   o Set mem_map to NODE_DATA(0)->node_mem_map - ARCH_PFN_OFFSET
>   o At each call to page_to_pfn() or pfn_to_page(), use mem_map without 
>     any additional offset
> 
> I don't see how free_area_init_node() changed except for callers 
> using mem_map directly.
> 

you're right the behaviour is the same with the old code and with your
patch that is:

If CONFIG_FLATMEM then free_area_init_node must be called:

free_area_init_node(..., ..., ..., ARCH_PFN_OFFSET, ...);

And it's quite dangerous because a user of this function must know the
implementation of pfn_to_page() or alloc_node_mem_map() to know that.

Therefore, what I proposed was to let free_area_init_node() work as
expected, so whatever the value of ARCH_PFN_OFFSET, this use

free_area_init_node(..., ..., ..., whatever, ...);

will define the start of mem as 'whatever' value. And if the user
wants to use the default start mem value then he can do both:

free_area_init_node(..., ..., ..., ARCH_PFN_OFFSET, ...);

or (equivalent):

free_area_init(...);

> ....
> 
> using mem_map directly. uh uh
> 
> Both of our patches are broken.
> 
> page_to_pfn() and pfn_to_page() both need ARCH_PFN_OFFSET to get PFNs,
> that's fine. However, I forgot that another assumption of the FLATMEM memory
> model is that mem_map[0] is the first valid struct page in the system. A

I would say that the first valid struct page in the system is

mem_map[PFN_UP(__pa(PAGE_OFFSET))] == mem_map[ARCH_PFN_OFFSET]

> number of architectures walk mem_map[] directly (cris and frv are examples)
> without offsetting based on this assumption.
> 

but they do have ARCH_PFN_OFFSET = 0, no ?

Walking mem_map[] directly should be avoid. 

If the mem start is different from 0 and ARCH_PFN_OFFSET is not set
then all patches are correct and mem_map[0] is valid.

But if the user set ARCH_PFN_OFFSET != 0, he tells to the system that
the start of memory is not 0, and mem_map[0] is now forbidden since no
page exist in this area. BTW the problem exists with the old code, if
the user do pfn_to_page(0), he will get an invalid page pointer.

		Franck

Re: [PATCH 1/1] Only use ARCH_PFN_OFFSET once during boot

[Mel Gorman]

On Fri, 7 Jul 2006, Franck Bui-Huu wrote:

> Mel Gorman wrote:
>> On Thu, 6 Jul 2006, Franck Bui-Huu wrote:
>>
>>> 2006/7/6, Mel Gorman <mel@csn.ul.ie>:
>>>> I think my patch does the job of moving ARCH_PFN_OFFSET out of the hot
>>>> path in a less risky fashion. However, if you are sure that callers to
>>>> free_area_init() and ARCH_PFN_OFFSET are ok after your patch, I'd be happy
>>>> to go with it. If you're not sure, I reckon my patch would be the way to
>>>> go.
>>>>
>>> Ok I try to explain better what I have in mind. Your patch changes the
>>> behaviour of free_area_init_node() in the sense that it doesn't work
>>> as expected if its fourth parameter is different from ARCH_PFN_OFFSET,
>>> it even becomes boggus IMHO. And I think it's valid to use it when
>>> FLATMEM model is selected.
>>
>> I'm missing something silly here.
>>
>> Before the patch, we have the following
>>   o Call free_area_initSOMETHING()
>>   o Set mem_map to NODE_DATA(0)->node_mem_map
>>   o At each call to page_to_pfn() or pfn_to_page(), offset mem_map by
>>     ARCH_PFN_OFFSET
>>
>> After the patch, we have
>>
>>   o Call free_area_initSOMETHING()
>>   o Set mem_map to NODE_DATA(0)->node_mem_map - ARCH_PFN_OFFSET
>>   o At each call to page_to_pfn() or pfn_to_page(), use mem_map without
>>     any additional offset
>>
>> I don't see how free_area_init_node() changed except for callers
>> using mem_map directly.
>>
>
> you're right the behaviour is the same with the old code and with your
> patch that is:
>
> If CONFIG_FLATMEM then free_area_init_node must be called:
>
> free_area_init_node(..., ..., ..., ARCH_PFN_OFFSET, ...);
>
> And it's quite dangerous because a user of this function must know the
> implementation of pfn_to_page() or alloc_node_mem_map() to know that.
>
> Therefore, what I proposed was to let free_area_init_node() work as
> expected, so whatever the value of ARCH_PFN_OFFSET, this use
>
> free_area_init_node(..., ..., ..., whatever, ...);
>
> will define the start of mem as 'whatever' value. And if the user
> wants to use the default start mem value then he can do both:
>
> free_area_init_node(..., ..., ..., ARCH_PFN_OFFSET, ...);
>
> or (equivalent):
>
> free_area_init(...);
>

Ok, I'm convinced. This change would make more sense but with direct users 
of mem_map, it is incomplete.

>> ....
>>
>> using mem_map directly. uh uh
>>
>> Both of our patches are broken.
>>
>> page_to_pfn() and pfn_to_page() both need ARCH_PFN_OFFSET to get PFNs,
>> that's fine. However, I forgot that another assumption of the FLATMEM memory
>> model is that mem_map[0] is the first valid struct page in the system. A
>
> I would say that the first valid struct page in the system is
>
> mem_map[PFN_UP(__pa(PAGE_OFFSET))] == mem_map[ARCH_PFN_OFFSET]
>

That's not the assumption users of mem_map[] are making.

>> number of architectures walk mem_map[] directly (cris and frv are examples)
>> without offsetting based on this assumption.
>>
>
> but they do have ARCH_PFN_OFFSET = 0, no ?
>

mel@arnold:~/linux-2.6.17-mm6-clean/include/asm-cris$ grep -r ARCH_PFN_OFFSET *
page.h:#define ARCH_PFN_OFFSET          (PAGE_OFFSET >> PAGE_SHIFT)

mel@arnold:~/linux-2.6.17-mm6-clean/arch/cris$ grep -r mem_map *
arch-v10/mm/init.c:      * mem_map page array.
arch-v32/mm/init.c:      * saves space in the mem_map page array.
arch-v32/mm/init.c:     mem_map = contig_page_data.node_mem_map;
mm/init.c:              if (PageReserved(mem_map+i))
mm/init.c:              else if (PageSwapCache(mem_map+i))
mm/init.c:              else if (!page_count(mem_map+i))
mm/init.c:              else if (page_count(mem_map+i) == 1)
mm/init.c:                      shared += page_count(mem_map+i) - 1;
mm/init.c:      if(!mem_map)
mm/init.c:              if (PageReserved(mem_map + tmp))

That would be a no. In the example of cris and elsewhere, show_mem() walks 
the mem_map array from max_mapnr to 0. If mem_map had been offset by 
ARCH_PFN_OFFSET during init, the first call to show_mem() would have had 
interesting results.

> Walking mem_map[] directly should be avoid.
>

Whether it should be avoided now or not, mem_map[] is walked directly. 
Historically, it was fine to do this. The full patch would need to do 
something like

o Rename mem_map to __mem_map[] to break incorrect users at compile time
o #define MEM_MAP (__mem_map + ARCH_PFN_OFFSET)
o Change all direct users of mem_map to MEM_MAP

While not exactly complicated, is it worth it?

> If the mem start is different from 0 and ARCH_PFN_OFFSET is not set
> then all patches are correct and mem_map[0] is valid.
>
> But if the user set ARCH_PFN_OFFSET != 0, he tells to the system that
> the start of memory is not 0, and mem_map[0] is now forbidden since no
> page exist in this area.

It's what happens thoug: ARCH_PFN_OFFSET != 0 and mem_map[0] is used.

> BTW the problem exists with the old code, if
> the user do pfn_to_page(0), he will get an invalid page pointer.
>

Good job they don't do that :/

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: [PATCH 1/1] Only use ARCH_PFN_OFFSET once during boot

[Franck Bui-Huu]

Mel Gorman wrote:
> On Fri, 7 Jul 2006, Franck Bui-Huu wrote:
> 
>> Therefore, what I proposed was to let free_area_init_node() work as
>> expected, so whatever the value of ARCH_PFN_OFFSET, this use
>>
>> free_area_init_node(..., ..., ..., whatever, ...);
>>
>> will define the start of mem as 'whatever' value. And if the user
>> wants to use the default start mem value then he can do both:
>>
>> free_area_init_node(..., ..., ..., ARCH_PFN_OFFSET, ...);
>>
>> or (equivalent):
>>
>> free_area_init(...);
>>
> 
> Ok, I'm convinced. This change would make more sense but with direct
> users of mem_map, it is incomplete.
> 

great !

>>> ....
>>>
>>> using mem_map directly. uh uh
>>>
>>> Both of our patches are broken.
>>>
>>> page_to_pfn() and pfn_to_page() both need ARCH_PFN_OFFSET to get PFNs,
>>> that's fine. However, I forgot that another assumption of the FLATMEM
>>> memory
>>> model is that mem_map[0] is the first valid struct page in the system. A
>>
>> I would say that the first valid struct page in the system is
>>
>> mem_map[PFN_UP(__pa(PAGE_OFFSET))] == mem_map[ARCH_PFN_OFFSET]
>>
> 
> That's not the assumption users of mem_map[] are making.
> 
>>> number of architectures walk mem_map[] directly (cris and frv are
>>> examples)
>>> without offsetting based on this assumption.
>>>
>>
>> but they do have ARCH_PFN_OFFSET = 0, no ?
>>
> 
> mel@arnold:~/linux-2.6.17-mm6-clean/include/asm-cris$ grep -r
> ARCH_PFN_OFFSET *
> page.h:#define ARCH_PFN_OFFSET          (PAGE_OFFSET >> PAGE_SHIFT)
> 
> mel@arnold:~/linux-2.6.17-mm6-clean/arch/cris$ grep -r mem_map *
> arch-v10/mm/init.c:      * mem_map page array.
> arch-v32/mm/init.c:      * saves space in the mem_map page array.
> arch-v32/mm/init.c:     mem_map = contig_page_data.node_mem_map;
> mm/init.c:              if (PageReserved(mem_map+i))
> mm/init.c:              else if (PageSwapCache(mem_map+i))
> mm/init.c:              else if (!page_count(mem_map+i))
> mm/init.c:              else if (page_count(mem_map+i) == 1)
> mm/init.c:                      shared += page_count(mem_map+i) - 1;
> mm/init.c:      if(!mem_map)
> mm/init.c:              if (PageReserved(mem_map + tmp))
> 
> That would be a no. In the example of cris and elsewhere, show_mem()
> walks the mem_map array from max_mapnr to 0. If mem_map had been offset
> by ARCH_PFN_OFFSET during init, the first call to show_mem() would have
> had interesting results.
> 
>> Walking mem_map[] directly should be avoid.
>>
> 
> Whether it should be avoided now or not, mem_map[] is walked directly.
> Historically, it was fine to do this. The full patch would need to do
> something like
> 
> o Rename mem_map to __mem_map[] to break incorrect users at compile time
> o #define MEM_MAP (__mem_map + ARCH_PFN_OFFSET)
> o Change all direct users of mem_map to MEM_MAP
>
> While not exactly complicated, is it worth it?
> 

It's always worth to fix broken code. But I don't think that's should
be done by this patch.

>> If the mem start is different from 0 and ARCH_PFN_OFFSET is not set
>> then all patches are correct and mem_map[0] is valid.
>>
>> But if the user set ARCH_PFN_OFFSET != 0, he tells to the system that
>> the start of memory is not 0, and mem_map[0] is now forbidden since no
>> page exist in this area.
> 
> It's what happens thoug: ARCH_PFN_OFFSET != 0 and mem_map[0] is used.
> 
>> BTW the problem exists with the old code, if
>> the user do pfn_to_page(0), he will get an invalid page pointer.
>>
> 
> Good job they don't do that :/
> 

so doing pfn_to_page(0) will crash and mem_map[0] is ok ? it sounds very
silly no ? 

Well, I think this arch has really really weird uses of mem_map. That may be
explained by the fact that it was implemented before the support of "mem start
is not 0" had been added.

Maybe it's time to make these arches aware of this ?

I CC'ed both frv and cris maitainers...

		Franck

Re: [PATCH 1/1] Only use ARCH_PFN_OFFSET once during boot

[Mel Gorman]

On Fri, 7 Jul 2006, Franck Bui-Huu wrote:

> Mel Gorman wrote:
>> On Fri, 7 Jul 2006, Franck Bui-Huu wrote:
>>
>>> Therefore, what I proposed was to let free_area_init_node() work as
>>> expected, so whatever the value of ARCH_PFN_OFFSET, this use
>>>
>>> free_area_init_node(..., ..., ..., whatever, ...);
>>>
>>> will define the start of mem as 'whatever' value. And if the user
>>> wants to use the default start mem value then he can do both:
>>>
>>> free_area_init_node(..., ..., ..., ARCH_PFN_OFFSET, ...);
>>>
>>> or (equivalent):
>>>
>>> free_area_init(...);
>>>
>>
>> Ok, I'm convinced. This change would make more sense but with direct
>> users of mem_map, it is incomplete.
>>
>
> great !
>
>>>> ....
>>>>
>>>> using mem_map directly. uh uh
>>>>
>>>> Both of our patches are broken.
>>>>
>>>> page_to_pfn() and pfn_to_page() both need ARCH_PFN_OFFSET to get PFNs,
>>>> that's fine. However, I forgot that another assumption of the FLATMEM
>>>> memory
>>>> model is that mem_map[0] is the first valid struct page in the system. A
>>>
>>> I would say that the first valid struct page in the system is
>>>
>>> mem_map[PFN_UP(__pa(PAGE_OFFSET))] == mem_map[ARCH_PFN_OFFSET]
>>>
>>
>> That's not the assumption users of mem_map[] are making.
>>
>>>> number of architectures walk mem_map[] directly (cris and frv are
>>>> examples)
>>>> without offsetting based on this assumption.
>>>>
>>>
>>> but they do have ARCH_PFN_OFFSET = 0, no ?
>>>
>>
>> mel@arnold:~/linux-2.6.17-mm6-clean/include/asm-cris$ grep -r
>> ARCH_PFN_OFFSET *
>> page.h:#define ARCH_PFN_OFFSET          (PAGE_OFFSET >> PAGE_SHIFT)
>>
>> mel@arnold:~/linux-2.6.17-mm6-clean/arch/cris$ grep -r mem_map *
>> arch-v10/mm/init.c:      * mem_map page array.
>> arch-v32/mm/init.c:      * saves space in the mem_map page array.
>> arch-v32/mm/init.c:     mem_map = contig_page_data.node_mem_map;
>> mm/init.c:              if (PageReserved(mem_map+i))
>> mm/init.c:              else if (PageSwapCache(mem_map+i))
>> mm/init.c:              else if (!page_count(mem_map+i))
>> mm/init.c:              else if (page_count(mem_map+i) == 1)
>> mm/init.c:                      shared += page_count(mem_map+i) - 1;
>> mm/init.c:      if(!mem_map)
>> mm/init.c:              if (PageReserved(mem_map + tmp))
>>
>> That would be a no. In the example of cris and elsewhere, show_mem()
>> walks the mem_map array from max_mapnr to 0. If mem_map had been offset
>> by ARCH_PFN_OFFSET during init, the first call to show_mem() would have
>> had interesting results.
>>
>>> Walking mem_map[] directly should be avoid.
>>>
>>
>> Whether it should be avoided now or not, mem_map[] is walked directly.
>> Historically, it was fine to do this. The full patch would need to do
>> something like
>>
>> o Rename mem_map to __mem_map[] to break incorrect users at compile time
>> o #define MEM_MAP (__mem_map + ARCH_PFN_OFFSET)
>> o Change all direct users of mem_map to MEM_MAP
>>
>> While not exactly complicated, is it worth it?
>>
>
> It's always worth to fix broken code.

It's a question of definition whether the code is actually broken or not. 
Using ARCH_PFN_OFFSET to fudge between mem_map starting at PFN 0 is 
deliberate, just as walking mem_map starting at mem_map[0] is deliberate.

Certainly, it is a bit confused that page_to_pfn and friends assume that 
mem_map[] begins at PFN 0, but direct walkers of mem_map[] assume that 
mem_map[] begins at the first valid struct page whether it's PFN 0 or not. 
It would be nicer to have things consistent.

> But I don't think that's should
> be done by this patch.
>

No, each of the direct users of mem_map would need to be fixed first and 
there are a fair number. http://lxr.free-electrons.com/ident?i=mem_map 
gives an indication of how many although a glance through shows that most 
users are a copied show_mem()

>>> If the mem start is different from 0 and ARCH_PFN_OFFSET is not set
>>> then all patches are correct and mem_map[0] is valid.
>>>
>>> But if the user set ARCH_PFN_OFFSET != 0, he tells to the system that
>>> the start of memory is not 0, and mem_map[0] is now forbidden since no
>>> page exist in this area.
>>
>> It's what happens thoug: ARCH_PFN_OFFSET != 0 and mem_map[0] is used.
>>
>>> BTW the problem exists with the old code, if
>>> the user do pfn_to_page(0), he will get an invalid page pointer.
>>>
>>
>> Good job they don't do that :/
>>
>
> so doing pfn_to_page(0) will crash and mem_map[0] is ok ? it sounds very
> silly no ?
>

I agree. I was pointing what the current assumptions related to mem_map[] 
are, not whether I think it's a good idea or not :)

> Well, I think this arch has really really weird uses of mem_map. That may be
> explained by the fact that it was implemented before the support of "mem start
> is not 0" had been added.
>

Yes, it would be. As I said, historically, using mem_map[0] was fine and 
the current oddness in FLATMEM memory model reflects that.

> Maybe it's time to make these arches aware of this ?
>
> I CC'ed both frv and cris maitainers...
>

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

RE: [PATCH 1/1] Only use ARCH_PFN_OFFSET once during boot

[Mikael Starvik]

Don't care too much about CRIS in this case. I will adopt to whatever you
do. As long as we can keep the flat memory model.

-----Original Message-----
From: Franck Bui-Huu [mailto:vagabon.xyz@gmail.com] 
Sent: Friday, July 07, 2006 1:42 PM
To: Mel Gorman; Mikael Starvik; dhowells@redhat.com
Cc: Franck Bui-Huu; akpm@osdl.org; Linux Kernel Mailing List
Subject: Re: [PATCH 1/1] Only use ARCH_PFN_OFFSET once during boot


Mel Gorman wrote:
> On Fri, 7 Jul 2006, Franck Bui-Huu wrote:
> 
>> Therefore, what I proposed was to let free_area_init_node() work as
>> expected, so whatever the value of ARCH_PFN_OFFSET, this use
>>
>> free_area_init_node(..., ..., ..., whatever, ...);
>>
>> will define the start of mem as 'whatever' value. And if the user
>> wants to use the default start mem value then he can do both:
>>
>> free_area_init_node(..., ..., ..., ARCH_PFN_OFFSET, ...);
>>
>> or (equivalent):
>>
>> free_area_init(...);
>>
> 
> Ok, I'm convinced. This change would make more sense but with direct
> users of mem_map, it is incomplete.
> 

great !

>>> ....
>>>
>>> using mem_map directly. uh uh
>>>
>>> Both of our patches are broken.
>>>
>>> page_to_pfn() and pfn_to_page() both need ARCH_PFN_OFFSET to get PFNs,
>>> that's fine. However, I forgot that another assumption of the FLATMEM
>>> memory
>>> model is that mem_map[0] is the first valid struct page in the system. A
>>
>> I would say that the first valid struct page in the system is
>>
>> mem_map[PFN_UP(__pa(PAGE_OFFSET))] == mem_map[ARCH_PFN_OFFSET]
>>
> 
> That's not the assumption users of mem_map[] are making.
> 
>>> number of architectures walk mem_map[] directly (cris and frv are
>>> examples)
>>> without offsetting based on this assumption.
>>>
>>
>> but they do have ARCH_PFN_OFFSET = 0, no ?
>>
> 
> mel@arnold:~/linux-2.6.17-mm6-clean/include/asm-cris$ grep -r
> ARCH_PFN_OFFSET *
> page.h:#define ARCH_PFN_OFFSET          (PAGE_OFFSET >> PAGE_SHIFT)
> 
> mel@arnold:~/linux-2.6.17-mm6-clean/arch/cris$ grep -r mem_map *
> arch-v10/mm/init.c:      * mem_map page array.
> arch-v32/mm/init.c:      * saves space in the mem_map page array.
> arch-v32/mm/init.c:     mem_map = contig_page_data.node_mem_map;
> mm/init.c:              if (PageReserved(mem_map+i))
> mm/init.c:              else if (PageSwapCache(mem_map+i))
> mm/init.c:              else if (!page_count(mem_map+i))
> mm/init.c:              else if (page_count(mem_map+i) == 1)
> mm/init.c:                      shared += page_count(mem_map+i) - 1;
> mm/init.c:      if(!mem_map)
> mm/init.c:              if (PageReserved(mem_map + tmp))
> 
> That would be a no. In the example of cris and elsewhere, show_mem()
> walks the mem_map array from max_mapnr to 0. If mem_map had been offset
> by ARCH_PFN_OFFSET during init, the first call to show_mem() would have
> had interesting results.
> 
>> Walking mem_map[] directly should be avoid.
>>
> 
> Whether it should be avoided now or not, mem_map[] is walked directly.
> Historically, it was fine to do this. The full patch would need to do
> something like
> 
> o Rename mem_map to __mem_map[] to break incorrect users at compile time
> o #define MEM_MAP (__mem_map + ARCH_PFN_OFFSET)
> o Change all direct users of mem_map to MEM_MAP
>
> While not exactly complicated, is it worth it?
> 

It's always worth to fix broken code. But I don't think that's should
be done by this patch.

>> If the mem start is different from 0 and ARCH_PFN_OFFSET is not set
>> then all patches are correct and mem_map[0] is valid.
>>
>> But if the user set ARCH_PFN_OFFSET != 0, he tells to the system that
>> the start of memory is not 0, and mem_map[0] is now forbidden since no
>> page exist in this area.
> 
> It's what happens thoug: ARCH_PFN_OFFSET != 0 and mem_map[0] is used.
> 
>> BTW the problem exists with the old code, if
>> the user do pfn_to_page(0), he will get an invalid page pointer.
>>
> 
> Good job they don't do that :/
> 

so doing pfn_to_page(0) will crash and mem_map[0] is ok ? it sounds very
silly no ? 

Well, I think this arch has really really weird uses of mem_map. That may be
explained by the fact that it was implemented before the support of "mem
start
is not 0" had been added.

Maybe it's time to make these arches aware of this ?

I CC'ed both frv and cris maitainers...

		Franck