diff mbox

netfilter: active obj WARN when cleaning up

Message ID 00000142b90da700-19f6b465-ff15-4b2b-9bcd-b91d71958b7f-000000@email.amazonses.com
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Christoph Lameter (Ampere) Dec. 3, 2013, 3:22 p.m. UTC 
On Mon, 2 Dec 2013, Greg KH wrote:

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

Ah ok. Fixed.

> > > > Index: linux/include/linux/slub_def.h
> > > > ===================================================================
> > > > --- linux.orig/include/linux/slub_def.h	2013-12-02 13:31:07.395905824 -0600
> > > > +++ linux/include/linux/slub_def.h	2013-12-02 13:31:07.385906101 -0600
> > > > @@ -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?
> >
> > 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 <cl@linux.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Sasha Levin Dec. 17, 2013, 7:53 p.m. UTC | #1 
On 12/03/2013 10:22 AM, Christoph Lameter wrote:
> On Mon, 2 Dec 2013, Greg KH wrote:
>
>> Your release function had 2 tabs for the lines, not one.
>
> Ah ok. Fixed.
>
>>>>> Index: linux/include/linux/slub_def.h
>>>>> ===================================================================
>>>>> --- linux.orig/include/linux/slub_def.h	2013-12-02 13:31:07.395905824 -0600
>>>>> +++ linux/include/linux/slub_def.h	2013-12-02 13:31:07.385906101 -0600
>>>>> @@ -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?
>>>
>>> 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 <cl@linux.com>

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Lameter (Ampere) Dec. 17, 2013, 8:37 p.m. UTC | #2 
On Tue, 17 Dec 2013, Sasha Levin wrote:

> 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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Gleixner Dec. 17, 2013, 10:01 p.m. UTC | #3 
On Tue, 17 Dec 2013, Sasha Levin wrote:

Cc'ing RTC folks

> I'm still seeing warnings with this patch applied:

That's not a sl*b issue.
 
> [   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
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sasha Levin Dec. 17, 2013, 10:09 p.m. UTC | #4 
On 12/17/2013 03:37 PM, Christoph Lameter wrote:
> On Tue, 17 Dec 2013, Sasha Levin wrote:
>
>> 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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

Index: linux/include/linux/slub_def.h
===================================================================
--- linux.orig/include/linux/slub_def.h	2013-12-03 09:19:26.214666210 -0600
+++ linux/include/linux/slub_def.h	2013-12-03 09:19:26.214666210 -0600
@@ -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 */
Index: linux/mm/slab.h
===================================================================
--- linux.orig/mm/slab.h	2013-12-03 09:19:26.214666210 -0600
+++ linux/mm/slab.h	2013-12-03 09:19:26.214666210 -0600
@@ -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;
Index: linux/mm/slab_common.c
===================================================================
--- linux.orig/mm/slab_common.c	2013-12-03 09:19:26.214666210 -0600
+++ linux/mm/slab_common.c	2013-12-03 09:19:54.373883727 -0600
@@ -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);
Index: linux/mm/slub.c
===================================================================
--- linux.orig/mm/slub.c	2013-12-03 09:19:26.214666210 -0600
+++ linux/mm/slub.c	2013-12-03 09:19:26.214666210 -0600
@@ -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)
 		/*