* [PATCH] timer: implement lockdep deadlock detection
@ 2009-01-26 22:59 Johannes Berg
2009-01-26 23:03 ` Johannes Berg
2009-01-26 23:07 ` Ingo Molnar
0 siblings, 2 replies; 22+ messages in thread
From: Johannes Berg @ 2009-01-26 22:59 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: Linux Kernel list, Peter Zijlstra, Ingo Molnar
This modifies the timer code in a way to allow lockdep to detect
deadlocks resulting from a lock being taken in the timer function
as well as around the del_timer_sync() call.
Validated with this module, otherwise bootup was clean.
--->%---
#include <linux/module.h>
#include <linux/timer.h>
#include <linux/spinlock.h>
static struct timer_list t;
static spinlock_t l;
MODULE_LICENSE("GPL");
void fn(unsigned long dummy)
{
spin_lock_irq(&l);
spin_unlock_irq(&l);
printk(KERN_DEBUG "TEST\n");
}
static int i(void)
{
spin_lock_init(&l);
setup_timer(&t, fn, 0);
mod_timer(&t, jiffies + HZ);
return 0;
}
module_init(i);
static void e(void)
{
spin_lock_irq(&l);
del_timer_sync(&t);
spin_unlock_irq(&l);
}
module_exit(e);
---%<---
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
include/linux/timer.h | 93 +++++++++++++++++++++++++++++++++++++++++---------
kernel/timer.c | 55 +++++++++++++++++++++++------
2 files changed, 121 insertions(+), 27 deletions(-)
--- wireless-testing.orig/include/linux/timer.h 2009-01-26 22:57:40.000000000 +0100
+++ wireless-testing/include/linux/timer.h 2009-01-26 22:57:52.000000000 +0100
@@ -21,52 +21,115 @@ struct timer_list {
char start_comm[16];
int start_pid;
#endif
+#ifdef CONFIG_LOCKDEP
+ struct lockdep_map lockdep_map;
+#endif
};
extern struct tvec_base boot_tvec_bases;
-#define TIMER_INITIALIZER(_function, _expires, _data) { \
- .entry = { .prev = TIMER_ENTRY_STATIC }, \
- .function = (_function), \
- .expires = (_expires), \
- .data = (_data), \
- .base = &boot_tvec_bases, \
+#ifdef CONFIG_LOCKDEP
+#define __TIMER_LOCKDEP_MAP_INITIALIZER(fn, name) \
+ .lockdep_map = STATIC_LOCKDEP_MAP_INIT(name, fn),
+#else
+#define __TIMER_LOCKDEP_MAP_INITIALIZER(fn, name)
+#endif
+
+#define TIMER_INITIALIZER(_function, _expires, _data) { \
+ .entry = { .prev = TIMER_ENTRY_STATIC }, \
+ .function = (_function), \
+ .expires = (_expires), \
+ .data = (_data), \
+ .base = &boot_tvec_bases, \
+ __TIMER_LOCKDEP_MAP_INITIALIZER((_function), #_function)\
}
#define DEFINE_TIMER(_name, _function, _expires, _data) \
struct timer_list _name = \
TIMER_INITIALIZER(_function, _expires, _data)
-void init_timer(struct timer_list *timer);
-void init_timer_deferrable(struct timer_list *timer);
+void init_timer_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
+void init_timer_deferrable_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
+
+#ifdef CONFIG_LOCKDEP
+#define init_timer(timer) \
+ do { \
+ static struct lock_class_key __key; \
+ init_timer_key((timer), #timer, &__key); \
+ } while (0)
+#define init_timer_deferrable(timer) \
+ do { \
+ static struct lock_class_key __key; \
+ init_timer_deferrable_key((timer), #timer, &__key); \
+ } while (0)
+#define init_timer_on_stack(timer) \
+ do { \
+ static struct lock_class_key __key; \
+ init_timer_on_stack_key((timer), #timer, &__key); \
+ } while (0)
+#define setup_timer(timer, fn, data) \
+ do { \
+ static struct lock_class_key __key; \
+ setup_timer_key((timer), #timer, &__key, (fn), (data));\
+ } while (0)
+#define setup_timer_on_stack(timer, fn, data) \
+ do { \
+ static struct lock_class_key __key; \
+ setup_timer_on_stack_key((timer), #timer, &__key, \
+ (fn), (data)); \
+ } while (0)
+#else
+#define init_timer(timer)\
+ init_timer_key((timer), NULL, NULL)
+#define init_timer_deferrable(timer)\
+ init_timer_deferrable_key((timer), NULL, NULL)
+#define init_timer_on_stack(timer)\
+ init_timer_on_stack_key((timer)), NULL, NULL)
+#define setup_timer(timer, fn, data)\
+ setup_timer_key((timer), NULL, NULL, (fn), (data))
+#define setup_timer_on_stack(timer, fn, data)\
+ setup_timer_on_stack_key((timer), NULL, NULL, (fn), (data))
+#endif
#ifdef CONFIG_DEBUG_OBJECTS_TIMERS
-extern void init_timer_on_stack(struct timer_list *timer);
+extern void init_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
extern void destroy_timer_on_stack(struct timer_list *timer);
#else
static inline void destroy_timer_on_stack(struct timer_list *timer) { }
-static inline void init_timer_on_stack(struct timer_list *timer)
+static inline void init_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
- init_timer(timer);
+ init_timer_key(timer, name, key);
}
#endif
-static inline void setup_timer(struct timer_list * timer,
+static inline void setup_timer_key(struct timer_list * timer,
+ const char *name,
+ struct lock_class_key *key,
void (*function)(unsigned long),
unsigned long data)
{
timer->function = function;
timer->data = data;
- init_timer(timer);
+ init_timer_key(timer, name, key);
}
-static inline void setup_timer_on_stack(struct timer_list *timer,
+static inline void setup_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key,
void (*function)(unsigned long),
unsigned long data)
{
timer->function = function;
timer->data = data;
- init_timer_on_stack(timer);
+ init_timer_on_stack_key(timer, name, key);
}
/**
--- wireless-testing.orig/kernel/timer.c 2009-01-26 22:57:40.000000000 +0100
+++ wireless-testing/kernel/timer.c 2009-01-26 23:37:33.000000000 +0100
@@ -491,14 +491,18 @@ static inline void debug_timer_free(stru
debug_object_free(timer, &timer_debug_descr);
}
-static void __init_timer(struct timer_list *timer);
-
-void init_timer_on_stack(struct timer_list *timer)
+static void __init_timer(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
+
+void init_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
debug_object_init_on_stack(timer, &timer_debug_descr);
- __init_timer(timer);
+ __init_timer(timer, name, key);
}
-EXPORT_SYMBOL_GPL(init_timer_on_stack);
+EXPORT_SYMBOL_GPL(init_timer_on_stack_key);
void destroy_timer_on_stack(struct timer_list *timer)
{
@@ -512,7 +516,9 @@ static inline void debug_timer_activate(
static inline void debug_timer_deactivate(struct timer_list *timer) { }
#endif
-static void __init_timer(struct timer_list *timer)
+static void __init_timer(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
timer->entry.next = NULL;
timer->base = __raw_get_cpu_var(tvec_bases);
@@ -521,6 +527,7 @@ static void __init_timer(struct timer_li
timer->start_pid = -1;
memset(timer->start_comm, 0, TASK_COMM_LEN);
#endif
+ lockdep_init_map(&timer->lockdep_map, name, key, 0);
}
/**
@@ -530,19 +537,23 @@ static void __init_timer(struct timer_li
* init_timer() must be done to a timer prior calling *any* of the
* other timer functions.
*/
-void init_timer(struct timer_list *timer)
+void init_timer_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
debug_timer_init(timer);
- __init_timer(timer);
+ __init_timer(timer, name, key);
}
-EXPORT_SYMBOL(init_timer);
+EXPORT_SYMBOL(init_timer_key);
-void init_timer_deferrable(struct timer_list *timer)
+void init_timer_deferrable_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
- init_timer(timer);
+ init_timer_key(timer, name, key);
timer_set_deferrable(timer);
}
-EXPORT_SYMBOL(init_timer_deferrable);
+EXPORT_SYMBOL(init_timer_deferrable_key);
static inline void detach_timer(struct timer_list *timer,
int clear_pending)
@@ -789,6 +800,15 @@ EXPORT_SYMBOL(try_to_del_timer_sync);
*/
int del_timer_sync(struct timer_list *timer)
{
+#ifdef CONFIG_LOCKDEP
+ unsigned long flags;
+
+ local_irq_save(flags);
+ lock_map_acquire(&timer->lockdep_map);
+ lock_map_release(&timer->lockdep_map);
+ local_irq_restore(flags);
+#endif
+
for (;;) {
int ret = try_to_del_timer_sync(timer);
if (ret >= 0)
@@ -861,10 +881,21 @@ static inline void __run_timers(struct t
set_running_timer(base, timer);
detach_timer(timer, 1);
+
spin_unlock_irq(&base->lock);
{
int preempt_count = preempt_count();
+
+ /* Couple the lock chain with the lock chain at
+ * del_timer_sync by acquiring the lock_map around
+ * the fn() call here and in del_timer_sync.
+ */
+ lock_map_acquire(&timer->lockdep_map);
+
fn(data);
+
+ lock_map_release(&timer->lockdep_map);
+
if (preempt_count != preempt_count()) {
printk(KERN_ERR "huh, entered %p "
"with preempt_count %08x, exited"
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] timer: implement lockdep deadlock detection
2009-01-26 22:59 [PATCH] timer: implement lockdep deadlock detection Johannes Berg
@ 2009-01-26 23:03 ` Johannes Berg
2009-01-26 23:07 ` Ingo Molnar
1 sibling, 0 replies; 22+ messages in thread
From: Johannes Berg @ 2009-01-26 23:03 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: Linux Kernel list, Peter Zijlstra, Ingo Molnar
[-- Attachment #1: Type: text/plain, Size: 3267 bytes --]
On Mon, 2009-01-26 at 23:59 +0100, Johannes Berg wrote:
> This modifies the timer code in a way to allow lockdep to detect
> deadlocks resulting from a lock being taken in the timer function
> as well as around the del_timer_sync() call.
>
> Validated with this module, otherwise bootup was clean.
Not having a proper stack trace in there is a little confusing:
[ 81.547455]
[ 81.547457] =======================================================
[ 81.547657] [ INFO: possible circular locking dependency detected ]
[ 81.547759] 2.6.29-rc2-wl-11979-gc32422d-dirty #24
[ 81.547857] -------------------------------------------------------
[ 81.547956] rmmod/3857 is trying to acquire lock:
[ 81.548055] (&t){-+..}, at: [<ffffffff80252960>] del_timer_sync+0x0/0xa0
[ 81.548303]
[ 81.548303] but task is already holding lock:
[ 81.548491] (&l){.+..}, at: [<ffffffffa11f9010>] e+0x10/0x30 [test]
[ 81.548736]
[ 81.548736] which lock already depends on the new lock.
[ 81.548737]
[ 81.549023]
[ 81.549023] the existing dependency chain (in reverse order) is:
[ 81.549212]
[ 81.549213] -> #1 (&l){.+..}:
[ 81.549544] [<ffffffffffffffff>] 0xffffffffffffffff
[ 81.549697]
[ 81.549697] -> #0 (&t){-+..}:
[ 81.550028] [<ffffffff802737b7>] check_prev_add+0x57/0x770
[ 81.550178] [<ffffffff802744d6>] validate_chain+0x606/0x6c0
[ 81.550322] [<ffffffff802749cf>] __lock_acquire+0x43f/0xa10
[ 81.550471] [<ffffffff80275031>] lock_acquire+0x91/0xc0
[ 81.550619] [<ffffffff8025299d>] del_timer_sync+0x3d/0xa0
[ 81.550767] [<ffffffffa11f901c>] e+0x1c/0x30 [test]
[ 81.550911] [<ffffffff80281d9b>] sys_delete_module+0x27b/0x2e0
[ 81.551062] [<ffffffff8020bd1b>] system_call_fastpath+0x16/0x1b
[ 81.551213] [<ffffffffffffffff>] 0xffffffffffffffff
[ 81.551361]
[ 81.551361] other info that might help us debug this:
[ 81.551362]
[ 81.551646] 1 lock held by rmmod/3857:
[ 81.551743] #0: (&l){.+..}, at: [<ffffffffa11f9010>] e+0x10/0x30 [test]
[ 81.552035]
[ 81.552035] stack backtrace:
[ 81.552222] Pid: 3857, comm: rmmod Not tainted 2.6.29-rc2-wl-11979-gc32422d-dirty #24
[ 81.552371] Call Trace:
[ 81.552469] [<ffffffff80273260>] print_circular_bug_tail+0xe0/0xf0
[ 81.552571] [<ffffffff802737b7>] check_prev_add+0x57/0x770
[ 81.552672] [<ffffffff802744d6>] validate_chain+0x606/0x6c0
[ 81.552773] [<ffffffff802749cf>] __lock_acquire+0x43f/0xa10
[ 81.552876] [<ffffffff8026fb04>] ? get_lock_stats+0x34/0x70
[ 81.552979] [<ffffffff8026fb04>] ? get_lock_stats+0x34/0x70
[ 81.553080] [<ffffffff80275031>] lock_acquire+0x91/0xc0
[ 81.553181] [<ffffffff80252960>] ? del_timer_sync+0x0/0xa0
[ 81.553284] [<ffffffff8025299d>] del_timer_sync+0x3d/0xa0
[ 81.553383] [<ffffffff80252960>] ? del_timer_sync+0x0/0xa0
[ 81.553487] [<ffffffffa11f901c>] e+0x1c/0x30 [test]
[ 81.553588] [<ffffffff80281d9b>] sys_delete_module+0x27b/0x2e0
[ 81.553690] [<ffffffff8020c76d>] ? retint_swapgs+0xe/0x13
[ 81.553792] [<ffffffff80272c52>] ? trace_hardirqs_on_caller+0x182/0x1e0
[ 81.553895] [<ffffffff8020bd1b>] system_call_fastpath+0x16/0x1b
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] timer: implement lockdep deadlock detection
2009-01-26 22:59 [PATCH] timer: implement lockdep deadlock detection Johannes Berg
2009-01-26 23:03 ` Johannes Berg
@ 2009-01-26 23:07 ` Ingo Molnar
2009-01-27 8:45 ` Johannes Berg
2009-01-27 8:46 ` [PATCH v2] " Johannes Berg
1 sibling, 2 replies; 22+ messages in thread
From: Ingo Molnar @ 2009-01-26 23:07 UTC (permalink / raw)
To: Johannes Berg; +Cc: Thomas Gleixner, Linux Kernel list, Peter Zijlstra
* Johannes Berg <johannes@sipsolutions.net> wrote:
> This modifies the timer code in a way to allow lockdep to detect
> deadlocks resulting from a lock being taken in the timer function as
> well as around the del_timer_sync() call.
>
> Validated with this module, otherwise bootup was clean.
That's a really neat trick ...
Curious: have you hit such a bug recently that motivated you to implement
it?
> +#ifdef CONFIG_LOCKDEP
> +#define init_timer(timer) \
> + do { \
> + static struct lock_class_key __key; \
> + init_timer_key((timer), #timer, &__key); \
> + } while (0)
> +#define init_timer_deferrable(timer) \
> + do { \
(Style detail: please put a newline after each macro block to make them
stand apart a bit more.)
> int del_timer_sync(struct timer_list *timer)
> {
> +#ifdef CONFIG_LOCKDEP
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + lock_map_acquire(&timer->lockdep_map);
> + lock_map_release(&timer->lockdep_map);
> + local_irq_restore(flags);
> +#endif
yummie. We have repeat bugs in this area that are rather tricky to find.
This will trigger them in a debuggable way.
> @@ -861,10 +881,21 @@ static inline void __run_timers(struct t
>
> set_running_timer(base, timer);
> detach_timer(timer, 1);
> +
> spin_unlock_irq(&base->lock);
Yes, the newline is needed there :-) [seriously]
> int preempt_count = preempt_count();
> +
> + /* Couple the lock chain with the lock chain at
> + * del_timer_sync by acquiring the lock_map around
> + * the fn() call here and in del_timer_sync.
> + */
Please use the standard multi-line comment style:
/*
* Comment .....
* ...... goes here:
*/
Looks good otherwise,
Thanks,
Ingo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] timer: implement lockdep deadlock detection
2009-01-26 23:07 ` Ingo Molnar
@ 2009-01-27 8:45 ` Johannes Berg
2009-01-27 8:46 ` [PATCH v2] " Johannes Berg
1 sibling, 0 replies; 22+ messages in thread
From: Johannes Berg @ 2009-01-27 8:45 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Thomas Gleixner, Linux Kernel list, Peter Zijlstra
[-- Attachment #1: Type: text/plain, Size: 2294 bytes --]
On Tue, 2009-01-27 at 00:07 +0100, Ingo Molnar wrote:
> * Johannes Berg <johannes@sipsolutions.net> wrote:
>
> > This modifies the timer code in a way to allow lockdep to detect
> > deadlocks resulting from a lock being taken in the timer function as
> > well as around the del_timer_sync() call.
> >
> > Validated with this module, otherwise bootup was clean.
>
> That's a really neat trick ...
Did it with workqueues earlier :)
> Curious: have you hit such a bug recently that motivated you to implement
> it?
No, I haven't hit a bug, but code inspection made me wonder whether or
not it could trigger. Didn't have a chance to try yet, had no hardware
available for this particular case.
> > +#ifdef CONFIG_LOCKDEP
> > +#define init_timer(timer) \
> > + do { \
> > + static struct lock_class_key __key; \
> > + init_timer_key((timer), #timer, &__key); \
> > + } while (0)
> > +#define init_timer_deferrable(timer) \
> > + do { \
>
> (Style detail: please put a newline after each macro block to make them
> stand apart a bit more.)
Sure.
> > int del_timer_sync(struct timer_list *timer)
> > {
> > +#ifdef CONFIG_LOCKDEP
> > + unsigned long flags;
> > +
> > + local_irq_save(flags);
> > + lock_map_acquire(&timer->lockdep_map);
> > + lock_map_release(&timer->lockdep_map);
> > + local_irq_restore(flags);
> > +#endif
>
> yummie. We have repeat bugs in this area that are rather tricky to find.
> This will trigger them in a debuggable way.
>
> > @@ -861,10 +881,21 @@ static inline void __run_timers(struct t
> >
> > set_running_timer(base, timer);
> > detach_timer(timer, 1);
> > +
> > spin_unlock_irq(&base->lock);
>
> Yes, the newline is needed there :-) [seriously]
Hah. I was editing code there in that area and removed it later again,
not realising I left the newline.
> > int preempt_count = preempt_count();
> > +
> > + /* Couple the lock chain with the lock chain at
> > + * del_timer_sync by acquiring the lock_map around
> > + * the fn() call here and in del_timer_sync.
> > + */
>
> Please use the standard multi-line comment style:
>
> /*
> * Comment .....
> * ...... goes here:
> */
Oops, yes.
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2] timer: implement lockdep deadlock detection
2009-01-26 23:07 ` Ingo Molnar
2009-01-27 8:45 ` Johannes Berg
@ 2009-01-27 8:46 ` Johannes Berg
2009-01-27 13:27 ` Ingo Molnar
1 sibling, 1 reply; 22+ messages in thread
From: Johannes Berg @ 2009-01-27 8:46 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Thomas Gleixner, Linux Kernel list, Peter Zijlstra
This modifies the timer code in a way to allow lockdep to detect
deadlocks resulting from a lock being taken in the timer function
as well as around the del_timer_sync() call.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
include/linux/timer.h | 97 ++++++++++++++++++++++++++++++++++++++++++--------
kernel/timer.c | 57 +++++++++++++++++++++++------
2 files changed, 127 insertions(+), 27 deletions(-)
--- wireless-testing.orig/include/linux/timer.h 2009-01-26 22:57:40.000000000 +0100
+++ wireless-testing/include/linux/timer.h 2009-01-27 09:43:21.000000000 +0100
@@ -21,52 +21,119 @@ struct timer_list {
char start_comm[16];
int start_pid;
#endif
+#ifdef CONFIG_LOCKDEP
+ struct lockdep_map lockdep_map;
+#endif
};
extern struct tvec_base boot_tvec_bases;
-#define TIMER_INITIALIZER(_function, _expires, _data) { \
- .entry = { .prev = TIMER_ENTRY_STATIC }, \
- .function = (_function), \
- .expires = (_expires), \
- .data = (_data), \
- .base = &boot_tvec_bases, \
+#ifdef CONFIG_LOCKDEP
+#define __TIMER_LOCKDEP_MAP_INITIALIZER(fn, name) \
+ .lockdep_map = STATIC_LOCKDEP_MAP_INIT(name, fn),
+#else
+#define __TIMER_LOCKDEP_MAP_INITIALIZER(fn, name)
+#endif
+
+#define TIMER_INITIALIZER(_function, _expires, _data) { \
+ .entry = { .prev = TIMER_ENTRY_STATIC }, \
+ .function = (_function), \
+ .expires = (_expires), \
+ .data = (_data), \
+ .base = &boot_tvec_bases, \
+ __TIMER_LOCKDEP_MAP_INITIALIZER((_function), #_function)\
}
#define DEFINE_TIMER(_name, _function, _expires, _data) \
struct timer_list _name = \
TIMER_INITIALIZER(_function, _expires, _data)
-void init_timer(struct timer_list *timer);
-void init_timer_deferrable(struct timer_list *timer);
+void init_timer_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
+void init_timer_deferrable_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
+
+#ifdef CONFIG_LOCKDEP
+#define init_timer(timer) \
+ do { \
+ static struct lock_class_key __key; \
+ init_timer_key((timer), #timer, &__key); \
+ } while (0)
+
+#define init_timer_deferrable(timer) \
+ do { \
+ static struct lock_class_key __key; \
+ init_timer_deferrable_key((timer), #timer, &__key); \
+ } while (0)
+
+#define init_timer_on_stack(timer) \
+ do { \
+ static struct lock_class_key __key; \
+ init_timer_on_stack_key((timer), #timer, &__key); \
+ } while (0)
+
+#define setup_timer(timer, fn, data) \
+ do { \
+ static struct lock_class_key __key; \
+ setup_timer_key((timer), #timer, &__key, (fn), (data));\
+ } while (0)
+
+#define setup_timer_on_stack(timer, fn, data) \
+ do { \
+ static struct lock_class_key __key; \
+ setup_timer_on_stack_key((timer), #timer, &__key, \
+ (fn), (data)); \
+ } while (0)
+#else
+#define init_timer(timer)\
+ init_timer_key((timer), NULL, NULL)
+#define init_timer_deferrable(timer)\
+ init_timer_deferrable_key((timer), NULL, NULL)
+#define init_timer_on_stack(timer)\
+ init_timer_on_stack_key((timer)), NULL, NULL)
+#define setup_timer(timer, fn, data)\
+ setup_timer_key((timer), NULL, NULL, (fn), (data))
+#define setup_timer_on_stack(timer, fn, data)\
+ setup_timer_on_stack_key((timer), NULL, NULL, (fn), (data))
+#endif
#ifdef CONFIG_DEBUG_OBJECTS_TIMERS
-extern void init_timer_on_stack(struct timer_list *timer);
+extern void init_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
extern void destroy_timer_on_stack(struct timer_list *timer);
#else
static inline void destroy_timer_on_stack(struct timer_list *timer) { }
-static inline void init_timer_on_stack(struct timer_list *timer)
+static inline void init_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
- init_timer(timer);
+ init_timer_key(timer, name, key);
}
#endif
-static inline void setup_timer(struct timer_list * timer,
+static inline void setup_timer_key(struct timer_list * timer,
+ const char *name,
+ struct lock_class_key *key,
void (*function)(unsigned long),
unsigned long data)
{
timer->function = function;
timer->data = data;
- init_timer(timer);
+ init_timer_key(timer, name, key);
}
-static inline void setup_timer_on_stack(struct timer_list *timer,
+static inline void setup_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key,
void (*function)(unsigned long),
unsigned long data)
{
timer->function = function;
timer->data = data;
- init_timer_on_stack(timer);
+ init_timer_on_stack_key(timer, name, key);
}
/**
--- wireless-testing.orig/kernel/timer.c 2009-01-26 22:57:40.000000000 +0100
+++ wireless-testing/kernel/timer.c 2009-01-27 09:45:48.000000000 +0100
@@ -491,14 +491,18 @@ static inline void debug_timer_free(stru
debug_object_free(timer, &timer_debug_descr);
}
-static void __init_timer(struct timer_list *timer);
-
-void init_timer_on_stack(struct timer_list *timer)
+static void __init_timer(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
+
+void init_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
debug_object_init_on_stack(timer, &timer_debug_descr);
- __init_timer(timer);
+ __init_timer(timer, name, key);
}
-EXPORT_SYMBOL_GPL(init_timer_on_stack);
+EXPORT_SYMBOL_GPL(init_timer_on_stack_key);
void destroy_timer_on_stack(struct timer_list *timer)
{
@@ -512,7 +516,9 @@ static inline void debug_timer_activate(
static inline void debug_timer_deactivate(struct timer_list *timer) { }
#endif
-static void __init_timer(struct timer_list *timer)
+static void __init_timer(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
timer->entry.next = NULL;
timer->base = __raw_get_cpu_var(tvec_bases);
@@ -521,6 +527,7 @@ static void __init_timer(struct timer_li
timer->start_pid = -1;
memset(timer->start_comm, 0, TASK_COMM_LEN);
#endif
+ lockdep_init_map(&timer->lockdep_map, name, key, 0);
}
/**
@@ -530,19 +537,23 @@ static void __init_timer(struct timer_li
* init_timer() must be done to a timer prior calling *any* of the
* other timer functions.
*/
-void init_timer(struct timer_list *timer)
+void init_timer_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
debug_timer_init(timer);
- __init_timer(timer);
+ __init_timer(timer, name, key);
}
-EXPORT_SYMBOL(init_timer);
+EXPORT_SYMBOL(init_timer_key);
-void init_timer_deferrable(struct timer_list *timer)
+void init_timer_deferrable_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
- init_timer(timer);
+ init_timer_key(timer, name, key);
timer_set_deferrable(timer);
}
-EXPORT_SYMBOL(init_timer_deferrable);
+EXPORT_SYMBOL(init_timer_deferrable_key);
static inline void detach_timer(struct timer_list *timer,
int clear_pending)
@@ -789,6 +800,15 @@ EXPORT_SYMBOL(try_to_del_timer_sync);
*/
int del_timer_sync(struct timer_list *timer)
{
+#ifdef CONFIG_LOCKDEP
+ unsigned long flags;
+
+ local_irq_save(flags);
+ lock_map_acquire(&timer->lockdep_map);
+ lock_map_release(&timer->lockdep_map);
+ local_irq_restore(flags);
+#endif
+
for (;;) {
int ret = try_to_del_timer_sync(timer);
if (ret >= 0)
@@ -861,10 +881,23 @@ static inline void __run_timers(struct t
set_running_timer(base, timer);
detach_timer(timer, 1);
+
spin_unlock_irq(&base->lock);
{
int preempt_count = preempt_count();
+
+ /*
+ * Couple the lock chain with the lock chain at
+ * del_timer_sync() by acquiring the lock_map
+ * around the fn() call here and in
+ * del_timer_sync().
+ */
+ lock_map_acquire(&timer->lockdep_map);
+
fn(data);
+
+ lock_map_release(&timer->lockdep_map);
+
if (preempt_count != preempt_count()) {
printk(KERN_ERR "huh, entered %p "
"with preempt_count %08x, exited"
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] timer: implement lockdep deadlock detection
2009-01-27 8:46 ` [PATCH v2] " Johannes Berg
@ 2009-01-27 13:27 ` Ingo Molnar
2009-01-27 13:41 ` Ingo Molnar
0 siblings, 1 reply; 22+ messages in thread
From: Ingo Molnar @ 2009-01-27 13:27 UTC (permalink / raw)
To: Johannes Berg; +Cc: Thomas Gleixner, Linux Kernel list, Peter Zijlstra
* Johannes Berg <johannes@sipsolutions.net> wrote:
> This modifies the timer code in a way to allow lockdep to detect
> deadlocks resulting from a lock being taken in the timer function as
> well as around the del_timer_sync() call.
>
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
> include/linux/timer.h | 97 ++++++++++++++++++++++++++++++++++++++++++--------
> kernel/timer.c | 57 +++++++++++++++++++++++------
> 2 files changed, 127 insertions(+), 27 deletions(-)
applied to tip/core/locking, thanks Johannes!
Ingo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] timer: implement lockdep deadlock detection
2009-01-27 13:27 ` Ingo Molnar
@ 2009-01-27 13:41 ` Ingo Molnar
2009-01-27 18:06 ` Johannes Berg
2009-01-27 18:12 ` [PATCH v2] " Johannes Berg
0 siblings, 2 replies; 22+ messages in thread
From: Ingo Molnar @ 2009-01-27 13:41 UTC (permalink / raw)
To: Johannes Berg; +Cc: Thomas Gleixner, Linux Kernel list, Peter Zijlstra
-tip testing found this build failure with your patch:
arch/x86/kernel/hpet.c:631: error: too few arguments to function ‘init_timer_on_stack_key’
you can pick up latest -tip via:
http://people.redhat.com/mingo/tip.git/README
Ingo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] timer: implement lockdep deadlock detection
2009-01-27 13:41 ` Ingo Molnar
@ 2009-01-27 18:06 ` Johannes Berg
2009-01-27 18:30 ` Peter Zijlstra
2009-01-27 18:12 ` [PATCH v2] " Johannes Berg
1 sibling, 1 reply; 22+ messages in thread
From: Johannes Berg @ 2009-01-27 18:06 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Thomas Gleixner, Linux Kernel list, Peter Zijlstra
[-- Attachment #1: Type: text/plain, Size: 348 bytes --]
On Tue, 2009-01-27 at 14:41 +0100, Ingo Molnar wrote:
> -tip testing found this build failure with your patch:
>
> arch/x86/kernel/hpet.c:631: error: too few arguments to function ‘init_timer_on_stack_key’
Odd, I'll look into this.
Meanwhile, I'd appreciate comments on
http://bugzilla.kernel.org/show_bug.cgi?id=12552
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] timer: implement lockdep deadlock detection
2009-01-27 13:41 ` Ingo Molnar
2009-01-27 18:06 ` Johannes Berg
@ 2009-01-27 18:12 ` Johannes Berg
1 sibling, 0 replies; 22+ messages in thread
From: Johannes Berg @ 2009-01-27 18:12 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Thomas Gleixner, Linux Kernel list, Peter Zijlstra
[-- Attachment #1: Type: text/plain, Size: 430 bytes --]
On Tue, 2009-01-27 at 14:41 +0100, Ingo Molnar wrote:
> -tip testing found this build failure with your patch:
>
> arch/x86/kernel/hpet.c:631: error: too few arguments to function ‘init_timer_on_stack_key’
#define init_timer_on_stack(timer)\
init_timer_on_stack_key((timer)), NULL, NULL)
needs to be
#define init_timer_on_stack(timer)\
init_timer_on_stack_key((timer), NULL, NULL)
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] timer: implement lockdep deadlock detection
2009-01-27 18:06 ` Johannes Berg
@ 2009-01-27 18:30 ` Peter Zijlstra
2009-01-27 18:33 ` Johannes Berg
2009-01-27 18:57 ` [PATCH v3] " Johannes Berg
0 siblings, 2 replies; 22+ messages in thread
From: Peter Zijlstra @ 2009-01-27 18:30 UTC (permalink / raw)
To: Johannes Berg; +Cc: Ingo Molnar, Thomas Gleixner, Linux Kernel list
On Tue, 2009-01-27 at 19:06 +0100, Johannes Berg wrote:
> On Tue, 2009-01-27 at 14:41 +0100, Ingo Molnar wrote:
> > -tip testing found this build failure with your patch:
> >
> > arch/x86/kernel/hpet.c:631: error: too few arguments to function ‘init_timer_on_stack_key’
>
> Odd, I'll look into this.
>
> Meanwhile, I'd appreciate comments on
> http://bugzilla.kernel.org/show_bug.cgi?id=12552
Seems valid to me, the workaround in run_workqueue() seems applicable
here as well.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] timer: implement lockdep deadlock detection
2009-01-27 18:30 ` Peter Zijlstra
@ 2009-01-27 18:33 ` Johannes Berg
2009-01-27 18:57 ` [PATCH v3] " Johannes Berg
1 sibling, 0 replies; 22+ messages in thread
From: Johannes Berg @ 2009-01-27 18:33 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Ingo Molnar, Thomas Gleixner, Linux Kernel list
[-- Attachment #1: Type: text/plain, Size: 617 bytes --]
On Tue, 2009-01-27 at 19:30 +0100, Peter Zijlstra wrote:
> On Tue, 2009-01-27 at 19:06 +0100, Johannes Berg wrote:
> > On Tue, 2009-01-27 at 14:41 +0100, Ingo Molnar wrote:
> > > -tip testing found this build failure with your patch:
> > >
> > > arch/x86/kernel/hpet.c:631: error: too few arguments to function ‘init_timer_on_stack_key’
> >
> > Odd, I'll look into this.
> >
> > Meanwhile, I'd appreciate comments on
> > http://bugzilla.kernel.org/show_bug.cgi?id=12552
>
> Seems valid to me, the workaround in run_workqueue() seems applicable
> here as well.
Alright, will fix.
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3] timer: implement lockdep deadlock detection
2009-01-27 18:30 ` Peter Zijlstra
2009-01-27 18:33 ` Johannes Berg
@ 2009-01-27 18:57 ` Johannes Berg
2009-01-28 8:20 ` Peter Zijlstra
1 sibling, 1 reply; 22+ messages in thread
From: Johannes Berg @ 2009-01-27 18:57 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Ingo Molnar, Thomas Gleixner, Linux Kernel list
This modifies the timer code in a way to allow lockdep to detect
deadlocks resulting from a lock being taken in the timer function
as well as around the del_timer_sync() call.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
Ingo, do you want an incremental patch instead?
v2: fix coding/comment style
v3: accept freeing timer from timer function, fix init_timer_on_stack
in no-lockdep case
include/linux/timer.h | 103 ++++++++++++++++++++++++++++++++++++++++++--------
kernel/timer.c | 70 ++++++++++++++++++++++++++++-----
2 files changed, 146 insertions(+), 27 deletions(-)
--- wireless-testing.orig/include/linux/timer.h 2009-01-27 10:12:22.000000000 +0100
+++ wireless-testing/include/linux/timer.h 2009-01-27 19:52:33.000000000 +0100
@@ -21,52 +21,125 @@ struct timer_list {
char start_comm[16];
int start_pid;
#endif
+#ifdef CONFIG_LOCKDEP
+ struct lockdep_map lockdep_map;
+#endif
};
extern struct tvec_base boot_tvec_bases;
-#define TIMER_INITIALIZER(_function, _expires, _data) { \
- .entry = { .prev = TIMER_ENTRY_STATIC }, \
- .function = (_function), \
- .expires = (_expires), \
- .data = (_data), \
- .base = &boot_tvec_bases, \
+#ifdef CONFIG_LOCKDEP
+/*
+ * NB: because we have to copy the lockdep_map, setting _key
+ * here is required, otherwise it could get initialised to the
+ * copy of the lockdep_map! We use the function pointer as the
+ * lockdep key here.
+ */
+#define __TIMER_LOCKDEP_MAP_INITIALIZER(fn, name) \
+ .lockdep_map = STATIC_LOCKDEP_MAP_INIT(name, fn),
+#else
+#define __TIMER_LOCKDEP_MAP_INITIALIZER(fn, name)
+#endif
+
+#define TIMER_INITIALIZER(_function, _expires, _data) { \
+ .entry = { .prev = TIMER_ENTRY_STATIC }, \
+ .function = (_function), \
+ .expires = (_expires), \
+ .data = (_data), \
+ .base = &boot_tvec_bases, \
+ __TIMER_LOCKDEP_MAP_INITIALIZER((_function), #_function)\
}
#define DEFINE_TIMER(_name, _function, _expires, _data) \
struct timer_list _name = \
TIMER_INITIALIZER(_function, _expires, _data)
-void init_timer(struct timer_list *timer);
-void init_timer_deferrable(struct timer_list *timer);
+void init_timer_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
+void init_timer_deferrable_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
+
+#ifdef CONFIG_LOCKDEP
+#define init_timer(timer) \
+ do { \
+ static struct lock_class_key __key; \
+ init_timer_key((timer), #timer, &__key); \
+ } while (0)
+
+#define init_timer_deferrable(timer) \
+ do { \
+ static struct lock_class_key __key; \
+ init_timer_deferrable_key((timer), #timer, &__key); \
+ } while (0)
+
+#define init_timer_on_stack(timer) \
+ do { \
+ static struct lock_class_key __key; \
+ init_timer_on_stack_key((timer), #timer, &__key); \
+ } while (0)
+
+#define setup_timer(timer, fn, data) \
+ do { \
+ static struct lock_class_key __key; \
+ setup_timer_key((timer), #timer, &__key, (fn), (data));\
+ } while (0)
+
+#define setup_timer_on_stack(timer, fn, data) \
+ do { \
+ static struct lock_class_key __key; \
+ setup_timer_on_stack_key((timer), #timer, &__key, \
+ (fn), (data)); \
+ } while (0)
+#else
+#define init_timer(timer)\
+ init_timer_key((timer), NULL, NULL)
+#define init_timer_deferrable(timer)\
+ init_timer_deferrable_key((timer), NULL, NULL)
+#define init_timer_on_stack(timer)\
+ init_timer_on_stack_key((timer), NULL, NULL)
+#define setup_timer(timer, fn, data)\
+ setup_timer_key((timer), NULL, NULL, (fn), (data))
+#define setup_timer_on_stack(timer, fn, data)\
+ setup_timer_on_stack_key((timer), NULL, NULL, (fn), (data))
+#endif
#ifdef CONFIG_DEBUG_OBJECTS_TIMERS
-extern void init_timer_on_stack(struct timer_list *timer);
+extern void init_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
extern void destroy_timer_on_stack(struct timer_list *timer);
#else
static inline void destroy_timer_on_stack(struct timer_list *timer) { }
-static inline void init_timer_on_stack(struct timer_list *timer)
+static inline void init_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
- init_timer(timer);
+ init_timer_key(timer, name, key);
}
#endif
-static inline void setup_timer(struct timer_list * timer,
+static inline void setup_timer_key(struct timer_list * timer,
+ const char *name,
+ struct lock_class_key *key,
void (*function)(unsigned long),
unsigned long data)
{
timer->function = function;
timer->data = data;
- init_timer(timer);
+ init_timer_key(timer, name, key);
}
-static inline void setup_timer_on_stack(struct timer_list *timer,
+static inline void setup_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key,
void (*function)(unsigned long),
unsigned long data)
{
timer->function = function;
timer->data = data;
- init_timer_on_stack(timer);
+ init_timer_on_stack_key(timer, name, key);
}
/**
--- wireless-testing.orig/kernel/timer.c 2009-01-27 10:12:22.000000000 +0100
+++ wireless-testing/kernel/timer.c 2009-01-27 19:50:36.000000000 +0100
@@ -491,14 +491,18 @@ static inline void debug_timer_free(stru
debug_object_free(timer, &timer_debug_descr);
}
-static void __init_timer(struct timer_list *timer);
-
-void init_timer_on_stack(struct timer_list *timer)
+static void __init_timer(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
+
+void init_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
debug_object_init_on_stack(timer, &timer_debug_descr);
- __init_timer(timer);
+ __init_timer(timer, name, key);
}
-EXPORT_SYMBOL_GPL(init_timer_on_stack);
+EXPORT_SYMBOL_GPL(init_timer_on_stack_key);
void destroy_timer_on_stack(struct timer_list *timer)
{
@@ -512,7 +516,9 @@ static inline void debug_timer_activate(
static inline void debug_timer_deactivate(struct timer_list *timer) { }
#endif
-static void __init_timer(struct timer_list *timer)
+static void __init_timer(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
timer->entry.next = NULL;
timer->base = __raw_get_cpu_var(tvec_bases);
@@ -521,6 +527,7 @@ static void __init_timer(struct timer_li
timer->start_pid = -1;
memset(timer->start_comm, 0, TASK_COMM_LEN);
#endif
+ lockdep_init_map(&timer->lockdep_map, name, key, 0);
}
/**
@@ -530,19 +537,23 @@ static void __init_timer(struct timer_li
* init_timer() must be done to a timer prior calling *any* of the
* other timer functions.
*/
-void init_timer(struct timer_list *timer)
+void init_timer_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
debug_timer_init(timer);
- __init_timer(timer);
+ __init_timer(timer, name, key);
}
-EXPORT_SYMBOL(init_timer);
+EXPORT_SYMBOL(init_timer_key);
-void init_timer_deferrable(struct timer_list *timer)
+void init_timer_deferrable_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
- init_timer(timer);
+ init_timer_key(timer, name, key);
timer_set_deferrable(timer);
}
-EXPORT_SYMBOL(init_timer_deferrable);
+EXPORT_SYMBOL(init_timer_deferrable_key);
static inline void detach_timer(struct timer_list *timer,
int clear_pending)
@@ -789,6 +800,15 @@ EXPORT_SYMBOL(try_to_del_timer_sync);
*/
int del_timer_sync(struct timer_list *timer)
{
+#ifdef CONFIG_LOCKDEP
+ unsigned long flags;
+
+ local_irq_save(flags);
+ lock_map_acquire(&timer->lockdep_map);
+ lock_map_release(&timer->lockdep_map);
+ local_irq_restore(flags);
+#endif
+
for (;;) {
int ret = try_to_del_timer_sync(timer);
if (ret >= 0)
@@ -861,10 +881,36 @@ static inline void __run_timers(struct t
set_running_timer(base, timer);
detach_timer(timer, 1);
+
spin_unlock_irq(&base->lock);
{
int preempt_count = preempt_count();
+
+#ifdef CONFIG_LOCKDEP
+ /*
+ * It is permissible to free the timer from
+ * inside the function that is called from
+ * it, this we need to take into account for
+ * lockdep too. To avoid bogus "held lock
+ * freed" warnings as well as problems when
+ * looking into timer->lockdep_map, make a
+ * copy and use that here.
+ */
+ struct lockdep_map lockdep_map =
+ timer->lockdep_map;
+#endif
+ /*
+ * Couple the lock chain with the lock chain at
+ * del_timer_sync() by acquiring the lock_map
+ * around the fn() call here and in
+ * del_timer_sync().
+ */
+ lock_map_acquire(&lockdep_map);
+
fn(data);
+
+ lock_map_release(&lockdep_map);
+
if (preempt_count != preempt_count()) {
printk(KERN_ERR "huh, entered %p "
"with preempt_count %08x, exited"
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3] timer: implement lockdep deadlock detection
2009-01-27 18:57 ` [PATCH v3] " Johannes Berg
@ 2009-01-28 8:20 ` Peter Zijlstra
2009-01-28 9:54 ` Johannes Berg
0 siblings, 1 reply; 22+ messages in thread
From: Peter Zijlstra @ 2009-01-28 8:20 UTC (permalink / raw)
To: Johannes Berg; +Cc: Ingo Molnar, Thomas Gleixner, Linux Kernel list
On Tue, 2009-01-27 at 19:57 +0100, Johannes Berg wrote:
> This modifies the timer code in a way to allow lockdep to detect
> deadlocks resulting from a lock being taken in the timer function
> as well as around the del_timer_sync() call.
>
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Nice, thanks!
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
> Ingo, do you want an incremental patch instead?
>
> v2: fix coding/comment style
> v3: accept freeing timer from timer function, fix init_timer_on_stack
> in no-lockdep case
>
> include/linux/timer.h | 103 ++++++++++++++++++++++++++++++++++++++++++--------
> kernel/timer.c | 70 ++++++++++++++++++++++++++++-----
> 2 files changed, 146 insertions(+), 27 deletions(-)
>
> --- wireless-testing.orig/include/linux/timer.h 2009-01-27 10:12:22.000000000 +0100
> +++ wireless-testing/include/linux/timer.h 2009-01-27 19:52:33.000000000 +0100
> @@ -21,52 +21,125 @@ struct timer_list {
> char start_comm[16];
> int start_pid;
> #endif
> +#ifdef CONFIG_LOCKDEP
> + struct lockdep_map lockdep_map;
> +#endif
> };
>
> extern struct tvec_base boot_tvec_bases;
>
> -#define TIMER_INITIALIZER(_function, _expires, _data) { \
> - .entry = { .prev = TIMER_ENTRY_STATIC }, \
> - .function = (_function), \
> - .expires = (_expires), \
> - .data = (_data), \
> - .base = &boot_tvec_bases, \
> +#ifdef CONFIG_LOCKDEP
> +/*
> + * NB: because we have to copy the lockdep_map, setting _key
> + * here is required, otherwise it could get initialised to the
> + * copy of the lockdep_map! We use the function pointer as the
> + * lockdep key here.
> + */
> +#define __TIMER_LOCKDEP_MAP_INITIALIZER(fn, name) \
> + .lockdep_map = STATIC_LOCKDEP_MAP_INIT(name, fn),
> +#else
> +#define __TIMER_LOCKDEP_MAP_INITIALIZER(fn, name)
> +#endif
> +
> +#define TIMER_INITIALIZER(_function, _expires, _data) { \
> + .entry = { .prev = TIMER_ENTRY_STATIC }, \
> + .function = (_function), \
> + .expires = (_expires), \
> + .data = (_data), \
> + .base = &boot_tvec_bases, \
> + __TIMER_LOCKDEP_MAP_INITIALIZER((_function), #_function)\
> }
>
> #define DEFINE_TIMER(_name, _function, _expires, _data) \
> struct timer_list _name = \
> TIMER_INITIALIZER(_function, _expires, _data)
>
> -void init_timer(struct timer_list *timer);
> -void init_timer_deferrable(struct timer_list *timer);
> +void init_timer_key(struct timer_list *timer,
> + const char *name,
> + struct lock_class_key *key);
> +void init_timer_deferrable_key(struct timer_list *timer,
> + const char *name,
> + struct lock_class_key *key);
> +
> +#ifdef CONFIG_LOCKDEP
> +#define init_timer(timer) \
> + do { \
> + static struct lock_class_key __key; \
> + init_timer_key((timer), #timer, &__key); \
> + } while (0)
> +
> +#define init_timer_deferrable(timer) \
> + do { \
> + static struct lock_class_key __key; \
> + init_timer_deferrable_key((timer), #timer, &__key); \
> + } while (0)
> +
> +#define init_timer_on_stack(timer) \
> + do { \
> + static struct lock_class_key __key; \
> + init_timer_on_stack_key((timer), #timer, &__key); \
> + } while (0)
> +
> +#define setup_timer(timer, fn, data) \
> + do { \
> + static struct lock_class_key __key; \
> + setup_timer_key((timer), #timer, &__key, (fn), (data));\
> + } while (0)
> +
> +#define setup_timer_on_stack(timer, fn, data) \
> + do { \
> + static struct lock_class_key __key; \
> + setup_timer_on_stack_key((timer), #timer, &__key, \
> + (fn), (data)); \
> + } while (0)
> +#else
> +#define init_timer(timer)\
> + init_timer_key((timer), NULL, NULL)
> +#define init_timer_deferrable(timer)\
> + init_timer_deferrable_key((timer), NULL, NULL)
> +#define init_timer_on_stack(timer)\
> + init_timer_on_stack_key((timer), NULL, NULL)
> +#define setup_timer(timer, fn, data)\
> + setup_timer_key((timer), NULL, NULL, (fn), (data))
> +#define setup_timer_on_stack(timer, fn, data)\
> + setup_timer_on_stack_key((timer), NULL, NULL, (fn), (data))
> +#endif
>
> #ifdef CONFIG_DEBUG_OBJECTS_TIMERS
> -extern void init_timer_on_stack(struct timer_list *timer);
> +extern void init_timer_on_stack_key(struct timer_list *timer,
> + const char *name,
> + struct lock_class_key *key);
> extern void destroy_timer_on_stack(struct timer_list *timer);
> #else
> static inline void destroy_timer_on_stack(struct timer_list *timer) { }
> -static inline void init_timer_on_stack(struct timer_list *timer)
> +static inline void init_timer_on_stack_key(struct timer_list *timer,
> + const char *name,
> + struct lock_class_key *key)
> {
> - init_timer(timer);
> + init_timer_key(timer, name, key);
> }
> #endif
>
> -static inline void setup_timer(struct timer_list * timer,
> +static inline void setup_timer_key(struct timer_list * timer,
> + const char *name,
> + struct lock_class_key *key,
> void (*function)(unsigned long),
> unsigned long data)
> {
> timer->function = function;
> timer->data = data;
> - init_timer(timer);
> + init_timer_key(timer, name, key);
> }
>
> -static inline void setup_timer_on_stack(struct timer_list *timer,
> +static inline void setup_timer_on_stack_key(struct timer_list *timer,
> + const char *name,
> + struct lock_class_key *key,
> void (*function)(unsigned long),
> unsigned long data)
> {
> timer->function = function;
> timer->data = data;
> - init_timer_on_stack(timer);
> + init_timer_on_stack_key(timer, name, key);
> }
>
> /**
> --- wireless-testing.orig/kernel/timer.c 2009-01-27 10:12:22.000000000 +0100
> +++ wireless-testing/kernel/timer.c 2009-01-27 19:50:36.000000000 +0100
> @@ -491,14 +491,18 @@ static inline void debug_timer_free(stru
> debug_object_free(timer, &timer_debug_descr);
> }
>
> -static void __init_timer(struct timer_list *timer);
> -
> -void init_timer_on_stack(struct timer_list *timer)
> +static void __init_timer(struct timer_list *timer,
> + const char *name,
> + struct lock_class_key *key);
> +
> +void init_timer_on_stack_key(struct timer_list *timer,
> + const char *name,
> + struct lock_class_key *key)
> {
> debug_object_init_on_stack(timer, &timer_debug_descr);
> - __init_timer(timer);
> + __init_timer(timer, name, key);
> }
> -EXPORT_SYMBOL_GPL(init_timer_on_stack);
> +EXPORT_SYMBOL_GPL(init_timer_on_stack_key);
>
> void destroy_timer_on_stack(struct timer_list *timer)
> {
> @@ -512,7 +516,9 @@ static inline void debug_timer_activate(
> static inline void debug_timer_deactivate(struct timer_list *timer) { }
> #endif
>
> -static void __init_timer(struct timer_list *timer)
> +static void __init_timer(struct timer_list *timer,
> + const char *name,
> + struct lock_class_key *key)
> {
> timer->entry.next = NULL;
> timer->base = __raw_get_cpu_var(tvec_bases);
> @@ -521,6 +527,7 @@ static void __init_timer(struct timer_li
> timer->start_pid = -1;
> memset(timer->start_comm, 0, TASK_COMM_LEN);
> #endif
> + lockdep_init_map(&timer->lockdep_map, name, key, 0);
> }
>
> /**
> @@ -530,19 +537,23 @@ static void __init_timer(struct timer_li
> * init_timer() must be done to a timer prior calling *any* of the
> * other timer functions.
> */
> -void init_timer(struct timer_list *timer)
> +void init_timer_key(struct timer_list *timer,
> + const char *name,
> + struct lock_class_key *key)
> {
> debug_timer_init(timer);
> - __init_timer(timer);
> + __init_timer(timer, name, key);
> }
> -EXPORT_SYMBOL(init_timer);
> +EXPORT_SYMBOL(init_timer_key);
>
> -void init_timer_deferrable(struct timer_list *timer)
> +void init_timer_deferrable_key(struct timer_list *timer,
> + const char *name,
> + struct lock_class_key *key)
> {
> - init_timer(timer);
> + init_timer_key(timer, name, key);
> timer_set_deferrable(timer);
> }
> -EXPORT_SYMBOL(init_timer_deferrable);
> +EXPORT_SYMBOL(init_timer_deferrable_key);
>
> static inline void detach_timer(struct timer_list *timer,
> int clear_pending)
> @@ -789,6 +800,15 @@ EXPORT_SYMBOL(try_to_del_timer_sync);
> */
> int del_timer_sync(struct timer_list *timer)
> {
> +#ifdef CONFIG_LOCKDEP
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + lock_map_acquire(&timer->lockdep_map);
> + lock_map_release(&timer->lockdep_map);
> + local_irq_restore(flags);
> +#endif
> +
> for (;;) {
> int ret = try_to_del_timer_sync(timer);
> if (ret >= 0)
> @@ -861,10 +881,36 @@ static inline void __run_timers(struct t
>
> set_running_timer(base, timer);
> detach_timer(timer, 1);
> +
> spin_unlock_irq(&base->lock);
> {
> int preempt_count = preempt_count();
> +
> +#ifdef CONFIG_LOCKDEP
> + /*
> + * It is permissible to free the timer from
> + * inside the function that is called from
> + * it, this we need to take into account for
> + * lockdep too. To avoid bogus "held lock
> + * freed" warnings as well as problems when
> + * looking into timer->lockdep_map, make a
> + * copy and use that here.
> + */
> + struct lockdep_map lockdep_map =
> + timer->lockdep_map;
> +#endif
> + /*
> + * Couple the lock chain with the lock chain at
> + * del_timer_sync() by acquiring the lock_map
> + * around the fn() call here and in
> + * del_timer_sync().
> + */
> + lock_map_acquire(&lockdep_map);
> +
> fn(data);
> +
> + lock_map_release(&lockdep_map);
> +
> if (preempt_count != preempt_count()) {
> printk(KERN_ERR "huh, entered %p "
> "with preempt_count %08x, exited"
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3] timer: implement lockdep deadlock detection
2009-01-28 8:20 ` Peter Zijlstra
@ 2009-01-28 9:54 ` Johannes Berg
2009-01-28 10:13 ` Peter Zijlstra
0 siblings, 1 reply; 22+ messages in thread
From: Johannes Berg @ 2009-01-28 9:54 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Ingo Molnar, Thomas Gleixner, Linux Kernel list
[-- Attachment #1: Type: text/plain, Size: 544 bytes --]
On Wed, 2009-01-28 at 09:20 +0100, Peter Zijlstra wrote:
> On Tue, 2009-01-27 at 19:57 +0100, Johannes Berg wrote:
> > This modifies the timer code in a way to allow lockdep to detect
> > deadlocks resulting from a lock being taken in the timer function
> > as well as around the del_timer_sync() call.
> >
> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
>
> Nice, thanks!
I actually got "trying to register non-static key" on my powerpc64
machine. Is there a possibility that functions are not static??
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3] timer: implement lockdep deadlock detection
2009-01-28 9:54 ` Johannes Berg
@ 2009-01-28 10:13 ` Peter Zijlstra
2009-01-28 10:54 ` Johannes Berg
0 siblings, 1 reply; 22+ messages in thread
From: Peter Zijlstra @ 2009-01-28 10:13 UTC (permalink / raw)
To: Johannes Berg; +Cc: Ingo Molnar, Thomas Gleixner, Linux Kernel list
On Wed, 2009-01-28 at 10:54 +0100, Johannes Berg wrote:
> On Wed, 2009-01-28 at 09:20 +0100, Peter Zijlstra wrote:
> > On Tue, 2009-01-27 at 19:57 +0100, Johannes Berg wrote:
> > > This modifies the timer code in a way to allow lockdep to detect
> > > deadlocks resulting from a lock being taken in the timer function
> > > as well as around the del_timer_sync() call.
> > >
> > > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> >
> > Nice, thanks!
>
> I actually got "trying to register non-static key" on my powerpc64
> machine. Is there a possibility that functions are not static??
Hmm, weird, afaict static_obj() includes both text and data, for the
core kernel as well as modules.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3] timer: implement lockdep deadlock detection
2009-01-28 10:13 ` Peter Zijlstra
@ 2009-01-28 10:54 ` Johannes Berg
2009-01-28 18:06 ` Johannes Berg
0 siblings, 1 reply; 22+ messages in thread
From: Johannes Berg @ 2009-01-28 10:54 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Ingo Molnar, Thomas Gleixner, Linux Kernel list
[-- Attachment #1: Type: text/plain, Size: 941 bytes --]
On Wed, 2009-01-28 at 11:13 +0100, Peter Zijlstra wrote:
> On Wed, 2009-01-28 at 10:54 +0100, Johannes Berg wrote:
> > On Wed, 2009-01-28 at 09:20 +0100, Peter Zijlstra wrote:
> > > On Tue, 2009-01-27 at 19:57 +0100, Johannes Berg wrote:
> > > > This modifies the timer code in a way to allow lockdep to detect
> > > > deadlocks resulting from a lock being taken in the timer function
> > > > as well as around the del_timer_sync() call.
> > > >
> > > > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> > >
> > > Nice, thanks!
> >
> > I actually got "trying to register non-static key" on my powerpc64
> > machine. Is there a possibility that functions are not static??
>
> Hmm, weird, afaict static_obj() includes both text and data, for the
> core kernel as well as modules.
Yeah, I'd think it should. I'll run it by the powerpc list when I get it
again, it only seems to happen very rarely.
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3] timer: implement lockdep deadlock detection
2009-01-28 10:54 ` Johannes Berg
@ 2009-01-28 18:06 ` Johannes Berg
2009-01-29 12:38 ` Ingo Molnar
2009-01-29 13:38 ` [PATCH v3] " Peter Zijlstra
0 siblings, 2 replies; 22+ messages in thread
From: Johannes Berg @ 2009-01-28 18:06 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Ingo Molnar, Thomas Gleixner, Linux Kernel list
On Wed, 2009-01-28 at 11:59 +0100, Johannes Berg wrote:
> > > I actually got "trying to register non-static key" on my powerpc64
> > > machine. Is there a possibility that functions are not static??
> >
> > Hmm, weird, afaict static_obj() includes both text and data, for the
> > core kernel as well as modules.
>
> Yeah, I'd think it should. I'll run it by the powerpc list when I get it
> again, it only seems to happen very rarely.
It's actually a generic bug.
delayed work structs are initialised like this:
#define __DELAYED_WORK_INITIALIZER(n, f) { \
.work = __WORK_INITIALIZER((n).work, (f)), \
.timer = TIMER_INITIALIZER(NULL, 0, 0), \
}
#define DECLARE_DELAYED_WORK(n, f) \
struct delayed_work n = __DELAYED_WORK_INITIALIZER(n, f)
Note the NULL function, which I used for the key of timers. Thus, the
key is NULL, and the name is "NULL".
Now, this means that my code for the run timer:
struct lockdep_map lockdep_map =
timer->lockdep_map;
will actually have lockdep_map here with a NULL key. Once it gets into
lock_map_acquire(&lockdep_map);
it'll try to register &lockdep_map as the key.
Interestingly, that doesn't seem to be a problem on x86_64, which would
appear to be a bug, the stack certainly isn't a static location.
The patch below fixes it by using the file/lineno of the static
definition as both the name and the key -- using it as the name means
you have a good chance of finding it if something goes wrong, and using
it as the key means we have a good key for it. The patch looks horrible
though. Any better ideas? If not, I think we should roll this into the
original patch.
johannes
---
include/linux/timer.h | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
--- wireless-testing.orig/include/linux/timer.h 2009-01-28 18:55:03.270071595 +0100
+++ wireless-testing/include/linux/timer.h 2009-01-28 19:00:41.326947619 +0100
@@ -32,23 +32,35 @@ extern struct tvec_base boot_tvec_bases;
/*
* NB: because we have to copy the lockdep_map, setting _key
* here is required, otherwise it could get initialised to the
- * copy of the lockdep_map! We use the function pointer as the
- * lockdep key here.
+ * copy of the lockdep_map! We use the pointer to and the string
+ * "<file>:<line>" as the key resp. the name of the lockdep_map.
*/
-#define __TIMER_LOCKDEP_MAP_INITIALIZER(fn, name) \
- .lockdep_map = STATIC_LOCKDEP_MAP_INIT(name, fn),
+#define __TIMER_LOCKDEP_MAP_INITIALIZER(name, key) \
+ .lockdep_map = STATIC_LOCKDEP_MAP_INIT(name, key),
#else
-#define __TIMER_LOCKDEP_MAP_INITIALIZER(fn, name)
+#define __TIMER_LOCKDEP_MAP_INITIALIZER(name, key)
#endif
-#define TIMER_INITIALIZER(_function, _expires, _data) { \
+#define ____TIMER_INITIALIZER(_function, _expires, _data, _name, _key) {\
.entry = { .prev = TIMER_ENTRY_STATIC }, \
.function = (_function), \
.expires = (_expires), \
.data = (_data), \
.base = &boot_tvec_bases, \
- __TIMER_LOCKDEP_MAP_INITIALIZER((_function), #_function)\
+ __TIMER_LOCKDEP_MAP_INITIALIZER((_name), (_key)) \
}
+/*
+ * All the indirection here is required to build the
+ * "<file>:<line>" string and the pointer to it.
+ */
+#define ___TIMER_INITIALIZER(_fn, _exp, _dat, _kn) \
+ ____TIMER_INITIALIZER(_fn, _exp, _dat, (_kn), &(_kn))
+#define __TIMER_INITIALIZER(_fn, _exp, _dat, _f, _c, _l) \
+ ___TIMER_INITIALIZER(_fn, _exp, _dat, _f # _c # _l)
+#define _TIMER_INITIALIZER(_fn, _exp, _dat, _f, _l) \
+ __TIMER_INITIALIZER(_fn, _exp, _dat, _f, :, _l)
+#define TIMER_INITIALIZER(_function, _expires, _data) \
+ _TIMER_INITIALIZER(_function, _expires, _data, __FILE__, __LINE__)
#define DEFINE_TIMER(_name, _function, _expires, _data) \
struct timer_list _name = \
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3] timer: implement lockdep deadlock detection
2009-01-28 18:06 ` Johannes Berg
@ 2009-01-29 12:38 ` Ingo Molnar
2009-01-29 15:03 ` [PATCH v4] " Johannes Berg
2009-01-29 13:38 ` [PATCH v3] " Peter Zijlstra
1 sibling, 1 reply; 22+ messages in thread
From: Ingo Molnar @ 2009-01-29 12:38 UTC (permalink / raw)
To: Johannes Berg; +Cc: Peter Zijlstra, Thomas Gleixner, Linux Kernel list
* Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2009-01-28 at 11:59 +0100, Johannes Berg wrote:
>
> > > > I actually got "trying to register non-static key" on my powerpc64
> > > > machine. Is there a possibility that functions are not static??
> > >
> > > Hmm, weird, afaict static_obj() includes both text and data, for the
> > > core kernel as well as modules.
> >
> > Yeah, I'd think it should. I'll run it by the powerpc list when I get it
> > again, it only seems to happen very rarely.
>
> It's actually a generic bug.
>
> delayed work structs are initialised like this:
>
> #define __DELAYED_WORK_INITIALIZER(n, f) { \
> .work = __WORK_INITIALIZER((n).work, (f)), \
> .timer = TIMER_INITIALIZER(NULL, 0, 0), \
> }
>
> #define DECLARE_DELAYED_WORK(n, f) \
> struct delayed_work n = __DELAYED_WORK_INITIALIZER(n, f)
>
>
> Note the NULL function, which I used for the key of timers. Thus, the
> key is NULL, and the name is "NULL".
>
> Now, this means that my code for the run timer:
>
> struct lockdep_map lockdep_map =
> timer->lockdep_map;
>
> will actually have lockdep_map here with a NULL key. Once it gets into
>
> lock_map_acquire(&lockdep_map);
>
> it'll try to register &lockdep_map as the key.
>
> Interestingly, that doesn't seem to be a problem on x86_64, which would
> appear to be a bug, the stack certainly isn't a static location.
>
> The patch below fixes it by using the file/lineno of the static
> definition as both the name and the key -- using it as the name means
> you have a good chance of finding it if something goes wrong, and using
> it as the key means we have a good key for it. The patch looks horrible
> though. Any better ideas? If not, I think we should roll this into the
> original patch.
I suspect this invalidates v3 - mind resending a full v4 patch?
Ingo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3] timer: implement lockdep deadlock detection
2009-01-28 18:06 ` Johannes Berg
2009-01-29 12:38 ` Ingo Molnar
@ 2009-01-29 13:38 ` Peter Zijlstra
2009-01-29 13:44 ` Arnd Bergmann
2009-01-29 14:25 ` Johannes Berg
1 sibling, 2 replies; 22+ messages in thread
From: Peter Zijlstra @ 2009-01-29 13:38 UTC (permalink / raw)
To: Johannes Berg; +Cc: Ingo Molnar, Thomas Gleixner, Linux Kernel list
On Wed, 2009-01-28 at 19:06 +0100, Johannes Berg wrote:
> +/*
> + * All the indirection here is required to build the
> + * "<file>:<line>" string and the pointer to it.
> + */
> +#define ___TIMER_INITIALIZER(_fn, _exp, _dat, _kn) \
> + ____TIMER_INITIALIZER(_fn, _exp, _dat, (_kn), &(_kn))
> +#define __TIMER_INITIALIZER(_fn, _exp, _dat, _f, _c, _l) \
> + ___TIMER_INITIALIZER(_fn, _exp, _dat, _f # _c # _l)
> +#define _TIMER_INITIALIZER(_fn, _exp, _dat, _f, _l) \
> + __TIMER_INITIALIZER(_fn, _exp, _dat, _f, :, _l)
> +#define TIMER_INITIALIZER(_function, _expires, _data) \
> + _TIMER_INITIALIZER(_function, _expires, _data, __FILE__, __LINE__)
The regular way to write that would be:
#define __STR(foo) #foo
#define STR(foo) __STR(foo)
__FILE__ ":" STR(__LINE__)
I'm sure I saw that stringify thing somewhere in the kernel before, but
can't find it. I recently added it to lockdep.c, maybe we should add it
to kernel.h ?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3] timer: implement lockdep deadlock detection
2009-01-29 13:38 ` [PATCH v3] " Peter Zijlstra
@ 2009-01-29 13:44 ` Arnd Bergmann
2009-01-29 14:25 ` Johannes Berg
1 sibling, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2009-01-29 13:44 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Johannes Berg, Ingo Molnar, Thomas Gleixner, Linux Kernel list
On Thursday 29 January 2009, Peter Zijlstra wrote:
> I'm sure I saw that stringify thing somewhere in the kernel before, but
> can't find it. I recently added it to lockdep.c, maybe we should add it
> to kernel.h ?
>
It's in <linux/stringify.h>.
Arnd <><
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3] timer: implement lockdep deadlock detection
2009-01-29 13:38 ` [PATCH v3] " Peter Zijlstra
2009-01-29 13:44 ` Arnd Bergmann
@ 2009-01-29 14:25 ` Johannes Berg
1 sibling, 0 replies; 22+ messages in thread
From: Johannes Berg @ 2009-01-29 14:25 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Ingo Molnar, Thomas Gleixner, Linux Kernel list
[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]
On Thu, 2009-01-29 at 14:38 +0100, Peter Zijlstra wrote:
> On Wed, 2009-01-28 at 19:06 +0100, Johannes Berg wrote:
>
> > +/*
> > + * All the indirection here is required to build the
> > + * "<file>:<line>" string and the pointer to it.
> > + */
> > +#define ___TIMER_INITIALIZER(_fn, _exp, _dat, _kn) \
> > + ____TIMER_INITIALIZER(_fn, _exp, _dat, (_kn), &(_kn))
> > +#define __TIMER_INITIALIZER(_fn, _exp, _dat, _f, _c, _l) \
> > + ___TIMER_INITIALIZER(_fn, _exp, _dat, _f # _c # _l)
> > +#define _TIMER_INITIALIZER(_fn, _exp, _dat, _f, _l) \
> > + __TIMER_INITIALIZER(_fn, _exp, _dat, _f, :, _l)
> > +#define TIMER_INITIALIZER(_function, _expires, _data) \
> > + _TIMER_INITIALIZER(_function, _expires, _data, __FILE__, __LINE__)
>
> The regular way to write that would be:
>
> #define __STR(foo) #foo
> #define STR(foo) __STR(foo)
>
> __FILE__ ":" STR(__LINE__)
>
> I'm sure I saw that stringify thing somewhere in the kernel before, but
> can't find it. I recently added it to lockdep.c, maybe we should add it
> to kernel.h ?
Ohh, hmm, my cpp could be stronger :) I'll try out with stringify and
send a complete v4.
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4] timer: implement lockdep deadlock detection
2009-01-29 12:38 ` Ingo Molnar
@ 2009-01-29 15:03 ` Johannes Berg
0 siblings, 0 replies; 22+ messages in thread
From: Johannes Berg @ 2009-01-29 15:03 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Peter Zijlstra, Thomas Gleixner, Linux Kernel list
This modifies the timer code in a way to allow lockdep to detect
deadlocks resulting from a lock being taken in the timer function
as well as around the del_timer_sync() call.
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
v2: fix coding/comment style
v3: accept freeing timer from timer function, fix init_timer_on_stack
in no-lockdep case
v4: fix statically declared timer names/lockdep keys
include/linux/timer.h | 93 +++++++++++++++++++++++++++++++++++++++++++++-----
kernel/timer.c | 70 +++++++++++++++++++++++++++++++------
2 files changed, 142 insertions(+), 21 deletions(-)
--- wireless-testing.orig/include/linux/timer.h 2009-01-28 11:33:17.000000000 +0100
+++ wireless-testing/include/linux/timer.h 2009-01-29 16:01:49.000000000 +0100
@@ -5,6 +5,7 @@
#include <linux/ktime.h>
#include <linux/stddef.h>
#include <linux/debugobjects.h>
+#include <linux/stringify.h>
struct tvec_base;
@@ -21,52 +22,126 @@ struct timer_list {
char start_comm[16];
int start_pid;
#endif
+#ifdef CONFIG_LOCKDEP
+ struct lockdep_map lockdep_map;
+#endif
};
extern struct tvec_base boot_tvec_bases;
+#ifdef CONFIG_LOCKDEP
+/*
+ * NB: because we have to copy the lockdep_map, setting the lockdep_map key
+ * (second argument) here is required, otherwise it could be initialised to
+ * the copy of the lockdep_map later! We use the pointer to and the string
+ * "<file>:<line>" as the key resp. the name of the lockdep_map.
+ */
+#define __TIMER_LOCKDEP_MAP_INITIALIZER(_kn) \
+ .lockdep_map = STATIC_LOCKDEP_MAP_INIT(_kn, &_kn),
+#else
+#define __TIMER_LOCKDEP_MAP_INITIALIZER(_kn)
+#endif
+
#define TIMER_INITIALIZER(_function, _expires, _data) { \
.entry = { .prev = TIMER_ENTRY_STATIC }, \
.function = (_function), \
.expires = (_expires), \
.data = (_data), \
.base = &boot_tvec_bases, \
+ __TIMER_LOCKDEP_MAP_INITIALIZER( \
+ __FILE__ ":" __stringify(__LINE__)) \
}
#define DEFINE_TIMER(_name, _function, _expires, _data) \
struct timer_list _name = \
TIMER_INITIALIZER(_function, _expires, _data)
-void init_timer(struct timer_list *timer);
-void init_timer_deferrable(struct timer_list *timer);
+void init_timer_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
+void init_timer_deferrable_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
+
+#ifdef CONFIG_LOCKDEP
+#define init_timer(timer) \
+ do { \
+ static struct lock_class_key __key; \
+ init_timer_key((timer), #timer, &__key); \
+ } while (0)
+
+#define init_timer_deferrable(timer) \
+ do { \
+ static struct lock_class_key __key; \
+ init_timer_deferrable_key((timer), #timer, &__key); \
+ } while (0)
+
+#define init_timer_on_stack(timer) \
+ do { \
+ static struct lock_class_key __key; \
+ init_timer_on_stack_key((timer), #timer, &__key); \
+ } while (0)
+
+#define setup_timer(timer, fn, data) \
+ do { \
+ static struct lock_class_key __key; \
+ setup_timer_key((timer), #timer, &__key, (fn), (data));\
+ } while (0)
+
+#define setup_timer_on_stack(timer, fn, data) \
+ do { \
+ static struct lock_class_key __key; \
+ setup_timer_on_stack_key((timer), #timer, &__key, \
+ (fn), (data)); \
+ } while (0)
+#else
+#define init_timer(timer)\
+ init_timer_key((timer), NULL, NULL)
+#define init_timer_deferrable(timer)\
+ init_timer_deferrable_key((timer), NULL, NULL)
+#define init_timer_on_stack(timer)\
+ init_timer_on_stack_key((timer), NULL, NULL)
+#define setup_timer(timer, fn, data)\
+ setup_timer_key((timer), NULL, NULL, (fn), (data))
+#define setup_timer_on_stack(timer, fn, data)\
+ setup_timer_on_stack_key((timer), NULL, NULL, (fn), (data))
+#endif
#ifdef CONFIG_DEBUG_OBJECTS_TIMERS
-extern void init_timer_on_stack(struct timer_list *timer);
+extern void init_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
extern void destroy_timer_on_stack(struct timer_list *timer);
#else
static inline void destroy_timer_on_stack(struct timer_list *timer) { }
-static inline void init_timer_on_stack(struct timer_list *timer)
+static inline void init_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
- init_timer(timer);
+ init_timer_key(timer, name, key);
}
#endif
-static inline void setup_timer(struct timer_list * timer,
+static inline void setup_timer_key(struct timer_list * timer,
+ const char *name,
+ struct lock_class_key *key,
void (*function)(unsigned long),
unsigned long data)
{
timer->function = function;
timer->data = data;
- init_timer(timer);
+ init_timer_key(timer, name, key);
}
-static inline void setup_timer_on_stack(struct timer_list *timer,
+static inline void setup_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key,
void (*function)(unsigned long),
unsigned long data)
{
timer->function = function;
timer->data = data;
- init_timer_on_stack(timer);
+ init_timer_on_stack_key(timer, name, key);
}
/**
--- wireless-testing.orig/kernel/timer.c 2009-01-28 11:33:17.000000000 +0100
+++ wireless-testing/kernel/timer.c 2009-01-28 11:34:26.000000000 +0100
@@ -491,14 +491,18 @@ static inline void debug_timer_free(stru
debug_object_free(timer, &timer_debug_descr);
}
-static void __init_timer(struct timer_list *timer);
-
-void init_timer_on_stack(struct timer_list *timer)
+static void __init_timer(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key);
+
+void init_timer_on_stack_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
debug_object_init_on_stack(timer, &timer_debug_descr);
- __init_timer(timer);
+ __init_timer(timer, name, key);
}
-EXPORT_SYMBOL_GPL(init_timer_on_stack);
+EXPORT_SYMBOL_GPL(init_timer_on_stack_key);
void destroy_timer_on_stack(struct timer_list *timer)
{
@@ -512,7 +516,9 @@ static inline void debug_timer_activate(
static inline void debug_timer_deactivate(struct timer_list *timer) { }
#endif
-static void __init_timer(struct timer_list *timer)
+static void __init_timer(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
timer->entry.next = NULL;
timer->base = __raw_get_cpu_var(tvec_bases);
@@ -521,6 +527,7 @@ static void __init_timer(struct timer_li
timer->start_pid = -1;
memset(timer->start_comm, 0, TASK_COMM_LEN);
#endif
+ lockdep_init_map(&timer->lockdep_map, name, key, 0);
}
/**
@@ -530,19 +537,23 @@ static void __init_timer(struct timer_li
* init_timer() must be done to a timer prior calling *any* of the
* other timer functions.
*/
-void init_timer(struct timer_list *timer)
+void init_timer_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
debug_timer_init(timer);
- __init_timer(timer);
+ __init_timer(timer, name, key);
}
-EXPORT_SYMBOL(init_timer);
+EXPORT_SYMBOL(init_timer_key);
-void init_timer_deferrable(struct timer_list *timer)
+void init_timer_deferrable_key(struct timer_list *timer,
+ const char *name,
+ struct lock_class_key *key)
{
- init_timer(timer);
+ init_timer_key(timer, name, key);
timer_set_deferrable(timer);
}
-EXPORT_SYMBOL(init_timer_deferrable);
+EXPORT_SYMBOL(init_timer_deferrable_key);
static inline void detach_timer(struct timer_list *timer,
int clear_pending)
@@ -789,6 +800,15 @@ EXPORT_SYMBOL(try_to_del_timer_sync);
*/
int del_timer_sync(struct timer_list *timer)
{
+#ifdef CONFIG_LOCKDEP
+ unsigned long flags;
+
+ local_irq_save(flags);
+ lock_map_acquire(&timer->lockdep_map);
+ lock_map_release(&timer->lockdep_map);
+ local_irq_restore(flags);
+#endif
+
for (;;) {
int ret = try_to_del_timer_sync(timer);
if (ret >= 0)
@@ -861,10 +881,36 @@ static inline void __run_timers(struct t
set_running_timer(base, timer);
detach_timer(timer, 1);
+
spin_unlock_irq(&base->lock);
{
int preempt_count = preempt_count();
+
+#ifdef CONFIG_LOCKDEP
+ /*
+ * It is permissible to free the timer from
+ * inside the function that is called from
+ * it, this we need to take into account for
+ * lockdep too. To avoid bogus "held lock
+ * freed" warnings as well as problems when
+ * looking into timer->lockdep_map, make a
+ * copy and use that here.
+ */
+ struct lockdep_map lockdep_map =
+ timer->lockdep_map;
+#endif
+ /*
+ * Couple the lock chain with the lock chain at
+ * del_timer_sync() by acquiring the lock_map
+ * around the fn() call here and in
+ * del_timer_sync().
+ */
+ lock_map_acquire(&lockdep_map);
+
fn(data);
+
+ lock_map_release(&lockdep_map);
+
if (preempt_count != preempt_count()) {
printk(KERN_ERR "huh, entered %p "
"with preempt_count %08x, exited"
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2009-01-29 15:03 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-01-26 22:59 [PATCH] timer: implement lockdep deadlock detection Johannes Berg
2009-01-26 23:03 ` Johannes Berg
2009-01-26 23:07 ` Ingo Molnar
2009-01-27 8:45 ` Johannes Berg
2009-01-27 8:46 ` [PATCH v2] " Johannes Berg
2009-01-27 13:27 ` Ingo Molnar
2009-01-27 13:41 ` Ingo Molnar
2009-01-27 18:06 ` Johannes Berg
2009-01-27 18:30 ` Peter Zijlstra
2009-01-27 18:33 ` Johannes Berg
2009-01-27 18:57 ` [PATCH v3] " Johannes Berg
2009-01-28 8:20 ` Peter Zijlstra
2009-01-28 9:54 ` Johannes Berg
2009-01-28 10:13 ` Peter Zijlstra
2009-01-28 10:54 ` Johannes Berg
2009-01-28 18:06 ` Johannes Berg
2009-01-29 12:38 ` Ingo Molnar
2009-01-29 15:03 ` [PATCH v4] " Johannes Berg
2009-01-29 13:38 ` [PATCH v3] " Peter Zijlstra
2009-01-29 13:44 ` Arnd Bergmann
2009-01-29 14:25 ` Johannes Berg
2009-01-27 18:12 ` [PATCH v2] " Johannes Berg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).