On Tue, Oct 27, 2009 at 01:28:19PM +1100, Benjamin Herrenschmidt wrote:

On Fri, 2009-10-16 at 16:22 +1100, David Gibson wrote:

Minor nits... if you can respin today I should push it out to -next

quoted

+void pgtable_cache_add(unsigned shift, void (*ctor)(void *))
+{
+	char *name;
+	unsigned long table_size = sizeof(void *) << shift;
+	unsigned long align = table_size;

This is a bit thick.. could use some air. Just separate the definitions
from the assignments so you can make the code breath a bit :-)

Ok.

Also the above warrants a comment explaining that this won't work for
PTE pages since sizeof(PTE) >= sizeof(void *) and the day we finally
move out of pte page == struct page, the code here will have to be
adapted.

Ok.

[snip]

quoted

 void pgtable_cache_init(void)
 {
-	pgtable_cache[0] = kmem_cache_create(pgtable_cache_name[0], PGD_TABLE_SIZE, PGD_TABLE_SIZE, SLAB_PANIC, pgd_ctor);
-	pgtable_cache[1] = kmem_cache_create(pgtable_cache_name[1], PMD_TABLE_SIZE, PMD_TABLE_SIZE, SLAB_PANIC, pmd_ctor);
+	pgtable_cache_add(PGD_INDEX_SIZE, pgd_ctor);
+	pgtable_cache_add(PMD_INDEX_SIZE, pmd_ctor);
+	if (!PGT_CACHE(PGD_INDEX_SIZE) || !PGT_CACHE(PMD_INDEX_SIZE))
+		panic("Couldn't allocate pgtable caches");
+	BUG_ON(PUD_INDEX_SIZE && !PGT_CACHE(PUD_INDEX_SIZE));
 }

panic vs. BUG_ON() ... could be a bit more consistent.

Uh.. there is actually a rationale for the difference here.  The
panic() is due to a a runtime error - couldn't allocate the caches -
which isn't necessarily a kernel bug (could be hardware error, or
ludicrously short on memory).

The trick is that allocating the PGD and PMD caches is supposed to
also create the PUD cache, because the PUD index size is always the
same as either the PGD or PUD cache.  If that's not true, we've broken
the assumptions the code is based on, hence BUG().

quoted

 #ifdef CONFIG_SPARSEMEM_VMEMMAP
Index: working-2.6/arch/powerpc/include/asm/pgalloc-64.h
===================================================================

--- working-2.6.orig/arch/powerpc/include/asm/pgalloc-64.h	2009-10-16 12:53:45.000000000 +1100
+++ working-2.6/arch/powerpc/include/asm/pgalloc-64.h	2009-10-16 12:53:51.000000000 +1100

@@ -11,27 +11,30 @@
 #include <linux/cpumask.h>
 #include <linux/percpu.h>
 
+/*
+ * This needs to be big enough to allow any pagetable sizes we need,
+ * but small enough to fit in the low bits of any page table pointer.
+ * In other words all pagetables, even tiny ones, must be aligned to
+ * allow at least enough low 0 bits to contain this value.
+ */
+#define MAX_PGTABLE_INDEX_SIZE	0xf

This also has the constraint of being a (power of 2) - 1... worth
mentioning somewhere ?

Also if you could comment somewhere that index size == 0 means a PTE
page ? Not totally obvious at first.

Ok, I've expanded on this comment.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson