Re: [PATCH] Slab cache name fixes / reiserfs boot bug fix.

From: Andi Kleen (ak@muc.de)
Date: Wed Jan 30 2002 - 19:48:17 EST


On Thu, Jan 31, 2002 at 12:52:45AM +0100, Linus Torvalds wrote:
> Why not just do
>
> #define VEC(x) { x, "biovec-" #x }
> static const struct {
> int size, char *name;
> } bvec_pools[BIOVEC_NR_POOLS] = {
> VEC(1), VEC(4), VEC(16),
> VEC(64), VEC(128), VEC(256)
> };
>
> and then register them with a nice
>
> for (i = 0; i < BIOVEC_NR_POOLS; i++) {
> bp->slab = kmem_cache_create(
> bvec_pools[i].name,
> bvec_pools[i].size,
> ...
>
> which is just simpler and doesn't have issues like fixed 16-byte arrays
> etc.

I reworked the patch now according to your suggestions. It'll use slightly
more cache now, but should be easier readable, has no fixed size char arrays
and is indeed cleaner. I kept the kmalloc slab names out of line for
cache reasons, but replaced the char array there with pointers to .text
strings.

Thanks,
-Andi

--- linux-2.5.3-work/fs/bio.c-SLABNAME Tue Jan 15 17:53:31 2002
+++ linux-2.5.3-work/fs/bio.c Thu Jan 31 01:40:37 2002
@@ -33,20 +33,24 @@
 
 struct biovec_pool {
         int size;
+ char *name;
         kmem_cache_t *slab;
         mempool_t *pool;
 };
 
-static struct biovec_pool bvec_array[BIOVEC_NR_POOLS];
-
 /*
  * if you change this list, also change bvec_alloc or things will
  * break badly! cannot be bigger than what you can fit into an
  * unsigned short
  */
-static const int bvec_pool_sizes[BIOVEC_NR_POOLS] = { 1, 4, 16, 64, 128, 256 };
 
-#define BIO_MAX_PAGES (bvec_pool_sizes[BIOVEC_NR_POOLS - 1])
+#define BV(x) { x, "biovec-" #x }
+static struct biovec_pool bvec_array[BIOVEC_NR_POOLS] = {
+ BV(1), BV(4), BV(16), BV(64), BV(128), BV(256)
+};
+#undef BV
+
+#define BIO_MAX_PAGES (bvec_array[BIOVEC_NR_POOLS - 1].size)
 
 static void *slab_pool_alloc(int gfp_mask, void *data)
 {
@@ -64,7 +68,7 @@
         struct bio_vec *bvl;
 
         /*
- * see comment near bvec_pool_sizes define!
+ * see comment near bvec_array define!
          */
         switch (nr) {
                 case 1 : *idx = 0; break;
@@ -452,21 +456,17 @@
 
 static void __init biovec_init_pool(void)
 {
- char name[16];
         int i, size;
 
- memset(&bvec_array, 0, sizeof(bvec_array));
-
         for (i = 0; i < BIOVEC_NR_POOLS; i++) {
                 struct biovec_pool *bp = bvec_array + i;
 
- size = bvec_pool_sizes[i] * sizeof(struct bio_vec);
+ size = bp->size * sizeof(struct bio_vec);
 
                 printk("biovec: init pool %d, %d entries, %d bytes\n", i,
- bvec_pool_sizes[i], size);
+ bp->size, size);
 
- snprintf(name, sizeof(name) - 1,"biovec-%d",bvec_pool_sizes[i]);
- bp->slab = kmem_cache_create(name, size, 0,
+ bp->slab = kmem_cache_create(bp->name, size, 0,
                                                 SLAB_HWCACHE_ALIGN, NULL, NULL);
                 if (!bp->slab)
                         panic("biovec: can't init slab cache\n");
@@ -474,7 +474,7 @@
                                         slab_pool_free, bp->slab);
                 if (!bp->pool)
                         panic("biovec: can't init mempool\n");
- bp->size = size;
+ bp->size = size;
         }
 }
 
--- linux-2.5.3-work/mm/slab.c-SLABNAME Tue Jan 15 17:53:36 2002
+++ linux-2.5.3-work/mm/slab.c Thu Jan 31 01:36:21 2002
@@ -186,8 +186,6 @@
  * manages a cache.
  */
 
-#define CACHE_NAMELEN 20 /* max name length for a slab cache */
-
 struct kmem_cache_s {
 /* 1) each alloc & free */
         /* full, partial first, then free */
@@ -225,7 +223,7 @@
         unsigned long failures;
 
 /* 3) cache creation/removal */
- char name[CACHE_NAMELEN];
+ const char *name;
         struct list_head next;
 #ifdef CONFIG_SMP
 /* 4) per-cpu data */
@@ -335,6 +333,7 @@
         kmem_cache_t *cs_dmacachep;
 } cache_sizes_t;
 
+/* These are the default caches for kmalloc. Custom caches can have other sizes. */
 static cache_sizes_t cache_sizes[] = {
 #if PAGE_SIZE == 4096
         { 32, NULL, NULL},
@@ -353,6 +352,29 @@
         {131072, NULL, NULL},
         { 0, NULL, NULL}
 };
+/* Must match cache_sizes above. Out of line to keep cache footprint low. */
+#define CN(x) { x, x " (DMA)" }
+static struct {
+ char *name;
+ char *name_dma;
+} cache_names[] = {
+#if PAGE_SIZE == 4096
+ CN("size-32"),
+#endif
+ CN("size-64"),
+ CN("size-128"),
+ CN("size-256"),
+ CN("size-512"),
+ CN("size-1024"),
+ CN("size-2048"),
+ CN("size-4096"),
+ CN("size-8192"),
+ CN("size-16384"),
+ CN("size-32768"),
+ CN("size-65536"),
+ CN("size-131072")
+};
+#undef CN
 
 /* internal cache of cache description objs */
 static kmem_cache_t cache_cache = {
@@ -437,7 +459,6 @@
 void __init kmem_cache_sizes_init(void)
 {
         cache_sizes_t *sizes = cache_sizes;
- char name[20];
         /*
          * Fragmentation resistance on low memory - only use bigger
          * page orders on machines with more than 32MB of memory.
@@ -450,9 +471,9 @@
                  * eliminates "false sharing".
                  * Note for systems short on memory removing the alignment will
                  * allow tighter packing of the smaller caches. */
- sprintf(name,"size-%Zd",sizes->cs_size);
                 if (!(sizes->cs_cachep =
- kmem_cache_create(name, sizes->cs_size,
+ kmem_cache_create(cache_names[sizes-cache_sizes].name,
+ sizes->cs_size,
                                         0, SLAB_HWCACHE_ALIGN, NULL, NULL))) {
                         BUG();
                 }
@@ -462,9 +483,10 @@
                         offslab_limit = sizes->cs_size-sizeof(slab_t);
                         offslab_limit /= 2;
                 }
- sprintf(name, "size-%Zd(DMA)",sizes->cs_size);
- sizes->cs_dmacachep = kmem_cache_create(name, sizes->cs_size, 0,
- SLAB_CACHE_DMA|SLAB_HWCACHE_ALIGN, NULL, NULL);
+ sizes->cs_dmacachep = kmem_cache_create(
+ cache_names[sizes-cache_sizes].name_dma,
+ sizes->cs_size, 0,
+ SLAB_CACHE_DMA|SLAB_HWCACHE_ALIGN, NULL, NULL);
                 if (!sizes->cs_dmacachep)
                         BUG();
                 sizes++;
@@ -604,6 +626,11 @@
  * Cannot be called within a int, but can be interrupted.
  * The @ctor is run when new pages are allocated by the cache
  * and the @dtor is run before the pages are handed back.
+ *
+ * @name must be valid until the cache is destroyed. This implies that
+ * the module calling this has to destroy the cache before getting
+ * unloaded.
+ *
  * The flags are
  *
  * %SLAB_POISON - Poison the slab with a known test pattern (a5a5a5a5)
@@ -632,7 +659,6 @@
          * Sanity checks... these are all serious usage bugs.
          */
         if ((!name) ||
- ((strlen(name) >= CACHE_NAMELEN - 1)) ||
                 in_interrupt() ||
                 (size < BYTES_PER_WORD) ||
                 (size > (1<<MAX_OBJ_ORDER)*PAGE_SIZE) ||
@@ -797,8 +823,7 @@
                 cachep->slabp_cache = kmem_find_general_cachep(slab_size,0);
         cachep->ctor = ctor;
         cachep->dtor = dtor;
- /* Copy name over so we don't have problems with unloaded modules */
- strcpy(cachep->name, name);
+ cachep->name = name;
 
 #ifdef CONFIG_SMP
         if (g_cpucache_up)
@@ -811,10 +836,20 @@
 
                 list_for_each(p, &cache_chain) {
                         kmem_cache_t *pc = list_entry(p, kmem_cache_t, next);
-
- /* The name field is constant - no lock needed. */
- if (!strcmp(pc->name, name))
- BUG();
+ char tmp;
+ /* This happens when the module gets unloaded and doesn't
+ destroy its slab cache and noone else reuses the vmalloc
+ area of the module. Print a warning. */
+ if (__get_user(tmp,pc->name)) {
+ printk("SLAB: cache with size %d has lost its name\n",
+ pc->objsize);
+ continue;
+ }
+ if (!strcmp(pc->name,name)) {
+ printk("kmem_cache_create: duplicate cache %s\n",name);
+ up(&cache_chain_sem);
+ BUG();
+ }
                 }
         }
 
@@ -1878,6 +1913,7 @@
                 unsigned long num_objs;
                 unsigned long active_slabs = 0;
                 unsigned long num_slabs;
+ const char *name;
                 cachep = list_entry(p, kmem_cache_t, next);
 
                 spin_lock_irq(&cachep->spinlock);
@@ -1906,8 +1942,15 @@
                 num_slabs+=active_slabs;
                 num_objs = num_slabs*cachep->num;
 
+ name = cachep->name;
+ {
+ char tmp;
+ if (__get_user(tmp, name))
+ name = "broken";
+ }
+
                 len += sprintf(page+len, "%-17s %6lu %6lu %6u %4lu %4lu %4u",
- cachep->name, active_objs, num_objs, cachep->objsize,
+ name, active_objs, num_objs, cachep->objsize,
                         active_slabs, num_slabs, (1<<cachep->gfporder));
 
 #if STATS
--- linux-2.5.3-work/drivers/scsi/scsi.c-SLABNAME Tue Jan 15 17:53:29 2002
+++ linux-2.5.3-work/drivers/scsi/scsi.c Thu Jan 31 01:35:16 2002
@@ -87,13 +87,16 @@
 
 struct scsi_host_sg_pool {
         int size;
+ char *name;
         kmem_cache_t *slab;
         mempool_t *pool;
 };
 
-static const int scsi_host_sg_pool_sizes[SG_MEMPOOL_NR] = { 8, 16, 32, 64, MAX_PHYS_SEGMENTS };
-struct scsi_host_sg_pool scsi_sg_pools[SG_MEMPOOL_NR];
-
+#define SP(x) { x, "sgpool-" #x }
+struct scsi_host_sg_pool scsi_sg_pools[SG_MEMPOOL_NR] = {
+ SP(8), SP(16), SP(32), SP(64), SP(MAX_PHYS_SEGMENTS)
+};
+#undef SP
 /*
    static const char RCSid[] = "$Header: /vger/u4/cvs/linux/drivers/scsi/scsi.c,v 1.38 1997/01/19 23:07:18 davem Exp $";
  */
@@ -2489,7 +2492,6 @@
 static int __init init_scsi(void)
 {
         struct proc_dir_entry *generic;
- char name[16];
         int i;
 
         printk(KERN_INFO "SCSI subsystem driver " REVISION "\n");
@@ -2499,18 +2501,15 @@
          */
         for (i = 0; i < SG_MEMPOOL_NR; i++) {
                 struct scsi_host_sg_pool *sgp = scsi_sg_pools + i;
- int size = scsi_host_sg_pool_sizes[i] * sizeof(struct scatterlist);
+ int size = sgp->size * sizeof(struct scatterlist);
 
- snprintf(name, sizeof(name) - 1, "sgpool-%d", scsi_host_sg_pool_sizes[i]);
- sgp->slab = kmem_cache_create(name, size, 0, SLAB_HWCACHE_ALIGN, NULL, NULL);
+ sgp->slab = kmem_cache_create(sgp->name, size, 0, SLAB_HWCACHE_ALIGN, NULL, NULL);
                 if (!sgp->slab)
                         panic("SCSI: can't init sg slab\n");
 
                 sgp->pool = mempool_create(SG_MEMPOOL_SIZE, scsi_pool_alloc, scsi_pool_free, sgp->slab);
                 if (!sgp->pool)
                         panic("SCSI: can't init sg mempool\n");
-
- sgp->size = size;
         }
 
         /*
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Thu Jan 31 2002 - 21:01:28 EST