Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755091AbdHYIlL (ORCPT ); Fri, 25 Aug 2017 04:41:11 -0400 Received: from LGEAMRELO13.lge.com ([156.147.23.53]:34311 "EHLO lgeamrelo13.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754942AbdHYIlI (ORCPT ); Fri, 25 Aug 2017 04:41:08 -0400 X-Original-SENDERIP: 156.147.1.121 X-Original-MAILFROM: byungchul.park@lge.com X-Original-SENDERIP: 10.177.222.33 X-Original-MAILFROM: byungchul.park@lge.com From: Byungchul Park To: tj@kernel.org, johannes.berg@intel.com Cc: peterz@infradead.org, mingo@kernel.org, tglx@linutronix.de, linux-kernel@vger.kernel.org, kernel-team@lge.com Subject: [RFC] workqueue: remove manual lockdep uses to detect deadlocks Date: Fri, 25 Aug 2017 17:41:03 +0900 Message-Id: <1503650463-14582-1-git-send-email-byungchul.park@lge.com> X-Mailer: git-send-email 1.9.1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7583 Lines: 234 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 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 --- 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 #include #include -#include #include #include #include @@ -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 #include #include -#include #include #include #include @@ -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