* [PATCH] tree/tiny rcu: Add debug RCU head option (v2)
@ 2010-03-19 1:30 Mathieu Desnoyers
2010-03-19 2:08 ` Paul E. McKenney
2010-03-19 4:13 ` Lai Jiangshan
0 siblings, 2 replies; 13+ messages in thread
From: Mathieu Desnoyers @ 2010-03-19 1:30 UTC (permalink / raw)
To: Paul E. McKenney
Cc: mingo, akpm, laijs, dipankar, josh, dvhltc, niv, tglx, peterz,
rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
Alexey Dobriyan, Peter Zijlstra, linux-kernel
Poisoning the rcu_head callback list.
Helps finding racy users of call_rcu(), which results in hangs because list
entries are overwritten and/or skipped.
A lot of non-initialized rcu list heads will be found with this option enabled.
It is important to do not just blindly initialize them before each call_rcu, but
rather to perform the initialization at the proper location, after the memory
has been allocated, so we can effectively detect incorrect call_rcu users.
This patch version does not use "debug objects", although it probably should.
Some day I might find time to look into this transition, but the patch is usable
as is.
The weird #ifdef in the networking code comes from an agreement with Eric
Dumazet about how to disable the build check for the networking code. For those
who wonder about the existence of this build-time check: it generates a build
error when the size of struct dst_entry grows (because it is very performance
sensitive). So the agreement here has been to disable the check when the
DEBUG_RCU_HEAD config option is active.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: mingo@elte.hu
CC: akpm@linux-foundation.org
CC: mingo@elte.hu
CC: laijs@cn.fujitsu.com
CC: dipankar@in.ibm.com
CC: josh@joshtriplett.org
CC: dvhltc@us.ibm.com
CC: niv@us.ibm.com
CC: tglx@linutronix.de
CC: peterz@infradead.org
CC: rostedt@goodmis.org
CC: Valdis.Kletnieks@vt.edu
CC: dhowells@redhat.com
CC: eric.dumazet@gmail.com
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
include/linux/rcupdate.h | 13 ++++++++++++-
include/net/dst.h | 2 ++
kernel/rcutiny.c | 9 +++++++++
kernel/rcutree.c | 10 ++++++++++
lib/Kconfig.debug | 9 +++++++++
5 files changed, 42 insertions(+), 1 deletion(-)
Index: linux-2.6-lttng/include/linux/rcupdate.h
===================================================================
--- linux-2.6-lttng.orig/include/linux/rcupdate.h 2010-03-18 20:27:25.000000000 -0400
+++ linux-2.6-lttng/include/linux/rcupdate.h 2010-03-18 20:30:54.000000000 -0400
@@ -49,6 +49,9 @@
struct rcu_head {
struct rcu_head *next;
void (*func)(struct rcu_head *head);
+#ifdef CONFIG_DEBUG_RCU_HEAD
+ struct rcu_head *debug;
+#endif
};
/* Exported common interfaces */
@@ -71,11 +74,19 @@ extern void rcu_init(void);
#error "Unknown RCU implementation specified to kernel configuration"
#endif
+#ifdef CONFIG_DEBUG_RCU_HEAD
+#define RCU_HEAD_INIT { .next = NULL, .func = NULL, .debug = NULL }
+#define INIT_RCU_HEAD(ptr) do { \
+ (ptr)->next = NULL; (ptr)->func = NULL; (ptr)->debug = NULL; \
+} while (0)
+#else
#define RCU_HEAD_INIT { .next = NULL, .func = NULL }
-#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT
#define INIT_RCU_HEAD(ptr) do { \
(ptr)->next = NULL; (ptr)->func = NULL; \
} while (0)
+#endif
+
+#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT
#ifdef CONFIG_DEBUG_LOCK_ALLOC
extern struct lockdep_map rcu_lock_map;
Index: linux-2.6-lttng/kernel/rcutree.c
===================================================================
--- linux-2.6-lttng.orig/kernel/rcutree.c 2010-03-18 20:27:25.000000000 -0400
+++ linux-2.6-lttng/kernel/rcutree.c 2010-03-18 20:31:16.000000000 -0400
@@ -39,6 +39,7 @@
#include <asm/atomic.h>
#include <linux/bitops.h>
#include <linux/module.h>
+#include <linux/poison.h>
#include <linux/completion.h>
#include <linux/moduleparam.h>
#include <linux/percpu.h>
@@ -1060,6 +1061,10 @@ static void rcu_do_batch(struct rcu_stat
next = list->next;
prefetch(next);
trace_rcu_tree_callback(list);
+#ifdef CONFIG_DEBUG_RCU_HEAD
+ WARN_ON_ONCE(list->debug != LIST_POISON1);
+ list->debug = NULL;
+#endif
list->func(list);
list = next;
if (++count >= rdp->blimit)
@@ -1350,6 +1355,11 @@ __call_rcu(struct rcu_head *head, void (
unsigned long flags;
struct rcu_data *rdp;
+#ifdef CONFIG_DEBUG_RCU_HEAD
+ WARN_ON_ONCE(head->debug);
+ head->debug = LIST_POISON1;
+#endif
+
head->func = func;
head->next = NULL;
Index: linux-2.6-lttng/lib/Kconfig.debug
===================================================================
--- linux-2.6-lttng.orig/lib/Kconfig.debug 2010-03-18 20:27:25.000000000 -0400
+++ linux-2.6-lttng/lib/Kconfig.debug 2010-03-18 20:27:38.000000000 -0400
@@ -661,6 +661,15 @@ config DEBUG_LIST
If unsure, say N.
+config DEBUG_RCU_HEAD
+ bool "Debug RCU callbacks"
+ depends on DEBUG_KERNEL
+ depends on TREE_RCU
+ help
+ Enable this to turn on debugging of RCU list heads (call_rcu() usage).
+ Seems to find problems more quickly with stress-tests in single-cpu
+ mode.
+
config DEBUG_SG
bool "Debug SG table operations"
depends on DEBUG_KERNEL
Index: linux-2.6-lttng/include/net/dst.h
===================================================================
--- linux-2.6-lttng.orig/include/net/dst.h 2010-03-18 20:27:25.000000000 -0400
+++ linux-2.6-lttng/include/net/dst.h 2010-03-18 20:35:02.000000000 -0400
@@ -159,7 +159,9 @@ static inline void dst_hold(struct dst_e
* If your kernel compilation stops here, please check
* __pad_to_align_refcnt declaration in struct dst_entry
*/
+#ifndef CONFIG_DEBUG_RCU_HEAD
BUILD_BUG_ON(offsetof(struct dst_entry, __refcnt) & 63);
+#endif
atomic_inc(&dst->__refcnt);
}
Index: linux-2.6-lttng/kernel/rcutiny.c
===================================================================
--- linux-2.6-lttng.orig/kernel/rcutiny.c 2010-03-18 20:35:14.000000000 -0400
+++ linux-2.6-lttng/kernel/rcutiny.c 2010-03-18 20:39:12.000000000 -0400
@@ -35,6 +35,7 @@
#include <linux/init.h>
#include <linux/time.h>
#include <linux/cpu.h>
+#include <linux/poison.h>
/* Global control variables for rcupdate callback mechanism. */
struct rcu_ctrlblk {
@@ -163,6 +164,10 @@ static void __rcu_process_callbacks(stru
while (list) {
next = list->next;
prefetch(next);
+#ifdef CONFIG_DEBUG_RCU_HEAD
+ WARN_ON_ONCE(list->debug != LIST_POISON1);
+ list->debug = NULL;
+#endif
list->func(list);
list = next;
}
@@ -210,6 +215,10 @@ static void __call_rcu(struct rcu_head *
{
unsigned long flags;
+#ifdef CONFIG_DEBUG_RCU_HEAD
+ WARN_ON_ONCE(head->debug);
+ head->debug = LIST_POISON1;
+#endif
head->func = func;
head->next = NULL;
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] tree/tiny rcu: Add debug RCU head option (v2) 2010-03-19 1:30 [PATCH] tree/tiny rcu: Add debug RCU head option (v2) Mathieu Desnoyers @ 2010-03-19 2:08 ` Paul E. McKenney 2010-03-19 2:33 ` Mathieu Desnoyers 2010-03-19 4:13 ` Lai Jiangshan 1 sibling, 1 reply; 13+ messages in thread From: Paul E. McKenney @ 2010-03-19 2:08 UTC (permalink / raw) To: Mathieu Desnoyers Cc: mingo, akpm, laijs, dipankar, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra, linux-kernel On Thu, Mar 18, 2010 at 09:30:24PM -0400, Mathieu Desnoyers wrote: > Poisoning the rcu_head callback list. > > Helps finding racy users of call_rcu(), which results in hangs because list > entries are overwritten and/or skipped. > > A lot of non-initialized rcu list heads will be found with this option enabled. > It is important to do not just blindly initialize them before each call_rcu, but > rather to perform the initialization at the proper location, after the memory > has been allocated, so we can effectively detect incorrect call_rcu users. > > This patch version does not use "debug objects", although it probably should. > Some day I might find time to look into this transition, but the patch is usable > as is. > > The weird #ifdef in the networking code comes from an agreement with Eric > Dumazet about how to disable the build check for the networking code. For those > who wonder about the existence of this build-time check: it generates a build > error when the size of struct dst_entry grows (because it is very performance > sensitive). So the agreement here has been to disable the check when the > DEBUG_RCU_HEAD config option is active. If this one finds at least one bug (or if the previous version found at least one bug -- bad memory day here), then I will push it. It can always be improved later, right? The patch looks good to me. Thanx, Paul > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > CC: mingo@elte.hu > CC: akpm@linux-foundation.org > CC: mingo@elte.hu > CC: laijs@cn.fujitsu.com > CC: dipankar@in.ibm.com > CC: josh@joshtriplett.org > CC: dvhltc@us.ibm.com > CC: niv@us.ibm.com > CC: tglx@linutronix.de > CC: peterz@infradead.org > CC: rostedt@goodmis.org > CC: Valdis.Kletnieks@vt.edu > CC: dhowells@redhat.com > CC: eric.dumazet@gmail.com > CC: Alexey Dobriyan <adobriyan@gmail.com> > CC: Peter Zijlstra <a.p.zijlstra@chello.nl> > --- > include/linux/rcupdate.h | 13 ++++++++++++- > include/net/dst.h | 2 ++ > kernel/rcutiny.c | 9 +++++++++ > kernel/rcutree.c | 10 ++++++++++ > lib/Kconfig.debug | 9 +++++++++ > 5 files changed, 42 insertions(+), 1 deletion(-) > > Index: linux-2.6-lttng/include/linux/rcupdate.h > =================================================================== > --- linux-2.6-lttng.orig/include/linux/rcupdate.h 2010-03-18 20:27:25.000000000 -0400 > +++ linux-2.6-lttng/include/linux/rcupdate.h 2010-03-18 20:30:54.000000000 -0400 > @@ -49,6 +49,9 @@ > struct rcu_head { > struct rcu_head *next; > void (*func)(struct rcu_head *head); > +#ifdef CONFIG_DEBUG_RCU_HEAD > + struct rcu_head *debug; > +#endif > }; > > /* Exported common interfaces */ > @@ -71,11 +74,19 @@ extern void rcu_init(void); > #error "Unknown RCU implementation specified to kernel configuration" > #endif > > +#ifdef CONFIG_DEBUG_RCU_HEAD > +#define RCU_HEAD_INIT { .next = NULL, .func = NULL, .debug = NULL } > +#define INIT_RCU_HEAD(ptr) do { \ > + (ptr)->next = NULL; (ptr)->func = NULL; (ptr)->debug = NULL; \ > +} while (0) > +#else > #define RCU_HEAD_INIT { .next = NULL, .func = NULL } > -#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT > #define INIT_RCU_HEAD(ptr) do { \ > (ptr)->next = NULL; (ptr)->func = NULL; \ > } while (0) > +#endif > + > +#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT > > #ifdef CONFIG_DEBUG_LOCK_ALLOC > extern struct lockdep_map rcu_lock_map; > Index: linux-2.6-lttng/kernel/rcutree.c > =================================================================== > --- linux-2.6-lttng.orig/kernel/rcutree.c 2010-03-18 20:27:25.000000000 -0400 > +++ linux-2.6-lttng/kernel/rcutree.c 2010-03-18 20:31:16.000000000 -0400 > @@ -39,6 +39,7 @@ > #include <asm/atomic.h> > #include <linux/bitops.h> > #include <linux/module.h> > +#include <linux/poison.h> > #include <linux/completion.h> > #include <linux/moduleparam.h> > #include <linux/percpu.h> > @@ -1060,6 +1061,10 @@ static void rcu_do_batch(struct rcu_stat > next = list->next; > prefetch(next); > trace_rcu_tree_callback(list); > +#ifdef CONFIG_DEBUG_RCU_HEAD > + WARN_ON_ONCE(list->debug != LIST_POISON1); > + list->debug = NULL; > +#endif > list->func(list); > list = next; > if (++count >= rdp->blimit) > @@ -1350,6 +1355,11 @@ __call_rcu(struct rcu_head *head, void ( > unsigned long flags; > struct rcu_data *rdp; > > +#ifdef CONFIG_DEBUG_RCU_HEAD > + WARN_ON_ONCE(head->debug); > + head->debug = LIST_POISON1; > +#endif > + > head->func = func; > head->next = NULL; > > Index: linux-2.6-lttng/lib/Kconfig.debug > =================================================================== > --- linux-2.6-lttng.orig/lib/Kconfig.debug 2010-03-18 20:27:25.000000000 -0400 > +++ linux-2.6-lttng/lib/Kconfig.debug 2010-03-18 20:27:38.000000000 -0400 > @@ -661,6 +661,15 @@ config DEBUG_LIST > > If unsure, say N. > > +config DEBUG_RCU_HEAD > + bool "Debug RCU callbacks" > + depends on DEBUG_KERNEL > + depends on TREE_RCU > + help > + Enable this to turn on debugging of RCU list heads (call_rcu() usage). > + Seems to find problems more quickly with stress-tests in single-cpu > + mode. > + > config DEBUG_SG > bool "Debug SG table operations" > depends on DEBUG_KERNEL > Index: linux-2.6-lttng/include/net/dst.h > =================================================================== > --- linux-2.6-lttng.orig/include/net/dst.h 2010-03-18 20:27:25.000000000 -0400 > +++ linux-2.6-lttng/include/net/dst.h 2010-03-18 20:35:02.000000000 -0400 > @@ -159,7 +159,9 @@ static inline void dst_hold(struct dst_e > * If your kernel compilation stops here, please check > * __pad_to_align_refcnt declaration in struct dst_entry > */ > +#ifndef CONFIG_DEBUG_RCU_HEAD > BUILD_BUG_ON(offsetof(struct dst_entry, __refcnt) & 63); > +#endif > atomic_inc(&dst->__refcnt); > } > > Index: linux-2.6-lttng/kernel/rcutiny.c > =================================================================== > --- linux-2.6-lttng.orig/kernel/rcutiny.c 2010-03-18 20:35:14.000000000 -0400 > +++ linux-2.6-lttng/kernel/rcutiny.c 2010-03-18 20:39:12.000000000 -0400 > @@ -35,6 +35,7 @@ > #include <linux/init.h> > #include <linux/time.h> > #include <linux/cpu.h> > +#include <linux/poison.h> > > /* Global control variables for rcupdate callback mechanism. */ > struct rcu_ctrlblk { > @@ -163,6 +164,10 @@ static void __rcu_process_callbacks(stru > while (list) { > next = list->next; > prefetch(next); > +#ifdef CONFIG_DEBUG_RCU_HEAD > + WARN_ON_ONCE(list->debug != LIST_POISON1); > + list->debug = NULL; > +#endif > list->func(list); > list = next; > } > @@ -210,6 +215,10 @@ static void __call_rcu(struct rcu_head * > { > unsigned long flags; > > +#ifdef CONFIG_DEBUG_RCU_HEAD > + WARN_ON_ONCE(head->debug); > + head->debug = LIST_POISON1; > +#endif > head->func = func; > head->next = NULL; > > > -- > Mathieu Desnoyers > Operating System Efficiency R&D Consultant > EfficiOS Inc. > http://www.efficios.com ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tree/tiny rcu: Add debug RCU head option (v2) 2010-03-19 2:08 ` Paul E. McKenney @ 2010-03-19 2:33 ` Mathieu Desnoyers 0 siblings, 0 replies; 13+ messages in thread From: Mathieu Desnoyers @ 2010-03-19 2:33 UTC (permalink / raw) To: Paul E. McKenney Cc: mingo, akpm, laijs, dipankar, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra, linux-kernel * Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote: > On Thu, Mar 18, 2010 at 09:30:24PM -0400, Mathieu Desnoyers wrote: > > Poisoning the rcu_head callback list. > > > > Helps finding racy users of call_rcu(), which results in hangs because list > > entries are overwritten and/or skipped. > > > > A lot of non-initialized rcu list heads will be found with this option enabled. > > It is important to do not just blindly initialize them before each call_rcu, but > > rather to perform the initialization at the proper location, after the memory > > has been allocated, so we can effectively detect incorrect call_rcu users. > > > > This patch version does not use "debug objects", although it probably should. > > Some day I might find time to look into this transition, but the patch is usable > > as is. > > > > The weird #ifdef in the networking code comes from an agreement with Eric > > Dumazet about how to disable the build check for the networking code. For those > > who wonder about the existence of this build-time check: it generates a build > > error when the size of struct dst_entry grows (because it is very performance > > sensitive). So the agreement here has been to disable the check when the > > DEBUG_RCU_HEAD config option is active. > > If this one finds at least one bug (or if the previous version found > at least one bug -- bad memory day here), then I will push it. I had to create this patch when I was trying to pinpoint some RCU-related issues. I think it turned out to be coming more from the cpu hotplug side than from the call_rcu() side, but this is really one more tool in the toolbox. Trying to find out if it detect actual misuses that we currently have will depend on tracking down all the non-initialized rcu_head structures. I already got a few for 2.6.32, but there seem to be more in 2.6.33. Again, I'll try to spot and correct them as time permits me. I already have spotted 6 non-initialized rcu heads, with more to come. > It can > always be improved later, right? Absolutely. > > The patch looks good to me. Great! :) Mathieu > > Thanx, Paul > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > > CC: mingo@elte.hu > > CC: akpm@linux-foundation.org > > CC: mingo@elte.hu > > CC: laijs@cn.fujitsu.com > > CC: dipankar@in.ibm.com > > CC: josh@joshtriplett.org > > CC: dvhltc@us.ibm.com > > CC: niv@us.ibm.com > > CC: tglx@linutronix.de > > CC: peterz@infradead.org > > CC: rostedt@goodmis.org > > CC: Valdis.Kletnieks@vt.edu > > CC: dhowells@redhat.com > > CC: eric.dumazet@gmail.com > > CC: Alexey Dobriyan <adobriyan@gmail.com> > > CC: Peter Zijlstra <a.p.zijlstra@chello.nl> > > --- > > include/linux/rcupdate.h | 13 ++++++++++++- > > include/net/dst.h | 2 ++ > > kernel/rcutiny.c | 9 +++++++++ > > kernel/rcutree.c | 10 ++++++++++ > > lib/Kconfig.debug | 9 +++++++++ > > 5 files changed, 42 insertions(+), 1 deletion(-) > > > > Index: linux-2.6-lttng/include/linux/rcupdate.h > > =================================================================== > > --- linux-2.6-lttng.orig/include/linux/rcupdate.h 2010-03-18 20:27:25.000000000 -0400 > > +++ linux-2.6-lttng/include/linux/rcupdate.h 2010-03-18 20:30:54.000000000 -0400 > > @@ -49,6 +49,9 @@ > > struct rcu_head { > > struct rcu_head *next; > > void (*func)(struct rcu_head *head); > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > + struct rcu_head *debug; > > +#endif > > }; > > > > /* Exported common interfaces */ > > @@ -71,11 +74,19 @@ extern void rcu_init(void); > > #error "Unknown RCU implementation specified to kernel configuration" > > #endif > > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > +#define RCU_HEAD_INIT { .next = NULL, .func = NULL, .debug = NULL } > > +#define INIT_RCU_HEAD(ptr) do { \ > > + (ptr)->next = NULL; (ptr)->func = NULL; (ptr)->debug = NULL; \ > > +} while (0) > > +#else > > #define RCU_HEAD_INIT { .next = NULL, .func = NULL } > > -#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT > > #define INIT_RCU_HEAD(ptr) do { \ > > (ptr)->next = NULL; (ptr)->func = NULL; \ > > } while (0) > > +#endif > > + > > +#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT > > > > #ifdef CONFIG_DEBUG_LOCK_ALLOC > > extern struct lockdep_map rcu_lock_map; > > Index: linux-2.6-lttng/kernel/rcutree.c > > =================================================================== > > --- linux-2.6-lttng.orig/kernel/rcutree.c 2010-03-18 20:27:25.000000000 -0400 > > +++ linux-2.6-lttng/kernel/rcutree.c 2010-03-18 20:31:16.000000000 -0400 > > @@ -39,6 +39,7 @@ > > #include <asm/atomic.h> > > #include <linux/bitops.h> > > #include <linux/module.h> > > +#include <linux/poison.h> > > #include <linux/completion.h> > > #include <linux/moduleparam.h> > > #include <linux/percpu.h> > > @@ -1060,6 +1061,10 @@ static void rcu_do_batch(struct rcu_stat > > next = list->next; > > prefetch(next); > > trace_rcu_tree_callback(list); > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > + WARN_ON_ONCE(list->debug != LIST_POISON1); > > + list->debug = NULL; > > +#endif > > list->func(list); > > list = next; > > if (++count >= rdp->blimit) > > @@ -1350,6 +1355,11 @@ __call_rcu(struct rcu_head *head, void ( > > unsigned long flags; > > struct rcu_data *rdp; > > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > + WARN_ON_ONCE(head->debug); > > + head->debug = LIST_POISON1; > > +#endif > > + > > head->func = func; > > head->next = NULL; > > > > Index: linux-2.6-lttng/lib/Kconfig.debug > > =================================================================== > > --- linux-2.6-lttng.orig/lib/Kconfig.debug 2010-03-18 20:27:25.000000000 -0400 > > +++ linux-2.6-lttng/lib/Kconfig.debug 2010-03-18 20:27:38.000000000 -0400 > > @@ -661,6 +661,15 @@ config DEBUG_LIST > > > > If unsure, say N. > > > > +config DEBUG_RCU_HEAD > > + bool "Debug RCU callbacks" > > + depends on DEBUG_KERNEL > > + depends on TREE_RCU > > + help > > + Enable this to turn on debugging of RCU list heads (call_rcu() usage). > > + Seems to find problems more quickly with stress-tests in single-cpu > > + mode. > > + > > config DEBUG_SG > > bool "Debug SG table operations" > > depends on DEBUG_KERNEL > > Index: linux-2.6-lttng/include/net/dst.h > > =================================================================== > > --- linux-2.6-lttng.orig/include/net/dst.h 2010-03-18 20:27:25.000000000 -0400 > > +++ linux-2.6-lttng/include/net/dst.h 2010-03-18 20:35:02.000000000 -0400 > > @@ -159,7 +159,9 @@ static inline void dst_hold(struct dst_e > > * If your kernel compilation stops here, please check > > * __pad_to_align_refcnt declaration in struct dst_entry > > */ > > +#ifndef CONFIG_DEBUG_RCU_HEAD > > BUILD_BUG_ON(offsetof(struct dst_entry, __refcnt) & 63); > > +#endif > > atomic_inc(&dst->__refcnt); > > } > > > > Index: linux-2.6-lttng/kernel/rcutiny.c > > =================================================================== > > --- linux-2.6-lttng.orig/kernel/rcutiny.c 2010-03-18 20:35:14.000000000 -0400 > > +++ linux-2.6-lttng/kernel/rcutiny.c 2010-03-18 20:39:12.000000000 -0400 > > @@ -35,6 +35,7 @@ > > #include <linux/init.h> > > #include <linux/time.h> > > #include <linux/cpu.h> > > +#include <linux/poison.h> > > > > /* Global control variables for rcupdate callback mechanism. */ > > struct rcu_ctrlblk { > > @@ -163,6 +164,10 @@ static void __rcu_process_callbacks(stru > > while (list) { > > next = list->next; > > prefetch(next); > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > + WARN_ON_ONCE(list->debug != LIST_POISON1); > > + list->debug = NULL; > > +#endif > > list->func(list); > > list = next; > > } > > @@ -210,6 +215,10 @@ static void __call_rcu(struct rcu_head * > > { > > unsigned long flags; > > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > + WARN_ON_ONCE(head->debug); > > + head->debug = LIST_POISON1; > > +#endif > > head->func = func; > > head->next = NULL; > > > > > > -- > > Mathieu Desnoyers > > Operating System Efficiency R&D Consultant > > EfficiOS Inc. > > http://www.efficios.com -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tree/tiny rcu: Add debug RCU head option (v2) 2010-03-19 1:30 [PATCH] tree/tiny rcu: Add debug RCU head option (v2) Mathieu Desnoyers 2010-03-19 2:08 ` Paul E. McKenney @ 2010-03-19 4:13 ` Lai Jiangshan 2010-03-19 5:05 ` Paul E. McKenney 2010-03-19 12:34 ` Christoph Lameter 1 sibling, 2 replies; 13+ messages in thread From: Lai Jiangshan @ 2010-03-19 4:13 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Paul E. McKenney, mingo, akpm, dipankar, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra, linux-kernel, Christoph Lameter, Pekka Enberg CC: Christoph Lameter <cl@linux-foundation.org> CC: Pekka Enberg <penberg@cs.helsinki.fi> I have written a patch like this before. but SLUB opposed me, so I did not post it. @ slub.c static void free_slab(struct kmem_cache *s, struct page *page) { if (unlikely(s->flags & SLAB_DESTROY_BY_RCU)) { /* * RCU free overloads the RCU head over the LRU */ struct rcu_head *head = (void *)&page->lru; call_rcu(head, rcu_free_slab); } else __free_slab(s, page); } I think it is not your patch's fault, but the above code has already existed. In your patch, struct rcu_head::debug will share the same memory of the other field of struct page, (especially share the struct page::flags which I encountered) I tested it very long time before, so I can't give you dmesg. Lai Mathieu Desnoyers wrote: > Poisoning the rcu_head callback list. > > Helps finding racy users of call_rcu(), which results in hangs because list > entries are overwritten and/or skipped. > > A lot of non-initialized rcu list heads will be found with this option enabled. > It is important to do not just blindly initialize them before each call_rcu, but > rather to perform the initialization at the proper location, after the memory > has been allocated, so we can effectively detect incorrect call_rcu users. > > This patch version does not use "debug objects", although it probably should. > Some day I might find time to look into this transition, but the patch is usable > as is. > > The weird #ifdef in the networking code comes from an agreement with Eric > Dumazet about how to disable the build check for the networking code. For those > who wonder about the existence of this build-time check: it generates a build > error when the size of struct dst_entry grows (because it is very performance > sensitive). So the agreement here has been to disable the check when the > DEBUG_RCU_HEAD config option is active. > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > CC: mingo@elte.hu > CC: akpm@linux-foundation.org > CC: mingo@elte.hu > CC: laijs@cn.fujitsu.com > CC: dipankar@in.ibm.com > CC: josh@joshtriplett.org > CC: dvhltc@us.ibm.com > CC: niv@us.ibm.com > CC: tglx@linutronix.de > CC: peterz@infradead.org > CC: rostedt@goodmis.org > CC: Valdis.Kletnieks@vt.edu > CC: dhowells@redhat.com > CC: eric.dumazet@gmail.com > CC: Alexey Dobriyan <adobriyan@gmail.com> > CC: Peter Zijlstra <a.p.zijlstra@chello.nl> > --- > include/linux/rcupdate.h | 13 ++++++++++++- > include/net/dst.h | 2 ++ > kernel/rcutiny.c | 9 +++++++++ > kernel/rcutree.c | 10 ++++++++++ > lib/Kconfig.debug | 9 +++++++++ > 5 files changed, 42 insertions(+), 1 deletion(-) > > Index: linux-2.6-lttng/include/linux/rcupdate.h > =================================================================== > --- linux-2.6-lttng.orig/include/linux/rcupdate.h 2010-03-18 20:27:25.000000000 -0400 > +++ linux-2.6-lttng/include/linux/rcupdate.h 2010-03-18 20:30:54.000000000 -0400 > @@ -49,6 +49,9 @@ > struct rcu_head { > struct rcu_head *next; > void (*func)(struct rcu_head *head); > +#ifdef CONFIG_DEBUG_RCU_HEAD > + struct rcu_head *debug; > +#endif > }; > > /* Exported common interfaces */ > @@ -71,11 +74,19 @@ extern void rcu_init(void); > #error "Unknown RCU implementation specified to kernel configuration" > #endif > > +#ifdef CONFIG_DEBUG_RCU_HEAD > +#define RCU_HEAD_INIT { .next = NULL, .func = NULL, .debug = NULL } > +#define INIT_RCU_HEAD(ptr) do { \ > + (ptr)->next = NULL; (ptr)->func = NULL; (ptr)->debug = NULL; \ > +} while (0) > +#else > #define RCU_HEAD_INIT { .next = NULL, .func = NULL } > -#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT > #define INIT_RCU_HEAD(ptr) do { \ > (ptr)->next = NULL; (ptr)->func = NULL; \ > } while (0) > +#endif > + > +#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT > > #ifdef CONFIG_DEBUG_LOCK_ALLOC > extern struct lockdep_map rcu_lock_map; > Index: linux-2.6-lttng/kernel/rcutree.c > =================================================================== > --- linux-2.6-lttng.orig/kernel/rcutree.c 2010-03-18 20:27:25.000000000 -0400 > +++ linux-2.6-lttng/kernel/rcutree.c 2010-03-18 20:31:16.000000000 -0400 > @@ -39,6 +39,7 @@ > #include <asm/atomic.h> > #include <linux/bitops.h> > #include <linux/module.h> > +#include <linux/poison.h> > #include <linux/completion.h> > #include <linux/moduleparam.h> > #include <linux/percpu.h> > @@ -1060,6 +1061,10 @@ static void rcu_do_batch(struct rcu_stat > next = list->next; > prefetch(next); > trace_rcu_tree_callback(list); > +#ifdef CONFIG_DEBUG_RCU_HEAD > + WARN_ON_ONCE(list->debug != LIST_POISON1); > + list->debug = NULL; > +#endif > list->func(list); > list = next; > if (++count >= rdp->blimit) > @@ -1350,6 +1355,11 @@ __call_rcu(struct rcu_head *head, void ( > unsigned long flags; > struct rcu_data *rdp; > > +#ifdef CONFIG_DEBUG_RCU_HEAD > + WARN_ON_ONCE(head->debug); > + head->debug = LIST_POISON1; > +#endif > + > head->func = func; > head->next = NULL; > > Index: linux-2.6-lttng/lib/Kconfig.debug > =================================================================== > --- linux-2.6-lttng.orig/lib/Kconfig.debug 2010-03-18 20:27:25.000000000 -0400 > +++ linux-2.6-lttng/lib/Kconfig.debug 2010-03-18 20:27:38.000000000 -0400 > @@ -661,6 +661,15 @@ config DEBUG_LIST > > If unsure, say N. > > +config DEBUG_RCU_HEAD > + bool "Debug RCU callbacks" > + depends on DEBUG_KERNEL > + depends on TREE_RCU > + help > + Enable this to turn on debugging of RCU list heads (call_rcu() usage). > + Seems to find problems more quickly with stress-tests in single-cpu > + mode. > + > config DEBUG_SG > bool "Debug SG table operations" > depends on DEBUG_KERNEL > Index: linux-2.6-lttng/include/net/dst.h > =================================================================== > --- linux-2.6-lttng.orig/include/net/dst.h 2010-03-18 20:27:25.000000000 -0400 > +++ linux-2.6-lttng/include/net/dst.h 2010-03-18 20:35:02.000000000 -0400 > @@ -159,7 +159,9 @@ static inline void dst_hold(struct dst_e > * If your kernel compilation stops here, please check > * __pad_to_align_refcnt declaration in struct dst_entry > */ > +#ifndef CONFIG_DEBUG_RCU_HEAD > BUILD_BUG_ON(offsetof(struct dst_entry, __refcnt) & 63); > +#endif > atomic_inc(&dst->__refcnt); > } > > Index: linux-2.6-lttng/kernel/rcutiny.c > =================================================================== > --- linux-2.6-lttng.orig/kernel/rcutiny.c 2010-03-18 20:35:14.000000000 -0400 > +++ linux-2.6-lttng/kernel/rcutiny.c 2010-03-18 20:39:12.000000000 -0400 > @@ -35,6 +35,7 @@ > #include <linux/init.h> > #include <linux/time.h> > #include <linux/cpu.h> > +#include <linux/poison.h> > > /* Global control variables for rcupdate callback mechanism. */ > struct rcu_ctrlblk { > @@ -163,6 +164,10 @@ static void __rcu_process_callbacks(stru > while (list) { > next = list->next; > prefetch(next); > +#ifdef CONFIG_DEBUG_RCU_HEAD > + WARN_ON_ONCE(list->debug != LIST_POISON1); > + list->debug = NULL; > +#endif > list->func(list); > list = next; > } > @@ -210,6 +215,10 @@ static void __call_rcu(struct rcu_head * > { > unsigned long flags; > > +#ifdef CONFIG_DEBUG_RCU_HEAD > + WARN_ON_ONCE(head->debug); > + head->debug = LIST_POISON1; > +#endif > head->func = func; > head->next = NULL; > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tree/tiny rcu: Add debug RCU head option (v2) 2010-03-19 4:13 ` Lai Jiangshan @ 2010-03-19 5:05 ` Paul E. McKenney 2010-03-19 12:59 ` Mathieu Desnoyers 2010-03-19 13:54 ` Mathieu Desnoyers 2010-03-19 12:34 ` Christoph Lameter 1 sibling, 2 replies; 13+ messages in thread From: Paul E. McKenney @ 2010-03-19 5:05 UTC (permalink / raw) To: Lai Jiangshan Cc: Mathieu Desnoyers, mingo, akpm, dipankar, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra, linux-kernel, Christoph Lameter, Pekka Enberg On Fri, Mar 19, 2010 at 12:13:53PM +0800, Lai Jiangshan wrote: > CC: Christoph Lameter <cl@linux-foundation.org> > CC: Pekka Enberg <penberg@cs.helsinki.fi> > > I have written a patch like this before. > > but SLUB opposed me, so I did not post it. > > @ slub.c > static void free_slab(struct kmem_cache *s, struct page *page) > { > if (unlikely(s->flags & SLAB_DESTROY_BY_RCU)) { > /* > * RCU free overloads the RCU head over the LRU > */ > struct rcu_head *head = (void *)&page->lru; > > call_rcu(head, rcu_free_slab); > } else > __free_slab(s, page); > } > > I think it is not your patch's fault, but the above code has > already existed. > > In your patch, struct rcu_head::debug will share the same memory of > the other field of struct page, (especially share the struct page::flags > which I encountered) > > I tested it very long time before, so I can't give you dmesg. Ouch!!! If this only causes a problem for SLUB, maybe a workaround is to make the config variable depend on !SLUB. But this would seem to increase the urgency of using the debug objects Thanx, Paul > Lai > > Mathieu Desnoyers wrote: > > Poisoning the rcu_head callback list. > > > > Helps finding racy users of call_rcu(), which results in hangs because list > > entries are overwritten and/or skipped. > > > > A lot of non-initialized rcu list heads will be found with this option enabled. > > It is important to do not just blindly initialize them before each call_rcu, but > > rather to perform the initialization at the proper location, after the memory > > has been allocated, so we can effectively detect incorrect call_rcu users. > > > > This patch version does not use "debug objects", although it probably should. > > Some day I might find time to look into this transition, but the patch is usable > > as is. > > > > The weird #ifdef in the networking code comes from an agreement with Eric > > Dumazet about how to disable the build check for the networking code. For those > > who wonder about the existence of this build-time check: it generates a build > > error when the size of struct dst_entry grows (because it is very performance > > sensitive). So the agreement here has been to disable the check when the > > DEBUG_RCU_HEAD config option is active. > > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > > CC: mingo@elte.hu > > CC: akpm@linux-foundation.org > > CC: mingo@elte.hu > > CC: laijs@cn.fujitsu.com > > CC: dipankar@in.ibm.com > > CC: josh@joshtriplett.org > > CC: dvhltc@us.ibm.com > > CC: niv@us.ibm.com > > CC: tglx@linutronix.de > > CC: peterz@infradead.org > > CC: rostedt@goodmis.org > > CC: Valdis.Kletnieks@vt.edu > > CC: dhowells@redhat.com > > CC: eric.dumazet@gmail.com > > CC: Alexey Dobriyan <adobriyan@gmail.com> > > CC: Peter Zijlstra <a.p.zijlstra@chello.nl> > > --- > > include/linux/rcupdate.h | 13 ++++++++++++- > > include/net/dst.h | 2 ++ > > kernel/rcutiny.c | 9 +++++++++ > > kernel/rcutree.c | 10 ++++++++++ > > lib/Kconfig.debug | 9 +++++++++ > > 5 files changed, 42 insertions(+), 1 deletion(-) > > > > Index: linux-2.6-lttng/include/linux/rcupdate.h > > =================================================================== > > --- linux-2.6-lttng.orig/include/linux/rcupdate.h 2010-03-18 20:27:25.000000000 -0400 > > +++ linux-2.6-lttng/include/linux/rcupdate.h 2010-03-18 20:30:54.000000000 -0400 > > @@ -49,6 +49,9 @@ > > struct rcu_head { > > struct rcu_head *next; > > void (*func)(struct rcu_head *head); > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > + struct rcu_head *debug; > > +#endif > > }; > > > > /* Exported common interfaces */ > > @@ -71,11 +74,19 @@ extern void rcu_init(void); > > #error "Unknown RCU implementation specified to kernel configuration" > > #endif > > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > +#define RCU_HEAD_INIT { .next = NULL, .func = NULL, .debug = NULL } > > +#define INIT_RCU_HEAD(ptr) do { \ > > + (ptr)->next = NULL; (ptr)->func = NULL; (ptr)->debug = NULL; \ > > +} while (0) > > +#else > > #define RCU_HEAD_INIT { .next = NULL, .func = NULL } > > -#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT > > #define INIT_RCU_HEAD(ptr) do { \ > > (ptr)->next = NULL; (ptr)->func = NULL; \ > > } while (0) > > +#endif > > + > > +#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT > > > > #ifdef CONFIG_DEBUG_LOCK_ALLOC > > extern struct lockdep_map rcu_lock_map; > > Index: linux-2.6-lttng/kernel/rcutree.c > > =================================================================== > > --- linux-2.6-lttng.orig/kernel/rcutree.c 2010-03-18 20:27:25.000000000 -0400 > > +++ linux-2.6-lttng/kernel/rcutree.c 2010-03-18 20:31:16.000000000 -0400 > > @@ -39,6 +39,7 @@ > > #include <asm/atomic.h> > > #include <linux/bitops.h> > > #include <linux/module.h> > > +#include <linux/poison.h> > > #include <linux/completion.h> > > #include <linux/moduleparam.h> > > #include <linux/percpu.h> > > @@ -1060,6 +1061,10 @@ static void rcu_do_batch(struct rcu_stat > > next = list->next; > > prefetch(next); > > trace_rcu_tree_callback(list); > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > + WARN_ON_ONCE(list->debug != LIST_POISON1); > > + list->debug = NULL; > > +#endif > > list->func(list); > > list = next; > > if (++count >= rdp->blimit) > > @@ -1350,6 +1355,11 @@ __call_rcu(struct rcu_head *head, void ( > > unsigned long flags; > > struct rcu_data *rdp; > > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > + WARN_ON_ONCE(head->debug); > > + head->debug = LIST_POISON1; > > +#endif > > + > > head->func = func; > > head->next = NULL; > > > > Index: linux-2.6-lttng/lib/Kconfig.debug > > =================================================================== > > --- linux-2.6-lttng.orig/lib/Kconfig.debug 2010-03-18 20:27:25.000000000 -0400 > > +++ linux-2.6-lttng/lib/Kconfig.debug 2010-03-18 20:27:38.000000000 -0400 > > @@ -661,6 +661,15 @@ config DEBUG_LIST > > > > If unsure, say N. > > > > +config DEBUG_RCU_HEAD > > + bool "Debug RCU callbacks" > > + depends on DEBUG_KERNEL > > + depends on TREE_RCU > > + help > > + Enable this to turn on debugging of RCU list heads (call_rcu() usage). > > + Seems to find problems more quickly with stress-tests in single-cpu > > + mode. > > + > > config DEBUG_SG > > bool "Debug SG table operations" > > depends on DEBUG_KERNEL > > Index: linux-2.6-lttng/include/net/dst.h > > =================================================================== > > --- linux-2.6-lttng.orig/include/net/dst.h 2010-03-18 20:27:25.000000000 -0400 > > +++ linux-2.6-lttng/include/net/dst.h 2010-03-18 20:35:02.000000000 -0400 > > @@ -159,7 +159,9 @@ static inline void dst_hold(struct dst_e > > * If your kernel compilation stops here, please check > > * __pad_to_align_refcnt declaration in struct dst_entry > > */ > > +#ifndef CONFIG_DEBUG_RCU_HEAD > > BUILD_BUG_ON(offsetof(struct dst_entry, __refcnt) & 63); > > +#endif > > atomic_inc(&dst->__refcnt); > > } > > > > Index: linux-2.6-lttng/kernel/rcutiny.c > > =================================================================== > > --- linux-2.6-lttng.orig/kernel/rcutiny.c 2010-03-18 20:35:14.000000000 -0400 > > +++ linux-2.6-lttng/kernel/rcutiny.c 2010-03-18 20:39:12.000000000 -0400 > > @@ -35,6 +35,7 @@ > > #include <linux/init.h> > > #include <linux/time.h> > > #include <linux/cpu.h> > > +#include <linux/poison.h> > > > > /* Global control variables for rcupdate callback mechanism. */ > > struct rcu_ctrlblk { > > @@ -163,6 +164,10 @@ static void __rcu_process_callbacks(stru > > while (list) { > > next = list->next; > > prefetch(next); > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > + WARN_ON_ONCE(list->debug != LIST_POISON1); > > + list->debug = NULL; > > +#endif > > list->func(list); > > list = next; > > } > > @@ -210,6 +215,10 @@ static void __call_rcu(struct rcu_head * > > { > > unsigned long flags; > > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > + WARN_ON_ONCE(head->debug); > > + head->debug = LIST_POISON1; > > +#endif > > head->func = func; > > head->next = NULL; > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tree/tiny rcu: Add debug RCU head option (v2) 2010-03-19 5:05 ` Paul E. McKenney @ 2010-03-19 12:59 ` Mathieu Desnoyers 2010-03-19 13:54 ` Mathieu Desnoyers 1 sibling, 0 replies; 13+ messages in thread From: Mathieu Desnoyers @ 2010-03-19 12:59 UTC (permalink / raw) To: Paul E. McKenney Cc: Lai Jiangshan, mingo, akpm, dipankar, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra, linux-kernel, Christoph Lameter, Pekka Enberg * Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote: > On Fri, Mar 19, 2010 at 12:13:53PM +0800, Lai Jiangshan wrote: > > CC: Christoph Lameter <cl@linux-foundation.org> > > CC: Pekka Enberg <penberg@cs.helsinki.fi> > > > > I have written a patch like this before. > > > > but SLUB opposed me, so I did not post it. > > > > @ slub.c > > static void free_slab(struct kmem_cache *s, struct page *page) > > { > > if (unlikely(s->flags & SLAB_DESTROY_BY_RCU)) { > > /* > > * RCU free overloads the RCU head over the LRU > > */ > > struct rcu_head *head = (void *)&page->lru; > > > > call_rcu(head, rcu_free_slab); > > } else > > __free_slab(s, page); > > } > > > > I think it is not your patch's fault, but the above code has > > already existed. > > > > In your patch, struct rcu_head::debug will share the same memory of > > the other field of struct page, (especially share the struct page::flags > > which I encountered) > > > > I tested it very long time before, so I can't give you dmesg. > > Ouch!!! > > If this only causes a problem for SLUB, maybe a workaround is to make > the config variable depend on !SLUB. But this would seem to increase > the urgency of using the debug objects Yep, that hurts. I think we'll have to move to debugobjects more quickly than expected. Thanks, Matheiu > > Thanx, Paul > > > Lai > > > > Mathieu Desnoyers wrote: > > > Poisoning the rcu_head callback list. > > > > > > Helps finding racy users of call_rcu(), which results in hangs because list > > > entries are overwritten and/or skipped. > > > > > > A lot of non-initialized rcu list heads will be found with this option enabled. > > > It is important to do not just blindly initialize them before each call_rcu, but > > > rather to perform the initialization at the proper location, after the memory > > > has been allocated, so we can effectively detect incorrect call_rcu users. > > > > > > This patch version does not use "debug objects", although it probably should. > > > Some day I might find time to look into this transition, but the patch is usable > > > as is. > > > > > > The weird #ifdef in the networking code comes from an agreement with Eric > > > Dumazet about how to disable the build check for the networking code. For those > > > who wonder about the existence of this build-time check: it generates a build > > > error when the size of struct dst_entry grows (because it is very performance > > > sensitive). So the agreement here has been to disable the check when the > > > DEBUG_RCU_HEAD config option is active. > > > > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > > CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > > > CC: mingo@elte.hu > > > CC: akpm@linux-foundation.org > > > CC: mingo@elte.hu > > > CC: laijs@cn.fujitsu.com > > > CC: dipankar@in.ibm.com > > > CC: josh@joshtriplett.org > > > CC: dvhltc@us.ibm.com > > > CC: niv@us.ibm.com > > > CC: tglx@linutronix.de > > > CC: peterz@infradead.org > > > CC: rostedt@goodmis.org > > > CC: Valdis.Kletnieks@vt.edu > > > CC: dhowells@redhat.com > > > CC: eric.dumazet@gmail.com > > > CC: Alexey Dobriyan <adobriyan@gmail.com> > > > CC: Peter Zijlstra <a.p.zijlstra@chello.nl> > > > --- > > > include/linux/rcupdate.h | 13 ++++++++++++- > > > include/net/dst.h | 2 ++ > > > kernel/rcutiny.c | 9 +++++++++ > > > kernel/rcutree.c | 10 ++++++++++ > > > lib/Kconfig.debug | 9 +++++++++ > > > 5 files changed, 42 insertions(+), 1 deletion(-) > > > > > > Index: linux-2.6-lttng/include/linux/rcupdate.h > > > =================================================================== > > > --- linux-2.6-lttng.orig/include/linux/rcupdate.h 2010-03-18 20:27:25.000000000 -0400 > > > +++ linux-2.6-lttng/include/linux/rcupdate.h 2010-03-18 20:30:54.000000000 -0400 > > > @@ -49,6 +49,9 @@ > > > struct rcu_head { > > > struct rcu_head *next; > > > void (*func)(struct rcu_head *head); > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > > + struct rcu_head *debug; > > > +#endif > > > }; > > > > > > /* Exported common interfaces */ > > > @@ -71,11 +74,19 @@ extern void rcu_init(void); > > > #error "Unknown RCU implementation specified to kernel configuration" > > > #endif > > > > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > > +#define RCU_HEAD_INIT { .next = NULL, .func = NULL, .debug = NULL } > > > +#define INIT_RCU_HEAD(ptr) do { \ > > > + (ptr)->next = NULL; (ptr)->func = NULL; (ptr)->debug = NULL; \ > > > +} while (0) > > > +#else > > > #define RCU_HEAD_INIT { .next = NULL, .func = NULL } > > > -#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT > > > #define INIT_RCU_HEAD(ptr) do { \ > > > (ptr)->next = NULL; (ptr)->func = NULL; \ > > > } while (0) > > > +#endif > > > + > > > +#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT > > > > > > #ifdef CONFIG_DEBUG_LOCK_ALLOC > > > extern struct lockdep_map rcu_lock_map; > > > Index: linux-2.6-lttng/kernel/rcutree.c > > > =================================================================== > > > --- linux-2.6-lttng.orig/kernel/rcutree.c 2010-03-18 20:27:25.000000000 -0400 > > > +++ linux-2.6-lttng/kernel/rcutree.c 2010-03-18 20:31:16.000000000 -0400 > > > @@ -39,6 +39,7 @@ > > > #include <asm/atomic.h> > > > #include <linux/bitops.h> > > > #include <linux/module.h> > > > +#include <linux/poison.h> > > > #include <linux/completion.h> > > > #include <linux/moduleparam.h> > > > #include <linux/percpu.h> > > > @@ -1060,6 +1061,10 @@ static void rcu_do_batch(struct rcu_stat > > > next = list->next; > > > prefetch(next); > > > trace_rcu_tree_callback(list); > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > > + WARN_ON_ONCE(list->debug != LIST_POISON1); > > > + list->debug = NULL; > > > +#endif > > > list->func(list); > > > list = next; > > > if (++count >= rdp->blimit) > > > @@ -1350,6 +1355,11 @@ __call_rcu(struct rcu_head *head, void ( > > > unsigned long flags; > > > struct rcu_data *rdp; > > > > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > > + WARN_ON_ONCE(head->debug); > > > + head->debug = LIST_POISON1; > > > +#endif > > > + > > > head->func = func; > > > head->next = NULL; > > > > > > Index: linux-2.6-lttng/lib/Kconfig.debug > > > =================================================================== > > > --- linux-2.6-lttng.orig/lib/Kconfig.debug 2010-03-18 20:27:25.000000000 -0400 > > > +++ linux-2.6-lttng/lib/Kconfig.debug 2010-03-18 20:27:38.000000000 -0400 > > > @@ -661,6 +661,15 @@ config DEBUG_LIST > > > > > > If unsure, say N. > > > > > > +config DEBUG_RCU_HEAD > > > + bool "Debug RCU callbacks" > > > + depends on DEBUG_KERNEL > > > + depends on TREE_RCU > > > + help > > > + Enable this to turn on debugging of RCU list heads (call_rcu() usage). > > > + Seems to find problems more quickly with stress-tests in single-cpu > > > + mode. > > > + > > > config DEBUG_SG > > > bool "Debug SG table operations" > > > depends on DEBUG_KERNEL > > > Index: linux-2.6-lttng/include/net/dst.h > > > =================================================================== > > > --- linux-2.6-lttng.orig/include/net/dst.h 2010-03-18 20:27:25.000000000 -0400 > > > +++ linux-2.6-lttng/include/net/dst.h 2010-03-18 20:35:02.000000000 -0400 > > > @@ -159,7 +159,9 @@ static inline void dst_hold(struct dst_e > > > * If your kernel compilation stops here, please check > > > * __pad_to_align_refcnt declaration in struct dst_entry > > > */ > > > +#ifndef CONFIG_DEBUG_RCU_HEAD > > > BUILD_BUG_ON(offsetof(struct dst_entry, __refcnt) & 63); > > > +#endif > > > atomic_inc(&dst->__refcnt); > > > } > > > > > > Index: linux-2.6-lttng/kernel/rcutiny.c > > > =================================================================== > > > --- linux-2.6-lttng.orig/kernel/rcutiny.c 2010-03-18 20:35:14.000000000 -0400 > > > +++ linux-2.6-lttng/kernel/rcutiny.c 2010-03-18 20:39:12.000000000 -0400 > > > @@ -35,6 +35,7 @@ > > > #include <linux/init.h> > > > #include <linux/time.h> > > > #include <linux/cpu.h> > > > +#include <linux/poison.h> > > > > > > /* Global control variables for rcupdate callback mechanism. */ > > > struct rcu_ctrlblk { > > > @@ -163,6 +164,10 @@ static void __rcu_process_callbacks(stru > > > while (list) { > > > next = list->next; > > > prefetch(next); > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > > + WARN_ON_ONCE(list->debug != LIST_POISON1); > > > + list->debug = NULL; > > > +#endif > > > list->func(list); > > > list = next; > > > } > > > @@ -210,6 +215,10 @@ static void __call_rcu(struct rcu_head * > > > { > > > unsigned long flags; > > > > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > > + WARN_ON_ONCE(head->debug); > > > + head->debug = LIST_POISON1; > > > +#endif > > > head->func = func; > > > head->next = NULL; > > > > > > > > -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tree/tiny rcu: Add debug RCU head option (v2) 2010-03-19 5:05 ` Paul E. McKenney 2010-03-19 12:59 ` Mathieu Desnoyers @ 2010-03-19 13:54 ` Mathieu Desnoyers 1 sibling, 0 replies; 13+ messages in thread From: Mathieu Desnoyers @ 2010-03-19 13:54 UTC (permalink / raw) To: Paul E. McKenney Cc: Lai Jiangshan, mingo, akpm, dipankar, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra, linux-kernel, Christoph Lameter, Pekka Enberg * Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote: > On Fri, Mar 19, 2010 at 12:13:53PM +0800, Lai Jiangshan wrote: > > CC: Christoph Lameter <cl@linux-foundation.org> > > CC: Pekka Enberg <penberg@cs.helsinki.fi> > > > > I have written a patch like this before. > > > > but SLUB opposed me, so I did not post it. > > > > @ slub.c > > static void free_slab(struct kmem_cache *s, struct page *page) > > { > > if (unlikely(s->flags & SLAB_DESTROY_BY_RCU)) { > > /* > > * RCU free overloads the RCU head over the LRU > > */ > > struct rcu_head *head = (void *)&page->lru; > > > > call_rcu(head, rcu_free_slab); > > } else > > __free_slab(s, page); > > } > > > > I think it is not your patch's fault, but the above code has > > already existed. > > > > In your patch, struct rcu_head::debug will share the same memory of > > the other field of struct page, (especially share the struct page::flags > > which I encountered) > > > > I tested it very long time before, so I can't give you dmesg. > > Ouch!!! > > If this only causes a problem for SLUB, maybe a workaround is to make > the config variable depend on !SLUB. But this would seem to increase > the urgency of using the debug objects Hrm.. looking into the port to debugobjects, there are a few things that are not precisely well set at the moment for it: - We need to have two distinct active states, with a check that we move between the two. This does not seem to be currently supported by debugobjects. - RCU heads are never explicitely freed. So yeah, that means we will have to modify a lot of callers. Thanks, Mathieu > > Thanx, Paul > > > Lai > > > > Mathieu Desnoyers wrote: > > > Poisoning the rcu_head callback list. > > > > > > Helps finding racy users of call_rcu(), which results in hangs because list > > > entries are overwritten and/or skipped. > > > > > > A lot of non-initialized rcu list heads will be found with this option enabled. > > > It is important to do not just blindly initialize them before each call_rcu, but > > > rather to perform the initialization at the proper location, after the memory > > > has been allocated, so we can effectively detect incorrect call_rcu users. > > > > > > This patch version does not use "debug objects", although it probably should. > > > Some day I might find time to look into this transition, but the patch is usable > > > as is. > > > > > > The weird #ifdef in the networking code comes from an agreement with Eric > > > Dumazet about how to disable the build check for the networking code. For those > > > who wonder about the existence of this build-time check: it generates a build > > > error when the size of struct dst_entry grows (because it is very performance > > > sensitive). So the agreement here has been to disable the check when the > > > DEBUG_RCU_HEAD config option is active. > > > > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > > CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > > > CC: mingo@elte.hu > > > CC: akpm@linux-foundation.org > > > CC: mingo@elte.hu > > > CC: laijs@cn.fujitsu.com > > > CC: dipankar@in.ibm.com > > > CC: josh@joshtriplett.org > > > CC: dvhltc@us.ibm.com > > > CC: niv@us.ibm.com > > > CC: tglx@linutronix.de > > > CC: peterz@infradead.org > > > CC: rostedt@goodmis.org > > > CC: Valdis.Kletnieks@vt.edu > > > CC: dhowells@redhat.com > > > CC: eric.dumazet@gmail.com > > > CC: Alexey Dobriyan <adobriyan@gmail.com> > > > CC: Peter Zijlstra <a.p.zijlstra@chello.nl> > > > --- > > > include/linux/rcupdate.h | 13 ++++++++++++- > > > include/net/dst.h | 2 ++ > > > kernel/rcutiny.c | 9 +++++++++ > > > kernel/rcutree.c | 10 ++++++++++ > > > lib/Kconfig.debug | 9 +++++++++ > > > 5 files changed, 42 insertions(+), 1 deletion(-) > > > > > > Index: linux-2.6-lttng/include/linux/rcupdate.h > > > =================================================================== > > > --- linux-2.6-lttng.orig/include/linux/rcupdate.h 2010-03-18 20:27:25.000000000 -0400 > > > +++ linux-2.6-lttng/include/linux/rcupdate.h 2010-03-18 20:30:54.000000000 -0400 > > > @@ -49,6 +49,9 @@ > > > struct rcu_head { > > > struct rcu_head *next; > > > void (*func)(struct rcu_head *head); > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > > + struct rcu_head *debug; > > > +#endif > > > }; > > > > > > /* Exported common interfaces */ > > > @@ -71,11 +74,19 @@ extern void rcu_init(void); > > > #error "Unknown RCU implementation specified to kernel configuration" > > > #endif > > > > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > > +#define RCU_HEAD_INIT { .next = NULL, .func = NULL, .debug = NULL } > > > +#define INIT_RCU_HEAD(ptr) do { \ > > > + (ptr)->next = NULL; (ptr)->func = NULL; (ptr)->debug = NULL; \ > > > +} while (0) > > > +#else > > > #define RCU_HEAD_INIT { .next = NULL, .func = NULL } > > > -#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT > > > #define INIT_RCU_HEAD(ptr) do { \ > > > (ptr)->next = NULL; (ptr)->func = NULL; \ > > > } while (0) > > > +#endif > > > + > > > +#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT > > > > > > #ifdef CONFIG_DEBUG_LOCK_ALLOC > > > extern struct lockdep_map rcu_lock_map; > > > Index: linux-2.6-lttng/kernel/rcutree.c > > > =================================================================== > > > --- linux-2.6-lttng.orig/kernel/rcutree.c 2010-03-18 20:27:25.000000000 -0400 > > > +++ linux-2.6-lttng/kernel/rcutree.c 2010-03-18 20:31:16.000000000 -0400 > > > @@ -39,6 +39,7 @@ > > > #include <asm/atomic.h> > > > #include <linux/bitops.h> > > > #include <linux/module.h> > > > +#include <linux/poison.h> > > > #include <linux/completion.h> > > > #include <linux/moduleparam.h> > > > #include <linux/percpu.h> > > > @@ -1060,6 +1061,10 @@ static void rcu_do_batch(struct rcu_stat > > > next = list->next; > > > prefetch(next); > > > trace_rcu_tree_callback(list); > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > > + WARN_ON_ONCE(list->debug != LIST_POISON1); > > > + list->debug = NULL; > > > +#endif > > > list->func(list); > > > list = next; > > > if (++count >= rdp->blimit) > > > @@ -1350,6 +1355,11 @@ __call_rcu(struct rcu_head *head, void ( > > > unsigned long flags; > > > struct rcu_data *rdp; > > > > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > > + WARN_ON_ONCE(head->debug); > > > + head->debug = LIST_POISON1; > > > +#endif > > > + > > > head->func = func; > > > head->next = NULL; > > > > > > Index: linux-2.6-lttng/lib/Kconfig.debug > > > =================================================================== > > > --- linux-2.6-lttng.orig/lib/Kconfig.debug 2010-03-18 20:27:25.000000000 -0400 > > > +++ linux-2.6-lttng/lib/Kconfig.debug 2010-03-18 20:27:38.000000000 -0400 > > > @@ -661,6 +661,15 @@ config DEBUG_LIST > > > > > > If unsure, say N. > > > > > > +config DEBUG_RCU_HEAD > > > + bool "Debug RCU callbacks" > > > + depends on DEBUG_KERNEL > > > + depends on TREE_RCU > > > + help > > > + Enable this to turn on debugging of RCU list heads (call_rcu() usage). > > > + Seems to find problems more quickly with stress-tests in single-cpu > > > + mode. > > > + > > > config DEBUG_SG > > > bool "Debug SG table operations" > > > depends on DEBUG_KERNEL > > > Index: linux-2.6-lttng/include/net/dst.h > > > =================================================================== > > > --- linux-2.6-lttng.orig/include/net/dst.h 2010-03-18 20:27:25.000000000 -0400 > > > +++ linux-2.6-lttng/include/net/dst.h 2010-03-18 20:35:02.000000000 -0400 > > > @@ -159,7 +159,9 @@ static inline void dst_hold(struct dst_e > > > * If your kernel compilation stops here, please check > > > * __pad_to_align_refcnt declaration in struct dst_entry > > > */ > > > +#ifndef CONFIG_DEBUG_RCU_HEAD > > > BUILD_BUG_ON(offsetof(struct dst_entry, __refcnt) & 63); > > > +#endif > > > atomic_inc(&dst->__refcnt); > > > } > > > > > > Index: linux-2.6-lttng/kernel/rcutiny.c > > > =================================================================== > > > --- linux-2.6-lttng.orig/kernel/rcutiny.c 2010-03-18 20:35:14.000000000 -0400 > > > +++ linux-2.6-lttng/kernel/rcutiny.c 2010-03-18 20:39:12.000000000 -0400 > > > @@ -35,6 +35,7 @@ > > > #include <linux/init.h> > > > #include <linux/time.h> > > > #include <linux/cpu.h> > > > +#include <linux/poison.h> > > > > > > /* Global control variables for rcupdate callback mechanism. */ > > > struct rcu_ctrlblk { > > > @@ -163,6 +164,10 @@ static void __rcu_process_callbacks(stru > > > while (list) { > > > next = list->next; > > > prefetch(next); > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > > + WARN_ON_ONCE(list->debug != LIST_POISON1); > > > + list->debug = NULL; > > > +#endif > > > list->func(list); > > > list = next; > > > } > > > @@ -210,6 +215,10 @@ static void __call_rcu(struct rcu_head * > > > { > > > unsigned long flags; > > > > > > +#ifdef CONFIG_DEBUG_RCU_HEAD > > > + WARN_ON_ONCE(head->debug); > > > + head->debug = LIST_POISON1; > > > +#endif > > > head->func = func; > > > head->next = NULL; > > > > > > > > -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tree/tiny rcu: Add debug RCU head option (v2) 2010-03-19 4:13 ` Lai Jiangshan 2010-03-19 5:05 ` Paul E. McKenney @ 2010-03-19 12:34 ` Christoph Lameter 2010-03-19 14:44 ` Paul E. McKenney 1 sibling, 1 reply; 13+ messages in thread From: Christoph Lameter @ 2010-03-19 12:34 UTC (permalink / raw) To: Lai Jiangshan Cc: Mathieu Desnoyers, Paul E. McKenney, mingo, akpm, dipankar, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra, linux-kernel, Pekka Enberg On Fri, 19 Mar 2010, Lai Jiangshan wrote: > but SLUB opposed me, so I did not post it. > > @ slub.c > static void free_slab(struct kmem_cache *s, struct page *page) > { > if (unlikely(s->flags & SLAB_DESTROY_BY_RCU)) { > /* > * RCU free overloads the RCU head over the LRU > */ > struct rcu_head *head = (void *)&page->lru; > > call_rcu(head, rcu_free_slab); > } else > __free_slab(s, page); > } > > I think it is not your patch's fault, but the above code has > already existed. > > In your patch, struct rcu_head::debug will share the same memory of > the other field of struct page, (especially share the struct page::flags > which I encountered) Right. I like the smallness of the RCU structure but if you must increase its size then we could: A) put the rcu structure at the end of each page like SLOB but that will take away memory that could be used by objects. B) Move the rcu object up by a word so that it overloads the index field in the page struct. That would give another exact fit but will cause trouble again if you bloat the structure some more. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tree/tiny rcu: Add debug RCU head option (v2) 2010-03-19 12:34 ` Christoph Lameter @ 2010-03-19 14:44 ` Paul E. McKenney 2010-03-19 16:56 ` Mathieu Desnoyers 0 siblings, 1 reply; 13+ messages in thread From: Paul E. McKenney @ 2010-03-19 14:44 UTC (permalink / raw) To: Christoph Lameter Cc: Lai Jiangshan, Mathieu Desnoyers, mingo, akpm, dipankar, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra, linux-kernel, Pekka Enberg On Fri, Mar 19, 2010 at 07:34:07AM -0500, Christoph Lameter wrote: > On Fri, 19 Mar 2010, Lai Jiangshan wrote: > > > but SLUB opposed me, so I did not post it. > > > > @ slub.c > > static void free_slab(struct kmem_cache *s, struct page *page) > > { > > if (unlikely(s->flags & SLAB_DESTROY_BY_RCU)) { > > /* > > * RCU free overloads the RCU head over the LRU > > */ > > struct rcu_head *head = (void *)&page->lru; > > > > call_rcu(head, rcu_free_slab); > > } else > > __free_slab(s, page); > > } > > > > I think it is not your patch's fault, but the above code has > > already existed. > > > > In your patch, struct rcu_head::debug will share the same memory of > > the other field of struct page, (especially share the struct page::flags > > which I encountered) > > Right. I like the smallness of the RCU structure but if you must > increase its size then we could: > > A) put the rcu structure at the end of each page like SLOB > but that will take away memory that could be used by objects. > > B) Move the rcu object up by a word so that it overloads the index field > in the page struct. That would give another exact fit but will cause > trouble again if you bloat the structure some more. The size increase is strictly under a debug option -- the non-debug size remains the same as always. And I expect that in the longer term, we will be able to shrink the size back down even with this debug option available. One question: do any of the other allocators care about the size of the struct rcu_head? If not, a temporary work-around would be to simply prohibit use of this debug option when running with SLUB. Thanx, Paul ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tree/tiny rcu: Add debug RCU head option (v2) 2010-03-19 14:44 ` Paul E. McKenney @ 2010-03-19 16:56 ` Mathieu Desnoyers 2010-03-19 17:11 ` Paul E. McKenney 0 siblings, 1 reply; 13+ messages in thread From: Mathieu Desnoyers @ 2010-03-19 16:56 UTC (permalink / raw) To: Paul E. McKenney Cc: Christoph Lameter, Lai Jiangshan, mingo, akpm, dipankar, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra, linux-kernel, Pekka Enberg * Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote: > On Fri, Mar 19, 2010 at 07:34:07AM -0500, Christoph Lameter wrote: > > On Fri, 19 Mar 2010, Lai Jiangshan wrote: > > > > > but SLUB opposed me, so I did not post it. > > > > > > @ slub.c > > > static void free_slab(struct kmem_cache *s, struct page *page) > > > { > > > if (unlikely(s->flags & SLAB_DESTROY_BY_RCU)) { > > > /* > > > * RCU free overloads the RCU head over the LRU > > > */ > > > struct rcu_head *head = (void *)&page->lru; > > > > > > call_rcu(head, rcu_free_slab); > > > } else > > > __free_slab(s, page); > > > } > > > > > > I think it is not your patch's fault, but the above code has > > > already existed. > > > > > > In your patch, struct rcu_head::debug will share the same memory of > > > the other field of struct page, (especially share the struct page::flags > > > which I encountered) > > > > Right. I like the smallness of the RCU structure but if you must > > increase its size then we could: > > > > A) put the rcu structure at the end of each page like SLOB > > but that will take away memory that could be used by objects. > > > > B) Move the rcu object up by a word so that it overloads the index field > > in the page struct. That would give another exact fit but will cause > > trouble again if you bloat the structure some more. > > The size increase is strictly under a debug option -- the non-debug > size remains the same as always. And I expect that in the longer term, > we will be able to shrink the size back down even with this debug option > available. > > One question: do any of the other allocators care about the size of the > struct rcu_head? If not, a temporary work-around would be to simply > prohibit use of this debug option when running with SLUB. > > Thanx, Paul Don't worry about the struct rcu_head size too much. I'm currently working on a debugobjects implementation. I should have something by the end of the day. Thanks, Mathieu -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tree/tiny rcu: Add debug RCU head option (v2) 2010-03-19 16:56 ` Mathieu Desnoyers @ 2010-03-19 17:11 ` Paul E. McKenney 2010-03-22 14:37 ` Christoph Lameter 0 siblings, 1 reply; 13+ messages in thread From: Paul E. McKenney @ 2010-03-19 17:11 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Christoph Lameter, Lai Jiangshan, mingo, akpm, dipankar, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra, linux-kernel, Pekka Enberg On Fri, Mar 19, 2010 at 12:56:38PM -0400, Mathieu Desnoyers wrote: > * Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote: > > On Fri, Mar 19, 2010 at 07:34:07AM -0500, Christoph Lameter wrote: > > > On Fri, 19 Mar 2010, Lai Jiangshan wrote: > > > > > > > but SLUB opposed me, so I did not post it. > > > > > > > > @ slub.c > > > > static void free_slab(struct kmem_cache *s, struct page *page) > > > > { > > > > if (unlikely(s->flags & SLAB_DESTROY_BY_RCU)) { > > > > /* > > > > * RCU free overloads the RCU head over the LRU > > > > */ > > > > struct rcu_head *head = (void *)&page->lru; > > > > > > > > call_rcu(head, rcu_free_slab); > > > > } else > > > > __free_slab(s, page); > > > > } > > > > > > > > I think it is not your patch's fault, but the above code has > > > > already existed. > > > > > > > > In your patch, struct rcu_head::debug will share the same memory of > > > > the other field of struct page, (especially share the struct page::flags > > > > which I encountered) > > > > > > Right. I like the smallness of the RCU structure but if you must > > > increase its size then we could: > > > > > > A) put the rcu structure at the end of each page like SLOB > > > but that will take away memory that could be used by objects. > > > > > > B) Move the rcu object up by a word so that it overloads the index field > > > in the page struct. That would give another exact fit but will cause > > > trouble again if you bloat the structure some more. > > > > The size increase is strictly under a debug option -- the non-debug > > size remains the same as always. And I expect that in the longer term, > > we will be able to shrink the size back down even with this debug option > > available. > > > > One question: do any of the other allocators care about the size of the > > struct rcu_head? If not, a temporary work-around would be to simply > > prohibit use of this debug option when running with SLUB. > > > > Thanx, Paul > > Don't worry about the struct rcu_head size too much. I'm currently working on a > debugobjects implementation. I should have something by the end of the day. Even better!!! Thank you, Mathieu!!! Thanx, Paul ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tree/tiny rcu: Add debug RCU head option (v2) 2010-03-19 17:11 ` Paul E. McKenney @ 2010-03-22 14:37 ` Christoph Lameter 2010-03-22 15:56 ` Paul E. McKenney 0 siblings, 1 reply; 13+ messages in thread From: Christoph Lameter @ 2010-03-22 14:37 UTC (permalink / raw) To: Paul E. McKenney Cc: Mathieu Desnoyers, Lai Jiangshan, mingo, akpm, dipankar, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra, linux-kernel, Pekka Enberg SLAB overloads "struct slab" with the rcu_head. Its not that critical there since the metadata allocated for each slab page is larger. SLUB does not have such a per slab page metadata structure and limits its metadata to what is available in the page struct. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tree/tiny rcu: Add debug RCU head option (v2) 2010-03-22 14:37 ` Christoph Lameter @ 2010-03-22 15:56 ` Paul E. McKenney 0 siblings, 0 replies; 13+ messages in thread From: Paul E. McKenney @ 2010-03-22 15:56 UTC (permalink / raw) To: Christoph Lameter Cc: Mathieu Desnoyers, Lai Jiangshan, mingo, akpm, dipankar, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra, linux-kernel, Pekka Enberg On Mon, Mar 22, 2010 at 09:37:55AM -0500, Christoph Lameter wrote: > SLAB overloads "struct slab" with the rcu_head. Its not that critical > there since the metadata allocated for each slab page is larger. SLUB does > not have such a per slab page metadata structure and limits its metadata > to what is available in the page struct. Thank you for the info! FWIW, Mathieu is now using debug objects, which permits him to keep rcu_head the same size. Thanx, Paul ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-03-22 15:56 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-03-19 1:30 [PATCH] tree/tiny rcu: Add debug RCU head option (v2) Mathieu Desnoyers 2010-03-19 2:08 ` Paul E. McKenney 2010-03-19 2:33 ` Mathieu Desnoyers 2010-03-19 4:13 ` Lai Jiangshan 2010-03-19 5:05 ` Paul E. McKenney 2010-03-19 12:59 ` Mathieu Desnoyers 2010-03-19 13:54 ` Mathieu Desnoyers 2010-03-19 12:34 ` Christoph Lameter 2010-03-19 14:44 ` Paul E. McKenney 2010-03-19 16:56 ` Mathieu Desnoyers 2010-03-19 17:11 ` Paul E. McKenney 2010-03-22 14:37 ` Christoph Lameter 2010-03-22 15:56 ` Paul E. McKenney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).