If a slab cache is removed before we have setup sysfs then simply skip over
the sysfs handling.
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Roland Dreier <[email protected]>
Signed-off-by: Christoph Lameter <[email protected]>
---
mm/slub.c | 7 +++++++
1 file changed, 7 insertions(+)
Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c 2010-07-06 15:13:48.000000000 -0500
+++ linux-2.6/mm/slub.c 2010-07-06 15:15:27.000000000 -0500
@@ -4507,6 +4507,13 @@ static int sysfs_slab_add(struct kmem_ca
static void sysfs_slab_remove(struct kmem_cache *s)
{
+ if (slab_state < SYSFS)
+ /*
+ * Sysfs has not been setup yet so no need to remove the
+ * cache from sysfs.
+ */
+ return;
+
kobject_uevent(&s->kobj, KOBJ_REMOVE);
kobject_del(&s->kobj);
kobject_put(&s->kobj);
On Fri, 9 Jul 2010, Christoph Lameter wrote:
> If a slab cache is removed before we have setup sysfs then simply skip over
> the sysfs handling.
>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Roland Dreier <[email protected]>
> Signed-off-by: Christoph Lameter <[email protected]>
Acked-by: David Rientjes <[email protected]>
I missed this case earlier because I didn't consider slab caches being
created and destroyed prior to slab_state == SYSFS, sorry!
On Wed, 2010-07-14 at 16:48 -0700, David Rientjes wrote:
> On Fri, 9 Jul 2010, Christoph Lameter wrote:
>
> > If a slab cache is removed before we have setup sysfs then simply skip over
> > the sysfs handling.
> >
> > Cc: Benjamin Herrenschmidt <[email protected]>
> > Cc: Roland Dreier <[email protected]>
> > Signed-off-by: Christoph Lameter <[email protected]>
>
> Acked-by: David Rientjes <[email protected]>
>
> I missed this case earlier because I didn't consider slab caches being
> created and destroyed prior to slab_state == SYSFS, sorry!
Ok so I may be a bit sleepy or something but I still fail to see how
this whole thing isn't totally racy...
AFAIK. By the time we switch the slab state, we -do- have all CPUs up
and can race happily between creating slab caches and creating the sysfs
files...
Ben.
On Mon, 19 Jul 2010, Benjamin Herrenschmidt wrote:
> On Wed, 2010-07-14 at 16:48 -0700, David Rientjes wrote:
> > On Fri, 9 Jul 2010, Christoph Lameter wrote:
> >
> > > If a slab cache is removed before we have setup sysfs then simply skip over
> > > the sysfs handling.
> > >
> > > Cc: Benjamin Herrenschmidt <[email protected]>
> > > Cc: Roland Dreier <[email protected]>
> > > Signed-off-by: Christoph Lameter <[email protected]>
> >
> > Acked-by: David Rientjes <[email protected]>
> >
> > I missed this case earlier because I didn't consider slab caches being
> > created and destroyed prior to slab_state == SYSFS, sorry!
>
> Ok so I may be a bit sleepy or something but I still fail to see how
> this whole thing isn't totally racy...
>
> AFAIK. By the time we switch the slab state, we -do- have all CPUs up
> and can race happily between creating slab caches and creating the sysfs
> files...
If kmem_cache_init_late() is called after all other processors are up then
we need to serialize the activities. But we cannot do that since the
slub_lock is taken during kmalloc() for dynamic dma creation (lockdep
will complain although we never use dma caches for sysfs....).
After removal of dynamic dma creation we can take the lock for all of slab
creation and removal.
Like in the following patch:
Subject: slub: Allow removal of slab caches during boot
Serialize kmem_cache_create and kmem_cache_destroy using the slub_lock. Only
possible after the use of the slub_lock during dynamic dma creation has been
removed.
Then make sure that the setup of the slab sysfs entries does not race
with kmem_cache_create and kmem_cache destroy.
If a slab cache is removed before we have setup sysfs then simply skip over
the sysfs handling.
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Roland Dreier <[email protected]>
Signed-off-by: Christoph Lameter <[email protected]>
---
mm/slub.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c 2010-07-19 11:02:15.000000000 -0500
+++ linux-2.6/mm/slub.c 2010-07-19 11:33:32.000000000 -0500
@@ -2490,7 +2490,6 @@ void kmem_cache_destroy(struct kmem_cach
s->refcount--;
if (!s->refcount) {
list_del(&s->list);
- up_write(&slub_lock);
if (kmem_cache_close(s)) {
printk(KERN_ERR "SLUB %s: %s called for cache that "
"still has objects.\n", s->name, __func__);
@@ -2499,8 +2498,8 @@ void kmem_cache_destroy(struct kmem_cach
if (s->flags & SLAB_DESTROY_BY_RCU)
rcu_barrier();
sysfs_slab_remove(s);
- } else
- up_write(&slub_lock);
+ }
+ up_write(&slub_lock);
}
EXPORT_SYMBOL(kmem_cache_destroy);
@@ -3226,14 +3225,12 @@ struct kmem_cache *kmem_cache_create(con
*/
s->objsize = max(s->objsize, (int)size);
s->inuse = max_t(int, s->inuse, ALIGN(size, sizeof(void *)));
- up_write(&slub_lock);
if (sysfs_slab_alias(s, name)) {
- down_write(&slub_lock);
s->refcount--;
- up_write(&slub_lock);
goto err;
}
+ up_write(&slub_lock);
return s;
}
@@ -3242,14 +3239,12 @@ struct kmem_cache *kmem_cache_create(con
if (kmem_cache_open(s, GFP_KERNEL, name,
size, align, flags, ctor)) {
list_add(&s->list, &slab_caches);
- up_write(&slub_lock);
if (sysfs_slab_add(s)) {
- down_write(&slub_lock);
list_del(&s->list);
- up_write(&slub_lock);
kfree(s);
goto err;
}
+ up_write(&slub_lock);
return s;
}
kfree(s);
@@ -4507,6 +4502,13 @@ static int sysfs_slab_add(struct kmem_ca
static void sysfs_slab_remove(struct kmem_cache *s)
{
+ if (slab_state < SYSFS)
+ /*
+ * Sysfs has not been setup yet so no need to remove the
+ * cache from sysfs.
+ */
+ return;
+
kobject_uevent(&s->kobj, KOBJ_REMOVE);
kobject_del(&s->kobj);
kobject_put(&s->kobj);
@@ -4552,8 +4554,11 @@ static int __init slab_sysfs_init(void)
struct kmem_cache *s;
int err;
+ down_write(&slub_lock);
+
slab_kset = kset_create_and_add("slab", &slab_uevent_ops, kernel_kobj);
if (!slab_kset) {
+ up_write(&slub_lock);
printk(KERN_ERR "Cannot register slab subsystem.\n");
return -ENOSYS;
}
@@ -4578,6 +4583,7 @@ static int __init slab_sysfs_init(void)
kfree(al);
}
+ up_write(&slub_lock);
resiliency_test();
return 0;
}
Christoph Lameter wrote:
>> Ok so I may be a bit sleepy or something but I still fail to see how
>> this whole thing isn't totally racy...
>>
>> AFAIK. By the time we switch the slab state, we -do- have all CPUs up
>> and can race happily between creating slab caches and creating the sysfs
>> files...
>
> If kmem_cache_init_late() is called after all other processors are up then
> we need to serialize the activities. But we cannot do that since the
> slub_lock is taken during kmalloc() for dynamic dma creation (lockdep
> will complain although we never use dma caches for sysfs....).
>
> After removal of dynamic dma creation we can take the lock for all of slab
> creation and removal.
>
> Like in the following patch:
>
> Subject: slub: Allow removal of slab caches during boot
>
> Serialize kmem_cache_create and kmem_cache_destroy using the slub_lock. Only
> possible after the use of the slub_lock during dynamic dma creation has been
> removed.
>
> Then make sure that the setup of the slab sysfs entries does not race
> with kmem_cache_create and kmem_cache destroy.
>
> If a slab cache is removed before we have setup sysfs then simply skip over
> the sysfs handling.
>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Roland Dreier <[email protected]>
> Signed-off-by: Christoph Lameter <[email protected]>
Christoph, Ben, should I queue this up for 2.6.36?
>
> ---
> mm/slub.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> Index: linux-2.6/mm/slub.c
> ===================================================================
> --- linux-2.6.orig/mm/slub.c 2010-07-19 11:02:15.000000000 -0500
> +++ linux-2.6/mm/slub.c 2010-07-19 11:33:32.000000000 -0500
> @@ -2490,7 +2490,6 @@ void kmem_cache_destroy(struct kmem_cach
> s->refcount--;
> if (!s->refcount) {
> list_del(&s->list);
> - up_write(&slub_lock);
> if (kmem_cache_close(s)) {
> printk(KERN_ERR "SLUB %s: %s called for cache that "
> "still has objects.\n", s->name, __func__);
> @@ -2499,8 +2498,8 @@ void kmem_cache_destroy(struct kmem_cach
> if (s->flags & SLAB_DESTROY_BY_RCU)
> rcu_barrier();
> sysfs_slab_remove(s);
> - } else
> - up_write(&slub_lock);
> + }
> + up_write(&slub_lock);
> }
> EXPORT_SYMBOL(kmem_cache_destroy);
>
> @@ -3226,14 +3225,12 @@ struct kmem_cache *kmem_cache_create(con
> */
> s->objsize = max(s->objsize, (int)size);
> s->inuse = max_t(int, s->inuse, ALIGN(size, sizeof(void *)));
> - up_write(&slub_lock);
>
> if (sysfs_slab_alias(s, name)) {
> - down_write(&slub_lock);
> s->refcount--;
> - up_write(&slub_lock);
> goto err;
> }
> + up_write(&slub_lock);
> return s;
> }
>
> @@ -3242,14 +3239,12 @@ struct kmem_cache *kmem_cache_create(con
> if (kmem_cache_open(s, GFP_KERNEL, name,
> size, align, flags, ctor)) {
> list_add(&s->list, &slab_caches);
> - up_write(&slub_lock);
> if (sysfs_slab_add(s)) {
> - down_write(&slub_lock);
> list_del(&s->list);
> - up_write(&slub_lock);
> kfree(s);
> goto err;
> }
> + up_write(&slub_lock);
> return s;
> }
> kfree(s);
> @@ -4507,6 +4502,13 @@ static int sysfs_slab_add(struct kmem_ca
>
> static void sysfs_slab_remove(struct kmem_cache *s)
> {
> + if (slab_state < SYSFS)
> + /*
> + * Sysfs has not been setup yet so no need to remove the
> + * cache from sysfs.
> + */
> + return;
> +
> kobject_uevent(&s->kobj, KOBJ_REMOVE);
> kobject_del(&s->kobj);
> kobject_put(&s->kobj);
> @@ -4552,8 +4554,11 @@ static int __init slab_sysfs_init(void)
> struct kmem_cache *s;
> int err;
>
> + down_write(&slub_lock);
> +
> slab_kset = kset_create_and_add("slab", &slab_uevent_ops, kernel_kobj);
> if (!slab_kset) {
> + up_write(&slub_lock);
> printk(KERN_ERR "Cannot register slab subsystem.\n");
> return -ENOSYS;
> }
> @@ -4578,6 +4583,7 @@ static int __init slab_sysfs_init(void)
> kfree(al);
> }
>
> + up_write(&slub_lock);
> resiliency_test();
> return 0;
> }
>
>
On Sat, 31 Jul 2010, Pekka Enberg wrote:
> Christoph, Ben, should I queue this up for 2.6.36?
Yes.
Christoph Lameter wrote:
> On Sat, 31 Jul 2010, Pekka Enberg wrote:
>
>> Christoph, Ben, should I queue this up for 2.6.36?
>
> Yes.
Applied, thanks!