Here is the combined patch. I've tested it only briefly, and I am
unsure of whether it still produces lockdep warnings for Daniel or
not. I wish it would not be applied anywhere unless it was
officially Reviewed-by: someone. In particular, I'm not quite
steady with the irq-safe locking (Thomas might want to have a look).

Thanks :)


Vegard


From 977cf583b79be7308d5e310711fe6038c8af96a4 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Thu, 28 Aug 2008 17:09:57 +0200
Subject: [PATCH] debugobjects: fix lockdep warning #2


We fix it by moving the actual freeing to outside the lock (the lock
now only protects the list).

The lock is also promoted to irq-safe (suggested by Dan).

Reported-by: Daniel J Blueman <daniel.blueman@gmail.com>
Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
---
 lib/debugobjects.c |   38 +++++++++++++++++++++++++++++---------
 1 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 19acf8c..acf9ed8 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -115,9 +115,10 @@ static struct debug_obj *lookup_object(void *addr, struct debug_bucket *b)
 static struct debug_obj *
 alloc_object(void *addr, struct debug_bucket *b, struct debug_obj_descr *descr)
 {
+	unsigned long flags;
 	struct debug_obj *obj = NULL;
 
-	spin_lock(&pool_lock);
+	spin_lock_irqsave(&pool_lock, flags);
 	if (obj_pool.first) {
 		obj	    = hlist_entry(obj_pool.first, typeof(*obj), node);
 
@@ -136,7 +137,7 @@ alloc_object(void *addr, struct debug_bucket *b, struct debug_obj_descr *descr)
 		if (obj_pool_free < obj_pool_min_free)
 			obj_pool_min_free = obj_pool_free;
 	}
-	spin_unlock(&pool_lock);
+	spin_unlock_irqrestore(&pool_lock, flags);
 
 	return obj;
 }
@@ -146,18 +147,19 @@ alloc_object(void *addr, struct debug_bucket *b, struct debug_obj_descr *descr)
  */
 static void free_object(struct debug_obj *obj)
 {
+	unsigned long flags;
 	unsigned long idx = ...
[ message continues ]

On Thu, 28 Aug 2008 17:32:14 +0200


What was the reason for this other change?  I'm sure Dan is a fine chap,

I suspect that we can avoid the hlist_del() here, perhaps with a little

and the other one.

But I'm not sure that it's worth putting effort into - leaving dead
objects strung onto a partially-live list is a little bit smelly IMO.

--

[ message continues ]

I really feel better, when we delete them instead of throwing them
away with pointers to each other.

Thanks,

	tglx
--

[ message continues ]

Hi Andrew, Vegard, Ingo,

On Fri, Aug 29, 2008 at 1:17 AM, Andrew Morton

IRQ-safe xtime_lock is taken, then pool_lock is taken in
__debug_object_init, which is potentially unsafe. Upgrading
pool_lock's usage to IRQ-safe ensures there can be no potential for

I've done some fairly heavy testing with the patch at it's current
state (ie with the upgraded pool_lock, explained above), and it _is_
in fact solid; I wasn't looking at the right setup previously.

(with the other XFS tweaks too) I'm not able to cause any
deadlocks/stack traces/warnings with maximum debugging [* the KVM
errors are another story], which would be the first time so far, so
the patch looks good for mainline and 2.6.27 is looking very strong!

Daniel

--- [*]

emulation failed (pagetable) rip bf9032c5 0f 6f 17 0f
__ratelimit: 1804664 callbacks suppressed
Fail to handle apic access vmexit! Offset is 0xf0
-- 
Daniel J Blueman
--

[ message continues ]

Linus,

Please pull the latest core/debugobjects git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git core/debugobjects

It contains a lockdep fix for the debug objects core code. Please note
that I added a helper function to move a hlist from one head to
another to avoid open coding it in the debugobjects code.

 Thanks,

	tglx

------------------>
Vegard Nossum (1):
      debugobjects: fix lockdep warning


 include/linux/list.h |   13 +++++++++++++
 lib/debugobjects.c   |   31 +++++++++++++++++++++++--------
 2 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/include/linux/list.h b/include/linux/list.h
index db35ef0..969f6e9 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -619,6 +619,19 @@ static inline void hlist_add_after(struct hlist_node *n,
 		next->next->pprev  = &next->next;
 }
 
+/*
+ * Move a list from one list head to another. Fixup the pprev
+ * reference of the first entry if it exists.
+ */
+static inline void hlist_move_list(struct hlist_head *old,
+				   struct hlist_head *new)
+{
+	new->first = old->first;
+	if (new->first)
+		new->first->pprev = &new->first;
+	old->first = NULL;
+}
+
 #define hlist_entry(ptr, type, member) container_of(ptr,type,member)
 
 #define hlist_for_each(pos, head) \
diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 45a6bde..e3ab374 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -112,6 +112,7 @@ static struct debug_obj *lookup_object(void *addr, struct debug_bucket *b)
 
 /*
  * Allocate a new object. If the pool is empty, switch off the debugger.
+ * Must be called with interrupts disabled.
  */
 static struct debug_obj *
 alloc_object(void *addr, struct debug_bucket *b, struct debug_obj_descr *descr)
@@ -148,17 +149,18 @@ alloc_object(void *addr, struct debug_bucket *b, struct debug_obj_descr *descr)
 static void free_object(struct debug_obj *obj)
 {
 	unsigned long idx = (unsigned long)(obj - ...
[ message continues ]

The irq safe locking is the easy part :) Calling free_object w/o the
hash bucket lock held is fine as we removed it already from the bucket

What you want is a helper function which moves the complete list from
one head to the other. I'll whip one up.

I'll push it through the tip tree.

Thanks,

	tglx
--

[ message continues ]