Fixes ancient slab bug
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 31 May 2011 17:38:44 +0000 (10:38 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:36:03 +0000 (17:36 -0700)
We shouldn't have been locking at all in kmem_cache_grow().  I made it
internal, since no outsiders should be calling it either.  Had we
properly locked, we would have deadlocked and noticed this when it was
written, 2.5 years ago.

Fun fact: I found this in a storm of bugs involving massive kthreading.
It looked like kmsgs (from a slab) were getting misrouted, and multiple
cores were clobbering each others kmsgs (including the kthread*
payload)...

kern/include/slab.h
kern/src/slab.c

index 7e094b4..7e5189c 100644 (file)
@@ -98,7 +98,6 @@ void *kmem_cache_alloc(struct kmem_cache *cp, int flags);
 void kmem_cache_free(struct kmem_cache *cp, void *buf);
 /* Back end: internal functions */
 void kmem_cache_init(void);
-void kmem_cache_grow(struct kmem_cache *cp);
 void kmem_cache_reap(struct kmem_cache *cp);
 
 /* Debug */
index 855c5a0..ac1b8de 100644 (file)
 struct kmem_cache_list kmem_caches;
 spinlock_t kmem_caches_lock;
 
+/* Backend/internal functions, defined later.  Grab the lock before calling
+ * these. */
+static void kmem_cache_grow(struct kmem_cache *cp);
+
 /* Cache of the kmem_cache objects, needed for bootstrapping */
 struct kmem_cache kmem_cache_cache;
 struct kmem_cache *kmem_slab_cache, *kmem_bufctl_cache;
@@ -240,11 +244,10 @@ void kmem_cache_free(struct kmem_cache *cp, void *buf)
  * Grab the cache lock before calling this.
  *
  * TODO: think about page colouring issues with kernel memory allocation. */
-void kmem_cache_grow(struct kmem_cache *cp)
+static void kmem_cache_grow(struct kmem_cache *cp)
 {
        struct kmem_slab *a_slab;
        struct kmem_bufctl *a_bufctl;
-       spin_unlock_irqsave(&cp->cache_lock);
        if (cp->obj_size <= SLAB_LARGE_CUTOFF) {
                // Just get a single page for small slabs
                page_t *a_page;
@@ -301,7 +304,6 @@ void kmem_cache_grow(struct kmem_cache *cp)
        }
        // add a_slab to the empty_list
        TAILQ_INSERT_HEAD(&cp->empty_slab_list, a_slab, link);
-       spin_unlock_irqsave(&cp->cache_lock);
 }
 
 /* This deallocs every slab from the empty list.  TODO: think a bit more about