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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
