Hello all,
This is _RFC_.
I want to request for comments about if it's reasonable conceptually. If
yes, I want to resend after working it more carefully.
Could you let me know your opinions about this?
----->8-----
>From 448360c343477fff63df766544eec4620657a59e Mon Sep 17 00:00:00 2001
From: Byungchul Park <byungchul.p...@lge.com>
Date: Fri, 25 Aug 2017 17:35:07 +0900
Subject: [RFC] workqueue: remove manual lockdep uses to detect deadlocks
We introduced the following commit to detect deadlocks caused by
wait_for_completion() in flush_{workqueue, work}() and other locks. But
now LOCKDEP_COMPLETIONS is introduced, such works are automatically done
by LOCKDEP_COMPLETIONS. So it doesn't have to be done manually anymore.
Removed it.
commit 4e6045f134784f4b158b3c0f7a282b04bd816887
workqueue: debug flushing deadlocks with lockdep
Signed-off-by: Byungchul Park <byungchul.p...@lge.com>
---
include/linux/workqueue.h | 43 -------------------------------------------
kernel/workqueue.c | 38 --------------------------------------
2 files changed, 81 deletions(-)
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index db6dc9d..91d0e14 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -8,7 +8,6 @@
#include <linux/timer.h>
#include <linux/linkage.h>
#include <linux/bitops.h>
-#include <linux/lockdep.h>
#include <linux/threads.h>
#include <linux/atomic.h>
#include <linux/cpumask.h>
@@ -101,9 +100,6 @@ struct work_struct {
atomic_long_t data;
struct list_head entry;
work_func_t func;
-#ifdef CONFIG_LOCKDEP
- struct lockdep_map lockdep_map;
-#endif
};
#define WORK_DATA_INIT() ATOMIC_LONG_INIT((unsigned
long)WORK_STRUCT_NO_POOL)
@@ -154,23 +150,10 @@ struct execute_work {
struct work_struct work;
};
-#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!
- */
-#define __WORK_INIT_LOCKDEP_MAP(n, k) \
- .lockdep_map = STATIC_LOCKDEP_MAP_INIT(n, k),
-#else
-#define __WORK_INIT_LOCKDEP_MAP(n, k)
-#endif
-
#define __WORK_INITIALIZER(n, f) { \
.data = WORK_DATA_STATIC_INIT(), \
.entry = { &(n).entry, &(n).entry }, \
.func = (f), \
- __WORK_INIT_LOCKDEP_MAP(#n, &(n)) \
}
#define __DELAYED_WORK_INITIALIZER(n, f, tflags) { \
@@ -211,26 +194,13 @@ static inline void destroy_delayed_work_on_stack(struct
delayed_work *work) { }
* assignment of the work data initializer allows the compiler
* to generate better code.
*/
-#ifdef CONFIG_LOCKDEP
#define __INIT_WORK(_work, _func, _onstack) \
do { \
- static struct lock_class_key __key; \
- \
__init_work((_work), _onstack); \
(_work)->data = (atomic_long_t) WORK_DATA_INIT(); \
- lockdep_init_map(&(_work)->lockdep_map, #_work, &__key, 0); \
INIT_LIST_HEAD(&(_work)->entry); \
(_work)->func = (_func); \
} while (0)
-#else
-#define __INIT_WORK(_work, _func, _onstack) \
- do { \
- __init_work((_work), _onstack); \
- (_work)->data = (atomic_long_t) WORK_DATA_INIT(); \
- INIT_LIST_HEAD(&(_work)->entry); \
- (_work)->func = (_func); \
- } while (0)
-#endif
#define INIT_WORK(_work, _func)
\
__INIT_WORK((_work), (_func), 0)
@@ -392,22 +362,9 @@ enum {
* RETURNS:
* Pointer to the allocated workqueue on success, %NULL on failure.
*/
-#ifdef CONFIG_LOCKDEP
-#define alloc_workqueue(fmt, flags, max_active, args...) \
-({ \
- static struct lock_class_key __key; \
- const char *__lock_name; \
- \
- __lock_name = #fmt#args; \
- \
- __alloc_workqueue_key((fmt), (flags), (max_active), \
- &__key, __lock_name, ##args); \
-})
-#else
#define alloc_workqueue(fmt, flags, max_active, args...) \
__alloc_workqueue_key((fmt), (flags), (max_active), \
NULL, NULL, ##args)
-#endif
/**
* alloc_ordered_workqueue - allocate an ordered workqueue
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index f128b3b..87d4bc2 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -40,7 +40,6 @@
#include <linux/freezer.h>
#include <linux/kallsyms.h>
#include <linux/debug_locks.h>
-#include <linux/lockdep.h>
#include <linux/idr.h>
#include <linux/jhash.h>
#include <linux/hashtable.h>
@@ -260,9 +259,6 @@ struct workqueue_struct {
#ifdef CONFIG_SYSFS
struct wq_device *wq_dev; /* I: for sysfs interface */
#endif
-#ifdef CONFIG_LOCKDEP
- struct lockdep_map lockdep_map;
-#endif
char name[WQ_NAME_LEN]; /* I: workqueue name */
/*
@@ -2024,18 +2020,7 @@ static void process_one_work(struct worker *worker,
struct work_struct *work)
bool cpu_intensive = pwq->wq->flags & WQ_CPU_INTENSIVE;
int work_color;
struct worker *collision;
-#ifdef CONFIG_LOCKDEP
- /*
- * It is permissible to free the struct work_struct 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
- * work->lockdep_map, make a copy and use that here.
- */
- struct lockdep_map lockdep_map;
- lockdep_copy_map(&lockdep_map, &work->lockdep_map);
-#endif
/* ensure we're on the correct CPU */
WARN_ON_ONCE(!(pool->flags & POOL_DISASSOCIATED) &&
raw_smp_processor_id() != pool->cpu);
@@ -2091,8 +2076,6 @@ static void process_one_work(struct worker *worker,
struct work_struct *work)
spin_unlock_irq(&pool->lock);
- lock_map_acquire_read(&pwq->wq->lockdep_map);
- lock_map_acquire(&lockdep_map);
crossrelease_hist_start(XHLOCK_PROC);
trace_workqueue_execute_start(work);
worker->current_func(work);
@@ -2102,8 +2085,6 @@ static void process_one_work(struct worker *worker,
struct work_struct *work)
*/
trace_workqueue_execute_end(work);
crossrelease_hist_end(XHLOCK_PROC);
- lock_map_release(&lockdep_map);
- lock_map_release(&pwq->wq->lockdep_map);
if (unlikely(in_atomic() || lockdep_depth(current) > 0)) {
pr_err("BUG: workqueue leaked lock or atomic: %s/0x%08x/%d\n"
@@ -2598,9 +2579,6 @@ void flush_workqueue(struct workqueue_struct *wq)
if (WARN_ON(!wq_online))
return;
- lock_map_acquire(&wq->lockdep_map);
- lock_map_release(&wq->lockdep_map);
-
mutex_lock(&wq->mutex);
/*
@@ -2825,18 +2803,6 @@ static bool start_flush_work(struct work_struct *work,
struct wq_barrier *barr)
insert_wq_barrier(pwq, barr, work, worker);
spin_unlock_irq(&pool->lock);
- /*
- * If @max_active is 1 or rescuer is in use, flushing another work
- * item on the same workqueue may lead to deadlock. Make sure the
- * flusher is not running on the same workqueue by verifying write
- * access.
- */
- if (pwq->wq->saved_max_active == 1 || pwq->wq->rescuer)
- lock_map_acquire(&pwq->wq->lockdep_map);
- else
- lock_map_acquire_read(&pwq->wq->lockdep_map);
- lock_map_release(&pwq->wq->lockdep_map);
-
return true;
already_gone:
spin_unlock_irq(&pool->lock);
@@ -2861,9 +2827,6 @@ bool flush_work(struct work_struct *work)
if (WARN_ON(!wq_online))
return false;
- lock_map_acquire(&work->lockdep_map);
- lock_map_release(&work->lockdep_map);
-
if (start_flush_work(work, &barr)) {
wait_for_completion(&barr.done);
destroy_work_on_stack(&barr.work);
@@ -3996,7 +3959,6 @@ struct workqueue_struct *__alloc_workqueue_key(const char
*fmt,
INIT_LIST_HEAD(&wq->flusher_overflow);
INIT_LIST_HEAD(&wq->maydays);
- lockdep_init_map(&wq->lockdep_map, lock_name, key, 0);
INIT_LIST_HEAD(&wq->list);
if (alloc_and_link_pwqs(wq) < 0)
--
1.9.1
