On Wed, 2009-09-09 at 15:59 +1000, David Gibson wrote:
> 6 files changed, 73 insertions(+), 108 deletions(-)
That's a pretty good start :-)
> +struct kmem_cache *pgtable_cache[PGF_SHIFT_MASK];
> +
> +void pgtable_cache_add(unsigned shift, void (*ctor)(void *))
> +{
> + char *name;
> + unsigned long table_size = sizeof(void *) << shift;
> + struct kmem_cache *new;
> +
> + BUG_ON((shift < 1) || (shift > PGF_SHIFT_MASK));
> + if (PGT_CACHE(shift))
> + return; /* Already have a cache of this size */
> + name = kasprintf(GFP_KERNEL, "pgtable-2^%d", shift);
> + new = kmem_cache_create(name, table_size, table_size, 0, ctor);
> + PGT_CACHE(shift) = new;
> +}
I'm getting partial to verbose boot nowadays :-) At least a pr_debug if
not a pr_info up there "Allocated pgtable for order %d" or something
like that might end up being of some use when debugging things.
> 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(!PGT_CACHE(PUD_INDEX_SIZE));
What if PUD_INDEX_SIZE is 0 ? (64k pages)
Couldn't we just do a
if (PUD_INDEX_SIZE)
pgtable_cache_add(PUD_INDEX_SIZE...)
If it's the same size as another cache it would just not do anything...
> -static inline void pgtable_free(pgtable_free_t pgf)
> +static inline void pgtable_free(void *table, unsigned index_size)
> {
> - void *p = (void *)(pgf.val & ~PGF_CACHENUM_MASK);
> - int cachenum = pgf.val & PGF_CACHENUM_MASK;
> -
> - if (cachenum == PTE_NONCACHE_NUM)
> - free_page((unsigned long)p);
> - else
> - kmem_cache_free(pgtable_cache[cachenum], p);
> + if (!index_size)
> + free_page((unsigned long)table);
> + else {
> + BUG_ON(index_size > PGF_SHIFT_MASK);
> + kmem_cache_free(PGT_CACHE(index_size), table);
> + }
> }
Out of curiosity, what is the index_size == 0 case for ? Do we still use
it ? Agh... found it ... we don't put PTE pages in a cache, just use
gfp. I suppose that's lower overhead.
> /* This needs to be big enough to allow for MMU_PAGE_COUNT + 2 to be stored
> * and small enough to fit in the low bits of any naturally aligned page
> * table cache entry. Arbitrarily set to 0x1f, that should give us some
> * room to grow
> */
The comment above will need updating (don't just remove it, please -do-
explain what it's all about :-)
> -#define PGF_CACHENUM_MASK 0x1f
> -
> -static inline pgtable_free_t pgtable_free_cache(void *p, int cachenum,
> - unsigned long mask)
> -{
> - BUG_ON(cachenum > PGF_CACHENUM_MASK);
> -
> - return (pgtable_free_t){.val = ((unsigned long) p & ~mask) | cachenum};
> -}
> +#define PGF_SHIFT_MASK 0xf
That does bring one question tho... You still fill the batch by sticking
the shift into the low bits of the table right ? Which means that your
table must have at least 4 bits 0 at the bottom, ie, it must at least
have 2 pointers on 64-bit and 4 on 32-bit. Maybe you should add some
runtime test for the later (your comparisons to 1 do the job for 64-bit
but not for 32-bit or did I miss something ?)
Overall, a really nice cleanup... the previous stuff was hairy and prone
to breakage (see how it broke it twice due to misaligned cache names
array during the build-up of the book3e support). I'm actually tempted,
with a bit more testing, to sneak that into .32 despite arriving a bit
late, because the current code is really fragile.
Cheers,
Ben.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
