[  418.312449] WARNING: CPU: 6 PID: 4178 at lib/debugobjects.c:260 debug_print_object+0x8d/0xb0()
[  418.313243] ODEBUG: free active (active state 0) object type: timer_list hint:

delayed_work_timer_fn+0x0/0x20

[  418.321101]  [<ffffffff812874d7>] kmem_cache_free+0x197/0x340
[  418.321101]  [<ffffffff81249e76>] kmem_cache_destroy+0x86/0xe0
[  418.321101]  [<ffffffff83d5d681>] nf_conntrack_cleanup_net_list+0x131/0x170

The debug code detects an active timer, which itself is part of a delayed work struct. The call comes from kmem_cache_destroy().

         kmem_cache_free(kmem_cache, s);

So debug object says: s contains an active timer. s is the kmem_cache which is destroyed from nf_conntrack_cleanup_net_list.

Now struct kmem_cache has in case of SLUB:

struct kobject kobj; /* For sysfs */

and struct kobject has:

#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
    struct delayed_work     release;

#endif

So this is the thing you want to look at:

commit c817a67ec (kobject: delayed kobject release: help find buggy drivers) added that delayed work thing.

I fear that does not work for kobjects which are embedded into something else.

Handing off to rmk, mm and kobject folks.

Thanks,

tglx

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Russell King - ARM Linux

2013-11-27 11:39 UTC #5 lore raw

Quoted 35 lines · Thomas Gleixner

Quoted 4 lines · Pablo Neira Ayuso
Quoted 1 line · Sasha Levin
Ping? I still see this warning.
Did your test include patch 0c3c6c00c6?
And how is that patch supposed to help?
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
[  418.312449] WARNING: CPU: 6 PID: 4178 at lib/debugobjects.c:260 debug_print_object+0x8d/0xb0()
[  418.313243] ODEBUG: free active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x20
[  418.321101]  [<ffffffff812874d7>] kmem_cache_free+0x197/0x340
[  418.321101]  [<ffffffff81249e76>] kmem_cache_destroy+0x86/0xe0
[  418.321101]  [<ffffffff83d5d681>] nf_conntrack_cleanup_net_list+0x131/0x170
The debug code detects an active timer, which itself is part of a delayed work struct. The call comes from kmem_cache_destroy().
         kmem_cache_free(kmem_cache, s);
So debug object says: s contains an active timer. s is the kmem_cache which is destroyed from nf_conntrack_cleanup_net_list.
Now struct kmem_cache has in case of SLUB:
struct kobject kobj; /* For sysfs */
and struct kobject has:
#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
    struct delayed_work     release;
#endif

Quoted 7 lines

So this is the thing you want to look at:
commit c817a67ec (kobject: delayed kobject release: help find buggy drivers) added that delayed work thing.
I fear that does not work for kobjects which are embedded into something else.

No, kobjects embedded into something else have their lifetime determined by the embedded kobject. That's rule #1 of kobjects - or rather reference counted objects.

The point at which the kobject gets destructed is when the release function is called. If it is destructed before that time, that's a violation of the reference counted nature of kobjects, and that's what the delay on releasing is designed to catch.

It's designed to catch code which does this exact path:

put(obj)
free(obj)

rather than code which does it the right way:

	put(obj)
		-> refcount becomes 0
			-> release function gets called
				->free(obj)

The former is unsafe because obj may have other references.

Thomas Gleixner

2013-11-27 13:29 UTC #6 lore raw

Quoted 58 lines · Russell King - ARM Linux

Quoted 35 lines · Thomas Gleixner
Quoted 4 lines · Pablo Neira Ayuso
Quoted 1 line · Sasha Levin
Ping? I still see this warning.
Did your test include patch 0c3c6c00c6?
And how is that patch supposed to help?
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
[  418.312449] WARNING: CPU: 6 PID: 4178 at lib/debugobjects.c:260 debug_print_object+0x8d/0xb0()
[  418.313243] ODEBUG: free active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x20
[  418.321101]  [<ffffffff812874d7>] kmem_cache_free+0x197/0x340
[  418.321101]  [<ffffffff81249e76>] kmem_cache_destroy+0x86/0xe0
[  418.321101]  [<ffffffff83d5d681>] nf_conntrack_cleanup_net_list+0x131/0x170
The debug code detects an active timer, which itself is part of a delayed work struct. The call comes from kmem_cache_destroy().
         kmem_cache_free(kmem_cache, s);
So debug object says: s contains an active timer. s is the kmem_cache which is destroyed from nf_conntrack_cleanup_net_list.
Now struct kmem_cache has in case of SLUB:
struct kobject kobj; /* For sysfs */
and struct kobject has:
#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
    struct delayed_work     release;
#endif
Quoted 7 lines
So this is the thing you want to look at:
commit c817a67ec (kobject: delayed kobject release: help find buggy drivers) added that delayed work thing.
I fear that does not work for kobjects which are embedded into something else.
No, kobjects embedded into something else have their lifetime determined by the embedded kobject. That's rule #1 of kobjects - or rather reference counted objects.
The point at which the kobject gets destructed is when the release function is called. If it is destructed before that time, that's a violation of the reference counted nature of kobjects, and that's what the delay on releasing is designed to catch.
It's designed to catch code which does this exact path:
put(obj)
free(obj)

Quoted 8 lines

rather than code which does it the right way:
	put(obj)
		-> refcount becomes 0
			-> release function gets called
				->free(obj)
The former is unsafe because obj may have other references.

Though the kobject is the only thing which has a delayed work embedded inside struct kmem_cache. And the debug object splat points at the kmem_cache_free() of the struct kmem_cache itself. That's why I assumed the wreckage around that place. And indeed:

kmem_cache_destroy(s)

    __kmem_cache_shutdown(s)
      sysfs_slab_remove(s)
        ....
	kobject_put(&s->kobj)
           kref_put(&kobj->kref, kobject_release);
	     kobject_release(kref)
    	       #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
	         schedule_delayed_work(&kobj->release)
	       #else
	        kobject_cleanup(kobj)
	       #endif

So in the CONFIG_DEBUG_KOBJECT_RELEASE=y case, schedule_delayed_work() _IS_ called which arms the timer. debugobjects catches the attempt to free struct kmem_cache which contains the armed timer.

So much for rule #1

Thanks,

tglx

Russell King - ARM Linux

2013-11-27 13:32 UTC #7 lore raw

Quoted 18 lines · Thomas Gleixner

Though the kobject is the only thing which has a delayed work embedded inside struct kmem_cache. And the debug object splat points at the kmem_cache_free() of the struct kmem_cache itself. That's why I assumed the wreckage around that place. And indeed:
kmem_cache_destroy(s)
    __kmem_cache_shutdown(s)
      sysfs_slab_remove(s)
        ....
	kobject_put(&s->kobj)
           kref_put(&kobj->kref, kobject_release);
	     kobject_release(kref)
    	       #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
	         schedule_delayed_work(&kobj->release)
	       #else
	        kobject_cleanup(kobj)
	       #endif

Quoted 3 lines

So in the CONFIG_DEBUG_KOBJECT_RELEASE=y case, schedule_delayed_work() _IS_ called which arms the timer. debugobjects catches the attempt to free struct kmem_cache which contains the armed timer.

You fail to show where the free is in the above path.

Russell King - ARM Linux

2013-11-27 13:40 UTC #8 lore raw

Quoted 19 lines · Russell King - ARM Linux

Quoted 18 lines · Thomas Gleixner
Though the kobject is the only thing which has a delayed work embedded inside struct kmem_cache. And the debug object splat points at the kmem_cache_free() of the struct kmem_cache itself. That's why I assumed the wreckage around that place. And indeed:
kmem_cache_destroy(s)
    __kmem_cache_shutdown(s)
      sysfs_slab_remove(s)
        ....
	kobject_put(&s->kobj)
           kref_put(&kobj->kref, kobject_release);
	     kobject_release(kref)
    	       #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
	         schedule_delayed_work(&kobj->release)
	       #else
	        kobject_cleanup(kobj)
	       #endif

Quoted 5 lines

Quoted 3 lines
So in the CONFIG_DEBUG_KOBJECT_RELEASE=y case, schedule_delayed_work() _IS_ called which arms the timer. debugobjects catches the attempt to free struct kmem_cache which contains the armed timer.
You fail to show where the free is in the above path.

Right, there's a kmem_cache_free(kmem_cache, s); by the slob code after the above sequence, which is The Bug(tm).

As I said, a kobject has its own lifetime. If you embed that into another structure, that structure inherits the lifetime of the kobject, which is from the point at which it's created to the point at which the kobject's release function is called.

So no, the code here is buggy. The kobject debugging has yet again found a violation of the kobject lifetime rules. slub needs fixing.

Thomas Gleixner

2013-11-27 13:44 UTC #9 lore raw

Quoted 29 lines · Russell King - ARM Linux

Quoted 19 lines · Russell King - ARM Linux
Quoted 18 lines · Thomas Gleixner
Though the kobject is the only thing which has a delayed work embedded inside struct kmem_cache. And the debug object splat points at the kmem_cache_free() of the struct kmem_cache itself. That's why I assumed the wreckage around that place. And indeed:
kmem_cache_destroy(s)
    __kmem_cache_shutdown(s)
      sysfs_slab_remove(s)
        ....
	kobject_put(&s->kobj)
           kref_put(&kobj->kref, kobject_release);
	     kobject_release(kref)
    	       #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
	         schedule_delayed_work(&kobj->release)
	       #else
	        kobject_cleanup(kobj)
	       #endif
Quoted 5 lines
Quoted 3 lines
So in the CONFIG_DEBUG_KOBJECT_RELEASE=y case, schedule_delayed_work() _IS_ called which arms the timer. debugobjects catches the attempt to free struct kmem_cache which contains the armed timer.
You fail to show where the free is in the above path.
Right, there's a kmem_cache_free(kmem_cache, s); by the slob code after the above sequence, which is The Bug(tm).

Quoted 7 lines

As I said, a kobject has its own lifetime. If you embed that into another structure, that structure inherits the lifetime of the kobject, which is from the point at which it's created to the point at which the kobject's release function is called.
So no, the code here is buggy. The kobject debugging has yet again found a violation of the kobject lifetime rules. slub needs fixing.

I leave that discussion to you, greg and the slub folks.

/me prepares deck chair, drinks and popcorn

Greg KH

2013-11-27 23:34 UTC #10 lore raw

Quoted 36 lines · Thomas Gleixner

Quoted 34 lines · Russell King - ARM Linux
Quoted 19 lines · Russell King - ARM Linux
Quoted 18 lines · Thomas Gleixner
Though the kobject is the only thing which has a delayed work embedded inside struct kmem_cache. And the debug object splat points at the kmem_cache_free() of the struct kmem_cache itself. That's why I assumed the wreckage around that place. And indeed:
kmem_cache_destroy(s)
    __kmem_cache_shutdown(s)
      sysfs_slab_remove(s)
        ....
	kobject_put(&s->kobj)
           kref_put(&kobj->kref, kobject_release);
	     kobject_release(kref)
    	       #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
	         schedule_delayed_work(&kobj->release)
	       #else
	        kobject_cleanup(kobj)
	       #endif
Quoted 5 lines
Quoted 3 lines
So in the CONFIG_DEBUG_KOBJECT_RELEASE=y case, schedule_delayed_work() _IS_ called which arms the timer. debugobjects catches the attempt to free struct kmem_cache which contains the armed timer.
You fail to show where the free is in the above path.
Right, there's a kmem_cache_free(kmem_cache, s); by the slob code after the above sequence, which is The Bug(tm).
As I said, a kobject has its own lifetime. If you embed that into another structure, that structure inherits the lifetime of the kobject, which is from the point at which it's created to the point at which the kobject's release function is called.

Quoted 4 lines

Quoted 2 lines
So no, the code here is buggy. The kobject debugging has yet again found a violation of the kobject lifetime rules. slub needs fixing.
I leave that discussion to you, greg and the slub folks.

/me grabs some popcorn from tglx

It's really not that much of a discussion, Documentation/kobject.txt has said this for years, it's as if no one even reads documentation anymore...

If you embed a kobject into a structure, you have to use the kobject for the reference counting of the structure, otherwise it's a bug. If you don't want to use a kobject to reference count the structure, don't embed it into it, use a pointer.

Are "slabs" never freed in the slub allocator? Surely someone should have seen the huge "this kobject doesn't have a release function" error message that the kernel should have spit out for it?

Just make the kobject "dynamic" instead of embedded in struct kmem_cache and all will be fine. I can't believe this code has been broken for this long.

thanks,

greg k-h

Christoph Lameter

2013-12-02 16:33 UTC #11 lore raw

Quoted 3 lines · Greg KH

Just make the kobject "dynamic" instead of embedded in struct kmem_cache and all will be fine. I can't believe this code has been broken for this long.

The slub code is was designed to use an embedded structure since we only get the kobj pointer passed to us from sysfs. If kobj is not embedded then how can we get from the sysfs object to the kmem_cache structure from the sysfs callbacks? Sysfs was designed to have embedded objects as far as I can recall.

Greg KH

2013-12-02 16:40 UTC #12 lore raw

Quoted 11 lines · Christoph Lameter

Quoted 3 lines · Greg KH
Just make the kobject "dynamic" instead of embedded in struct kmem_cache and all will be fine. I can't believe this code has been broken for this long.
The slub code is was designed to use an embedded structure since we only get the kobj pointer passed to us from sysfs. If kobj is not embedded then how can we get from the sysfs object to the kmem_cache structure from the sysfs callbacks? Sysfs was designed to have embedded objects as far as I can recall.

Yes, it's designed to have embedded objects, so then use it that way and clean up the structure when the kobject goes away. Don't use a different reference count for your structure than the one in the kobject and think that all will be fine.

greg k-h

Christoph Lameter

2013-12-02 17:18 UTC #13 lore raw

Quoted 13 lines · Greg KH

Quoted 11 lines · Christoph Lameter
Quoted 3 lines · Greg KH
Just make the kobject "dynamic" instead of embedded in struct kmem_cache and all will be fine. I can't believe this code has been broken for this long.
The slub code is was designed to use an embedded structure since we only get the kobj pointer passed to us from sysfs. If kobj is not embedded then how can we get from the sysfs object to the kmem_cache structure from the sysfs callbacks? Sysfs was designed to have embedded objects as far as I can recall.

Quoted 4 lines

Yes, it's designed to have embedded objects, so then use it that way and clean up the structure when the kobject goes away. Don't use a different reference count for your structure than the one in the kobject and think that all will be fine.

We need our own reference count. So we just have to defer the release of the kmem_cache struct until the ->release callback is triggered. The put of the embedded kobject must be the last action on the kmem_cache structure which will then trigger release and that will trigger the kmem_cache_free().

Greg KH

2013-12-02 17:26 UTC #14 lore raw

Quoted 20 lines · Christoph Lameter

Quoted 13 lines · Greg KH
Quoted 11 lines · Christoph Lameter
Quoted 3 lines · Greg KH
Just make the kobject "dynamic" instead of embedded in struct kmem_cache and all will be fine. I can't believe this code has been broken for this long.
The slub code is was designed to use an embedded structure since we only get the kobj pointer passed to us from sysfs. If kobj is not embedded then how can we get from the sysfs object to the kmem_cache structure from the sysfs callbacks? Sysfs was designed to have embedded objects as far as I can recall.
Quoted 4 lines
Yes, it's designed to have embedded objects, so then use it that way and clean up the structure when the kobject goes away. Don't use a different reference count for your structure than the one in the kobject and think that all will be fine.

Quoted 6 lines

We need our own reference count. So we just have to defer the release of the kmem_cache struct until the ->release callback is triggered. The put of the embedded kobject must be the last action on the kmem_cache structure which will then trigger release and that will trigger the kmem_cache_free().

Ok, that sounds reasonable, or you can just create a "tiny" structure for the kobject that has a pointer back to your kmem_cache structure that you can then reference from the show/store functions. Either is fine with me.

thanks,

greg k-h

Christoph Lameter

2013-12-02 19:00 UTC #15 lore raw

Quoted 8 lines · Greg KH

Quoted 7 lines
We need our own reference count. So we just have to defer the release of the kmem_cache struct until the ->release callback is triggered. The put of the embedded kobject must be the last action on the kmem_cache structure which will then trigger release and that will trigger the kmem_cache_free().

Quoted 4 lines

Ok, that sounds reasonable, or you can just create a "tiny" structure for the kobject that has a pointer back to your kmem_cache structure that you can then reference from the show/store functions. Either is fine with me.

Problem is that the release field is only available if CONFIG_DEBUG_KOBJECT_RELEASE is enabled. Without the callback I cannot tell when it is legit to release the kobject structure unless I keep scanning it once in awhile.

Greg KH

2013-12-02 19:08 UTC #16 lore raw

Quoted 15 lines · Christoph Lameter

Quoted 8 lines · Greg KH
Quoted 7 lines
We need our own reference count. So we just have to defer the release of the kmem_cache struct until the ->release callback is triggered. The put of the embedded kobject must be the last action on the kmem_cache structure which will then trigger release and that will trigger the kmem_cache_free().
Quoted 4 lines
Ok, that sounds reasonable, or you can just create a "tiny" structure for the kobject that has a pointer back to your kmem_cache structure that you can then reference from the show/store functions. Either is fine with me.

Quoted 4 lines

Problem is that the release field is only available if CONFIG_DEBUG_KOBJECT_RELEASE is enabled. Without the callback I cannot tell when it is legit to release the kobject structure unless I keep scanning it once in awhile.

No, the release callback is in the kobj_type, not the kobject itself.

Christoph Lameter

2013-12-02 19:41 UTC #17 lore raw

Quoted 1 line · Greg KH

No, the release callback is in the kobj_type, not the kobject itself.

Ahh... Ok. Patch follows:

Subject: slub: use sysfs'es release mechanism for kmem_cache

Sysfs has a release mechanism. Use that to release the kmem_cache structure if CONFIG_SYSFS is enabled.

Signed-off-by Christoph Lameter <[email protected]>

include/linux/slub_def.h +4 -0

Index: linux/include/linux/slub_def.h
===================================================================
@@ -98,4 +98,8 @@ struct kmem_cache {
 	struct kmem_cache_node *node[MAX_NUMNODES];
 };
 
+#ifdef CONFIG_SYSFS
+#define SLAB_SUPPORTS_SYSFS
+#endif
+
 #endif /* _LINUX_SLUB_DEF_H */

mm/slab.h +2 -0

Index: linux/mm/slab.h
===================================================================
@@ -57,6 +57,7 @@ struct mem_cgroup;
 struct kmem_cache *
 __kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
 		   size_t align, unsigned long flags, void (*ctor)(void *));
+void sysfs_slab_remove(struct kmem_cache *);
 #else
 static inline struct kmem_cache *
 __kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,

@@ -91,6 +92,7 @@ __kmem_cache_alias(struct mem_cgroup *me
 #define CACHE_CREATE_MASK (SLAB_CORE_FLAGS | SLAB_DEBUG_FLAGS | SLAB_CACHE_FLAGS)
 
 int __kmem_cache_shutdown(struct kmem_cache *);
+void slab_kmem_cache_release(struct kmem_cache *);
 
 struct seq_file;
 struct file;

mm/slab_common.c +12 -2

Index: linux/mm/slab_common.c
===================================================================
@@ -251,6 +251,12 @@ kmem_cache_create(const char *name, size
 }
 EXPORT_SYMBOL(kmem_cache_create);
 
+void slab_kmem_cache_release(struct kmem_cache *s)
+{
+		kfree(s->name);
+		kmem_cache_free(kmem_cache, s);
+}
+
 void kmem_cache_destroy(struct kmem_cache *s)
 {
 	/* Destroy all the children caches if we aren't a memcg cache */

@@ -268,8 +274,12 @@ void kmem_cache_destroy(struct kmem_cach
 				rcu_barrier();
 
 			memcg_release_cache(s);
-			kfree(s->name);
-			kmem_cache_free(kmem_cache, s);
+#ifdef SLAB_SUPPORTS_SYSFS
+			sysfs_slab_remove(s);
+#else
+			slab_kmem_cache_release();
+
+#endif
 		} else {
 			list_add(&s->list, &slab_caches);
 			mutex_unlock(&slab_mutex);

mm/slub.c +8 ~~-19~~

Index: linux/mm/slub.c
===================================================================
@@ -210,7 +210,6 @@ enum track_item { TRACK_ALLOC, TRACK_FRE
 #ifdef CONFIG_SYSFS
 static int sysfs_slab_add(struct kmem_cache *);
 static int sysfs_slab_alias(struct kmem_cache *, const char *);
-static void sysfs_slab_remove(struct kmem_cache *);
 static void memcg_propagate_slab_attrs(struct kmem_cache *s);
 #else
 static inline int sysfs_slab_add(struct kmem_cache *s) { return 0; }

@@ -3208,23 +3207,7 @@ static inline int kmem_cache_close(struc
 
 int __kmem_cache_shutdown(struct kmem_cache *s)
 {
-	int rc = kmem_cache_close(s);
-
-	if (!rc) {
-		/*
-		 * We do the same lock strategy around sysfs_slab_add, see
-		 * __kmem_cache_create. Because this is pretty much the last
-		 * operation we do and the lock will be released shortly after
-		 * that in slab_common.c, we could just move sysfs_slab_remove
-		 * to a later point in common code. We should do that when we
-		 * have a common sysfs framework for all allocators.
-		 */
-		mutex_unlock(&slab_mutex);
-		sysfs_slab_remove(s);
-		mutex_lock(&slab_mutex);
-	}
-
-	return rc;
+	return kmem_cache_close(s);
 }
 
 /********************************************************************

@@ -5073,6 +5056,11 @@ static void memcg_propagate_slab_attrs(s
 #endif
 }
 
+static void kmem_cache_release(struct kobject *k)
+{
+	slab_kmem_cache_release(to_slab(k));
+}
+
 static const struct sysfs_ops slab_sysfs_ops = {
 	.show = slab_attr_show,
 	.store = slab_attr_store,

@@ -5080,6 +5068,7 @@ static const struct sysfs_ops slab_sysfs
 
 static struct kobj_type slab_ktype = {
 	.sysfs_ops = &slab_sysfs_ops,
+	.release = kmem_cache_release,
 };
 
 static int uevent_filter(struct kset *kset, struct kobject *kobj)

@@ -5184,7 +5173,7 @@ static int sysfs_slab_add(struct kmem_ca
 	return 0;
 }
 
-static void sysfs_slab_remove(struct kmem_cache *s)
+void sysfs_slab_remove(struct kmem_cache *s)
 {
 	if (slab_state < FULL)
 		/*

Greg KH

2013-12-02 21:22 UTC #18 lore raw

Quoted 7 lines · Christoph Lameter

Quoted 1 line · Greg KH
No, the release callback is in the kobj_type, not the kobject itself.
Ahh... Ok. Patch follows:

Quoted 6 lines

Subject: slub: use sysfs'es release mechanism for kmem_cache
Sysfs has a release mechanism. Use that to release the kmem_cache structure if CONFIG_SYSFS is enabled.
Signed-off-by Christoph Lameter <[email protected]>

That looks good, if you fix the indentation issue :)

include/linux/slub_def.h struct kmem_cache { · line 102

Quoted 11 lines

Index: linux/include/linux/slub_def.h
===================================================================
include/linux/slub_def.h +2 -0
@@ -98,4 +98,8 @@ struct kmem_cache {
 	struct kmem_cache_node *node[MAX_NUMNODES];
 };
 
+#ifdef CONFIG_SYSFS
+#define SLAB_SUPPORTS_SYSFS

Why even define this? Why not just use CONFIG_SYSFS?

thanks,

greg k-h

Christoph Lameter

2013-12-02 21:55 UTC #19 lore raw

Quoted 3 lines · Greg KH

Quoted 1 line
Signed-off-by Christoph Lameter <[email protected]>
That looks good, if you fix the indentation issue :)

Huh?

Quoted 8 lines

Quoted 8 lines
Index: linux/include/linux/slub_def.h
===================================================================
include/linux/slub_def.h +0 -0
@@ -98,4 +98,8 @@ struct kmem_cache {
 	struct kmem_cache_node *node[MAX_NUMNODES];
 };
 

Quoted 4 lines

include/linux/slub_def.h struct kmem_cache { · line 102
Quoted 2 lines
include/linux/slub_def.h +2 -0
+#ifdef CONFIG_SYSFS
+#define SLAB_SUPPORTS_SYSFS
Why even define this? Why not just use CONFIG_SYSFS?

Because not all slab allocators currently support SYSFS and there is the need to have different code now in slab_common.c depending on the configuration of the allocator.

Greg KH

2013-12-02 22:22 UTC #20 lore raw

Quoted 7 lines · Christoph Lameter

Quoted 3 lines · Greg KH
Quoted 1 line
Signed-off-by Christoph Lameter <[email protected]>
That looks good, if you fix the indentation issue :)
Huh?

Your release function had 2 tabs for the lines, not one.

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Index: linux/include/linux/slub_def.h
===================================================================
include/linux/slub_def.h +0 -0
@@ -98,4 +98,8 @@ struct kmem_cache {
 	struct kmem_cache_node *node[MAX_NUMNODES];
 };
 

Quoted 8 lines

Quoted 4 lines
include/linux/slub_def.h struct kmem_cache { · line 102
Quoted 2 lines
include/linux/slub_def.h +2 -0
+#ifdef CONFIG_SYSFS
+#define SLAB_SUPPORTS_SYSFS
Why even define this? Why not just use CONFIG_SYSFS?
Because not all slab allocators currently support SYSFS and there is the need to have different code now in slab_common.c depending on the configuration of the allocator.

But you are defining something that you only ever check once, why not just use CONFIG_SYSFS instead as it makes more sense, not the other way around.

thanks,

greg k-h

Christoph Lameter

2013-12-03 15:22 UTC #21 lore raw

Quoted 1 line · Greg KH

Your release function had 2 tabs for the lines, not one.

Ah ok. Fixed.

Quoted 13 lines

Quoted 13 lines
Quoted 12 lines
include/linux/slub_def.h struct kmem_cache { · line 102
Quoted 10 lines
Index: linux/include/linux/slub_def.h
===================================================================
include/linux/slub_def.h +2 -0
@@ -98,4 +98,8 @@ struct kmem_cache {
 	struct kmem_cache_node *node[MAX_NUMNODES];
 };
 
+#ifdef CONFIG_SYSFS
+#define SLAB_SUPPORTS_SYSFS
Why even define this? Why not just use CONFIG_SYSFS?

Quoted 7 lines

Quoted 3 lines
Because not all slab allocators currently support SYSFS and there is the need to have different code now in slab_common.c depending on the configuration of the allocator.
But you are defining something that you only ever check once, why not just use CONFIG_SYSFS instead as it makes more sense, not the other way around.

We cannot use CONFIG_SYSFS otherwise it would break SLAB since some of the code modified is shared between allocators. SLAB currently does not support sysfs. When we add that then we can get rid of the #define.

Subject: slub: use sysfs'es release mechanism for kmem_cache

Sysfs has a release mechanism. Use that to release the kmem_cache structure if CONFIG_SYSFS is enabled.

Signed-off-by Christoph Lameter <[email protected]>

include/linux/slub_def.h +4 -0

Index: linux/include/linux/slub_def.h
===================================================================
@@ -98,4 +98,8 @@ struct kmem_cache {
 	struct kmem_cache_node *node[MAX_NUMNODES];
 };
 
+#ifdef CONFIG_SYSFS
+#define SLAB_SUPPORTS_SYSFS
+#endif
+
 #endif /* _LINUX_SLUB_DEF_H */

mm/slab.h +2 -0

Index: linux/mm/slab.h
===================================================================
@@ -57,6 +57,7 @@ struct mem_cgroup;
 struct kmem_cache *
 __kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
 		   size_t align, unsigned long flags, void (*ctor)(void *));
+void sysfs_slab_remove(struct kmem_cache *);
 #else
 static inline struct kmem_cache *
 __kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,

@@ -91,6 +92,7 @@ __kmem_cache_alias(struct mem_cgroup *me
 #define CACHE_CREATE_MASK (SLAB_CORE_FLAGS | SLAB_DEBUG_FLAGS | SLAB_CACHE_FLAGS)
 
 int __kmem_cache_shutdown(struct kmem_cache *);
+void slab_kmem_cache_release(struct kmem_cache *);
 
 struct seq_file;
 struct file;

mm/slab_common.c +12 -2

Index: linux/mm/slab_common.c
===================================================================
@@ -251,6 +251,12 @@ kmem_cache_create(const char *name, size
 }
 EXPORT_SYMBOL(kmem_cache_create);
 
+void slab_kmem_cache_release(struct kmem_cache *s)
+{
+	kfree(s->name);
+	kmem_cache_free(kmem_cache, s);
+}
+
 void kmem_cache_destroy(struct kmem_cache *s)
 {
 	/* Destroy all the children caches if we aren't a memcg cache */

@@ -268,8 +274,12 @@ void kmem_cache_destroy(struct kmem_cach
 				rcu_barrier();
 
 			memcg_release_cache(s);
-			kfree(s->name);
-			kmem_cache_free(kmem_cache, s);
+#ifdef SLAB_SUPPORTS_SYSFS
+			sysfs_slab_remove(s);
+#else
+			slab_kmem_cache_release();
+
+#endif
 		} else {
 			list_add(&s->list, &slab_caches);
 			mutex_unlock(&slab_mutex);

mm/slub.c +8 ~~-19~~

Index: linux/mm/slub.c
===================================================================
@@ -210,7 +210,6 @@ enum track_item { TRACK_ALLOC, TRACK_FRE
 #ifdef CONFIG_SYSFS
 static int sysfs_slab_add(struct kmem_cache *);
 static int sysfs_slab_alias(struct kmem_cache *, const char *);
-static void sysfs_slab_remove(struct kmem_cache *);
 static void memcg_propagate_slab_attrs(struct kmem_cache *s);
 #else
 static inline int sysfs_slab_add(struct kmem_cache *s) { return 0; }

@@ -3208,23 +3207,7 @@ static inline int kmem_cache_close(struc
 
 int __kmem_cache_shutdown(struct kmem_cache *s)
 {
-	int rc = kmem_cache_close(s);
-
-	if (!rc) {
-		/*
-		 * We do the same lock strategy around sysfs_slab_add, see
-		 * __kmem_cache_create. Because this is pretty much the last
-		 * operation we do and the lock will be released shortly after
-		 * that in slab_common.c, we could just move sysfs_slab_remove
-		 * to a later point in common code. We should do that when we
-		 * have a common sysfs framework for all allocators.
-		 */
-		mutex_unlock(&slab_mutex);
-		sysfs_slab_remove(s);
-		mutex_lock(&slab_mutex);
-	}
-
-	return rc;
+	return kmem_cache_close(s);
 }
 
 /********************************************************************

@@ -5073,6 +5056,11 @@ static void memcg_propagate_slab_attrs(s
 #endif
 }
 
+static void kmem_cache_release(struct kobject *k)
+{
+	slab_kmem_cache_release(to_slab(k));
+}
+
 static const struct sysfs_ops slab_sysfs_ops = {
 	.show = slab_attr_show,
 	.store = slab_attr_store,

@@ -5080,6 +5068,7 @@ static const struct sysfs_ops slab_sysfs
 
 static struct kobj_type slab_ktype = {
 	.sysfs_ops = &slab_sysfs_ops,
+	.release = kmem_cache_release,
 };
 
 static int uevent_filter(struct kset *kset, struct kobject *kobj)

@@ -5184,7 +5173,7 @@ static int sysfs_slab_add(struct kmem_ca
 	return 0;
 }
 
-static void sysfs_slab_remove(struct kmem_cache *s)
+void sysfs_slab_remove(struct kmem_cache *s)
 {
 	if (slab_state < FULL)
 		/*

Sasha Levin

author

2013-12-17 19:53 UTC #22 lore raw

Quoted 31 lines · Christoph Lameter

Quoted 1 line · Greg KH
Your release function had 2 tabs for the lines, not one.
Ah ok. Fixed.
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
include/linux/slub_def.h struct kmem_cache { · line 102
Quoted 10 lines
Index: linux/include/linux/slub_def.h
===================================================================
include/linux/slub_def.h +2 -0
@@ -98,4 +98,8 @@ struct kmem_cache {
  	struct kmem_cache_node *node[MAX_NUMNODES];
  };
 
+#ifdef CONFIG_SYSFS
+#define SLAB_SUPPORTS_SYSFS
Why even define this? Why not just use CONFIG_SYSFS?
Quoted 7 lines
Quoted 3 lines
Because not all slab allocators currently support SYSFS and there is the need to have different code now in slab_common.c depending on the configuration of the allocator.
But you are defining something that you only ever check once, why not just use CONFIG_SYSFS instead as it makes more sense, not the other way around.
We cannot use CONFIG_SYSFS otherwise it would break SLAB since some of the code modified is shared between allocators. SLAB currently does not support sysfs. When we add that then we can get rid of the #define.

Quoted 6 lines

Subject: slub: use sysfs'es release mechanism for kmem_cache
Sysfs has a release mechanism. Use that to release the kmem_cache structure if CONFIG_SYSFS is enabled.
Signed-off-by Christoph Lameter <[email protected]>

I'm still seeing warnings with this patch applied:

[   24.900482] WARNING: CPU: 12 PID: 3654 at lib/debugobjects.c:260 debug_print_object+
0x8d/0xb0()
[   24.900482] ODEBUG: free active (active state 0) object type: timer_list hint: delay
ed_work_timer_fn+0x0/0x20
[   24.900482] Modules linked in:
[   24.900482] CPU: 12 PID: 3654 Comm: kworker/12:1 Tainted: G        W    3.13.0-rc4-n
ext-20131217-sasha-00013-ga878504-dirty #4149
[   24.900482] Workqueue: events kobject_delayed_cleanup
[   24.900482]  0000000000000104 ffff8804f429bae8 ffffffff8439501c ffffffff8555a92c
[   24.900482]  ffff8804f429bb38 ffff8804f429bb28 ffffffff8112f8ac ffff8804f429bb58
[   24.900482]  ffffffff856a9413 ffff880826333530 ffffffff85c68c40 ffffffff8801bb58
[   24.900482] Call Trace:
[   24.900482]  [<ffffffff8439501c>] dump_stack+0x52/0x7f
[   24.900482]  [<ffffffff8112f8ac>] warn_slowpath_common+0x8c/0xc0
[   24.900482]  [<ffffffff8112f996>] warn_slowpath_fmt+0x46/0x50
[   24.900482]  [<ffffffff81adb50d>] debug_print_object+0x8d/0xb0
[   24.900482]  [<ffffffff81153090>] ? __queue_work+0x3f0/0x3f0
[   24.900482]  [<ffffffff81adbd15>] __debug_check_no_obj_freed+0xa5/0x220
[   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff81adbea5>] debug_check_no_obj_freed+0x15/0x20
[   24.900482]  [<ffffffff812ad54f>] kfree+0x21f/0x2e0
[   24.900482]  [<ffffffff832b1acb>] rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff8207efd5>] device_release+0x65/0xc0
[   24.900482]  [<ffffffff81ab05e5>] kobject_cleanup+0x145/0x190
[   24.900482]  [<ffffffff81ab063d>] kobject_delayed_cleanup+0xd/0x10
[   24.900482]  [<ffffffff81153a60>] process_one_work+0x320/0x530
[   24.900482]  [<ffffffff81153940>] ? process_one_work+0x200/0x530
[   24.900482]  [<ffffffff81155fe5>] worker_thread+0x215/0x350
[   24.900482]  [<ffffffff81155dd0>] ? manage_workers+0x180/0x180
[   24.900482]  [<ffffffff8115c9c5>] kthread+0x105/0x110
[   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
[   24.900482]  [<ffffffff843a5e7c>] ret_from_fork+0x7c/0xb0
[   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
[   24.900482] ---[ end trace 45529ebf79b2573e ]---

Thanks,
Sasha

Christoph Lameter

2013-12-17 20:37 UTC #23 lore raw

Quoted 1 line · Sasha Levin

I'm still seeing warnings with this patch applied:

Looks like this is related to some device release mechanism that frees twice?

I do not see any kmem_cache management functions in the backtrace and therefore would guess that this is not the same issue.

Sasha Levin

author

2013-12-17 22:09 UTC #24 lore raw

Quoted 9 lines · Christoph Lameter

Quoted 1 line · Sasha Levin
I'm still seeing warnings with this patch applied:
Looks like this is related to some device release mechanism that frees twice?
I do not see any kmem_cache management functions in the backtrace and therefore would guess that this is not the same issue.

rgr, sorry about that - I grepped for 'timer_list' which seemed to have been in all previous kmem_cache spews and thought this one is related.

If that's not the case, your patch works for me.

Thanks,
Sasha

Thomas Gleixner

2013-12-17 22:01 UTC #25 lore raw

In reply to Sasha Levin

On Tue, 17 Dec 2013, Sasha Levin wrote:

Cc'ing RTC folks

Quoted 1 line

I'm still seeing warnings with this patch applied:

That's not a sl*b issue.

Quoted 43 lines

[   24.900482] WARNING: CPU: 12 PID: 3654 at lib/debugobjects.c:260
debug_print_object+
0x8d/0xb0()
[   24.900482] ODEBUG: free active (active state 0) object type: timer_list
hint: delay
ed_work_timer_fn+0x0/0x20
[   24.900482] Modules linked in:
[   24.900482] CPU: 12 PID: 3654 Comm: kworker/12:1 Tainted: G        W
3.13.0-rc4-n
ext-20131217-sasha-00013-ga878504-dirty #4149
[   24.900482] Workqueue: events kobject_delayed_cleanup
[   24.900482]  0000000000000104 ffff8804f429bae8 ffffffff8439501c
ffffffff8555a92c
[   24.900482]  ffff8804f429bb38 ffff8804f429bb28 ffffffff8112f8ac
ffff8804f429bb58
[   24.900482]  ffffffff856a9413 ffff880826333530 ffffffff85c68c40
ffffffff8801bb58
[   24.900482] Call Trace:
[   24.900482]  [<ffffffff8439501c>] dump_stack+0x52/0x7f
[   24.900482]  [<ffffffff8112f8ac>] warn_slowpath_common+0x8c/0xc0
[   24.900482]  [<ffffffff8112f996>] warn_slowpath_fmt+0x46/0x50
[   24.900482]  [<ffffffff81adb50d>] debug_print_object+0x8d/0xb0
[   24.900482]  [<ffffffff81153090>] ? __queue_work+0x3f0/0x3f0
[   24.900482]  [<ffffffff81adbd15>] __debug_check_no_obj_freed+0xa5/0x220
[   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff81adbea5>] debug_check_no_obj_freed+0x15/0x20
[   24.900482]  [<ffffffff812ad54f>] kfree+0x21f/0x2e0
[   24.900482]  [<ffffffff832b1acb>] rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff8207efd5>] device_release+0x65/0xc0
[   24.900482]  [<ffffffff81ab05e5>] kobject_cleanup+0x145/0x190
[   24.900482]  [<ffffffff81ab063d>] kobject_delayed_cleanup+0xd/0x10
[   24.900482]  [<ffffffff81153a60>] process_one_work+0x320/0x530
[   24.900482]  [<ffffffff81153940>] ? process_one_work+0x200/0x530
[   24.900482]  [<ffffffff81155fe5>] worker_thread+0x215/0x350
[   24.900482]  [<ffffffff81155dd0>] ? manage_workers+0x180/0x180
[   24.900482]  [<ffffffff8115c9c5>] kthread+0x105/0x110
[   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
[   24.900482]  [<ffffffff843a5e7c>] ret_from_fork+0x7c/0xb0
[   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
[   24.900482] ---[ end trace 45529ebf79b2573e ]---

Quoted 4 lines

Thanks,
Sasha

Bart Van Assche

2013-12-19 12:12 UTC #26 lore raw

In reply to Sasha Levin

Quoted 35 lines · Sasha Levin

I'm still seeing warnings with this patch applied:

[   24.900482] WARNING: CPU: 12 PID: 3654 at lib/debugobjects.c:260
debug_print_object+
0x8d/0xb0()
[   24.900482] ODEBUG: free active (active state 0) object type:
timer_list hint: delay
ed_work_timer_fn+0x0/0x20
[   24.900482] Modules linked in:
[   24.900482] CPU: 12 PID: 3654 Comm: kworker/12:1 Tainted: G       
W    3.13.0-rc4-n
ext-20131217-sasha-00013-ga878504-dirty #4149
[   24.900482] Workqueue: events kobject_delayed_cleanup
[   24.900482]  0000000000000104 ffff8804f429bae8 ffffffff8439501c
ffffffff8555a92c
[   24.900482]  ffff8804f429bb38 ffff8804f429bb28 ffffffff8112f8ac
ffff8804f429bb58
[   24.900482]  ffffffff856a9413 ffff880826333530 ffffffff85c68c40
ffffffff8801bb58
[   24.900482] Call Trace:
[   24.900482]  [<ffffffff8439501c>] dump_stack+0x52/0x7f
[   24.900482]  [<ffffffff8112f8ac>] warn_slowpath_common+0x8c/0xc0
[   24.900482]  [<ffffffff8112f996>] warn_slowpath_fmt+0x46/0x50
[   24.900482]  [<ffffffff81adb50d>] debug_print_object+0x8d/0xb0
[   24.900482]  [<ffffffff81153090>] ? __queue_work+0x3f0/0x3f0
[   24.900482]  [<ffffffff81adbd15>] __debug_check_no_obj_freed+0xa5/0x220
[   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff81adbea5>] debug_check_no_obj_freed+0x15/0x20
[   24.900482]  [<ffffffff812ad54f>] kfree+0x21f/0x2e0
[   24.900482]  [<ffffffff832b1acb>] rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff8207efd5>] device_release+0x65/0xc0
[   24.900482]  [<ffffffff81ab05e5>] kobject_cleanup+0x145/0x190
[   24.900482]  [<ffffffff81ab063d>] kobject_delayed_cleanup+0xd/0x10
[   24.900482]  [<ffffffff81153a60>] process_one_work+0x320/0x530

Quoted 8 lines

[   24.900482]  [<ffffffff81153940>] ? process_one_work+0x200/0x530
[   24.900482]  [<ffffffff81155fe5>] worker_thread+0x215/0x350
[   24.900482]  [<ffffffff81155dd0>] ? manage_workers+0x180/0x180
[   24.900482]  [<ffffffff8115c9c5>] kthread+0x105/0x110
[   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
[   24.900482]  [<ffffffff843a5e7c>] ret_from_fork+0x7c/0xb0
[   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
[   24.900482] ---[ end trace 45529ebf79b2573e ]---

Can anyone tell me whether the patch below makes sense ? Please note that I'm not familiar with the timer subsystem.

kernel/timer.c +2 -0

index accfd24..828b666 100644
@@ -969,6 +969,8 @@ int del_timer(struct timer_list *timer)
                base = lock_timer_base(timer, &flags);
                ret = detach_if_pending(timer, base, true);
                spin_unlock_irqrestore(&base->lock, flags);
+       } else {
+               debug_deactivate(timer);
        }
 
        return ret;

Bart Van Assche

2013-12-19 14:20 UTC #27 lore raw

Quoted 43 lines · Bart Van Assche

Quoted 33 lines · Sasha Levin

I'm still seeing warnings with this patch applied:

[   24.900482] WARNING: CPU: 12 PID: 3654 at lib/debugobjects.c:260
debug_print_object+0x8d/0xb0()
[   24.900482] ODEBUG: free active (active state 0) object type:
timer_list hint: delayed_work_timer_fn+0x0/0x20
[   24.900482] Modules linked in:
[   24.900482] CPU: 12 PID: 3654 Comm: kworker/12:1 Tainted: G       
W    3.13.0-rc4-n
ext-20131217-sasha-00013-ga878504-dirty #4149
[   24.900482] Workqueue: events kobject_delayed_cleanup
[   24.900482]  0000000000000104 ffff8804f429bae8 ffffffff8439501c
ffffffff8555a92c
[   24.900482]  ffff8804f429bb38 ffff8804f429bb28 ffffffff8112f8ac
ffff8804f429bb58
[   24.900482]  ffffffff856a9413 ffff880826333530 ffffffff85c68c40
ffffffff8801bb58
[   24.900482] Call Trace:
[   24.900482]  [<ffffffff8439501c>] dump_stack+0x52/0x7f
[   24.900482]  [<ffffffff8112f8ac>] warn_slowpath_common+0x8c/0xc0
[   24.900482]  [<ffffffff8112f996>] warn_slowpath_fmt+0x46/0x50
[   24.900482]  [<ffffffff81adb50d>] debug_print_object+0x8d/0xb0
[   24.900482]  [<ffffffff81153090>] ? __queue_work+0x3f0/0x3f0
[   24.900482]  [<ffffffff81adbd15>] __debug_check_no_obj_freed+0xa5/0x220
[   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff832b1acb>] ? rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff81adbea5>] debug_check_no_obj_freed+0x15/0x20
[   24.900482]  [<ffffffff812ad54f>] kfree+0x21f/0x2e0
[   24.900482]  [<ffffffff832b1acb>] rtc_device_release+0x2b/0x40
[   24.900482]  [<ffffffff8207efd5>] device_release+0x65/0xc0
[   24.900482]  [<ffffffff81ab05e5>] kobject_cleanup+0x145/0x190
[   24.900482]  [<ffffffff81ab063d>] kobject_delayed_cleanup+0xd/0x10
[   24.900482]  [<ffffffff81153a60>] process_one_work+0x320/0x530

Quoted 8 lines

[   24.900482]  [<ffffffff81153940>] ? process_one_work+0x200/0x530
[   24.900482]  [<ffffffff81155fe5>] worker_thread+0x215/0x350
[   24.900482]  [<ffffffff81155dd0>] ? manage_workers+0x180/0x180
[   24.900482]  [<ffffffff8115c9c5>] kthread+0x105/0x110
[   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
[   24.900482]  [<ffffffff843a5e7c>] ret_from_fork+0x7c/0xb0
[   24.900482]  [<ffffffff8115c8c0>] ? set_kthreadd_affinity+0x30/0x30
[   24.900482] ---[ end trace 45529ebf79b2573e ]---

Quoted 3 lines

Can anyone tell me whether the patch below makes sense ?
[ ... ]

(replying to my own e-mail)

Please ignore the patch in the previous e-mail - it did not make sense.

Regarding the warning above: the "delayed_work_timer_fn+0x0/0x20" hint probably indicates an attempt to free a delayed_work structure that is embedded in struct rtc_device and that is still scheduled. It's unfortunate that debug_check_no_obj_freed() does not print the address of the offending object and the argument passed to kfree(). That information would allow to compute the offset of the embedded delayed_work structure make it easier to figure out what's going on.

Bart.

Thomas Gleixner

2013-11-27 13:41 UTC #28 lore raw

In reply to Russell King - ARM Linux

Quoted 8 lines · Russell King - ARM Linux

Quoted 7 lines · Thomas Gleixner
Though the kobject is the only thing which has a delayed work embedded inside struct kmem_cache. And the debug object splat points at the kmem_cache_free() of the struct kmem_cache itself. That's why I assumed the wreckage around that place. And indeed:
kmem_cache_destroy(s)
__kmem_cache_shutdown(s)

Quoted 10 lines

      sysfs_slab_remove(s)
        ....
	kobject_put(&s->kobj)
           kref_put(&kobj->kref, kobject_release);
	         kobject_release(kref)
    	       #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
	         schedule_delayed_work(&kobj->release)
	       #else
	        kobject_cleanup(kobj)
	       #endif

	kmem_cache_free(s);

Quoted 6 lines

Quoted 4 lines
So in the CONFIG_DEBUG_KOBJECT_RELEASE=y case, schedule_delayed_work() _IS_ called which arms the timer. debugobjects catches the attempt to free struct kmem_cache which contains the armed timer.
You fail to show where the free is in the above path.

So, yes. it's the issue you are trying to catch, but that code in question made already sure, that there are no references held, because it detached itself from sysfs before calling kobject_put().

Thanks,

tglx