lkml.org 
[lkml]   [2007]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/2] Generic Virtual Memmap suport for SPARSEMEM
    On Mon, 2 Apr 2007, Dave Hansen wrote:

    > First of all, nice set of patches.
    >
    > On Sat, 2007-03-31 at 23:10 -0800, Christoph Lameter wrote:
    > > --- linux-2.6.21-rc5-mm2.orig/include/asm-generic/memory_model.h 2007-03-31 22:47:14.000000000 -0700
    > > +++ linux-2.6.21-rc5-mm2/include/asm-generic/memory_model.h 2007-03-31 22:59:35.000000000 -0700
    > > @@ -47,6 +47,13 @@
    > > })
    > >
    > > #elif defined(CONFIG_SPARSEMEM)
    > > +#ifdef CONFIG_SPARSE_VIRTUAL
    > > +/*
    > > + * We have a virtual memmap that makes lookups very simple
    > > + */
    > > +#define __pfn_to_page(pfn) (vmemmap + (pfn))
    > > +#define __page_to_pfn(page) ((page) - vmemmap)
    > > +#else
    > > /*
    > > * Note: section's mem_map is encorded to reflect its start_pfn.
    > > * section[i].section_mem_map == mem_map's address - start_pfn;
    > > @@ -62,6 +69,7 @@
    > > struct mem_section *__sec = __pfn_to_section(__pfn); \
    > > __section_mem_map_addr(__sec) + __pfn; \
    > > })
    > > +#endif
    > > #endif /* CONFIG_FLATMEM/DISCONTIGMEM/SPARSEMEM */
    >
    > Any chance this can be done without embedding this inside another
    > #ifdef? I really hate untangling the mess when an #endif goes
    > missing.
    >
    > Any reason this can't just be another #elif?

    Sure.

    > > + } 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.

    > 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.

    > 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.

    > > +static void vmemmap_pop_pmd(pud_t *pud, unsigned long addr,
    > > + unsigned long end, int node)
    > > +{
    > > + pmd_t *pmd;
    > > +
    > > + end = pmd_addr_end(addr, end);
    > > +
    > > + for (pmd = pmd_offset(pud, addr); addr < end;
    > > + pmd++, addr += PMD_SIZE) {
    > > + if (pmd_none(*pmd)) {
    > > + void *block;
    > > + pte_t pte;
    > > +
    > > + block = vmemmap_alloc_block(PMD_SIZE, node);
    > > + pte = pfn_pte(__pa(block) >> PAGE_SHIFT,
    > > + PAGE_KERNEL);
    > > + pte_mkdirty(pte);
    > > + pte_mkwrite(pte);
    > > + pte_mkyoung(pte);
    > > + mk_pte_huge(pte);
    > > + set_pmd(pmd, __pmd(pte_val(pte)));
    > > + }
    > > + }
    > > +}
    >
    > Nitpick: I think this would look quite a bit neater with a little less
    > indentation.
    >
    > How about making the loop start with
    >
    > if (!pmd_none(*pmd))
    > continue;
    >
    > It should bring the rest of the code in a bit and make that long line
    > more readable.

    V2 has the pmd setup separated out in a separate function.

    > > +{
    > > + pgd_t *pgd;
    > > + unsigned long addr = (unsigned long)(start_page);
    > > + unsigned long end = pgd_addr_end(addr,
    > > + (unsigned long)((start_page + nr)));
    >
    > There appear to be a few extra parentheses on these lines.

    Right. Fixed.

    > > + for (pgd = pgd_offset_k(addr); addr < end;
    > > + pgd++, addr += PGDIR_SIZE) {
    > > +
    > > + if (pgd_none(*pgd))
    > > + pgd_populate(&init_mm, pgd,
    > > + vmemmap_alloc_block(PAGE_SIZE, node));
    > > + vmemmap_pop_pud(pgd, addr, end, node);
    > > + }
    > > +}
    > > +#endif
    > > +#endif /* CONFIG_SPARSE_VIRTUAL */
    >
    > We don't really need these #ifdefs embedded inside of each other,
    > either, right? Kconfig should take care of enforcing the dependency.

    Ok.

    > > +#ifdef CONFIG_SPARSE_VIRTUAL
    > > + map = pfn_to_page(pnum * PAGES_PER_SECTION);
    > > + vmemmap_populate(map, PAGES_PER_SECTION, nid);
    > > +#else
    > > map = alloc_bootmem_node(NODE_DATA(nid),
    > > sizeof(struct page) * PAGES_PER_SECTION);
    > > +#endif
    >
    > We really worked hard to keep #ifdefs out of the code flow in that file
    > and keep it as clean as possible. Could we hide this behind a helper?

    This is a major diffference in how sparsemem works. I think this needs to
    stay.

    >
    > 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.


    -
    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/

    \
     
     \ /
      Last update: 2007-04-02 23:03    [W:3.026 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site