login
Search
Hi, Here is an updated version of the rcu head debugobjects, following the comments I received in the last rounds. It applies on top of -tip, based on 2.6.34-rc2, commit 2e958f219d2b8d192d44e2472a214b3a93c44673 Unless people have any objection, it should be ready to be merged. I think the appropriate maintainer to perform this merge would be Paul E. McKenney, because this patchset is RCU-related. Thanks, Mathieu -- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68 --
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 | 11 ++++++++ lib/debugobjects.c | 59 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 67 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-22 10:06:23.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,15 @@ 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: + * - 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); + ...
Helps finding racy users of call_rcu(), which results in hangs because list entries are overwritten and/or skipped. Changelog since v3: - Include comments from Lai Jiangshan 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. [bissectability warning] This patch and "kernel call_rcu usage: initialize rcu_head structures" should be applied together. 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 | 168 +++++++++++++++++++++++++++++++++++++++++++++++ kernel/rcutiny.c | 2 kernel/rcutree.c | 2 lib/Kconfig.debug | 6 + 5 files changed, 235 insertions(+), 4 deletions(-) Index: ...
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Agreed, and I'm fine with the networking parts, so: Acked-by: David S. Miller <davem@davemloft.net> --
This should be very helpful in tracking down otherwise-painful bugs!!! Thank you, Mathieu!!! I am happy to apply this, especially given Dave Miller's Acked-by. A few questions and comments: o Patches 1/6, 2/6, 3/6: Was the intent for the three Subject lines to read as follows? [patch 1/6] Revert "net: remove INIT_RCU_HEAD() usage" [patch 2/6] Revert "netfilter: don't use INIT_RCU_HEAD()" [patch 3/6] Revert "net: don't use INIT_RCU_HEAD" o Patch 4/6 looks good to me, and given that Thomas hasn't complained, I am guessing that he is OK with it. o Would it be possible to make this bisectable as follows? a. Insert a new patch after current patch 4/6 that defines destroy_rcu_head_on_stack(), rcu_head_init_on_stack(), and rcu_head_init() with their !CONFIG_DEBUG_OBJECTS_RCU_HEAD definitions. b. Move current patch 6/6 to this position. c. Move current patch 5/6 to this position, updating to reflect the new patch added in (a) above. o Patch 6/6: Would it be possible to use the object_is_on_stack() function defined in include/linux/sched.h instead of passing in the flag on_stack to bdi_work_init()? It looks like fs/fs-writeback.c already includes include/linux/sched.h, so shouldn't be a problem from a #include-hell viewpoint. Please let me know if I am missing something on any of the above. If these changes seem reasonable to you, please either submit a new patch set or let me know that you are OK with me making these changes. Thanx, Paul --
Yep. Oops, got burnt by git show > patches/patchname.patch. Will fix and Wow, that's cool! We learn about exciting internal API functions every day, isn't life great ? I will definitely change the fs-writeback.c code to make use of it. We might event want to go further. A similar scheme could be used for the rcu_head debugobject activation fixup. Basically, I need a way to distinguish between: A) objects on stack and allocated objects and B) objects statically initialized So either we use something resembling: if (object_is_on_stack() || object_is_allocated()) or if (object_is_static()) As soon as I get the information about the static vs stack/alloc, I'll complete the update and re-send the updated patchset. Thanks, -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com --
Something close to "object_is_static()" would be "kernel_text_address()", but it only considers the text section addresses. I'd need something that would be broader than that, containing all core kernel and module bss and data sections. Still looking... Thanks, Mathieu -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com --
I was actually feeling pretty good about remembering object_is_on_stack() and finding it again. ;-) I am not seeing anything the identifies data or bss, and in any case, other situations such as per-CPU, __read_mostly, and who knows what all else would also need to be handled. So in the short term, my guess would be that it would be best to provide the three functions (possibly renaming them as noted above), but to leave the responsibility for figuring out which to invoke with the caller. Always happy to be proven wrong, of course! Thanx, Paul --
I think I found a neat way to do object_is_static(), which is all we need here. Can you have a look at the following patch ? If you think it's right, I'll add it to the patchset for the next submission. extable and module add object is static Adds an API to extable.c to check if an object is statically defined. This permits, along with object_is_on_stack(), to figure out is an object is either statically defined (object_is_static()) or allocated on the stack (object_is_on_stack()). Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> CC: David S. Miller <davem@davemloft.net> 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/kernel.h | 2 + kernel/extable.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) Index: linux.trees.git/include/linux/kernel.h =================================================================== --- linux.trees.git.orig/include/linux/kernel.h 2010-03-27 19:27:02.000000000 -0400 +++ linux.trees.git/include/linux/kernel.h 2010-03-27 19:28:02.000000000 -0400 @@ -218,6 +218,8 @@ extern int __kernel_text_address(unsigne extern int kernel_text_address(unsigned long addr); extern int func_ptr_is_kernel_text(void *ptr); +extern int object_is_static(void *obj); + struct pid; extern struct pid *session_of_pgrp(struct pid *pgrp); Index: linux.trees.git/kernel/extable.c =================================================================== --- linux.trees.git.orig/kernel/extable.c 2010-03-27 19:28:06.000000000 -0400 +++ linux.trees.git/kernel/extable.c 2010-03-27 19:54:40.000000000 -0400 @@ -60,6 +60,14 ...
Looks plausible to me! Rusty, any thoughts? --
This may only correct for UP. You may need arch-special codes for SMP. --
looking at include/linux/percpu.h:
#ifndef PERCPU_ENOUGH_ROOM
#define PERCPU_ENOUGH_ROOM \
(ALIGN(__per_cpu_end - __per_cpu_start, SMP_CACHE_BYTES) + \
PERCPU_MODULE_RESERVE)
#endif
I was under the impression that most architectures were keeping their per-cpu
data within the __per_cpu_start .. __per_cpu_end range. But I see that ia64
is the only one to redefine PERCPU_ENOUGH_ROOM. I'm not sure if it can be a
problem. Is that what you had in mind ?
(adding Tony and Fenghua in CC so they can confirm)
Thanks,
Mathieu
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
That's great! Tejun, can you point me out to an update version of these patches ? I am particularly interested in being able to know the range of statically defined per-cpu data. Thanks, Mathieu -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com --
Hello, http://thread.gmane.org/gmane.linux.kernel/958794/focus=959493 These were waiting for Rusty's ACK. They got ACKed today and will be pushed to mainline through percpu tree soonish. Thanks. -- tejun --
OK. I just figured that I could initialize the rcu_heads in all cases in the debugobject fixup anyway, so I guess I won't need "object_is_static()" after all. But I can keep the patch around so it can eventually be re-sent to standardize the debugobjects activation fixups. They currently need to keep a flag around to identify statically defined objects. Thanks, -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com --
Hi Paul, Thinking about the rcu head init topic, we might be able to drop the init_rcu_head() initializer. The idea is the following: - We need init_rcu_head_on_stack()/destroy_rcu_head_on_stack(). - call_rcu() populates the rcu_head and normally does not care about it being pre-initialized. - The activation fixup can detect if a non-initialized rcu head is being activated and just perform the fixup without complaining. - If we have two call_rcu() in a row in the same GP on the same rcu_head, the activation check will detect it. So either we remove all the init_rcu_head(), as was originally proposed, or we use one that is a no-op on !DEBUG configs and initialize the object with DEBUG configs. That removes the dependency on object_is_static(). Thoughts ? Thanks, Mathieu -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com --
If I understand correctly, this does sound good. Here is what I think you are proposing: o call_rcu() and friends only complain if handed an rcu_head structure that is still queued awaiting a grace period. They don't care otherwise. o rcu_do_batch() complains unless the rcu_head structure has most recently been enqueued by call_rcu() or one if its friends. Did I get it right? Thanx, Paul --
Exactly. Thanks, -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com --
Well, I'm a bit wary about that. The reason is that we really want
the annotation of:
init_on_stack();
....
destroy_on_stack();
instead of the confusing:
init();
....
destroy_on_stack();
So having an automatism in the bdi_work_init() function will people
make forget to put the destroy_on_stack() annotation into it.
The flag is horrible as well. How about this:
/* helper function, do not use in code ! */
__bdi_work_init(....., onstack)
{
....
if (on_stack) {
work.state |= WS_ONSTACK;
init_rcu_head_on_stack(&work.rcu_head);
} else {
....
}
See, how this moves also the "work.state |= WS_ONSTACK;" line out of
the calling code.
bdi_work_init(...)
{
__bdi_work_init(...., false);
}
bdi_work_init_on_stack(...)
{
__bdi_work_init(...., true);
}
out of the code.
To make it complete, please do not use the asymmetric:
destroy_rcu_head_on_stack(&work.rcu_head);
Create a helper function:
bdi_destroy_work_on_stack(...)
{
destroy_rcu_head_on_stack(work->rcu_head);
}
That makes it way more readable and we did that with the other on
stack initializers as well.
Thanks,
tglx
--
Hello, Thomas, I must defer to you on this one. ;-) Thanx, Paul --
These changes are queued for v3. Thanks Thomas! -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com --
