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@xxxxxxxxx>:
>>> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/