From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <46ADF83B.3050406@shadowen.org> Date: Mon, 30 Jul 2007 15:39:55 +0100 From: Andy Whitcroft MIME-Version: 1.0 Subject: Re: [PATCH 3/7] Generic Virtual Memmap support for SPARSEMEM References: <20070714152058.GA12478@infradead.org> In-Reply-To: <20070714152058.GA12478@infradead.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org Return-Path: To: Christoph Hellwig Cc: linux-mm@kvack.org, linux-arch@vger.kernel.org, Nick Piggin , Christoph Lameter , Mel Gorman List-ID: 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 >> #include >> #include >> +#include >> +#include >> >> /* >> * 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 . > > 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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org