Skip to contentSkip to conversation

Slab cache name fixes / reiserfs boot bug fix.

PATCH In review Andi Kleen 1 participant 4 messages Jan 30, 2002 – Jan 31, 2002

Subsystems (4)
Networking Memory management VFS SCSI
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/1 Slab cache name fixes / reiserfs boot bug fix.

Hi Linus,

As discussed earlier this patch cleans up the way the names of slab caches are stored. The caller is now expected to keep the storage, instead of copying and arbitarily imposing a name length limit on the slab names. This fixes the reiserfs boot time BUG() that is still in 2.5.3.

I had to fix some callers that were creating cache names on the stack, that is why the patch is a bit larger.

I didn't audit the complete tree that modules do always destroy their caches on unloading, when they do not do it this patch will hopefully detect it on the next cat /proc/slabinfo (and otherwise not care as it was the situation before).

I addressed all the criticisms that came to earlier versions of the patch, except for the one that proposed to "strdup" the name because that would have needed a custom allocator just for that (kmem_cache_create cannot use kmalloc)

Patch against 2.5.3. Please apply.

Thanks,
-Andi

fs/bio.c renamed from fs/bio.c-SLABNAME +3 -2
@@ -45,6 +45,7 @@
  * unsigned short
  */
 static const int bvec_pool_sizes[BIOVEC_NR_POOLS] = { 1, 4, 16, 64, 128, 256 };
+static char bvec_pool_names[BIOVEC_NR_POOLS][16];
 
 #define BIO_MAX_PAGES	(bvec_pool_sizes[BIOVEC_NR_POOLS - 1])
 

@@ -452,20 +453,20 @@
 
 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;
+		char *name = bvec_pool_names[i]; 
 
 		size = bvec_pool_sizes[i] * sizeof(struct bio_vec);
 
 		printk("biovec: init pool %d, %d entries, %d bytes\n", i,
 						bvec_pool_sizes[i], size);
 
-		snprintf(name, sizeof(name) - 1,"biovec-%d",bvec_pool_sizes[i]);
+		sprintf(name, "biovec-%d",bvec_pool_sizes[i]);
 		bp->slab = kmem_cache_create(name, size, 0,
 						SLAB_HWCACHE_ALIGN, NULL, NULL);
 		if (!bp->slab)
mm/slab.c renamed from mm/slab.c-SLABNAME +57 -17

154 lines not on this page · raw message

include/net/neighbour.h renamed from include/net/neighbour.h-SLABNAME +1 -1
@@ -147,7 +147,7 @@
 	int			(*pconstructor)(struct pneigh_entry *);
 	void			(*pdestructor)(struct pneigh_entry *);
 	void			(*proxy_redo)(struct sk_buff *skb);
-	char			*id;
+	char			id[20];
 	struct neigh_parms	parms;
 	/* HACK. gc_* shoul follow parms without a gap! */
 	int			gc_interval;
drivers/scsi/scsi.c renamed from drivers/scsi/scsi.c-SLABNAME +3 -2
@@ -93,6 +93,7 @@
 
 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];
+static char scsi_host_sg_names[SG_MEMPOOL_NR][18]; 
 
 /*
    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 +2490,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");

@@ -2498,10 +2498,11 @@
 	 * setup sg memory pools
 	 */
 	for (i = 0; i < SG_MEMPOOL_NR; i++) {
+		char *name = scsi_host_sg_names[i]; 
 		struct scsi_host_sg_pool *sgp = scsi_sg_pools + i;
 		int size = scsi_host_sg_pool_sizes[i] * sizeof(struct scatterlist);
 
-		snprintf(name, sizeof(name) - 1, "sgpool-%d", scsi_host_sg_pool_sizes[i]);
+		sprintf(name, "sgpool-%d", scsi_host_sg_pool_sizes[i]);
 		sgp->slab = kmem_cache_create(name, size, 0, SLAB_HWCACHE_ALIGN, NULL, NULL);
 		if (!sgp->slab)
 			panic("SCSI: can't init sg slab\n");
author
#1 lore raw
Quoted 3 lines · Linus Torvalds

which is just simpler and doesn't have issues like fixed 16-byte arrays etc.

I did it this way to keep the stuff cache neutral (keep the names or pointer to names out of the critical cache lines), but if you prefer the merged version I'll do that in a jiffie.

-Andi

author
#2 lore raw
In reply to Andi Kleen
Quoted 12 lines · Linus Torvalds

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

Quoted 8 lines
	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

fs/bio.c renamed from fs/bio.c-SLABNAME +13 -13
@@ -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;	
 	}
 }
mm/slab.c renamed from mm/slab.c-SLABNAME +60 -17

157 lines not on this page · raw message

drivers/scsi/scsi.c renamed from drivers/scsi/scsi.c-SLABNAME +8 -9
@@ -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;
 	}
 
 	/*
author
#3 lore raw
In reply to Andi Kleen
Quoted 5 lines · Oleg Drokin

Hello!

Quoted 1 line · Andi Kleen

slab names. This fixes the reiserfs boot time BUG() that is still in 2.5.3.

Sorry, What BUG()? The one I know that was related to "reiserfs_inode_cache"

It seems to be gone already, yes. Sorry for the confusion. Anyways, the slab name patch should be the right thing to fix the interface.

-Andi