Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751739AbdH2I75 (ORCPT ); Tue, 29 Aug 2017 04:59:57 -0400 Received: from merlin.infradead.org ([205.233.59.134]:37024 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751680AbdH2I7x (ORCPT ); Tue, 29 Aug 2017 04:59:53 -0400 Date: Tue, 29 Aug 2017 10:59:39 +0200 From: Peter Zijlstra To: Byungchul Park Cc: mingo@kernel.org, tj@kernel.org, boqun.feng@gmail.com, david@fromorbit.com, johannes@sipsolutions.net, oleg@redhat.com, linux-kernel@vger.kernel.org, kernel-team@lge.com Subject: Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Message-ID: <20170829085939.ggmb6xiohw67micb@hirez.programming.kicks-ass.net> References: <20170823115843.662056844@infradead.org> <20170823121432.990701317@infradead.org> <20170824021840.GC6772@X58A-UD3R> <20170824140240.t4imrpvussebfimm@hirez.programming.kicks-ass.net> <20170825011114.GA3858@X58A-UD3R> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170825011114.GA3858@X58A-UD3R> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8543 Lines: 246 On Fri, Aug 25, 2017 at 10:11:14AM +0900, Byungchul Park wrote: > I meant, this seems to be led from your mis-understanding of > crossrelease_hist_{start, end}(). I have, several times now, explained why PROC is special. You seem to still think it can be used like the soft/hard-irq ones, this is fundamentally not so. Does something like so help? --- Subject: lockdep: Untangle xhlock history save/restore from task independence Where XHLOCK_{SOFT,HARD} are save/restore points in the xhlocks[] to ensure the temporal IRQ events don't interact with task state, the XHLOCK_PROC is a fundament different beast that just happens to share the interface. The purpose of XHLOCK_PROC is to annotate independent execution inside one task. For example workqueues, each work should appear to run in its own 'pristine' 'task'. Remove XHLOCK_PROC in favour of its own interface to avoid confusion. Signed-off-by: Peter Zijlstra (Intel) --- include/linux/irqflags.h | 4 +-- include/linux/lockdep.h | 7 +++-- kernel/locking/lockdep.c | 79 +++++++++++++++++++++++------------------------- kernel/workqueue.c | 9 +++--- 4 files changed, 48 insertions(+), 51 deletions(-) diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h index 9bc050bc81b2..5fdd93bb9300 100644 --- a/include/linux/irqflags.h +++ b/include/linux/irqflags.h @@ -26,7 +26,7 @@ # define trace_hardirq_enter() \ do { \ current->hardirq_context++; \ - crossrelease_hist_start(XHLOCK_HARD, 0);\ + crossrelease_hist_start(XHLOCK_HARD); \ } while (0) # define trace_hardirq_exit() \ do { \ @@ -36,7 +36,7 @@ do { \ # define lockdep_softirq_enter() \ do { \ current->softirq_context++; \ - crossrelease_hist_start(XHLOCK_SOFT, 0);\ + crossrelease_hist_start(XHLOCK_SOFT); \ } while (0) # define lockdep_softirq_exit() \ do { \ diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h index 78bb7133abed..bfa8e0b0d6f1 100644 --- a/include/linux/lockdep.h +++ b/include/linux/lockdep.h @@ -551,7 +551,6 @@ struct pin_cookie { }; enum xhlock_context_t { XHLOCK_HARD, XHLOCK_SOFT, - XHLOCK_PROC, XHLOCK_CTX_NR, }; @@ -580,8 +579,9 @@ extern void lock_commit_crosslock(struct lockdep_map *lock); #define STATIC_LOCKDEP_MAP_INIT(_name, _key) \ { .name = (_name), .key = (void *)(_key), .cross = 0, } -extern void crossrelease_hist_start(enum xhlock_context_t c, bool force); +extern void crossrelease_hist_start(enum xhlock_context_t c); extern void crossrelease_hist_end(enum xhlock_context_t c); +extern void lockdep_invariant_state(bool force); extern void lockdep_init_task(struct task_struct *task); extern void lockdep_free_task(struct task_struct *task); #else /* !CROSSRELEASE */ @@ -593,8 +593,9 @@ extern void lockdep_free_task(struct task_struct *task); #define STATIC_LOCKDEP_MAP_INIT(_name, _key) \ { .name = (_name), .key = (void *)(_key), } -static inline void crossrelease_hist_start(enum xhlock_context_t c, bool force) {} +static inline void crossrelease_hist_start(enum xhlock_context_t c) {} static inline void crossrelease_hist_end(enum xhlock_context_t c) {} +static inline void lockdep_invariant_state(bool force) {} static inline void lockdep_init_task(struct task_struct *task) {} static inline void lockdep_free_task(struct task_struct *task) {} #endif /* CROSSRELEASE */ diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index f73ca595b81e..44c8d0d17170 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -4623,13 +4623,8 @@ asmlinkage __visible void lockdep_sys_exit(void) /* * The lock history for each syscall should be independent. So wipe the * slate clean on return to userspace. - * - * crossrelease_hist_end() works well here even when getting here - * without starting (i.e. just after forking), because it rolls back - * the index to point to the last entry, which is already invalid. */ - crossrelease_hist_end(XHLOCK_PROC); - crossrelease_hist_start(XHLOCK_PROC, false); + lockdep_invariant_state(false); } void lockdep_rcu_suspicious(const char *file, const int line, const char *s) @@ -4723,19 +4718,47 @@ static inline void invalidate_xhlock(struct hist_lock *xhlock) } /* - * Lock history stacks; we have 3 nested lock history stacks: + * Lock history stacks; we have 2 nested lock history stacks: * * HARD(IRQ) * SOFT(IRQ) - * PROC(ess) * * The thing is that once we complete a HARD/SOFT IRQ the future task locks * should not depend on any of the locks observed while running the IRQ. So * what we do is rewind the history buffer and erase all our knowledge of that * temporal event. - * - * The PROCess one is special though; it is used to annotate independence - * inside a task. + */ + +void crossrelease_hist_start(enum xhlock_context_t c) +{ + struct task_struct *cur = current; + + if (!cur->xhlocks) + return; + + cur->xhlock_idx_hist[c] = cur->xhlock_idx; + cur->hist_id_save[c] = cur->hist_id; +} + +void crossrelease_hist_end(enum xhlock_context_t c) +{ + struct task_struct *cur = current; + + if (cur->xhlocks) { + unsigned int idx = cur->xhlock_idx_hist[c]; + struct hist_lock *h = &xhlock(idx); + + cur->xhlock_idx = idx; + + /* Check if the ring was overwritten. */ + if (h->hist_id != cur->hist_id_save[c]) + invalidate_xhlock(h); + } +} + +/* + * lockdep_invariant_state() is used to annotate independence inside a task, to + * make one task look like multiple independent 'tasks'. * * Take for instance workqueues; each work is independent of the last. The * completion of a future work does not depend on the completion of a past work @@ -4758,40 +4781,14 @@ static inline void invalidate_xhlock(struct hist_lock *xhlock) * entry. Similarly, independence per-definition means it does not depend on * prior state. */ -void crossrelease_hist_start(enum xhlock_context_t c, bool force) +void lockdep_invariant_state(bool force) { - struct task_struct *cur = current; - - if (!cur->xhlocks) - return; - /* * We call this at an invariant point, no current state, no history. + * Verify the former, enforce the latter. */ - if (c == XHLOCK_PROC) { - /* verified the former, ensure the latter */ - WARN_ON_ONCE(!force && cur->lockdep_depth); - invalidate_xhlock(&xhlock(cur->xhlock_idx)); - } - - cur->xhlock_idx_hist[c] = cur->xhlock_idx; - cur->hist_id_save[c] = cur->hist_id; -} - -void crossrelease_hist_end(enum xhlock_context_t c) -{ - struct task_struct *cur = current; - - if (cur->xhlocks) { - unsigned int idx = cur->xhlock_idx_hist[c]; - struct hist_lock *h = &xhlock(idx); - - cur->xhlock_idx = idx; - - /* Check if the ring was overwritten. */ - if (h->hist_id != cur->hist_id_save[c]) - invalidate_xhlock(h); - } + WARN_ON_ONCE(!force && current->lockdep_depth); + invalidate_xhlock(&xhlock(current->xhlock_idx)); } static int cross_lock(struct lockdep_map *lock) diff --git a/kernel/workqueue.c b/kernel/workqueue.c index c0331891dec1..ab3c0dc8c7ed 100644 --- a/kernel/workqueue.c +++ b/kernel/workqueue.c @@ -2094,8 +2094,8 @@ __acquires(&pool->lock) lock_map_acquire(&pwq->wq->lockdep_map); lock_map_acquire(&lockdep_map); /* - * Strictly speaking we should do start(PROC) without holding any - * locks, that is, before these two lock_map_acquire()'s. + * Strictly speaking we should mark the invariant state without holding + * any locks, that is, before these two lock_map_acquire()'s. * * However, that would result in: * @@ -2107,14 +2107,14 @@ __acquires(&pool->lock) * Which would create W1->C->W1 dependencies, even though there is no * actual deadlock possible. There are two solutions, using a * read-recursive acquire on the work(queue) 'locks', but this will then - * hit the lockdep limitation on recursive locks, or simly discard + * hit the lockdep limitation on recursive locks, or simply discard * these locks. * * AFAICT there is no possible deadlock scenario between the * flush_work() and complete() primitives (except for single-threaded * workqueues), so hiding them isn't a problem. */ - crossrelease_hist_start(XHLOCK_PROC, true); + lockdep_invariant_state(true); trace_workqueue_execute_start(work); worker->current_func(work); /* @@ -2122,7 +2122,6 @@ __acquires(&pool->lock) * point will only record its address. */ trace_workqueue_execute_end(work); - crossrelease_hist_end(XHLOCK_PROC); lock_map_release(&lockdep_map); lock_map_release(&pwq->wq->lockdep_map);