Re: [PATCH -mm] workqueue: debug possible lockups in flush_workqueue
On Thu, Apr 19, 2007 at 08:14:16AM +0200, Ingo Molnar wrote: > > * Jarek Poplawski <[EMAIL PROTECTED]> wrote: > > > Here is my patch proposal for detecting possible lockups, when > > flush_workqueue caller holds a lock (e.g. rtnl_lock) also used in work > > functions. > > looks good in principle - did you test it and it caught a bug that wasnt > caught before? Yes, but it was only my own testing bug... (I'm not a good tester, sorry). > > > +#ifdef CONFIG_PROVE_LOCKING > > +/* Detect possible flush_workqueue() lockup with circular dependency > > check. */ > > +static struct lockdep_map flush_dep_map = { .name = "flush_dep_map" }; > > +#endif > > > +#ifdef CONFIG_PROVE_LOCKING > > + /* lockdep dependency: flush_dep_map (read) before any lock: */ > > + lock_acquire(_dep_map, 0, 0, 1, 2, _THIS_IP_); > > +#endif > > i think the #ifdef should only be needed for the .name initialization - > both lock_acquire() and lock_release() maps to NOP if PROVE_LOCKING is > off. There is also DEBUG_LOCK_ALLOC without PROVE_LOCKING possibility, which isn't usable here. Jarek P. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] workqueue: debug possible lockups in flush_workqueue
* Jarek Poplawski <[EMAIL PROTECTED]> wrote: > Here is my patch proposal for detecting possible lockups, when > flush_workqueue caller holds a lock (e.g. rtnl_lock) also used in work > functions. looks good in principle - did you test it and it caught a bug that wasnt caught before? > +#ifdef CONFIG_PROVE_LOCKING > +/* Detect possible flush_workqueue() lockup with circular dependency check. > */ > +static struct lockdep_map flush_dep_map = { .name = "flush_dep_map" }; > +#endif > +#ifdef CONFIG_PROVE_LOCKING > + /* lockdep dependency: flush_dep_map (read) before any lock: */ > + lock_acquire(_dep_map, 0, 0, 1, 2, _THIS_IP_); > +#endif i think the #ifdef should only be needed for the .name initialization - both lock_acquire() and lock_release() maps to NOP if PROVE_LOCKING is off. Ingo - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] workqueue: debug possible lockups in flush_workqueue
* Jarek Poplawski [EMAIL PROTECTED] wrote: Here is my patch proposal for detecting possible lockups, when flush_workqueue caller holds a lock (e.g. rtnl_lock) also used in work functions. looks good in principle - did you test it and it caught a bug that wasnt caught before? +#ifdef CONFIG_PROVE_LOCKING +/* Detect possible flush_workqueue() lockup with circular dependency check. */ +static struct lockdep_map flush_dep_map = { .name = flush_dep_map }; +#endif +#ifdef CONFIG_PROVE_LOCKING + /* lockdep dependency: flush_dep_map (read) before any lock: */ + lock_acquire(flush_dep_map, 0, 0, 1, 2, _THIS_IP_); +#endif i think the #ifdef should only be needed for the .name initialization - both lock_acquire() and lock_release() maps to NOP if PROVE_LOCKING is off. Ingo - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] workqueue: debug possible lockups in flush_workqueue
On Thu, Apr 19, 2007 at 08:14:16AM +0200, Ingo Molnar wrote: * Jarek Poplawski [EMAIL PROTECTED] wrote: Here is my patch proposal for detecting possible lockups, when flush_workqueue caller holds a lock (e.g. rtnl_lock) also used in work functions. looks good in principle - did you test it and it caught a bug that wasnt caught before? Yes, but it was only my own testing bug... (I'm not a good tester, sorry). +#ifdef CONFIG_PROVE_LOCKING +/* Detect possible flush_workqueue() lockup with circular dependency check. */ +static struct lockdep_map flush_dep_map = { .name = flush_dep_map }; +#endif +#ifdef CONFIG_PROVE_LOCKING + /* lockdep dependency: flush_dep_map (read) before any lock: */ + lock_acquire(flush_dep_map, 0, 0, 1, 2, _THIS_IP_); +#endif i think the #ifdef should only be needed for the .name initialization - both lock_acquire() and lock_release() maps to NOP if PROVE_LOCKING is off. There is also DEBUG_LOCK_ALLOC without PROVE_LOCKING possibility, which isn't usable here. Jarek P. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH -mm] workqueue: debug possible lockups in flush_workqueue
Hi, Here is my patch proposal for detecting possible lockups, when flush_workqueue caller holds a lock (e.g. rtnl_lock) also used in work functions. Regards, Jarek P. Signed-off-by: Jarek Poplawski <[EMAIL PROTECTED]> --- diff -Nurp 2.6.21-rc6-mm1-/kernel/workqueue.c 2.6.21-rc6-mm1/kernel/workqueue.c --- 2.6.21-rc6-mm1-/kernel/workqueue.c 2007-04-18 20:07:45.0 +0200 +++ 2.6.21-rc6-mm1/kernel/workqueue.c 2007-04-18 21:29:50.0 +0200 @@ -67,6 +67,12 @@ struct workqueue_struct { /* All the per-cpu workqueues on the system, for hotplug cpu to add/remove threads to each one as cpus come/go. */ static DEFINE_MUTEX(workqueue_mutex); + +#ifdef CONFIG_PROVE_LOCKING +/* Detect possible flush_workqueue() lockup with circular dependency check. */ +static struct lockdep_map flush_dep_map = { .name = "flush_dep_map" }; +#endif + static LIST_HEAD(workqueues); static int singlethread_cpu __read_mostly; @@ -247,8 +253,15 @@ static void run_workqueue(struct cpu_wor BUG_ON(get_wq_data(work) != cwq); work_clear_pending(work); +#ifdef CONFIG_PROVE_LOCKING + /* lockdep dependency: flush_dep_map (read) before any lock: */ + lock_acquire(_dep_map, 0, 0, 1, 2, _THIS_IP_); +#endif f(work); +#ifdef CONFIG_PROVE_LOCKING + lock_release(_dep_map, 1, _THIS_IP_); +#endif if (unlikely(in_atomic() || lockdep_depth(current) > 0)) { printk(KERN_ERR "BUG: workqueue leaked lock or atomic: " "%s/0x%08x/%d\n", @@ -389,6 +402,14 @@ void fastcall flush_workqueue(struct wor int cpu; might_sleep(); +#ifdef CONFIG_PROVE_LOCKING + /* +* Add lockdep dependency: flush_dep_map (exclusive) +* after any held mutex or rwsem. +*/ + lock_acquire(_dep_map, 0, 0, 0, 2, _THIS_IP_); + lock_release(_dep_map, 1, _THIS_IP_); +#endif for_each_cpu_mask(cpu, *cpu_map) flush_cpu_workqueue(per_cpu_ptr(wq->cpu_wq, cpu)); } - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH -mm] workqueue: debug possible lockups in flush_workqueue
Hi, Here is my patch proposal for detecting possible lockups, when flush_workqueue caller holds a lock (e.g. rtnl_lock) also used in work functions. Regards, Jarek P. Signed-off-by: Jarek Poplawski [EMAIL PROTECTED] --- diff -Nurp 2.6.21-rc6-mm1-/kernel/workqueue.c 2.6.21-rc6-mm1/kernel/workqueue.c --- 2.6.21-rc6-mm1-/kernel/workqueue.c 2007-04-18 20:07:45.0 +0200 +++ 2.6.21-rc6-mm1/kernel/workqueue.c 2007-04-18 21:29:50.0 +0200 @@ -67,6 +67,12 @@ struct workqueue_struct { /* All the per-cpu workqueues on the system, for hotplug cpu to add/remove threads to each one as cpus come/go. */ static DEFINE_MUTEX(workqueue_mutex); + +#ifdef CONFIG_PROVE_LOCKING +/* Detect possible flush_workqueue() lockup with circular dependency check. */ +static struct lockdep_map flush_dep_map = { .name = flush_dep_map }; +#endif + static LIST_HEAD(workqueues); static int singlethread_cpu __read_mostly; @@ -247,8 +253,15 @@ static void run_workqueue(struct cpu_wor BUG_ON(get_wq_data(work) != cwq); work_clear_pending(work); +#ifdef CONFIG_PROVE_LOCKING + /* lockdep dependency: flush_dep_map (read) before any lock: */ + lock_acquire(flush_dep_map, 0, 0, 1, 2, _THIS_IP_); +#endif f(work); +#ifdef CONFIG_PROVE_LOCKING + lock_release(flush_dep_map, 1, _THIS_IP_); +#endif if (unlikely(in_atomic() || lockdep_depth(current) 0)) { printk(KERN_ERR BUG: workqueue leaked lock or atomic: %s/0x%08x/%d\n, @@ -389,6 +402,14 @@ void fastcall flush_workqueue(struct wor int cpu; might_sleep(); +#ifdef CONFIG_PROVE_LOCKING + /* +* Add lockdep dependency: flush_dep_map (exclusive) +* after any held mutex or rwsem. +*/ + lock_acquire(flush_dep_map, 0, 0, 0, 2, _THIS_IP_); + lock_release(flush_dep_map, 1, _THIS_IP_); +#endif for_each_cpu_mask(cpu, *cpu_map) flush_cpu_workqueue(per_cpu_ptr(wq-cpu_wq, cpu)); } - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/