Re: [PATCH 3/7] Generic Virtual Memmap support for SPARSEMEM

Christoph Hellwig
2007-07-14T15:20:58Z

> --- a/include/asm-generic/memory_model.h
> +++ b/include/asm-generic/memory_model.h
> @@ -46,6 +46,12 @@
> __pgdat->node_start_pfn; \
> })
>
> +#elif defined(CONFIG_SPARSEMEM_VMEMMAP)
> +
> +/* memmap is virtually contigious. */
> +#define __pfn_to_page(pfn) (vmemmap + (pfn))
> +#define __page_to_pfn(page) ((page) - vmemmap)
> +
> #elif defined(CONFIG_SPARSEMEM)

nice ifdef mess you have here. and an sm-generic file should be something
truely generic instead of a complete ifdef forest. I think we'd be
much better off duplicating the two lines above in architectures using
it anyway.

> diff --git a/mm/sparse.c b/mm/sparse.c
> index d6678ab..5cc6e74 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -9,6 +9,8 @@
> #include <linux/spinlock.h>
> #include <linux/vmalloc.h>
> #include <asm/dma.h>
> +#include <asm/pgalloc.h>
> +#include <asm/pgtable.h>
>
> /*
> * Permanent SPARSEMEM data:
> @@ -218,6 +220,192 @@ void *alloc_bootmem_high_node(pg_data_t *pgdat, unsigned long size)
> return NULL;
> }
>
> +#ifdef CONFIG_SPARSEMEM_VMEMMAP
> +/*
> + * Virtual Memory Map support
> + *
> + * (C) 2007 sgi. Christoph Lameter <clameter@sgi.com>.

When did we start putting copyright lines and large block comment in the
middle of the file?

Please sort this and the ifdef mess out, I suspect a new file for this
code would be best.

> +void * __meminit vmemmap_alloc_block(unsigned long size, int node)

void * __meminit vmemmap_alloc_block(unsigned long size, int node)

> +#ifndef CONFIG_ARCH_POPULATES_SPARSEMEM_VMEMMAP
> +void __meminit vmemmap_verify(pte_t *pte, int node,
> + unsigned long start, unsigned long end)
> +{
> + unsigned long pfn = pte_pfn(*pte);
> + int actual_node = early_pfn_to_nid(pfn);
> +
> + if (actual_node != node)
> + printk(KERN_WARNING "[%lx-%lx] potential offnode "
> + "page_structs\n", start, end - 1);
> +}

Given tht this function is a tiny noop please just put them into the
arch dir for !CONFIG_ARCH_POPULATES_SPARSEMEM_VMEMMAP architectures
and save yourself both the ifdef mess and the config option.