Messages in this thread | | | Date | Fri, 7 Jul 2006 13:31:06 +0100 (IST) | From | Mel Gorman <> | Subject | Re: [PATCH 1/1] Only use ARCH_PFN_OFFSET once during boot |
| |
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 - 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/
| |