Re: [PATCH 1/2] Generic Virtual Memmap suport for SPARSEMEM

Dave Hansen
2007-04-02T21:22:12Z

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