[RFC patch 0/3] RCU head debug objects

[RFC patch 0/3] RCU head debug objects

[Mathieu Desnoyers]

Hi,

This patchset introduces a debugobjects-based rcu list head debugging
infrastructure. The first patch adds the ability to keep track of a "state
machine" into debugobjects. The state machine logic is all contained in the
type-aware caller. Debugobjects only keep track of the current state (a single
integer). A state validation/transition API is provided to debugobjects users.

This patchset is based on 2.6.33.1.

Comments are welcome,

Mathieu

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

[RFC patch 1/3] Debugobjects transition check

[Mathieu Desnoyers]

[-- Attachment #1: debugobjects-transition-check.patch --]
[-- Type: text/plain, Size: 4741 bytes --]

Implement a basic state machine checker in the debugobjects.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
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>
---
 include/linux/debugobjects.h |    9 ++++++
 lib/debugobjects.c           |   59 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 65 insertions(+), 3 deletions(-)

Index: linux-2.6-lttng/include/linux/debugobjects.h
===================================================================
--- linux-2.6-lttng.orig/include/linux/debugobjects.h	2010-03-19 15:26:02.000000000 -0400
+++ linux-2.6-lttng/include/linux/debugobjects.h	2010-03-19 15:26:06.000000000 -0400
@@ -20,12 +20,14 @@ struct debug_obj_descr;
  * struct debug_obj - representaion of an tracked object
  * @node:	hlist node to link the object into the tracker list
  * @state:	tracked object state
+ * @astate:	current active state
  * @object:	pointer to the real object
  * @descr:	pointer to an object type specific debug description structure
  */
 struct debug_obj {
 	struct hlist_node	node;
 	enum debug_obj_state	state;
+	unsigned int		astate;
 	void			*object;
 	struct debug_obj_descr	*descr;
 };
@@ -60,6 +62,13 @@ extern void debug_object_deactivate(void
 extern void debug_object_destroy   (void *addr, struct debug_obj_descr *descr);
 extern void debug_object_free      (void *addr, struct debug_obj_descr *descr);
 
+/*
+ * Active state must return to 0 before deactivation.
+ */
+extern void
+debug_object_active_state(void *addr, struct debug_obj_descr *descr,
+			  unsigned int expect, unsigned int next);
+
 extern void debug_objects_early_init(void);
 extern void debug_objects_mem_init(void);
 #else
Index: linux-2.6-lttng/lib/debugobjects.c
===================================================================
--- linux-2.6-lttng.orig/lib/debugobjects.c	2010-03-19 15:26:02.000000000 -0400
+++ linux-2.6-lttng/lib/debugobjects.c	2010-03-19 15:26:06.000000000 -0400
@@ -140,6 +140,7 @@ alloc_object(void *addr, struct debug_bu
 		obj->object = addr;
 		obj->descr  = descr;
 		obj->state  = ODEBUG_STATE_NONE;
+		obj->astate = 0;
 		hlist_del(&obj->node);
 
 		hlist_add_head(&obj->node, &b->list);
@@ -251,8 +252,10 @@ static void debug_print_object(struct de
 
 	if (limit < 5 && obj->descr != descr_test) {
 		limit++;
-		WARN(1, KERN_ERR "ODEBUG: %s %s object type: %s\n", msg,
-		       obj_states[obj->state], obj->descr->name);
+		WARN(1, KERN_ERR "ODEBUG: %s %s (active state %u) "
+				 "object type: %s\n",
+			msg, obj_states[obj->state], obj->astate,
+			obj->descr->name);
 	}
 	debug_objects_warnings++;
 }
@@ -446,7 +449,10 @@ void debug_object_deactivate(void *addr,
 		case ODEBUG_STATE_INIT:
 		case ODEBUG_STATE_INACTIVE:
 		case ODEBUG_STATE_ACTIVE:
-			obj->state = ODEBUG_STATE_INACTIVE;
+			if (!obj->astate)
+				obj->state = ODEBUG_STATE_INACTIVE;
+			else
+				debug_print_object(obj, "deactivate");
 			break;
 
 		case ODEBUG_STATE_DESTROYED:
@@ -552,6 +558,53 @@ out_unlock:
 	raw_spin_unlock_irqrestore(&db->lock, flags);
 }
 
+/**
+ * debug_object_active_state - debug checks object usage state machine
+ * @addr:	address of the object
+ * @descr:	pointer to an object specific debug description structure
+ * @expect:	expected state
+ * @next:	state to move to if expected state is found
+ */
+void
+debug_object_active_state(void *addr, struct debug_obj_descr *descr,
+			  unsigned int expect, unsigned int next)
+{
+	struct debug_bucket *db;
+	struct debug_obj *obj;
+	unsigned long flags;
+
+	if (!debug_objects_enabled)
+		return;
+
+	db = get_bucket((unsigned long) addr);
+
+	raw_spin_lock_irqsave(&db->lock, flags);
+
+	obj = lookup_object(addr, db);
+	if (obj) {
+		switch (obj->state) {
+		case ODEBUG_STATE_ACTIVE:
+			if (obj->astate == expect)
+				obj->astate = next;
+			else
+				debug_print_object(obj, "active_state");
+			break;
+
+		default:
+			debug_print_object(obj, "active_state");
+			break;
+		}
+	} else {
+		struct debug_obj o = { .object = addr,
+				       .state = ODEBUG_STATE_NOTAVAILABLE,
+				       .descr = descr };
+
+		debug_print_object(&o, "active_state");
+	}
+
+	raw_spin_unlock_irqrestore(&db->lock, flags);
+}
+
 #ifdef CONFIG_DEBUG_OBJECTS_FREE
 static void __debug_check_no_obj_freed(const void *address, unsigned long size)
 {

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

[RFC patch 2/3] tree/tiny rcu: Add debug RCU head objects (v3)

[Mathieu Desnoyers]

[-- Attachment #1: rcu-head-debug.patch --]
[-- Type: text/plain, Size: 10894 bytes --]

Helps finding racy users of call_rcu(), which results in hangs because list
entries are overwritten and/or skipped.

This new patch version is based on the debugobjects with the newly introduced
"active state" tracker.

Non-initialized entries are all considered as "statically initialized". An
activation fixup (triggered by call_rcu()) takes care of performing the debug
object initialization without issuing any warning. Since we cannot increase the
size of struct rcu_head, I don't see much room to put an identifier for
statically initialized rcu_head structures. So for now, we have to live without
"activation without explicit init" detection. But the main purpose of this debug
option is to detect double-activations (double call_rcu() use of a rcu_head
before the callback is executed), which is correctly addressed here.

This also detects potential internal RCU callback corruption, which would cause
the callbacks to be executed twice.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
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>
---
 include/linux/rcupdate.h |   61 ++++++++++++++++++-
 kernel/rcupdate.c        |  148 +++++++++++++++++++++++++++++++++++++++++++++++
 kernel/rcutiny.c         |    2 
 kernel/rcutree.c         |    2 
 lib/Kconfig.debug        |    6 +
 5 files changed, 215 insertions(+), 4 deletions(-)

Index: linux-2.6-lttng/include/linux/rcupdate.h
===================================================================
--- linux-2.6-lttng.orig/include/linux/rcupdate.h	2010-03-19 15:26:02.000000000 -0400
+++ linux-2.6-lttng/include/linux/rcupdate.h	2010-03-19 15:26:06.000000000 -0400
@@ -40,6 +40,7 @@
 #include <linux/seqlock.h>
 #include <linux/lockdep.h>
 #include <linux/completion.h>
+#include <linux/debugobjects.h>
 
 /**
  * struct rcu_head - callback structure for use with RCU
@@ -71,11 +72,26 @@ extern void rcu_init(void);
 #error "Unknown RCU implementation specified to kernel configuration"
 #endif
 
+/* For dynamic initialization of rcu_head allocated in memory */
+extern void rcu_head_init(struct rcu_head *head);
+
+/* For static initialization of rcu_head */
 #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)
+
+/* For dynamic initialization and destruction of rcu_head on the stack */
+#ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
+extern void rcu_head_init_on_stack(struct rcu_head *head);
+extern void destroy_rcu_head_on_stack(struct rcu_head *head);
+#else	/* !CONFIG_DEBUG_OBJECTS_RCU_HEAD */
+static inline void rcu_head_init_on_stack(struct rcu_head *head)
+{
+	rcu_head_init(head);
+}
+
+static inline void destroy_rcu_head_on_stack(struct rcu_head *head)
+{
+}
+#endif	/* #else !CONFIG_DEBUG_OBJECTS_RCU_HEAD */
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 extern struct lockdep_map rcu_lock_map;
@@ -299,4 +315,41 @@ extern void call_rcu(struct rcu_head *he
 extern void call_rcu_bh(struct rcu_head *head,
 			void (*func)(struct rcu_head *head));
 
+/*
+ * debug_rcu_head_queue()/debug_rcu_head_unqueue() are used internally
+ * by call_rcu() and rcu callback execution, and are therefore not part of the
+ * RCU API. Leaving in rcupdate.h because they are used by all RCU flavors.
+ */
+
+#ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
+# define STATE_RCU_HEAD_READY	0
+# define STATE_RCU_HEAD_QUEUED	1
+
+extern struct debug_obj_descr rcuhead_debug_descr;
+
+static inline void debug_rcu_head_queue(struct rcu_head *head)
+{
+	debug_object_activate(head, &rcuhead_debug_descr);
+	debug_object_active_state(head, &rcuhead_debug_descr,
+				  STATE_RCU_HEAD_READY,
+				  STATE_RCU_HEAD_QUEUED);
+}
+
+static inline void debug_rcu_head_unqueue(struct rcu_head *head)
+{
+	debug_object_active_state(head, &rcuhead_debug_descr,
+				  STATE_RCU_HEAD_QUEUED,
+				  STATE_RCU_HEAD_READY);
+	debug_object_deactivate(head, &rcuhead_debug_descr);
+}
+#else	/* !CONFIG_DEBUG_OBJECTS_RCU_HEAD */
+static inline void debug_rcu_head_queue(struct rcu_head *head)
+{
+}
+
+static inline void debug_rcu_head_unqueue(struct rcu_head *head)
+{
+}
+#endif	/* #else !CONFIG_DEBUG_OBJECTS_RCU_HEAD */
+
 #endif /* __LINUX_RCUPDATE_H */
Index: linux-2.6-lttng/kernel/rcutree.c
===================================================================
--- linux-2.6-lttng.orig/kernel/rcutree.c	2010-03-19 15:26:02.000000000 -0400
+++ linux-2.6-lttng/kernel/rcutree.c	2010-03-19 16:02:31.000000000 -0400
@@ -1060,6 +1060,7 @@ static void rcu_do_batch(struct rcu_stat
 		next = list->next;
 		prefetch(next);
 		trace_rcu_tree_callback(list);
+		debug_rcu_head_unqueue(list);
 		list->func(list);
 		list = next;
 		if (++count >= rdp->blimit)
@@ -1350,6 +1351,7 @@ __call_rcu(struct rcu_head *head, void (
 	unsigned long flags;
 	struct rcu_data *rdp;
 
+	debug_rcu_head_queue(head);
 	head->func = func;
 	head->next = NULL;
 
Index: linux-2.6-lttng/lib/Kconfig.debug
===================================================================
--- linux-2.6-lttng.orig/lib/Kconfig.debug	2010-03-19 15:26:02.000000000 -0400
+++ linux-2.6-lttng/lib/Kconfig.debug	2010-03-19 15:26:06.000000000 -0400
@@ -306,6 +306,12 @@ config DEBUG_OBJECTS_WORK
 	  work queue routines to track the life time of work objects and
 	  validate the work operations.
 
+config DEBUG_OBJECTS_RCU_HEAD
+	bool "Debug RCU callbacks objects"
+	depends on DEBUG_OBJECTS
+	help
+	  Enable this to turn on debugging of RCU list heads (call_rcu() usage).
+
 config DEBUG_OBJECTS_ENABLE_DEFAULT
 	int "debug_objects bootup default value (0-1)"
         range 0 1
Index: linux-2.6-lttng/kernel/rcutiny.c
===================================================================
--- linux-2.6-lttng.orig/kernel/rcutiny.c	2010-03-19 15:26:02.000000000 -0400
+++ linux-2.6-lttng/kernel/rcutiny.c	2010-03-19 16:02:31.000000000 -0400
@@ -163,6 +163,7 @@ static void __rcu_process_callbacks(stru
 	while (list) {
 		next = list->next;
 		prefetch(next);
+		debug_rcu_head_unqueue(list);
 		list->func(list);
 		list = next;
 	}
@@ -210,6 +211,7 @@ static void __call_rcu(struct rcu_head *
 {
 	unsigned long flags;
 
+	debug_rcu_head_queue(head);
 	head->func = func;
 	head->next = NULL;
 
Index: linux-2.6-lttng/kernel/rcupdate.c
===================================================================
--- linux-2.6-lttng.orig/kernel/rcupdate.c	2010-03-19 15:26:02.000000000 -0400
+++ linux-2.6-lttng/kernel/rcupdate.c	2010-03-19 16:26:12.000000000 -0400
@@ -63,3 +63,151 @@ void wakeme_after_rcu(struct rcu_head  *
 	rcu = container_of(head, struct rcu_synchronize, head);
 	complete(&rcu->completion);
 }
+
+#ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
+static inline void debug_rcu_head_init(struct rcu_head *head)
+{
+	debug_object_init(head, &rcuhead_debug_descr);
+}
+
+static inline void debug_rcu_head_free(struct rcu_head *head)
+{
+	debug_object_free(head, &rcuhead_debug_descr);
+}
+
+/*
+ * fixup_init is called when:
+ * - an active object is initialized
+ */
+static int rcuhead_fixup_init(void *addr, enum debug_obj_state state)
+{
+	struct rcu_head *head = addr;
+
+	switch (state) {
+	case ODEBUG_STATE_ACTIVE:
+		/*
+		 * Ensure that queued callbacks are all executed.
+		 * If we detect that we are nested in a RCU read-side critical
+		 * section, we should simply fail, otherwise we would deadlock.
+		 */
+		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
+		    irqs_disabled()) {
+			WARN_ON(1);
+			return 0;
+		}
+		rcu_barrier();
+		debug_object_init(head, &rcuhead_debug_descr);
+		return 1;
+	default:
+		return 0;
+	}
+}
+
+/*
+ * fixup_activate is called when:
+ * - an active object is activated
+ * - an unknown object is activated (might be a statically initialized object)
+ * Activation is performed internally by call_rcu().
+ * Let's make it valid to activate a static object.
+ */
+static int rcuhead_fixup_activate(void *addr, enum debug_obj_state state)
+{
+	struct rcu_head *head = addr;
+
+	switch (state) {
+
+	case ODEBUG_STATE_NOTAVAILABLE:
+		/*
+		 * This is not really a fixup. The work struct was
+		 * statically initialized. We just make sure that it
+		 * is tracked in the object tracker.
+		 */
+		debug_object_init(head, &rcuhead_debug_descr);
+		debug_object_activate(head, &rcuhead_debug_descr);
+		return 0;
+
+	case ODEBUG_STATE_ACTIVE:
+		/*
+		 * Ensure that queued callbacks are all executed.
+		 * If we detect that we are nested in a RCU read-side critical
+		 * section, we should simply fail, otherwise we would deadlock.
+		 */
+		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
+		    irqs_disabled()) {
+			WARN_ON(1);
+			return 0;
+		}
+		rcu_barrier();
+		debug_object_activate(head, &rcuhead_debug_descr);
+		return 1;
+
+	default:
+		return 0;
+	}
+}
+
+/*
+ * fixup_free is called when:
+ * - an active object is freed
+ */
+static int rcuhead_fixup_free(void *addr, enum debug_obj_state state)
+{
+	struct rcu_head *head = addr;
+
+	switch (state) {
+	case ODEBUG_STATE_ACTIVE:
+		/*
+		 * Ensure that queued callbacks are all executed.
+		 * If we detect that we are nested in a RCU read-side critical
+		 * section, we should simply fail, otherwise we would deadlock.
+		 */
+		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
+		    irqs_disabled()) {
+			WARN_ON(1);
+			return 0;
+		}
+		rcu_barrier();
+		debug_object_free(head, &rcuhead_debug_descr);
+		return 1;
+	default:
+		return 0;
+	}
+}
+
+void rcu_head_init_on_stack(struct rcu_head *head)
+{
+	debug_object_init_on_stack(head, &rcuhead_debug_descr);
+	rcu_head_init(head);
+}
+EXPORT_SYMBOL_GPL(rcu_head_init_on_stack);
+
+void destroy_rcu_head_on_stack(struct rcu_head *head)
+{
+	debug_object_free(head, &rcuhead_debug_descr);
+}
+EXPORT_SYMBOL_GPL(destroy_rcu_head_on_stack);
+
+struct debug_obj_descr rcuhead_debug_descr = {
+	.name = "rcu_head",
+	.fixup_init = rcuhead_fixup_init,
+	.fixup_activate = rcuhead_fixup_activate,
+	.fixup_free = rcuhead_fixup_free,
+};
+EXPORT_SYMBOL_GPL(rcuhead_debug_descr);
+#else	/* !CONFIG_DEBUG_OBJECTS_RCU_HEAD */
+static inline void debug_rcu_head_init(struct rcu_head *head)
+{
+}
+#endif /* #else !CONFIG_DEBUG_OBJECTS_RCU_HEAD */
+
+/**
+ * rcu_head_init - initialize a RCU head
+ * @head:     the rcu head to be initialized
+ */
+void rcu_head_init(struct rcu_head *head)
+{
+	debug_rcu_head_init(head);
+	head->next = NULL;
+	head->func = NULL;
+}
+EXPORT_SYMBOL_GPL(rcu_head_init);

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

[RFC patch 3/3] kernel call_rcu usage: initialize rcu_head structures

[Mathieu Desnoyers]

[-- Attachment #1: rcu-init-null.patch --]
[-- Type: text/plain, Size: 17342 bytes --]

Initialize rcu_head structures with rcu_head_init() before passing them to
call_rcu().

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
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>
---
 block/cfq-iosched.c                 |    2 +-
 block/genhd.c                       |    2 +-
 fs/file.c                           |    4 ++--
 fs/fs-writeback.c                   |   13 +++++++++----
 fs/partitions/check.c               |    2 +-
 kernel/fork.c                       |    1 +
 kernel/pid.c                        |    1 +
 kernel/rcutiny.c                    |    6 ++++++
 kernel/rcutorture.c                 |    2 ++
 kernel/rcutree.c                    |    4 ++++
 mm/backing-dev.c                    |    2 +-
 mm/slob.c                           |    2 +-
 net/core/drop_monitor.c             |    2 +-
 net/ipv6/sit.c                      |    2 +-
 net/ipv6/xfrm6_tunnel.c             |    2 +-
 net/netfilter/nf_conntrack_expect.c |    2 +-
 net/netfilter/nf_conntrack_extend.c |    2 +-
 net/netfilter/nfnetlink_queue.c     |    2 +-
 net/netlabel/netlabel_domainhash.c  |    2 +-
 net/netlabel/netlabel_unlabeled.c   |    6 +++---
 net/sctp/bind_addr.c                |    2 +-
 net/sctp/ipv6.c                     |    2 +-
 net/sctp/protocol.c                 |    2 +-
 23 files changed, 43 insertions(+), 24 deletions(-)

Index: linux-2.6-lttng/kernel/pid.c
===================================================================
--- linux-2.6-lttng.orig/kernel/pid.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/kernel/pid.c	2010-03-19 13:28:06.000000000 -0400
@@ -265,6 +265,7 @@ struct pid *alloc_pid(struct pid_namespa
 
 	get_pid_ns(ns);
 	pid->level = ns->level;
+	rcu_head_init(&pid->rcu);
 	atomic_set(&pid->count, 1);
 	for (type = 0; type < PIDTYPE_MAX; ++type)
 		INIT_HLIST_HEAD(&pid->tasks[type]);
Index: linux-2.6-lttng/kernel/fork.c
===================================================================
--- linux-2.6-lttng.orig/kernel/fork.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/kernel/fork.c	2010-03-19 13:28:06.000000000 -0400
@@ -1062,6 +1062,7 @@ static struct task_struct *copy_process(
 	INIT_LIST_HEAD(&p->children);
 	INIT_LIST_HEAD(&p->sibling);
 	rcu_copy_process(p);
+	rcu_head_init(&p->rcu);
 	p->vfork_done = NULL;
 	spin_lock_init(&p->alloc_lock);
 
Index: linux-2.6-lttng/kernel/rcutiny.c
===================================================================
--- linux-2.6-lttng.orig/kernel/rcutiny.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/kernel/rcutiny.c	2010-03-19 13:28:06.000000000 -0400
@@ -246,11 +246,13 @@ void rcu_barrier(void)
 {
 	struct rcu_synchronize rcu;
 
+	rcu_head_init_on_stack(&rcu.head);
 	init_completion(&rcu.completion);
 	/* Will wake me after RCU finished. */
 	call_rcu(&rcu.head, wakeme_after_rcu);
 	/* Wait for it. */
 	wait_for_completion(&rcu.completion);
+	destroy_rcu_head_on_stack(&rcu.head);
 }
 EXPORT_SYMBOL_GPL(rcu_barrier);
 
@@ -258,11 +260,13 @@ void rcu_barrier_bh(void)
 {
 	struct rcu_synchronize rcu;
 
+	rcu_head_init_on_stack(&rcu.head);
 	init_completion(&rcu.completion);
 	/* Will wake me after RCU finished. */
 	call_rcu_bh(&rcu.head, wakeme_after_rcu);
 	/* Wait for it. */
 	wait_for_completion(&rcu.completion);
+	destroy_rcu_head_on_stack(&rcu.head);
 }
 EXPORT_SYMBOL_GPL(rcu_barrier_bh);
 
@@ -270,11 +274,13 @@ void rcu_barrier_sched(void)
 {
 	struct rcu_synchronize rcu;
 
+	rcu_head_init_on_stack(&rcu.head);
 	init_completion(&rcu.completion);
 	/* Will wake me after RCU finished. */
 	call_rcu_sched(&rcu.head, wakeme_after_rcu);
 	/* Wait for it. */
 	wait_for_completion(&rcu.completion);
+	destroy_rcu_head_on_stack(&rcu.head);
 }
 EXPORT_SYMBOL_GPL(rcu_barrier_sched);
 
Index: linux-2.6-lttng/kernel/rcutorture.c
===================================================================
--- linux-2.6-lttng.orig/kernel/rcutorture.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/kernel/rcutorture.c	2010-03-19 13:28:06.000000000 -0400
@@ -450,9 +450,11 @@ static void rcu_bh_torture_synchronize(v
 {
 	struct rcu_bh_torture_synchronize rcu;
 
+	rcu_head_init_on_stack(&rcu.head);
 	init_completion(&rcu.completion);
 	call_rcu_bh(&rcu.head, rcu_bh_torture_wakeme_after_cb);
 	wait_for_completion(&rcu.completion);
+	destroy_rcu_head_on_stack(&rcu.head);
 }
 
 static struct rcu_torture_ops rcu_bh_ops = {
Index: linux-2.6-lttng/kernel/rcutree.c
===================================================================
--- linux-2.6-lttng.orig/kernel/rcutree.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/kernel/rcutree.c	2010-03-19 13:28:06.000000000 -0400
@@ -1449,11 +1449,13 @@ void synchronize_sched(void)
 	if (rcu_blocking_is_gp())
 		return;
 
+	rcu_head_init_on_stack(&rcu.head);
 	init_completion(&rcu.completion);
 	/* Will wake me after RCU finished. */
 	call_rcu_sched(&rcu.head, wakeme_after_rcu);
 	/* Wait for it. */
 	wait_for_completion(&rcu.completion);
+	destroy_rcu_head_on_stack(&rcu.head);
 }
 EXPORT_SYMBOL_GPL(synchronize_sched);
 
@@ -1473,11 +1475,13 @@ void synchronize_rcu_bh(void)
 	if (rcu_blocking_is_gp())
 		return;
 
+	rcu_head_init_on_stack(&rcu.head);
 	init_completion(&rcu.completion);
 	/* Will wake me after RCU finished. */
 	call_rcu_bh(&rcu.head, wakeme_after_rcu);
 	/* Wait for it. */
 	wait_for_completion(&rcu.completion);
+	destroy_rcu_head_on_stack(&rcu.head);
 }
 EXPORT_SYMBOL_GPL(synchronize_rcu_bh);
 
Index: linux-2.6-lttng/mm/backing-dev.c
===================================================================
--- linux-2.6-lttng.orig/mm/backing-dev.c	2010-03-19 13:23:40.000000000 -0400
+++ linux-2.6-lttng/mm/backing-dev.c	2010-03-19 13:28:06.000000000 -0400
@@ -653,7 +653,7 @@ int bdi_init(struct backing_dev_info *bd
 	bdi->max_ratio = 100;
 	bdi->max_prop_frac = PROP_FRAC_BASE;
 	spin_lock_init(&bdi->wb_lock);
-	INIT_RCU_HEAD(&bdi->rcu_head);
+	rcu_head_init(&bdi->rcu_head);
 	INIT_LIST_HEAD(&bdi->bdi_list);
 	INIT_LIST_HEAD(&bdi->wb_list);
 	INIT_LIST_HEAD(&bdi->work_list);
Index: linux-2.6-lttng/mm/slob.c
===================================================================
--- linux-2.6-lttng.orig/mm/slob.c	2010-03-19 13:23:40.000000000 -0400
+++ linux-2.6-lttng/mm/slob.c	2010-03-19 13:28:06.000000000 -0400
@@ -647,7 +647,7 @@ void kmem_cache_free(struct kmem_cache *
 	if (unlikely(c->flags & SLAB_DESTROY_BY_RCU)) {
 		struct slob_rcu *slob_rcu;
 		slob_rcu = b + (c->size - sizeof(struct slob_rcu));
-		INIT_RCU_HEAD(&slob_rcu->head);
+		rcu_head_init(&slob_rcu->head);
 		slob_rcu->size = c->size;
 		call_rcu(&slob_rcu->head, kmem_rcu_free);
 	} else {
Index: linux-2.6-lttng/fs/file.c
===================================================================
--- linux-2.6-lttng.orig/fs/file.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/fs/file.c	2010-03-19 13:28:06.000000000 -0400
@@ -178,7 +178,7 @@ static struct fdtable * alloc_fdtable(un
 	fdt->open_fds = (fd_set *)data;
 	data += nr / BITS_PER_BYTE;
 	fdt->close_on_exec = (fd_set *)data;
-	INIT_RCU_HEAD(&fdt->rcu);
+	rcu_head_init(&fdt->rcu);
 	fdt->next = NULL;
 
 	return fdt;
@@ -312,7 +312,7 @@ struct files_struct *dup_fd(struct files
 	new_fdt->close_on_exec = (fd_set *)&newf->close_on_exec_init;
 	new_fdt->open_fds = (fd_set *)&newf->open_fds_init;
 	new_fdt->fd = &newf->fd_array[0];
-	INIT_RCU_HEAD(&new_fdt->rcu);
+	rcu_head_init(&new_fdt->rcu);
 	new_fdt->next = NULL;
 
 	spin_lock(&oldf->file_lock);
Index: linux-2.6-lttng/fs/fs-writeback.c
===================================================================
--- linux-2.6-lttng.orig/fs/fs-writeback.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/fs/fs-writeback.c	2010-03-19 13:28:06.000000000 -0400
@@ -75,9 +75,13 @@ static inline bool bdi_work_on_stack(str
 }
 
 static inline void bdi_work_init(struct bdi_work *work,
-				 struct wb_writeback_args *args)
+				 struct wb_writeback_args *args,
+				 int on_stack)
 {
-	INIT_RCU_HEAD(&work->rcu_head);
+	if (on_stack)
+		rcu_head_init_on_stack(&work->rcu_head);
+	else
+		rcu_head_init(&work->rcu_head);
 	work->args = *args;
 	work->state = WS_USED;
 }
@@ -201,7 +205,7 @@ static void bdi_alloc_queue_work(struct 
 	 */
 	work = kmalloc(sizeof(*work), GFP_ATOMIC);
 	if (work) {
-		bdi_work_init(work, args);
+		bdi_work_init(work, args, 0);
 		bdi_queue_work(bdi, work);
 	} else {
 		struct bdi_writeback *wb = &bdi->wb;
@@ -232,11 +236,12 @@ static void bdi_sync_writeback(struct ba
 	};
 	struct bdi_work work;
 
-	bdi_work_init(&work, &args);
+	bdi_work_init(&work, &args, 1);
 	work.state |= WS_ONSTACK;
 
 	bdi_queue_work(bdi, &work);
 	bdi_wait_on_work_clear(&work);
+	destroy_rcu_head_on_stack(&work.rcu_head);
 }
 
 /**
Index: linux-2.6-lttng/fs/partitions/check.c
===================================================================
--- linux-2.6-lttng.orig/fs/partitions/check.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/fs/partitions/check.c	2010-03-19 13:28:06.000000000 -0400
@@ -454,7 +454,7 @@ struct hd_struct *add_partition(struct g
 	}
 
 	/* everything is up and running, commence */
-	INIT_RCU_HEAD(&p->rcu_head);
+	rcu_head_init(&p->rcu_head);
 	rcu_assign_pointer(ptbl->part[partno], p);
 
 	/* suppress uevent if the disk supresses it */
Index: linux-2.6-lttng/block/cfq-iosched.c
===================================================================
--- linux-2.6-lttng.orig/block/cfq-iosched.c	2010-03-19 13:23:40.000000000 -0400
+++ linux-2.6-lttng/block/cfq-iosched.c	2010-03-19 13:28:06.000000000 -0400
@@ -3730,7 +3730,7 @@ static void *cfq_init_queue(struct reque
 	 * second, in order to have larger depth for async operations.
 	 */
 	cfqd->last_delayed_sync = jiffies - HZ;
-	INIT_RCU_HEAD(&cfqd->rcu);
+	rcu_head_init(&cfqd->rcu);
 	return cfqd;
 }
 
Index: linux-2.6-lttng/block/genhd.c
===================================================================
--- linux-2.6-lttng.orig/block/genhd.c	2010-03-19 13:23:40.000000000 -0400
+++ linux-2.6-lttng/block/genhd.c	2010-03-19 13:28:06.000000000 -0400
@@ -987,7 +987,7 @@ int disk_expand_part_tbl(struct gendisk 
 	if (!new_ptbl)
 		return -ENOMEM;
 
-	INIT_RCU_HEAD(&new_ptbl->rcu_head);
+	rcu_head_init(&new_ptbl->rcu_head);
 	new_ptbl->len = target;
 
 	for (i = 0; i < len; i++)
Index: linux-2.6-lttng/net/netfilter/nfnetlink_queue.c
===================================================================
--- linux-2.6-lttng.orig/net/netfilter/nfnetlink_queue.c	2010-03-19 13:23:40.000000000 -0400
+++ linux-2.6-lttng/net/netfilter/nfnetlink_queue.c	2010-03-19 13:28:06.000000000 -0400
@@ -112,7 +112,7 @@ instance_create(u_int16_t queue_num, int
 	inst->copy_mode = NFQNL_COPY_NONE;
 	spin_lock_init(&inst->lock);
 	INIT_LIST_HEAD(&inst->queue_list);
-	INIT_RCU_HEAD(&inst->rcu);
+	rcu_head_init(&inst->rcu);
 
 	if (!try_module_get(THIS_MODULE)) {
 		err = -EAGAIN;
Index: linux-2.6-lttng/net/core/drop_monitor.c
===================================================================
--- linux-2.6-lttng.orig/net/core/drop_monitor.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/net/core/drop_monitor.c	2010-03-19 13:28:06.000000000 -0400
@@ -296,7 +296,7 @@ static int dropmon_net_event(struct noti
 
 		new_stat->dev = dev;
 		new_stat->last_rx = jiffies;
-		INIT_RCU_HEAD(&new_stat->rcu);
+		rcu_head_init(&new_stat->rcu);
 		spin_lock(&trace_state_lock);
 		list_add_rcu(&new_stat->list, &hw_stats_list);
 		spin_unlock(&trace_state_lock);
Index: linux-2.6-lttng/net/ipv6/sit.c
===================================================================
--- linux-2.6-lttng.orig/net/ipv6/sit.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/net/ipv6/sit.c	2010-03-19 13:28:06.000000000 -0400
@@ -364,7 +364,7 @@ ipip6_tunnel_add_prl(struct ip_tunnel *t
 		goto out;
 	}
 
-	INIT_RCU_HEAD(&p->rcu_head);
+	rcu_head_init(&p->rcu_head);
 	p->next = t->prl;
 	p->addr = a->addr;
 	p->flags = a->flags;
Index: linux-2.6-lttng/net/ipv6/xfrm6_tunnel.c
===================================================================
--- linux-2.6-lttng.orig/net/ipv6/xfrm6_tunnel.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/net/ipv6/xfrm6_tunnel.c	2010-03-19 13:28:06.000000000 -0400
@@ -187,7 +187,7 @@ alloc_spi:
 	if (!x6spi)
 		goto out;
 
-	INIT_RCU_HEAD(&x6spi->rcu_head);
+	rcu_head_init(&x6spi->rcu_head);
 	memcpy(&x6spi->addr, saddr, sizeof(x6spi->addr));
 	x6spi->spi = spi;
 	atomic_set(&x6spi->refcnt, 1);
Index: linux-2.6-lttng/net/netfilter/nf_conntrack_expect.c
===================================================================
--- linux-2.6-lttng.orig/net/netfilter/nf_conntrack_expect.c	2010-03-19 13:23:40.000000000 -0400
+++ linux-2.6-lttng/net/netfilter/nf_conntrack_expect.c	2010-03-19 13:28:06.000000000 -0400
@@ -232,7 +232,7 @@ struct nf_conntrack_expect *nf_ct_expect
 
 	new->master = me;
 	atomic_set(&new->use, 1);
-	INIT_RCU_HEAD(&new->rcu);
+	rcu_head_init(&new->rcu);
 	return new;
 }
 EXPORT_SYMBOL_GPL(nf_ct_expect_alloc);
Index: linux-2.6-lttng/net/netfilter/nf_conntrack_extend.c
===================================================================
--- linux-2.6-lttng.orig/net/netfilter/nf_conntrack_extend.c	2010-03-19 13:23:40.000000000 -0400
+++ linux-2.6-lttng/net/netfilter/nf_conntrack_extend.c	2010-03-19 13:28:06.000000000 -0400
@@ -59,7 +59,7 @@ nf_ct_ext_create(struct nf_ct_ext **ext,
 	if (!*ext)
 		return NULL;
 
-	INIT_RCU_HEAD(&(*ext)->rcu);
+	rcu_head_init(&(*ext)->rcu);
 	(*ext)->offset[id] = off;
 	(*ext)->len = len;
 
Index: linux-2.6-lttng/net/netlabel/netlabel_domainhash.c
===================================================================
--- linux-2.6-lttng.orig/net/netlabel/netlabel_domainhash.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/net/netlabel/netlabel_domainhash.c	2010-03-19 13:28:06.000000000 -0400
@@ -315,7 +315,7 @@ int netlbl_domhsh_add(struct netlbl_dom_
 		entry_old = netlbl_domhsh_search_def(entry->domain);
 	if (entry_old == NULL) {
 		entry->valid = 1;
-		INIT_RCU_HEAD(&entry->rcu);
+		rcu_head_init(&entry->rcu);
 
 		if (entry->domain != NULL) {
 			u32 bkt = netlbl_domhsh_hash(entry->domain);
Index: linux-2.6-lttng/net/netlabel/netlabel_unlabeled.c
===================================================================
--- linux-2.6-lttng.orig/net/netlabel/netlabel_unlabeled.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/net/netlabel/netlabel_unlabeled.c	2010-03-19 13:28:06.000000000 -0400
@@ -327,7 +327,7 @@ static int netlbl_unlhsh_add_addr4(struc
 	entry->list.addr = addr->s_addr & mask->s_addr;
 	entry->list.mask = mask->s_addr;
 	entry->list.valid = 1;
-	INIT_RCU_HEAD(&entry->rcu);
+	rcu_head_init(&entry->rcu);
 	entry->secid = secid;
 
 	spin_lock(&netlbl_unlhsh_lock);
@@ -373,7 +373,7 @@ static int netlbl_unlhsh_add_addr6(struc
 	entry->list.addr.s6_addr32[3] &= mask->s6_addr32[3];
 	ipv6_addr_copy(&entry->list.mask, mask);
 	entry->list.valid = 1;
-	INIT_RCU_HEAD(&entry->rcu);
+	rcu_head_init(&entry->rcu);
 	entry->secid = secid;
 
 	spin_lock(&netlbl_unlhsh_lock);
@@ -410,7 +410,7 @@ static struct netlbl_unlhsh_iface *netlb
 	INIT_LIST_HEAD(&iface->addr4_list);
 	INIT_LIST_HEAD(&iface->addr6_list);
 	iface->valid = 1;
-	INIT_RCU_HEAD(&iface->rcu);
+	rcu_head_init(&iface->rcu);
 
 	spin_lock(&netlbl_unlhsh_lock);
 	if (ifindex > 0) {
Index: linux-2.6-lttng/net/sctp/bind_addr.c
===================================================================
--- linux-2.6-lttng.orig/net/sctp/bind_addr.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/net/sctp/bind_addr.c	2010-03-19 13:28:06.000000000 -0400
@@ -186,7 +186,7 @@ int sctp_add_bind_addr(struct sctp_bind_
 	addr->valid = 1;
 
 	INIT_LIST_HEAD(&addr->list);
-	INIT_RCU_HEAD(&addr->rcu);
+	rcu_head_init(&addr->rcu);
 
 	/* We always hold a socket lock when calling this function,
 	 * and that acts as a writer synchronizing lock.
Index: linux-2.6-lttng/net/sctp/ipv6.c
===================================================================
--- linux-2.6-lttng.orig/net/sctp/ipv6.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/net/sctp/ipv6.c	2010-03-19 13:28:06.000000000 -0400
@@ -381,7 +381,7 @@ static void sctp_v6_copy_addrlist(struct
 			addr->a.v6.sin6_scope_id = dev->ifindex;
 			addr->valid = 1;
 			INIT_LIST_HEAD(&addr->list);
-			INIT_RCU_HEAD(&addr->rcu);
+			rcu_head_init(&addr->rcu);
 			list_add_tail(&addr->list, addrlist);
 		}
 	}
Index: linux-2.6-lttng/net/sctp/protocol.c
===================================================================
--- linux-2.6-lttng.orig/net/sctp/protocol.c	2010-03-19 13:23:39.000000000 -0400
+++ linux-2.6-lttng/net/sctp/protocol.c	2010-03-19 13:28:06.000000000 -0400
@@ -188,7 +188,7 @@ static void sctp_v4_copy_addrlist(struct
 			addr->a.v4.sin_addr.s_addr = ifa->ifa_local;
 			addr->valid = 1;
 			INIT_LIST_HEAD(&addr->list);
-			INIT_RCU_HEAD(&addr->rcu);
+			rcu_head_init(&addr->rcu);
 			list_add_tail(&addr->list, addrlist);
 		}
 	}

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [RFC patch 2/3] tree/tiny rcu: Add debug RCU head objects (v3)

[Alexey Dobriyan]

On Fri, Mar 19, 2010 at 04:47:41PM -0400, Mathieu Desnoyers wrote:
> Helps finding racy users of call_rcu(), which results in hangs because list
> entries are overwritten and/or skipped.
> 
> This new patch version is based on the debugobjects with the newly introduced
> "active state" tracker.
> 
> Non-initialized entries are all considered as "statically initialized". An
> activation fixup (triggered by call_rcu()) takes care of performing the debug
> object initialization without issuing any warning. Since we cannot increase the
> size of struct rcu_head, I don't see much room to put an identifier for
> statically initialized rcu_head structures. So for now, we have to live without
> "activation without explicit init" detection. But the main purpose of this debug
> option is to detect double-activations (double call_rcu() use of a rcu_head
> before the callback is executed), which is correctly addressed here.
> 
> This also detects potential internal RCU callback corruption, which would cause
> the callbacks to be executed twice.

Is this useful?

Basic usage is so there no double call_rcu():

	if (atomic_dec_and_test())
		call_rcu()

Re: [RFC patch 2/3] tree/tiny rcu: Add debug RCU head objects (v3)

[Paul E. McKenney]

On Sat, Mar 20, 2010 at 12:10:00AM +0200, Alexey Dobriyan wrote:
> On Fri, Mar 19, 2010 at 04:47:41PM -0400, Mathieu Desnoyers wrote:
> > Helps finding racy users of call_rcu(), which results in hangs because list
> > entries are overwritten and/or skipped.
> > 
> > This new patch version is based on the debugobjects with the newly introduced
> > "active state" tracker.
> > 
> > Non-initialized entries are all considered as "statically initialized". An
> > activation fixup (triggered by call_rcu()) takes care of performing the debug
> > object initialization without issuing any warning. Since we cannot increase the
> > size of struct rcu_head, I don't see much room to put an identifier for
> > statically initialized rcu_head structures. So for now, we have to live without
> > "activation without explicit init" detection. But the main purpose of this debug
> > option is to detect double-activations (double call_rcu() use of a rcu_head
> > before the callback is executed), which is correctly addressed here.
> > 
> > This also detects potential internal RCU callback corruption, which would cause
> > the callbacks to be executed twice.
> 
> Is this useful?
> 
> Basic usage is so there no double call_rcu():
> 
> 	if (atomic_dec_and_test())
> 		call_rcu()

I believe that it is.  There have been a few cases of call_rcu() being
invoked twice without a grace period between the two invocations.
Mathieu's patch would catch this sort of misbehavior.

That said, I do agree that if everyone followed the rules, there would
be no need for Mathieu's patch -- and there would be no need for much
else, besides.  ;-)

							Thanx, Paul

Re: [RFC patch 2/3] tree/tiny rcu: Add debug RCU head objects (v3)

[Lai Jiangshan]

Very nice you do not touch struct rcu_head.

Mathieu Desnoyers wrote:

> +
> +#ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
> +# define STATE_RCU_HEAD_READY	0
> +# define STATE_RCU_HEAD_QUEUED	1
> +
> +extern struct debug_obj_descr rcuhead_debug_descr;
> +
> +static inline void debug_rcu_head_queue(struct rcu_head *head)
> +{
> +	debug_object_activate(head, &rcuhead_debug_descr);
> +	debug_object_active_state(head, &rcuhead_debug_descr,
> +				  STATE_RCU_HEAD_READY,
> +				  STATE_RCU_HEAD_QUEUED);
> +}
> +
> +static inline void debug_rcu_head_unqueue(struct rcu_head *head)
> +{
> +	debug_object_active_state(head, &rcuhead_debug_descr,
> +				  STATE_RCU_HEAD_QUEUED,
> +				  STATE_RCU_HEAD_READY);
> +	debug_object_deactivate(head, &rcuhead_debug_descr);
> +}

I'm a little foolish. I don't know why we need
STATE_RCU_HEAD_READY/QUEUED.

1) obj->astate does not be set to STATE_RCU_HEAD_READY in
debug_object_activate/init().

Maybe you need to add a function:
debug_object_activate_with_astate(head, descr, init_astate);
(the same as debug_object_activate(), but init obj->astate also)

Or you need:
@debugobjects.h
# define DEBUG_OBJECT_ASTATE_INIT 0
@rcupdate.c
# define STATE_RCU_HEAD_READY	DEBUG_OBJECT_ASTATE_INIT
# define STATE_RCU_HEAD_QUEUED	(STATE_RCU_HEAD_READY + 1)

2) In debug_rcu_head_queue(), debug_object_active_state()
   is always success when debug_object_activate() is success.

   In debug_rcu_head_unqueue(), debug_object_active_state()
   is always success if RCU subsystem is running correctly.

(Correct me if I'm wrong) So we don't need debug_object_active_state()
here if we just find racy users of call_rcu().

> + */
> +static int rcuhead_fixup_init(void *addr, enum debug_obj_state state)
> +{
> +	struct rcu_head *head = addr;
> +
> +	switch (state) {
> +	case ODEBUG_STATE_ACTIVE:
> +		/*
> +		 * Ensure that queued callbacks are all executed.
> +		 * If we detect that we are nested in a RCU read-side critical
> +		 * section, we should simply fail, otherwise we would deadlock.
> +		 */
> +		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
> +		    irqs_disabled()) {
> +			WARN_ON(1);
> +			return 0;
> +		}

preempt_count() != 0 ??? What happen when !CONFIG_PREEMPT?

> +		rcu_barrier();

We may need call rcu_barrier(), rcu_barrier_sched(), rcu_barrier_bh() together?
rcu_barrier() does not ensure to flush callbacks of RCU_BH, RCU_SCHED.

> +		debug_object_init(head, &rcuhead_debug_descr);
> +		return 1;
> +	default:
> +		return 0;
> +	}
> +}
> +

Re: [RFC patch 2/3] tree/tiny rcu: Add debug RCU head objects (v3)

[Mathieu Desnoyers]

* Lai Jiangshan (laijs@cn.fujitsu.com) wrote:
> Very nice you do not touch struct rcu_head.
> 
> Mathieu Desnoyers wrote:
> 
> > +
> > +#ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
> > +# define STATE_RCU_HEAD_READY	0
> > +# define STATE_RCU_HEAD_QUEUED	1
> > +
> > +extern struct debug_obj_descr rcuhead_debug_descr;
> > +
> > +static inline void debug_rcu_head_queue(struct rcu_head *head)
> > +{
> > +	debug_object_activate(head, &rcuhead_debug_descr);
> > +	debug_object_active_state(head, &rcuhead_debug_descr,
> > +				  STATE_RCU_HEAD_READY,
> > +				  STATE_RCU_HEAD_QUEUED);
> > +}
> > +
> > +static inline void debug_rcu_head_unqueue(struct rcu_head *head)
> > +{
> > +	debug_object_active_state(head, &rcuhead_debug_descr,
> > +				  STATE_RCU_HEAD_QUEUED,
> > +				  STATE_RCU_HEAD_READY);
> > +	debug_object_deactivate(head, &rcuhead_debug_descr);
> > +}
> 
> I'm a little foolish. I don't know why we need
> STATE_RCU_HEAD_READY/QUEUED.
> 
> 1) obj->astate does not be set to STATE_RCU_HEAD_READY in
> debug_object_activate/init().

Yes it is. astate is set to 0 upon activation, and STATE_RCU_HEAD_READY is 0.
This is the initial state of the state machine (and also the accepting state).
Maybe I could document it a little bit more. Will write:

+/*
+ * Active state:
+ * - Set at 0 upon initialization.
+ * - Must return to 0 before deactivation.
+ */
+extern void
+debug_object_active_state(void *addr, struct debug_obj_descr *descr,
+                         unsigned int expect, unsigned int next);
+


> 
> Maybe you need to add a function:
> debug_object_activate_with_astate(head, descr, init_astate);
> (the same as debug_object_activate(), but init obj->astate also)
> 
> Or you need:
> @debugobjects.h
> # define DEBUG_OBJECT_ASTATE_INIT 0
> @rcupdate.c
> # define STATE_RCU_HEAD_READY	DEBUG_OBJECT_ASTATE_INIT
> # define STATE_RCU_HEAD_QUEUED	(STATE_RCU_HEAD_READY + 1)
> 
> 2) In debug_rcu_head_queue(), debug_object_active_state()
>    is always success when debug_object_activate() is success.
> 
>    In debug_rcu_head_unqueue(), debug_object_active_state()
>    is always success if RCU subsystem is running correctly.
> 
> (Correct me if I'm wrong) So we don't need debug_object_active_state()
> here if we just find racy users of call_rcu().

Yes, but this debug infrastructure also detects problems that may arise in the
RCU subsystem itself: e.g. a rcu head that would be executed on two CPUs, which
could be caused by races between cpu hotplug and the RCU subsystem. I'm not
saying that we have these races currently, but that the RCU subsystems is now so
tied to complex parts of the kernel (scheduler, cpu hotplug) that it's bound to
have nasty bugs eventually.

So without the state machine, what we don't have in debugobjects is the ability
to detect a second "deactivation": debugobjects considers that an object can be
deactivated more than once without error. But in this case, just one
deactivation should be allowed. I thought that this state machine scheme would
allow following an object life more closely than just the
init/activate/deactivate/destroy/free phases. Maybe there could be ways to
combine the calls ? But.. given that it's for debugging purposes, do we care
that much about performance that it's worth creating new API members, making the
API more obscure ?


> 
> > + */
> > +static int rcuhead_fixup_init(void *addr, enum debug_obj_state state)
> > +{
> > +	struct rcu_head *head = addr;
> > +
> > +	switch (state) {
> > +	case ODEBUG_STATE_ACTIVE:
> > +		/*
> > +		 * Ensure that queued callbacks are all executed.
> > +		 * If we detect that we are nested in a RCU read-side critical
> > +		 * section, we should simply fail, otherwise we would deadlock.
> > +		 */
> > +		if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
> > +		    irqs_disabled()) {
> > +			WARN_ON(1);
> > +			return 0;
> > +		}
> 
> preempt_count() != 0 ??? What happen when !CONFIG_PREEMPT?

Will change for:

#ifndef CONFIG_PREEMPT
                WARN_ON(1);
                return 0;
#else
                if (rcu_preempt_depth() != 0 || preempt_count() != 0 ||
                    irqs_disabled()) {
                        WARN_ON(1);
                        return 0;
                }
                rcu_barrier();
                rcu_barrier_sched();
                rcu_barrier_bh();
                debug_object_activate(head, &rcuhead_debug_descr);
                return 1;
#endif

> 
> > +		rcu_barrier();
> 
> We may need call rcu_barrier(), rcu_barrier_sched(), rcu_barrier_bh() together?
> rcu_barrier() does not ensure to flush callbacks of RCU_BH, RCU_SCHED.

Right, will do.

Thanks,

Mathieu


> 
> > +		debug_object_init(head, &rcuhead_debug_descr);
> > +		return 1;
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com