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

[Andy Whitcroft <apw@shadowen.org>]

Christoph Hellwig wrote:
>> --- 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.

The code itself is generic in the sense its architecture neutral.  This
is "per memory model" code.  I am wondering however why it is in an
asm-anything include file here.  This seems to the world like it should
be in include/linux/memory_model.h.

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

I will have a look at how this would look pulled out into separate .c files.

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

Will also look that over and see how it comes out.

-apw