Oops when accessing /proc/lockdep_chains

Oops when accessing /proc/lockdep_chains

[Eric Sesterhenn]

hi,

with current -git (aka -rc2) i sometime get the following oops
when doing a cat /proc/lockdep_chains
The other /proc/lockdep* files dont cause any errors

I dont get it after a fresh reboot :| But was able to reproduce it when
running my testscripts, I'll try to narrow it down.

[  584.458673] BUG: unable to handle kernel paging request at d1a27580
[  584.459010] IP: [<c049403e>] strnlen+0xe/0x20
[  584.459340] Oops: 0000 [#1] DEBUG_PAGEALLOC
[  584.459596] Modules linked in: [last unloaded: rcutorture]
[  584.459923] 
[  584.460038] Pid: 9047, comm: cat Not tainted (2.6.27-rc2 #26)
[  584.460245] EIP: 0060:[<c049403e>] EFLAGS: 00010297 CPU: 0
[  584.460394] EIP is at strnlen+0xe/0x20
[  584.460532] EAX: d1a27580 EBX: c86efca8 ECX: d1a27580 EDX: fffffffe
[  584.460682] ESI: 00000000 EDI: c86f0000 EBP: c87fdce8 ESP: c87fdce8
[  584.460890]  DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
[  584.461035] Process cat (pid: 9047, ti=c87fd000 task=c87d2420 task.ti=c87fd000)
[  584.461193] Stack: c87fdd04 c0493036 c010317c d1a27580 c86efca8 00000000 c87fde5c c87fde34 
[  584.462138]        c0493457 ffffffff ffffffff 00000000 ffffffff ffffffff 00000000 00000358 
[  584.463021]        c86efca8 ffffffff c86f0000 ffffffff ffffffff ffffffff 0000000f c0a3b725 
[  584.463964] Call Trace:
[  584.464173]  [<c0493036>] ? string+0x26/0xa0
[  584.464510]  [<c010317c>] ? restore_nocheck_notrace+0x0/0xe
[  584.464859]  [<c0493457>] ? vsnprintf+0x3a7/0x6a0
[  584.465141]  [<c04940ac>] ? trace_hardirqs_on_thunk+0xc/0x10
[  584.465219]  [<c01059de>] ? do_softirq+0x3e/0xb0
[  584.465219]  [<c010317c>] ? restore_nocheck_notrace+0x0/0xe
[  584.465219]  [<c04934c4>] ? vsnprintf+0x414/0x6a0
[  584.465219]  [<c0149837>] ? __lock_acquire+0x257/0xe50
[  584.465219]  [<c01b46c2>] ? seq_printf+0x32/0x60
[  584.465219]  [<c014aa81>] ? print_name+0x31/0xb0
[  584.465219]  [<c0149260>] ? mark_held_locks+0x40/0x80
[  584.465219]  [<c01494ab>] ? trace_hardirqs_on+0xb/0x10
[  584.465219]  [<c0149409>] ? trace_hardirqs_on_caller+0xc9/0x160
[  584.465219]  [<c0830f64>] ? __mutex_lock_common+0x1e4/0x2e0
[  584.465219]  [<c01b4c1a>] ? seq_read+0x2a/0x2a0
[  584.465219]  [<c01b46c2>] ? seq_printf+0x32/0x60
[  584.465219]  [<c014ab6f>] ? lc_show+0x6f/0xc0
[  584.465219]  [<c01b4d5f>] ? seq_read+0x16f/0x2a0
[  584.465219]  [<c01b4bf0>] ? seq_read+0x0/0x2a0
[  584.465219]  [<c01d58f2>] ? proc_reg_read+0x62/0x90
[  584.465219]  [<c0199269>] ? vfs_read+0x99/0x160
[  584.465219]  [<c01d5890>] ? proc_reg_read+0x0/0x90
[  584.465219]  [<c0199842>] ? sys_read+0x42/0x70
[  584.465219]  [<c0103059>] ? sysenter_do_call+0x12/0x31
[  584.465219]  =======================
[  584.465219] Code: 57 0f 1f 44 00 00 85 c9 89 c7 89 d0 74 05 f2 ae 75 01 4f 89 f8 5f 5d c3 90 8d 74 26 00 55 89 e5 0f 1f 44 00 00 89 c1 89 c8 eb 06 <80> 38 00 74 07 40 4a 83 fa ff 75 f4 29 c8 5d c3 90 90 55 89 e5 
[  584.465219] EIP: [<c049403e>] strnlen+0xe/0x20 SS:ESP 0068:c87fdce8
[  584.465219] ---[ end trace 560310b80b6f2ef3 ]---

[  657.508210] BUG: unable to handle kernel paging request at d1a27580
[  657.508546] IP: [<c049403e>] strnlen+0xe/0x20
[  657.508827] *pde = 0ef53067 *pte = 00000000 
[  657.509067] Oops: 0000 [#2] DEBUG_PAGEALLOC
[  657.509364] Modules linked in: [last unloaded: rcutorture]
[  657.509382] 
[  657.509382] Pid: 14555, comm: cat Tainted: G      D   (2.6.27-rc2 #26)
[  657.509382] EIP: 0060:[<c049403e>] EFLAGS: 00010297 CPU: 0
[  657.509382] EIP is at strnlen+0xe/0x20
[  657.509382] EAX: d1a27580 EBX: c86ef36c ECX: d1a27580 EDX: fffffffe
[  657.509382] ESI: 00000000 EDI: c86f0000 EBP: c867fce8 ESP: c867fce8
[  657.509382]  DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
[  657.509382] Process cat (pid: 14555, ti=c867f000 task=c86a4420 task.ti=c867f000)
[  657.509382] Stack: c867fd04 c0493036 00000000 d1a27580 c86ef36c 00000000 c867fe5c c867fe34 
[  657.509382]        c0493457 ffffffff ffffffff 00000000 00000000 c867fd44 00000046 00000c94 
[  657.509382]        c86ef36c ffffffff c86f0000 ffffffff ffffffff ffffffff 0000000f c0a3b725 
[  657.509382] Call Trace:
[  657.509382]  [<c0493036>] ? string+0x26/0xa0
[  657.509382]  [<c0493457>] ? vsnprintf+0x3a7/0x6a0
[  657.509382]  [<c012417b>] ? try_to_wake_up+0x6b/0xf0
[  657.509382]  [<c04934c4>] ? vsnprintf+0x414/0x6a0
[  657.509382]  [<c0514aed>] ? n_tty_receive_buf+0x63d/0x1230
[  657.509382]  [<c01189ff>] ? __change_page_attr_set_clr+0xcf/0x4e0
[  657.509382]  [<c0164ef0>] ? handle_level_irq+0x0/0xd0
[  657.509382]  [<c0146d84>] ? trace_hardirqs_off_caller+0x14/0xa0
[  657.509382]  [<c01b46c2>] ? seq_printf+0x32/0x60
[  657.509382]  [<c014aa81>] ? print_name+0x31/0xb0
[  657.509382]  [<c0146d84>] ? trace_hardirqs_off_caller+0x14/0xa0
[  657.509382]  [<c0149355>] ? trace_hardirqs_on_caller+0x15/0x160
[  657.509382]  [<c0830f64>] ? __mutex_lock_common+0x1e4/0x2e0
[  657.509382]  [<c01b4c1a>] ? seq_read+0x2a/0x2a0
[  657.509382]  [<c01b46c2>] ? seq_printf+0x32/0x60
[  657.509382]  [<c014ab6f>] ? lc_show+0x6f/0xc0
[  657.509382]  [<c01b4d5f>] ? seq_read+0x16f/0x2a0
[  657.509382]  [<c01b4bf0>] ? seq_read+0x0/0x2a0
[  657.509382]  [<c01d58f2>] ? proc_reg_read+0x62/0x90
[  657.509382]  [<c0199269>] ? vfs_read+0x99/0x160
[  657.509382]  [<c01d5890>] ? proc_reg_read+0x0/0x90
[  657.509382]  [<c0199842>] ? sys_read+0x42/0x70
[  657.509382]  [<c0103059>] ? sysenter_do_call+0x12/0x31
[  657.509382]  [<c0124abf>] ? __cleanup_sighand+0x1f/0x30
[  657.509382]  =======================
[  657.509382] Code: 57 0f 1f 44 00 00 85 c9 89 c7 89 d0 74 05 f2 ae 75 01 4f 89 f8 5f 5d c3 90 8d 74 26 00 55 89 e5 0f 1f 44 00 00 89 c1 89 c8 eb 06 <80> 38 00 74 07 40 4a 83 fa ff 75 f4 29 c8 5d c3 90 90 55 89 e5 
[  657.509382] EIP: [<c049403e>] strnlen+0xe/0x20 SS:ESP 0068:c867fce8
[  657.510433] ---[ end trace 560310b80b6f2ef3 ]---



Greetings, Eric

Re: Oops when accessing /proc/lockdep_chains

[Eric Sesterhenn]

* Eric Sesterhenn (snakebyte@gmx.de) wrote:
> hi,
> 
> with current -git (aka -rc2) i sometime get the following oops
> when doing a cat /proc/lockdep_chains
> The other /proc/lockdep* files dont cause any errors
> 
> I dont get it after a fresh reboot :| But was able to reproduce it when
> running my testscripts, I'll try to narrow it down.

I can easily reproduce this with

modprobe rcutorture; sleep 10s; rmmod rcutorture; cat
/proc/lockdep_chains > /dev/null

Greetings, Eric

Re: Oops when accessing /proc/lockdep_chains

[Rabin Vincent]

On Wed, Aug 06, 2008 at 02:41:34PM +0200, Eric Sesterhenn wrote:
> * Eric Sesterhenn (snakebyte@gmx.de) wrote:
> > hi,
> > 
> > with current -git (aka -rc2) i sometime get the following oops
> > when doing a cat /proc/lockdep_chains
> > The other /proc/lockdep* files dont cause any errors
> > 
> > I dont get it after a fresh reboot :| But was able to reproduce it when
> > running my testscripts, I'll try to narrow it down.
> 
> I can easily reproduce this with
> 
> modprobe rcutorture; sleep 10s; rmmod rcutorture; cat
> /proc/lockdep_chains > /dev/null

Indeed, it oopses after any module which creates a lock is unloaded.
I'll send a patch for this shortly.

Rabin

[PATCH] lockdep: handle chains involving classes defined in modules

[Rabin Vincent]

/proc/lockdep_chains currently oopses after any module which creates and
uses a lock is unloaded.  This is because one of the chains involves a
class which was defined in the module just unloaded.

The classes are already correctly taken care of using the
all_lock_classes which keeps track of all active lock classses.  Add a
similar all_lock_chains list and use it for keeping track of chains.

Reported-by: Eric Sesterhenn <snakebyte@gmx.de>
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 include/linux/lockdep.h    |    3 +-
 kernel/lockdep.c           |   53 ++++++++++++++++++++++++++++++++++++++-----
 kernel/lockdep_internals.h |    2 +-
 kernel/lockdep_proc.c      |   17 ++++++++++----
 4 files changed, 61 insertions(+), 14 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 2486eb4..735d06b 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -185,7 +185,8 @@ struct lock_chain {
 	u8				irq_context;
 	u8				depth;
 	u16				base;
-	struct list_head		entry;
+	struct list_head		hash_entry;
+	struct list_head		lock_entry;
 	u64				chain_key;
 };
 
diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index d38a643..1b3537b 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -253,6 +253,11 @@ LIST_HEAD(all_lock_classes);
 static struct list_head classhash_table[CLASSHASH_SIZE];
 
 /*
+ * We also keep a global list of all lock chains.
+ */
+LIST_HEAD(all_lock_chains);
+
+/*
  * We put the lock dependency chains into a hash-table as well, to cache
  * their existence:
  */
@@ -1462,7 +1467,7 @@ out_bug:
 }
 
 unsigned long nr_lock_chains;
-struct lock_chain lock_chains[MAX_LOCKDEP_CHAINS];
+static struct lock_chain lock_chains[MAX_LOCKDEP_CHAINS];
 int nr_chain_hlocks;
 static u16 chain_hlocks[MAX_LOCKDEP_CHAIN_HLOCKS];
 
@@ -1493,7 +1498,7 @@ static inline int lookup_chain_cache(struct task_struct *curr,
 	 * We can walk it lock-free, because entries only get added
 	 * to the hash:
 	 */
-	list_for_each_entry(chain, hash_head, entry) {
+	list_for_each_entry(chain, hash_head, hash_entry) {
 		if (chain->chain_key == chain_key) {
 cache_hit:
 			debug_atomic_inc(&chain_lookup_hits);
@@ -1517,7 +1522,7 @@ cache_hit:
 	/*
 	 * We have to walk the chain again locked - to avoid duplicates:
 	 */
-	list_for_each_entry(chain, hash_head, entry) {
+	list_for_each_entry(chain, hash_head, hash_entry) {
 		if (chain->chain_key == chain_key) {
 			graph_unlock();
 			goto cache_hit;
@@ -1559,7 +1564,8 @@ cache_hit:
 		}
 		chain_hlocks[chain->base + j] = class - lock_classes;
 	}
-	list_add_tail_rcu(&chain->entry, hash_head);
+	list_add_tail_rcu(&chain->hash_entry, hash_head);
+	list_add_tail_rcu(&chain->lock_entry, &all_lock_chains);
 	debug_atomic_inc(&chain_lookup_misses);
 	inc_chains();
 
@@ -2967,6 +2973,8 @@ void lockdep_reset(void)
 	debug_locks = 1;
 	for (i = 0; i < CHAINHASH_SIZE; i++)
 		INIT_LIST_HEAD(chainhash_table + i);
+	for (i = 0; i < CLASSHASH_SIZE; i++)
+		INIT_LIST_HEAD(classhash_table + i);
 	raw_local_irq_restore(flags);
 }
 
@@ -2990,6 +2998,15 @@ static void zap_class(struct lock_class *class)
 
 }
 
+static void zap_chain(struct lock_chain *chain)
+{
+	/*
+	 * Unhash the chain and remove it from the all_lock_chains list:
+	 */
+	list_del_rcu(&chain->hash_entry);
+	list_del_rcu(&chain->lock_entry);
+}
+
 static inline int within(const void *addr, void *start, unsigned long size)
 {
 	return addr >= start && addr < start + size;
@@ -2997,23 +3014,45 @@ static inline int within(const void *addr, void *start, unsigned long size)
 
 void lockdep_free_key_range(void *start, unsigned long size)
 {
-	struct lock_class *class, *next;
+	struct lock_class *class, *nextclass;
+	struct lock_chain *chain, *nextchain;
 	struct list_head *head;
 	unsigned long flags;
-	int i;
+	int i, j;
 	int locked;
 
 	raw_local_irq_save(flags);
 	locked = graph_lock();
 
 	/*
+	 * Unhash all chains that involve classes created by this module:
+	 */
+	for (i = 0; i < CHAINHASH_SIZE; i++) {
+		head = chainhash_table + i;
+		if (list_empty(head))
+			continue;
+		list_for_each_entry_safe(chain, nextchain, head, hash_entry) {
+			for (j = 0; j < chain->depth; j++) {
+				class = lock_classes +
+					chain_hlocks[chain->base + j];
+
+				if (within(class->key, start, size) ||
+					within(class->name, start, size)) {
+					zap_chain(chain);
+					break;
+				}
+			}
+		}
+	}
+
+	/*
 	 * Unhash all classes that were created by this module:
 	 */
 	for (i = 0; i < CLASSHASH_SIZE; i++) {
 		head = classhash_table + i;
 		if (list_empty(head))
 			continue;
-		list_for_each_entry_safe(class, next, head, hash_entry) {
+		list_for_each_entry_safe(class, nextclass, head, hash_entry) {
 			if (within(class->key, start, size))
 				zap_class(class);
 			else if (within(class->name, start, size))
diff --git a/kernel/lockdep_internals.h b/kernel/lockdep_internals.h
index c3600a0..ebf2ecb 100644
--- a/kernel/lockdep_internals.h
+++ b/kernel/lockdep_internals.h
@@ -32,7 +32,7 @@
 #define MAX_STACK_TRACE_ENTRIES	262144UL
 
 extern struct list_head all_lock_classes;
-extern struct lock_chain lock_chains[];
+extern struct list_head all_lock_chains;
 
 extern void
 get_usage_chars(struct lock_class *class, char *c1, char *c2, char *c3, char *c4);
diff --git a/kernel/lockdep_proc.c b/kernel/lockdep_proc.c
index 9b0e940..4f447fd 100644
--- a/kernel/lockdep_proc.c
+++ b/kernel/lockdep_proc.c
@@ -190,8 +190,9 @@ static void *lc_next(struct seq_file *m, void *v, loff_t *pos)
 	else {
 		chain = v;
 
-		if (*pos < nr_lock_chains)
-			chain = lock_chains + *pos;
+		if (chain->lock_entry.next != &all_lock_chains)
+			chain = list_entry(chain->lock_entry.next,
+					   struct lock_chain, lock_entry);
 		else
 			chain = NULL;
 	}
@@ -201,11 +202,16 @@ static void *lc_next(struct seq_file *m, void *v, loff_t *pos)
 
 static void *lc_start(struct seq_file *m, loff_t *pos)
 {
+	struct lock_chain *chain;
+	loff_t i = 0;
+
 	if (*pos == 0)
 		return SEQ_START_TOKEN;
 
-	if (*pos < nr_lock_chains)
-		return lock_chains + *pos;
+	list_for_each_entry(chain, &all_lock_chains, lock_entry) {
+		if (++i == *pos)
+			return chain;
+	}
 
 	return NULL;
 }
@@ -252,7 +258,8 @@ static int lockdep_chains_open(struct inode *inode, struct file *file)
 		struct seq_file *m = file->private_data;
 
 		if (nr_lock_chains)
-			m->private = lock_chains;
+			m->private = list_entry(all_lock_chains.next,
+					struct lock_chain, lock_entry);
 		else
 			m->private = NULL;
 	}
-- 
1.5.6.3

Re: [PATCH] lockdep: handle chains involving classes defined in modules

[Huang Ying]

Hi, Rabin,

I think there is a simpler method to deal with this.

- Mark class as useless during zap_class()
- When output lock_chain, if some classes are useless, do not output the
class.

Best Regards,
Huang Ying

On Fri, 2008-08-08 at 02:27 +0530, Rabin Vincent wrote:
> /proc/lockdep_chains currently oopses after any module which creates and
> uses a lock is unloaded.  This is because one of the chains involves a
> class which was defined in the module just unloaded.
> 
> The classes are already correctly taken care of using the
> all_lock_classes which keeps track of all active lock classses.  Add a
> similar all_lock_chains list and use it for keeping track of chains.
> 
> Reported-by: Eric Sesterhenn <snakebyte@gmx.de>
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  include/linux/lockdep.h    |    3 +-
>  kernel/lockdep.c           |   53 ++++++++++++++++++++++++++++++++++++++-----
>  kernel/lockdep_internals.h |    2 +-
>  kernel/lockdep_proc.c      |   17 ++++++++++----
>  4 files changed, 61 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 2486eb4..735d06b 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -185,7 +185,8 @@ struct lock_chain {
>  	u8				irq_context;
>  	u8				depth;
>  	u16				base;
> -	struct list_head		entry;
> +	struct list_head		hash_entry;
> +	struct list_head		lock_entry;
>  	u64				chain_key;
>  };
>  
> diff --git a/kernel/lockdep.c b/kernel/lockdep.c
> index d38a643..1b3537b 100644
> --- a/kernel/lockdep.c
> +++ b/kernel/lockdep.c
> @@ -253,6 +253,11 @@ LIST_HEAD(all_lock_classes);
>  static struct list_head classhash_table[CLASSHASH_SIZE];
>  
>  /*
> + * We also keep a global list of all lock chains.
> + */
> +LIST_HEAD(all_lock_chains);
> +
> +/*
>   * We put the lock dependency chains into a hash-table as well, to cache
>   * their existence:
>   */
> @@ -1462,7 +1467,7 @@ out_bug:
>  }
>  
>  unsigned long nr_lock_chains;
> -struct lock_chain lock_chains[MAX_LOCKDEP_CHAINS];
> +static struct lock_chain lock_chains[MAX_LOCKDEP_CHAINS];
>  int nr_chain_hlocks;
>  static u16 chain_hlocks[MAX_LOCKDEP_CHAIN_HLOCKS];
>  
> @@ -1493,7 +1498,7 @@ static inline int lookup_chain_cache(struct task_struct *curr,
>  	 * We can walk it lock-free, because entries only get added
>  	 * to the hash:
>  	 */
> -	list_for_each_entry(chain, hash_head, entry) {
> +	list_for_each_entry(chain, hash_head, hash_entry) {
>  		if (chain->chain_key == chain_key) {
>  cache_hit:
>  			debug_atomic_inc(&chain_lookup_hits);
> @@ -1517,7 +1522,7 @@ cache_hit:
>  	/*
>  	 * We have to walk the chain again locked - to avoid duplicates:
>  	 */
> -	list_for_each_entry(chain, hash_head, entry) {
> +	list_for_each_entry(chain, hash_head, hash_entry) {
>  		if (chain->chain_key == chain_key) {
>  			graph_unlock();
>  			goto cache_hit;
> @@ -1559,7 +1564,8 @@ cache_hit:
>  		}
>  		chain_hlocks[chain->base + j] = class - lock_classes;
>  	}
> -	list_add_tail_rcu(&chain->entry, hash_head);
> +	list_add_tail_rcu(&chain->hash_entry, hash_head);
> +	list_add_tail_rcu(&chain->lock_entry, &all_lock_chains);
>  	debug_atomic_inc(&chain_lookup_misses);
>  	inc_chains();
>  
> @@ -2967,6 +2973,8 @@ void lockdep_reset(void)
>  	debug_locks = 1;
>  	for (i = 0; i < CHAINHASH_SIZE; i++)
>  		INIT_LIST_HEAD(chainhash_table + i);
> +	for (i = 0; i < CLASSHASH_SIZE; i++)
> +		INIT_LIST_HEAD(classhash_table + i);
>  	raw_local_irq_restore(flags);
>  }
>  
> @@ -2990,6 +2998,15 @@ static void zap_class(struct lock_class *class)
>  
>  }
>  
> +static void zap_chain(struct lock_chain *chain)
> +{
> +	/*
> +	 * Unhash the chain and remove it from the all_lock_chains list:
> +	 */
> +	list_del_rcu(&chain->hash_entry);
> +	list_del_rcu(&chain->lock_entry);
> +}
> +
>  static inline int within(const void *addr, void *start, unsigned long size)
>  {
>  	return addr >= start && addr < start + size;
> @@ -2997,23 +3014,45 @@ static inline int within(const void *addr, void *start, unsigned long size)
>  
>  void lockdep_free_key_range(void *start, unsigned long size)
>  {
> -	struct lock_class *class, *next;
> +	struct lock_class *class, *nextclass;
> +	struct lock_chain *chain, *nextchain;
>  	struct list_head *head;
>  	unsigned long flags;
> -	int i;
> +	int i, j;
>  	int locked;
>  
>  	raw_local_irq_save(flags);
>  	locked = graph_lock();
>  
>  	/*
> +	 * Unhash all chains that involve classes created by this module:
> +	 */
> +	for (i = 0; i < CHAINHASH_SIZE; i++) {
> +		head = chainhash_table + i;
> +		if (list_empty(head))
> +			continue;
> +		list_for_each_entry_safe(chain, nextchain, head, hash_entry) {
> +			for (j = 0; j < chain->depth; j++) {
> +				class = lock_classes +
> +					chain_hlocks[chain->base + j];
> +
> +				if (within(class->key, start, size) ||
> +					within(class->name, start, size)) {
> +					zap_chain(chain);
> +					break;
> +				}
> +			}
> +		}
> +	}
> +
> +	/*
>  	 * Unhash all classes that were created by this module:
>  	 */
>  	for (i = 0; i < CLASSHASH_SIZE; i++) {
>  		head = classhash_table + i;
>  		if (list_empty(head))
>  			continue;
> -		list_for_each_entry_safe(class, next, head, hash_entry) {
> +		list_for_each_entry_safe(class, nextclass, head, hash_entry) {
>  			if (within(class->key, start, size))
>  				zap_class(class);
>  			else if (within(class->name, start, size))
> diff --git a/kernel/lockdep_internals.h b/kernel/lockdep_internals.h
> index c3600a0..ebf2ecb 100644
> --- a/kernel/lockdep_internals.h
> +++ b/kernel/lockdep_internals.h
> @@ -32,7 +32,7 @@
>  #define MAX_STACK_TRACE_ENTRIES	262144UL
>  
>  extern struct list_head all_lock_classes;
> -extern struct lock_chain lock_chains[];
> +extern struct list_head all_lock_chains;
>  
>  extern void
>  get_usage_chars(struct lock_class *class, char *c1, char *c2, char *c3, char *c4);
> diff --git a/kernel/lockdep_proc.c b/kernel/lockdep_proc.c
> index 9b0e940..4f447fd 100644
> --- a/kernel/lockdep_proc.c
> +++ b/kernel/lockdep_proc.c
> @@ -190,8 +190,9 @@ static void *lc_next(struct seq_file *m, void *v, loff_t *pos)
>  	else {
>  		chain = v;
>  
> -		if (*pos < nr_lock_chains)
> -			chain = lock_chains + *pos;
> +		if (chain->lock_entry.next != &all_lock_chains)
> +			chain = list_entry(chain->lock_entry.next,
> +					   struct lock_chain, lock_entry);
>  		else
>  			chain = NULL;
>  	}
> @@ -201,11 +202,16 @@ static void *lc_next(struct seq_file *m, void *v, loff_t *pos)
>  
>  static void *lc_start(struct seq_file *m, loff_t *pos)
>  {
> +	struct lock_chain *chain;
> +	loff_t i = 0;
> +
>  	if (*pos == 0)
>  		return SEQ_START_TOKEN;
>  
> -	if (*pos < nr_lock_chains)
> -		return lock_chains + *pos;
> +	list_for_each_entry(chain, &all_lock_chains, lock_entry) {
> +		if (++i == *pos)
> +			return chain;
> +	}
>  
>  	return NULL;
>  }
> @@ -252,7 +258,8 @@ static int lockdep_chains_open(struct inode *inode, struct file *file)
>  		struct seq_file *m = file->private_data;
>  
>  		if (nr_lock_chains)
> -			m->private = lock_chains;
> +			m->private = list_entry(all_lock_chains.next,
> +					struct lock_chain, lock_entry);
>  		else
>  			m->private = NULL;
>  	}

Re: [PATCH] lockdep: handle chains involving classes defined in modules

[Peter Zijlstra]

On Fri, 2008-08-08 at 02:27 +0530, Rabin Vincent wrote:
> /proc/lockdep_chains currently oopses after any module which creates and
> uses a lock is unloaded.  This is because one of the chains involves a
> class which was defined in the module just unloaded.
> 
> The classes are already correctly taken care of using the
> all_lock_classes which keeps track of all active lock classses.  Add a
> similar all_lock_chains list and use it for keeping track of chains.
> 
> Reported-by: Eric Sesterhenn <snakebyte@gmx.de>
> Signed-off-by: Rabin Vincent <rabin@rab.in>

Looks good - I was about to start poking at this and then I found your
patch in my mailbox, most appreciated!

Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

> ---
>  include/linux/lockdep.h    |    3 +-
>  kernel/lockdep.c           |   53 ++++++++++++++++++++++++++++++++++++++-----
>  kernel/lockdep_internals.h |    2 +-
>  kernel/lockdep_proc.c      |   17 ++++++++++----
>  4 files changed, 61 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 2486eb4..735d06b 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -185,7 +185,8 @@ struct lock_chain {
>  	u8				irq_context;
>  	u8				depth;
>  	u16				base;
> -	struct list_head		entry;
> +	struct list_head		hash_entry;
> +	struct list_head		lock_entry;
>  	u64				chain_key;
>  };
>  
> diff --git a/kernel/lockdep.c b/kernel/lockdep.c
> index d38a643..1b3537b 100644
> --- a/kernel/lockdep.c
> +++ b/kernel/lockdep.c
> @@ -253,6 +253,11 @@ LIST_HEAD(all_lock_classes);
>  static struct list_head classhash_table[CLASSHASH_SIZE];
>  
>  /*
> + * We also keep a global list of all lock chains.
> + */
> +LIST_HEAD(all_lock_chains);
> +
> +/*
>   * We put the lock dependency chains into a hash-table as well, to cache
>   * their existence:
>   */
> @@ -1462,7 +1467,7 @@ out_bug:
>  }
>  
>  unsigned long nr_lock_chains;
> -struct lock_chain lock_chains[MAX_LOCKDEP_CHAINS];
> +static struct lock_chain lock_chains[MAX_LOCKDEP_CHAINS];
>  int nr_chain_hlocks;
>  static u16 chain_hlocks[MAX_LOCKDEP_CHAIN_HLOCKS];
>  
> @@ -1493,7 +1498,7 @@ static inline int lookup_chain_cache(struct task_struct *curr,
>  	 * We can walk it lock-free, because entries only get added
>  	 * to the hash:
>  	 */
> -	list_for_each_entry(chain, hash_head, entry) {
> +	list_for_each_entry(chain, hash_head, hash_entry) {
>  		if (chain->chain_key == chain_key) {
>  cache_hit:
>  			debug_atomic_inc(&chain_lookup_hits);
> @@ -1517,7 +1522,7 @@ cache_hit:
>  	/*
>  	 * We have to walk the chain again locked - to avoid duplicates:
>  	 */
> -	list_for_each_entry(chain, hash_head, entry) {
> +	list_for_each_entry(chain, hash_head, hash_entry) {
>  		if (chain->chain_key == chain_key) {
>  			graph_unlock();
>  			goto cache_hit;
> @@ -1559,7 +1564,8 @@ cache_hit:
>  		}
>  		chain_hlocks[chain->base + j] = class - lock_classes;
>  	}
> -	list_add_tail_rcu(&chain->entry, hash_head);
> +	list_add_tail_rcu(&chain->hash_entry, hash_head);
> +	list_add_tail_rcu(&chain->lock_entry, &all_lock_chains);
>  	debug_atomic_inc(&chain_lookup_misses);
>  	inc_chains();
>  
> @@ -2967,6 +2973,8 @@ void lockdep_reset(void)
>  	debug_locks = 1;
>  	for (i = 0; i < CHAINHASH_SIZE; i++)
>  		INIT_LIST_HEAD(chainhash_table + i);
> +	for (i = 0; i < CLASSHASH_SIZE; i++)
> +		INIT_LIST_HEAD(classhash_table + i);
>  	raw_local_irq_restore(flags);
>  }
>  
> @@ -2990,6 +2998,15 @@ static void zap_class(struct lock_class *class)
>  
>  }
>  
> +static void zap_chain(struct lock_chain *chain)
> +{
> +	/*
> +	 * Unhash the chain and remove it from the all_lock_chains list:
> +	 */
> +	list_del_rcu(&chain->hash_entry);
> +	list_del_rcu(&chain->lock_entry);
> +}
> +
>  static inline int within(const void *addr, void *start, unsigned long size)
>  {
>  	return addr >= start && addr < start + size;
> @@ -2997,23 +3014,45 @@ static inline int within(const void *addr, void *start, unsigned long size)
>  
>  void lockdep_free_key_range(void *start, unsigned long size)
>  {
> -	struct lock_class *class, *next;
> +	struct lock_class *class, *nextclass;
> +	struct lock_chain *chain, *nextchain;
>  	struct list_head *head;
>  	unsigned long flags;
> -	int i;
> +	int i, j;
>  	int locked;
>  
>  	raw_local_irq_save(flags);
>  	locked = graph_lock();
>  
>  	/*
> +	 * Unhash all chains that involve classes created by this module:
> +	 */
> +	for (i = 0; i < CHAINHASH_SIZE; i++) {
> +		head = chainhash_table + i;
> +		if (list_empty(head))
> +			continue;
> +		list_for_each_entry_safe(chain, nextchain, head, hash_entry) {
> +			for (j = 0; j < chain->depth; j++) {
> +				class = lock_classes +
> +					chain_hlocks[chain->base + j];
> +
> +				if (within(class->key, start, size) ||
> +					within(class->name, start, size)) {
> +					zap_chain(chain);
> +					break;
> +				}
> +			}
> +		}
> +	}
> +
> +	/*
>  	 * Unhash all classes that were created by this module:
>  	 */
>  	for (i = 0; i < CLASSHASH_SIZE; i++) {
>  		head = classhash_table + i;
>  		if (list_empty(head))
>  			continue;
> -		list_for_each_entry_safe(class, next, head, hash_entry) {
> +		list_for_each_entry_safe(class, nextclass, head, hash_entry) {
>  			if (within(class->key, start, size))
>  				zap_class(class);
>  			else if (within(class->name, start, size))
> diff --git a/kernel/lockdep_internals.h b/kernel/lockdep_internals.h
> index c3600a0..ebf2ecb 100644
> --- a/kernel/lockdep_internals.h
> +++ b/kernel/lockdep_internals.h
> @@ -32,7 +32,7 @@
>  #define MAX_STACK_TRACE_ENTRIES	262144UL
>  
>  extern struct list_head all_lock_classes;
> -extern struct lock_chain lock_chains[];
> +extern struct list_head all_lock_chains;
>  
>  extern void
>  get_usage_chars(struct lock_class *class, char *c1, char *c2, char *c3, char *c4);
> diff --git a/kernel/lockdep_proc.c b/kernel/lockdep_proc.c
> index 9b0e940..4f447fd 100644
> --- a/kernel/lockdep_proc.c
> +++ b/kernel/lockdep_proc.c
> @@ -190,8 +190,9 @@ static void *lc_next(struct seq_file *m, void *v, loff_t *pos)
>  	else {
>  		chain = v;
>  
> -		if (*pos < nr_lock_chains)
> -			chain = lock_chains + *pos;
> +		if (chain->lock_entry.next != &all_lock_chains)
> +			chain = list_entry(chain->lock_entry.next,
> +					   struct lock_chain, lock_entry);
>  		else
>  			chain = NULL;
>  	}
> @@ -201,11 +202,16 @@ static void *lc_next(struct seq_file *m, void *v, loff_t *pos)
>  
>  static void *lc_start(struct seq_file *m, loff_t *pos)
>  {
> +	struct lock_chain *chain;
> +	loff_t i = 0;
> +
>  	if (*pos == 0)
>  		return SEQ_START_TOKEN;
>  
> -	if (*pos < nr_lock_chains)
> -		return lock_chains + *pos;
> +	list_for_each_entry(chain, &all_lock_chains, lock_entry) {
> +		if (++i == *pos)
> +			return chain;
> +	}
>  
>  	return NULL;
>  }
> @@ -252,7 +258,8 @@ static int lockdep_chains_open(struct inode *inode, struct file *file)
>  		struct seq_file *m = file->private_data;
>  
>  		if (nr_lock_chains)
> -			m->private = lock_chains;
> +			m->private = list_entry(all_lock_chains.next,
> +					struct lock_chain, lock_entry);
>  		else
>  			m->private = NULL;
>  	}

Re: [PATCH] lockdep: handle chains involving classes defined in modules

[Rabin Vincent]

On Fri, Aug 08, 2008 at 11:24:37AM +0800, Huang Ying wrote:
> On Fri, 2008-08-08 at 02:27 +0530, Rabin Vincent wrote:
> > /proc/lockdep_chains currently oopses after any module which creates and
> > uses a lock is unloaded.  This is because one of the chains involves a
> > class which was defined in the module just unloaded.
> > 
> > The classes are already correctly taken care of using the
> > all_lock_classes which keeps track of all active lock classses.  Add a
> > similar all_lock_chains list and use it for keeping track of chains.
> > 
[...]
> 
> I think there is a simpler method to deal with this.

Yes.  I went with the all_lock_chains list approach because there was
similar code already being used to keep track of lock_class structures.

> - Mark class as useless during zap_class()
> - When output lock_chain, if some classes are useless, do not output the
> class.

Like the patch below?  I set ->key to NULL after zapping the class and
use that as a condition to not print the class' information.  The only
issue is that with this patch there will be some chains output with no
locks listed under them.

---
    lockdep: handle chains involving classes defined in modules
    
    /proc/lockdep_chains currently oopses after any module which creates and
    uses a lock is unloaded.  This is because one of the chains involves a
    class which was defined in the module just unloaded.
    
    Solve this by marking the classes as unused and not printing information
    about the unused classes.
    
    Reported-by: Eric Sesterhenn <snakebyte@gmx.de>
    Signed-off-by: Rabin Vincent <rabin@rab.in>

diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index d38a643..8ade874 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -2988,6 +2988,7 @@ static void zap_class(struct lock_class *class)
 	list_del_rcu(&class->hash_entry);
 	list_del_rcu(&class->lock_entry);
 
+	class->key = NULL;
 }
 
 static inline int within(const void *addr, void *start, unsigned long size)
diff --git a/kernel/lockdep_proc.c b/kernel/lockdep_proc.c
index 9b0e940..f09b6c7 100644
--- a/kernel/lockdep_proc.c
+++ b/kernel/lockdep_proc.c
@@ -229,6 +229,9 @@ static int lc_show(struct seq_file *m, void *v)
 
 	for (i = 0; i < chain->depth; i++) {
 		class = lock_chain_get_class(chain, i);
+		if (!class->key)
+			continue;
+
 		seq_printf(m, "[%p] ", class->key);
 		print_name(m, class);
 		seq_puts(m, "\n");

Re: [PATCH] lockdep: handle chains involving classes defined in modules

[Huang Ying]

On Sat, 2008-08-09 at 03:55 +0530, Rabin Vincent wrote:
> On Fri, Aug 08, 2008 at 11:24:37AM +0800, Huang Ying wrote:
> > On Fri, 2008-08-08 at 02:27 +0530, Rabin Vincent wrote:
> > > /proc/lockdep_chains currently oopses after any module which creates and
> > > uses a lock is unloaded.  This is because one of the chains involves a
> > > class which was defined in the module just unloaded.
> > > 
> > > The classes are already correctly taken care of using the
> > > all_lock_classes which keeps track of all active lock classses.  Add a
> > > similar all_lock_chains list and use it for keeping track of chains.
> > > 
> [...]
> > 
> > I think there is a simpler method to deal with this.
> 
> Yes.  I went with the all_lock_chains list approach because there was
> similar code already being used to keep track of lock_class structures.
> 
> > - Mark class as useless during zap_class()
> > - When output lock_chain, if some classes are useless, do not output the
> > class.
> 
> Like the patch below?  I set ->key to NULL after zapping the class and
> use that as a condition to not print the class' information.  The only
> issue is that with this patch there will be some chains output with no
> locks listed under them.
> 
> ---
>     lockdep: handle chains involving classes defined in modules
>     
>     /proc/lockdep_chains currently oopses after any module which creates and
>     uses a lock is unloaded.  This is because one of the chains involves a
>     class which was defined in the module just unloaded.
>     
>     Solve this by marking the classes as unused and not printing information
>     about the unused classes.
>     
>     Reported-by: Eric Sesterhenn <snakebyte@gmx.de>
>     Signed-off-by: Rabin Vincent <rabin@rab.in>
> 
> diff --git a/kernel/lockdep.c b/kernel/lockdep.c
> index d38a643..8ade874 100644
> --- a/kernel/lockdep.c
> +++ b/kernel/lockdep.c
> @@ -2988,6 +2988,7 @@ static void zap_class(struct lock_class *class)
>  	list_del_rcu(&class->hash_entry);
>  	list_del_rcu(&class->lock_entry);
>  
> +	class->key = NULL;
>  }
>  
>  static inline int within(const void *addr, void *start, unsigned long size)
> diff --git a/kernel/lockdep_proc.c b/kernel/lockdep_proc.c
> index 9b0e940..f09b6c7 100644
> --- a/kernel/lockdep_proc.c
> +++ b/kernel/lockdep_proc.c
> @@ -229,6 +229,9 @@ static int lc_show(struct seq_file *m, void *v)
>  
>  	for (i = 0; i < chain->depth; i++) {
>  		class = lock_chain_get_class(chain, i);
> +		if (!class->key)
> +			continue;
> +
>  		seq_printf(m, "[%p] ", class->key);
>  		print_name(m, class);
>  		seq_puts(m, "\n");

I think this patch is OK.

Acked-by: Huang Ying <ying.huang@intel.com>

Best Regards,
Huang Ying