On Mon, 2007-04-02 at 14:00 -0700, Christoph Lameter wrote: > On Mon, 2 Apr 2007, Dave Hansen wrote: > > > + } else > > > + return __alloc_bootmem_node(NODE_DATA(node), size, size, > > > + __pa(MAX_DMA_ADDRESS)); > > > +} > > > > Hmmmmmmm. Can we combine this with sparse_index_alloc()? Also, why not > > just use the slab for this? > > Use a slab for page sized allocations? No.
Why not? We use it above for sparse_index_alloc() and if it is doing something wrong, I'd love to fix it. Can you elaborate? > > Let's get rid of the _block() part, too. I'm not sure it does any good. > > At least make it _bytes() so that we know what the units are. Also, if > > you're just going to round up internally and _not_ use the slab, can you > > just make the argument in pages, or even order? > > Its used for page sized allocations. Ok, then let's make it take pages of some kind or its argument. > > Can you think of any times when we'd want that BUG_ON() to be a > > WARN_ON(), instead? I can see preferring having my mem_map[] on the > > wrong node than hitting a BUG(). > > We should probably have some error handling there instead of the BUG. > > > > +#ifndef ARCH_POPULATES_VIRTUAL_MEMMAP > > > +/* > > > + * Virtual memmap populate functionality for architectures that support > > > + * PMDs for huge pages like i386, x86_64 etc. > > > + */ > > > > How about: > > > > /* > > * Virtual memmap support for architectures that use Linux pagetables > > * natively in hardware, and support mapping huge pages with PMD > > * entries. > > */ > > > > It wouldn't make sense to map the vmemmap area with Linux pagetables on > > an arch that didn't use them in hardware, right? So, perhaps this > > doesn't quite belong in mm/sparse.c. Perhaps we need > > arch/x86/sparse.c. ;) > > I just extended this in V2 to also work on IA64. Its pretty generic. Can you extend it to work on ppc? ;) You haven't posted V2, right? > > > > map = alloc_remap(nid, sizeof(struct page) * PAGES_PER_SECTION); > > if (map) > > return map; > > > > + map = alloc_vmemmap(map, PAGES_PER_SECTION, nid); > > + if (map) > > + return map; > > + > > map = alloc_bootmem_node(NODE_DATA(nid), > > sizeof(struct page) * PAGES_PER_SECTION); > > if (map) > > return map; > > > > Then, do whatever magic you want in alloc_vmemmap(). > > That would break if alloc_vmemmap returns NULL because it cannot allocate > memory. OK, that makes sense. However, it would still be nice to hide that #ifdef somewhere that people are a bit less likely to run into it. It's just one #ifdef, so if you can kill it, great. Otherwise, they pile up over time and _do_ cause real readability problems. -- Dave - 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/
