Skip to contentSkip to conversation

Remove destructor support from slab allocators

PATCH In review [email protected] 4 participants 9 messages May 11, 2007 – May 12, 2007

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

cover Remove destructor support from slab allocators

This removes the last use of a slab destructor in the sh arch and then removes destructor support.

#1 lore raw

PATCH 1/2 From: Paul Mundt <[email protected]>

Quoted 5 lines

I'll take a look at tidying up the PMB slab, getting rid of the dtor shouldn't be terribly painful. I simply opted to do the list management there since others were doing it for the PGD slab cache at the time that was written.

And here's the bit for dropping pmb_cache_dtor(), moving the list management up to pmb_alloc() and pmb_free().

With this applied, we're all set for killing off slab destructors from the kernel entirely.

--

 arch/sh/mm/pmb.c |   79 ++++++++++++++++++++++++++-----------------------------
 1 file changed, 38 insertions(+), 41 deletions(-)
arch/sh/mm/pmb.c +38 -41
Index: linux-2.6.21-mm2/arch/sh/mm/pmb.c
===================================================================
@@ -3,7 +3,7 @@
  *
  * Privileged Space Mapping Buffer (PMB) Support.
  *
- * Copyright (C) 2005, 2006 Paul Mundt
+ * Copyright (C) 2005, 2006, 2007 Paul Mundt
  *
  * P1/P2 Section mapping definitions from map32.h, which was:
  *
@@ -68,6 +68,32 @@ static inline unsigned long mk_pmb_data(
 	return mk_pmb_entry(entry) | PMB_DATA;
 }
 
+static DEFINE_SPINLOCK(pmb_list_lock);
+static struct pmb_entry *pmb_list;
+
+static inline void pmb_list_add(struct pmb_entry *pmbe)
+{
+	struct pmb_entry **p, *tmp;
+
+	p = &pmb_list;
+	while ((tmp = *p) != NULL)
+		p = &tmp->next;
+
+	pmbe->next = tmp;
+	*p = pmbe;
+}
+
+static inline void pmb_list_del(struct pmb_entry *pmbe)
+{
+	struct pmb_entry **p, *tmp;
+
+	for (p = &pmb_list; (tmp = *p); p = &tmp->next)
+		if (tmp == pmbe) {
+			*p = tmp->next;
+			return;
+		}
+}
+
 struct pmb_entry *pmb_alloc(unsigned long vpn, unsigned long ppn,
 			    unsigned long flags)
 {
@@ -81,11 +107,19 @@ struct pmb_entry *pmb_alloc(unsigned lon
 	pmbe->ppn	= ppn;
 	pmbe->flags	= flags;
 
+	spin_lock_irq(&pmb_list_lock);
+	pmb_list_add(pmbe);
+	spin_unlock_irq(&pmb_list_lock);
+
 	return pmbe;
 }
 
 void pmb_free(struct pmb_entry *pmbe)
 {
+	spin_lock_irq(&pmb_list_lock);
+	pmb_list_del(pmbe);
+	spin_unlock_irq(&pmb_list_lock);
+
 	kmem_cache_free(pmb_cache, pmbe);
 }
 
@@ -167,31 +201,6 @@ void clear_pmb_entry(struct pmb_entry *p
 	clear_bit(entry, &pmb_map);
 }
 
-static DEFINE_SPINLOCK(pmb_list_lock);
-static struct pmb_entry *pmb_list;
-
-static inline void pmb_list_add(struct pmb_entry *pmbe)
-{
-	struct pmb_entry **p, *tmp;
-
-	p = &pmb_list;
-	while ((tmp = *p) != NULL)
-		p = &tmp->next;
-
-	pmbe->next = tmp;
-	*p = pmbe;
-}
-
-static inline void pmb_list_del(struct pmb_entry *pmbe)
-{
-	struct pmb_entry **p, *tmp;
-
-	for (p = &pmb_list; (tmp = *p); p = &tmp->next)
-		if (tmp == pmbe) {
-			*p = tmp->next;
-			return;
-		}
-}
 
 static struct {
 	unsigned long size;
@@ -283,25 +292,14 @@ void pmb_unmap(unsigned long addr)
 	} while (pmbe);
 }
 
-static void pmb_cache_ctor(void *pmb, struct kmem_cache *cachep, unsigned long flags)
+static void pmb_cache_ctor(void *pmb, struct kmem_cache *cachep,
+			   unsigned long flags)
 {
 	struct pmb_entry *pmbe = pmb;
 
 	memset(pmb, 0, sizeof(struct pmb_entry));
 
-	spin_lock_irq(&pmb_list_lock);
-
 	pmbe->entry = PMB_NO_ENTRY;
-	pmb_list_add(pmbe);
-
-	spin_unlock_irq(&pmb_list_lock);
-}
-
-static void pmb_cache_dtor(void *pmb, struct kmem_cache *cachep, unsigned long flags)
-{
-	spin_lock_irq(&pmb_list_lock);
-	pmb_list_del(pmb);
-	spin_unlock_irq(&pmb_list_lock);
 }
 
 static int __init pmb_init(void)
@@ -312,8 +310,7 @@ static int __init pmb_init(void)
 	BUG_ON(unlikely(nr_entries >= NR_PMB_ENTRIES));
 
 	pmb_cache = kmem_cache_create("pmb", sizeof(struct pmb_entry), 0,
-				      SLAB_PANIC, pmb_cache_ctor,
-				      pmb_cache_dtor);
+				      SLAB_PANIC, pmb_cache_ctor, NULL);
 
 	jump_to_P2();
 
maintainer
#2 lore raw

Re: [patch 1/2] From: Paul Mundt <[email protected]>

Quoted 10 lines · On Fri, 11 May 2007 09:57:50 -0700 [email protected] wrote:
Quoted 4 lines

I'll take a look at tidying up the PMB slab, getting rid of the dtor shouldn't be terribly painful. I simply opted to do the list management there since others were doing it for the PGD slab cache at the time that was written.

And here's the bit for dropping pmb_cache_dtor(), moving the list management up to pmb_alloc() and pmb_free().

With this applied, we're all set for killing off slab destructors from the kernel entirely.

hm, this is already in Paul's git tree.

If we're going to slam all this into 2.6.22 then I can just tempdrop Paul's tree.

However I think we've done enough slab work for 2.6.22 now so I'm inclined to queue these changes for 2.6.23. That would mean that the slab changes in -mm have a dependency on the sh git tree which I am sure to forget about. If I end up merging these changes before Paul merges his tree, sh will break. Presumably Paul will notice this ;)

#3 lore raw

Re: [patch 1/2] From: Paul Mundt <[email protected]>

Quoted 5 lines · Andrew Morton

However I think we've done enough slab work for 2.6.22 now so I'm inclined to queue these changes for 2.6.23. That would mean that the slab changes in -mm have a dependency on the sh git tree which I am sure to forget about. If I end up merging these changes before Paul merges his tree, sh will break. Presumably Paul will notice this ;)

Ok. Only mm is fine for what I have planned. I want to add a kmem_cache_ops structure for 2.6.23. Maybe I can use the now useless dtor field of kmem_cache_create for this?

#4 lore raw

Re: [patch 1/2] From: Paul Mundt <[email protected]>

In reply to Andrew Morton
Quoted 16 lines · Andrew Morton
Quoted 10 lines · On Fri, 11 May 2007 09:57:50 -0700 [email protected] wrote:
Quoted 4 lines

I'll take a look at tidying up the PMB slab, getting rid of the dtor shouldn't be terribly painful. I simply opted to do the list management there since others were doing it for the PGD slab cache at the time that was written.

And here's the bit for dropping pmb_cache_dtor(), moving the list management up to pmb_alloc() and pmb_free().

With this applied, we're all set for killing off slab destructors from the kernel entirely.

hm, this is already in Paul's git tree.

Quoted 8 lines

If we're going to slam all this into 2.6.22 then I can just tempdrop Paul's tree.

However I think we've done enough slab work for 2.6.22 now so I'm inclined to queue these changes for 2.6.23. That would mean that the slab changes in -mm have a dependency on the sh git tree which I am sure to forget about. If I end up merging these changes before Paul merges his tree, sh will break. Presumably Paul will notice this ;)

I can prune it from my tree if you'd rather just bundle these together, I wasn't sure what the timeline for these changes were, so I opted just to toss the PMB rework in my git tree ahead of time.

On the other hand, if Christoph's changes are going to be queued for 2.6.23, the PMB changes will trickle in well before then anyways.

maintainer
#5 lore raw

Re: [patch 1/2] From: Paul Mundt <[email protected]>

Quoted 26 lines · On Sat, 12 May 2007 10:33:00 +0900 Paul Mundt <[email protected]> wrote:
Quoted 16 lines · Andrew Morton
Quoted 10 lines · On Fri, 11 May 2007 09:57:50 -0700 [email protected] wrote:
Quoted 4 lines

I'll take a look at tidying up the PMB slab, getting rid of the dtor shouldn't be terribly painful. I simply opted to do the list management there since others were doing it for the PGD slab cache at the time that was written.

And here's the bit for dropping pmb_cache_dtor(), moving the list management up to pmb_alloc() and pmb_free().

With this applied, we're all set for killing off slab destructors from the kernel entirely.

hm, this is already in Paul's git tree.

Quoted 8 lines

If we're going to slam all this into 2.6.22 then I can just tempdrop Paul's tree.

However I think we've done enough slab work for 2.6.22 now so I'm inclined to queue these changes for 2.6.23. That would mean that the slab changes in -mm have a dependency on the sh git tree which I am sure to forget about. If I end up merging these changes before Paul merges his tree, sh will break. Presumably Paul will notice this ;)

Quoted 6 lines

I can prune it from my tree if you'd rather just bundle these together, I wasn't sure what the timeline for these changes were, so I opted just to toss the PMB rework in my git tree ahead of time.

On the other hand, if Christoph's changes are going to be queued for 2.6.23, the PMB changes will trickle in well before then anyways.

It looks like we'll be going the latter trickle-in way, thanks.

#6 lore raw

PATCH 2/2 Slab allocators: Drop support for destructors

In reply to [email protected]

There is no user of destructors left. There is no reason why we should keep checking for destructors calls in the slab allocators.

The RFC for this patch was discussed at
http://marc.info/?l=linux-kernel&m=117882364330705&w=2

Destructors were mainly used for list management which required them to take a spinlock. Taking a spinlock in a destructor is a bit risky since the slab allocators may run the destructors anytime they decide a slab is no longer needed.

Patch drops destructor support. Any attempt to use a destructor will BUG().

---
 include/linux/slub_def.h |    1 -
 mm/slab.c                |   27 ++-------------------------
 mm/slob.c                |    6 +-----
 mm/slub.c                |   46 +++++++++++++++-------------------------------
 4 files changed, 18 insertions(+), 62 deletions(-)
include/linux/slub_def.h +0 -1
Index: linux-2.6.21-mm2/include/linux/slub_def.h
===================================================================
@@ -40,7 +40,6 @@ struct kmem_cache {
 	int objects;		/* Number of objects in slab */
 	int refcount;		/* Refcount for slab cache destroy */
 	void (*ctor)(void *, struct kmem_cache *, unsigned long);
-	void (*dtor)(void *, struct kmem_cache *, unsigned long);
 	int inuse;		/* Offset to metadata */
 	int align;		/* Alignment */
 	const char *name;	/* Name (only for display!) */
mm/slab.c +2 -25
Index: linux-2.6.21-mm2/mm/slab.c
===================================================================
@@ -409,9 +409,6 @@ struct kmem_cache {
 	/* constructor func */
 	void (*ctor) (void *, struct kmem_cache *, unsigned long);
 
-	/* de-constructor func */
-	void (*dtor) (void *, struct kmem_cache *, unsigned long);
-
 /* 5) cache creation/removal */
 	const char *name;
 	struct list_head next;
@@ -1911,20 +1908,11 @@ static void slab_destroy_objs(struct kme
 				slab_error(cachep, "end of a freed object "
 					   "was overwritten");
 		}
-		if (cachep->dtor && !(cachep->flags & SLAB_POISON))
-			(cachep->dtor) (objp + obj_offset(cachep), cachep, 0);
 	}
 }
 #else
 static void slab_destroy_objs(struct kmem_cache *cachep, struct slab *slabp)
 {
-	if (cachep->dtor) {
-		int i;
-		for (i = 0; i < cachep->num; i++) {
-			void *objp = index_to_obj(cachep, slabp, i);
-			(cachep->dtor) (objp, cachep, 0);
-		}
-	}
 }
 #endif
 
@@ -2124,7 +2112,7 @@ static int setup_cpu_cache(struct kmem_c
  * @align: The required alignment for the objects.
  * @flags: SLAB flags
  * @ctor: A constructor for the objects.
- * @dtor: A destructor for the objects.
+ * @dtor: A destructor for the objects (not implemented anymore).
  *
  * Returns a ptr to the cache on success, NULL on failure.
  * Cannot be called within a int, but can be interrupted.
@@ -2159,7 +2147,7 @@ kmem_cache_create (const char *name, siz
 	 * Sanity checks... these are all serious usage bugs.
 	 */
 	if (!name || in_interrupt() || (size < BYTES_PER_WORD) ||
-	    (size > (1 << MAX_OBJ_ORDER) * PAGE_SIZE) || (dtor && !ctor)) {
+	    (size > (1 << MAX_OBJ_ORDER) * PAGE_SIZE) || dtor) {
 		printk(KERN_ERR "%s: Early error in slab %s\n", __FUNCTION__,
 				name);
 		BUG();
@@ -2213,9 +2201,6 @@ kmem_cache_create (const char *name, siz
 	if (flags & SLAB_DESTROY_BY_RCU)
 		BUG_ON(flags & SLAB_POISON);
 #endif
-	if (flags & SLAB_DESTROY_BY_RCU)
-		BUG_ON(dtor);
-
 	/*
 	 * Always checks flags, a caller might be expecting debug support which
 	 * isn't available.
@@ -2370,7 +2355,6 @@ kmem_cache_create (const char *name, siz
 		BUG_ON(!cachep->slabp_cache);
 	}
 	cachep->ctor = ctor;
-	cachep->dtor = dtor;
 	cachep->name = name;
 
 	if (setup_cpu_cache(cachep)) {
@@ -2835,7 +2819,6 @@ failed:
  * Perform extra freeing checks:
  * - detect bad pointers.
  * - POISON/RED_ZONE checking
- * - destructor calls, for caches with POISON+dtor
  */
 static void kfree_debugcheck(const void *objp)
 {
@@ -2894,12 +2877,6 @@ static void *cache_free_debugcheck(struc
 	BUG_ON(objnr >= cachep->num);
 	BUG_ON(objp != index_to_obj(cachep, slabp, objnr));
 
-	if (cachep->flags & SLAB_POISON && cachep->dtor) {
-		/* we want to cache poison the object,
-		 * call the destruction callback
-		 */
-		cachep->dtor(objp + obj_offset(cachep), cachep, 0);
-	}
 #ifdef CONFIG_DEBUG_SLAB_LEAK
 	slab_bufctl(slabp)[objnr] = BUFCTL_FREE;
 #endif
mm/slob.c +1 -5
Index: linux-2.6.21-mm2/mm/slob.c
===================================================================
@@ -268,7 +268,6 @@ struct kmem_cache {
 	unsigned int size, align;
 	const char *name;
 	void (*ctor)(void *, struct kmem_cache *, unsigned long);
-	void (*dtor)(void *, struct kmem_cache *, unsigned long);
 };
 
 struct kmem_cache *kmem_cache_create(const char *name, size_t size,
@@ -278,13 +277,13 @@ struct kmem_cache *kmem_cache_create(con
 {
 	struct kmem_cache *c;
 
+	BUG_ON(dtor);
 	c = slob_alloc(sizeof(struct kmem_cache), flags, 0);
 
 	if (c) {
 		c->name = name;
 		c->size = size;
 		c->ctor = ctor;
-		c->dtor = dtor;
 		/* ignore alignment unless it's forced */
 		c->align = (flags & SLAB_HWCACHE_ALIGN) ? SLOB_ALIGN : 0;
 		if (c->align < align)
@@ -330,9 +329,6 @@ EXPORT_SYMBOL(kmem_cache_zalloc);
 
 void kmem_cache_free(struct kmem_cache *c, void *b)
 {
-	if (c->dtor)
-		c->dtor(b, c, 0);
-
 	if (c->size < PAGE_SIZE)
 		slob_free(b, c->size);
 	else
mm/slub.c +15 -31

138 lines not on this page · raw message

Acked-by on v1 2/2
#7 lore raw

Re: [patch 2/2] Slab allocators: Drop support for destructors

Quoted 2 lines · [email protected] wrote:

There is no user of destructors left. There is no reason why we should keep checking for destructors calls in the slab allocators.

Looks good to me.

Acked-by on v1 2/2
#8 lore raw

Re: [patch 2/2] Slab allocators: Drop support for destructors

In reply to [email protected]
Quoted 11 lines · On Fri, May 11, 2007 at 09:57:51AM -0700, [email protected] wrote:

There is no user of destructors left. There is no reason why we should keep checking for destructors calls in the slab allocators.

The RFC for this patch was discussed at
http://marc.info/?l=linux-kernel&m=117882364330705&w=2

Destructors were mainly used for list management which required them to take a spinlock. Taking a spinlock in a destructor is a bit risky since the slab allocators may run the destructors anytime they decide a slab is no longer needed.

Quoted 6 lines

Patch drops destructor support. Any attempt to use a destructor will BUG().