workqueue/lockdep: Explicitly initialize wq_barrier::done::map
Subsystems (2)
PATCH 1/1 workqueue/lockdep: Explicitly initialize wq_barrier::done::map
With CROSSRELEASE feature introduced, such a potential deadlock is reported:
Quoted 5 lines
Worker A : acquired of wfc.work -> wait for
cpu_hotplug_lockto be released
Task B : acquired ofcpu_hotplug_lock-> wait for lock#3 to be released
Task C : acquired of lock#3 -> wait for completion ofbarr->done
(Task C is inlru_add_drain_all_cpuslocked())
Worker D : wait for wfc.work to be released -> will completebarr->done
However, given this very case, such a dead lock could not happen because Task C's barr->done and Worker D's barr->done can not be the same instance. So this is a false positive.
The reason of this false positive is we initialize all wq_barrier::done at insert_wq_barrier() via init_completion(), which makes them belong to the same lock class, therefore, impossible circles are reported.
To fix this, explicitly initialize the lockdep map for wq_barrier::done in insert_wq_barrier(), so that the lock class key of wq_barrier::done is a subkey of the corresponding work_struct, as a result we won't build a dependency between a wq_barrier with a unrelated work, and we can differ wq barriers based on the related works, so the false positive above is avoided.
Also define the empty lockdep_init_map_crosslock() for !CROSSRELEASE to make the code simple and away from unnecessary #ifdefs.
--- include/linux/lockdep.h | 1 + kernel/workqueue.c | 11 ++++++++++- 2 files changed, 11 insertions(+), 1 deletion(-)
include/linux/lockdep.h +1 -0
@@ -583,6 +583,7 @@ extern void crossrelease_hist_end(enum xhlock_context_t c);extern void lockdep_init_task (struct task_struct *task);extern void lockdep_free_task (struct task_struct *task);#else +#define lockdep_init_map_crosslock (m, n, k, s) do {} while (0)/* * To initialize a lockdep_map statically use this macro. * Note that _name must not be NULL.
kernel/workqueue.c +10 -1
@@ -2476,7 +2476,16 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,*/ INIT_WORK_ONSTACK (&barr->work, wq_barrier_func);__set_bit (WORK_STRUCT_PENDING_BIT ,work_data_bits (&barr->work)); -init_completion (&barr->done); + +/* +* Explicitly init the crosslock for wq_barrier::done, make its lock +* key is a subkey of the corresponding work. As a result we won't +* build a dependency between a wq_barrier::done and a unrelated work. +*/ +lockdep_init_map_crosslock ((struct lockdep_map *)&barr->done.map, +"(complete)wq_barr::done" , + target->lockdep_map.key,1 ); +__init_completion (&barr->done); barr->task = current;/*
Signature · 1 line
2.14.1
tip-bot for Boqun Feng
merge bot[tip:locking/core] locking/lockdep: Explicitly initialize wq_barrier::done::map
locking/lockdep: Explicitly initialize wq_barrier::done::map
Commit-ID: 52fa5bc5cbba089f09bc2c372e3432f3f3e48051 Gitweb: http://git.kernel.org/tip/52fa5bc5cbba089f09bc2c372e3432f3f3e48051 Author: Boqun Feng <[email protected]> AuthorDate: Thu, 17 Aug 2017 17:46:12 +0800 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 17 Aug 2017 12:12:33 +0200
locking/lockdep: Explicitly initialize wq_barrier::done::map
With the new lockdep crossrelease feature, which checks completions usage, a false positive is reported in the workqueue code:
Quoted 5 lines
Worker A : acquired of wfc.work -> wait for
cpu_hotplug_lockto be released
Task B : acquired ofcpu_hotplug_lock-> wait for lock#3 to be released
Task C : acquired of lock#3 -> wait for completion ofbarr->done
(Task C is inlru_add_drain_all_cpuslocked())
Worker D : wait for wfc.work to be released -> will completebarr->done
Such a dead lock can not happen because Task C's barr->done and Worker D's barr->done can not be the same instance.
The reason of this false positive is we initialize all wq_barrier::done at insert_wq_barrier() via init_completion(), which makes them belong to the same lock class, therefore, impossible circles are reported.
To fix this, explicitly initialize the lockdep map for wq_barrier::done in insert_wq_barrier(), so that the lock class key of wq_barrier::done is a subkey of the corresponding work_struct, as a result we won't build a dependency between a wq_barrier with a unrelated work, and we can differ wq barriers based on the related works, so the false positive above is avoided.
Also define the empty lockdep_init_map_crosslock() for !CROSSRELEASE to make the code simple and away from unnecessary #ifdefs.
--- include/linux/lockdep.h | 1 + kernel/workqueue.c | 11 ++++++++++- 2 files changed, 11 insertions(+), 1 deletion(-)
include/linux/lockdep.h +1 -0
@@ -583,6 +583,7 @@ extern void crossrelease_hist_end(enum xhlock_context_t c);extern void lockdep_init_task (struct task_struct *task);extern void lockdep_free_task (struct task_struct *task);#else +#define lockdep_init_map_crosslock (m, n, k, s) do {} while (0)/* * To initialize a lockdep_map statically use this macro. * Note that _name must not be NULL.
kernel/workqueue.c +10 -1
@@ -2476,7 +2476,16 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,*/ INIT_WORK_ONSTACK (&barr->work, wq_barrier_func);__set_bit (WORK_STRUCT_PENDING_BIT ,work_data_bits (&barr->work)); -init_completion (&barr->done); + +/* +* Explicitly init the crosslock for wq_barrier::done, make its lock +* key a subkey of the corresponding work. As a result we won't +* build a dependency between wq_barrier::done and unrelated work. +*/ +lockdep_init_map_crosslock ((struct lockdep_map *)&barr->done.map, +"(complete)wq_barr::done" , + target->lockdep_map.key,1 ); +__init_completion (&barr->done); barr->task = current;/*
Quoted 24 lines · Boqun Feng
With CROSSRELEASE feature introduced, such a potential deadlock is reported:
Quoted 5 lines
Worker A : acquired of wfc.work -> wait for
cpu_hotplug_lockto be released
Task B : acquired ofcpu_hotplug_lock-> wait for lock#3 to be released
Task C : acquired of lock#3 -> wait for completion ofbarr->done
(Task C is inlru_add_drain_all_cpuslocked())
Worker D : wait for wfc.work to be released -> will completebarr->doneHowever, given this very case, such a dead lock could not happen because Task C's
barr->doneand Worker D'sbarr->donecan not be the same instance. So this is a false positive.The reason of this false positive is we initialize all
wq_barrier::doneatinsert_wq_barrier()viainit_completion(), which makes them belong to the same lock class, therefore, impossible circles are reported.To fix this, explicitly initialize the lockdep map for
wq_barrier::doneininsert_wq_barrier(), so that the lock class key ofwq_barrier::doneis a subkey of the correspondingwork_struct, as a result we won't build a dependency between awq_barrierwith a unrelated work, and we can differ wq barriers based on the related works, so the false positive above is avoided.
Quoted 6 lines
Also define the empty
lockdep_init_map_crosslock()for !CROSSRELEASE to make the code simple and away from unnecessary #ifdefs.Reported-by Ingo Molnar <[email protected]>Signed-off-by Boqun Feng <[email protected]>Cc Byungchul Park <[email protected]>
Thanks.
Signature · 1 line
tejun