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

[mel@skynet.ie (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