Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1705149 > unrolled thread

[PATCH v8 00/14] lockdep: Implement crossrelease feature

Started byByungchul Park <byungchul.park@lge.com>
First post2017-08-07 09:20 +0200
Last post2017-08-10 13:50 +0200
Articles 12 on this page of 32 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v8 00/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-07 09:20 +0200
    [PATCH v8 08/14] lockdep: Make print_circular_bug() aware of crossrelease Byungchul Park <byungchul.park@lge.com> - 2017-08-07 09:20 +0200
      [tip:locking/core] locking/lockdep: Make print_circular_bug() aware  of crossrelease tip-bot for Byungchul Park <tipbot@zytor.com> - 2017-08-10 14:30 +0200
    [PATCH v8 05/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-07 09:20 +0200
      Re: [PATCH v8 05/14] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-08-09 16:10 +0200
        Re: [PATCH v8 05/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-10 03:40 +0200
          Re: [PATCH v8 05/14] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-08-10 11:30 +0200
      [tip:locking/core] locking/lockdep: Implement the 'crossrelease'  feature tip-bot for Byungchul Park <tipbot@zytor.com> - 2017-08-10 14:30 +0200
    [PATCH v8 11/14] lockdep: Apply crossrelease to PG_locked locks Byungchul Park <byungchul.park@lge.com> - 2017-08-07 09:20 +0200
      Re: [PATCH v8 11/14] lockdep: Apply crossrelease to PG_locked locks Byungchul Park <byungchul.park@lge.com> - 2017-08-10 03:40 +0200
        Re: [PATCH v8 11/14] lockdep: Apply crossrelease to PG_locked locks Peter Zijlstra <peterz@infradead.org> - 2017-08-10 11:30 +0200
    [PATCH v8 10/14] pagemap.h: Remove trailing white space Byungchul Park <byungchul.park@lge.com> - 2017-08-07 09:20 +0200
    [PATCH v8 07/14] lockdep: Handle non(or multi)-acquisition of a crosslock Byungchul Park <byungchul.park@lge.com> - 2017-08-07 09:20 +0200
      [tip:locking/core] locking/lockdep: Handle non(or  multi)-acquisition of a crosslock tip-bot for Byungchul Park <tipbot@zytor.com> - 2017-08-10 14:30 +0200
    [PATCH v8 12/14] lockdep: Apply lock_acquire(release) on __Set(__Clear)PageLocked Byungchul Park <byungchul.park@lge.com> - 2017-08-07 09:20 +0200
    [PATCH v8 14/14] lockdep: Crossrelease feature documentation Byungchul Park <byungchul.park@lge.com> - 2017-08-07 09:20 +0200
      [tip:locking/core] locking/lockdep: Add 'crossrelease' feature  documentation tip-bot for Byungchul Park <tipbot@zytor.com> - 2017-08-10 14:30 +0200
    [PATCH v8 09/14] lockdep: Apply crossrelease to completions Byungchul Park <byungchul.park@lge.com> - 2017-08-07 09:20 +0200
      Re: [PATCH v8 09/14] lockdep: Apply crossrelease to completions Peter Zijlstra <peterz@infradead.org> - 2017-08-09 12:00 +0200
        Re: [PATCH v8 09/14] lockdep: Apply crossrelease to completions Peter Zijlstra <peterz@infradead.org> - 2017-08-09 12:30 +0200
          Re: [PATCH v8 09/14] lockdep: Apply crossrelease to completions Byungchul Park <byungchul.park@lge.com> - 2017-08-10 03:30 +0200
      [tip:locking/core] locking/lockdep: Apply crossrelease to  completions tip-bot for Byungchul Park <tipbot@zytor.com> - 2017-08-10 14:30 +0200
    [PATCH v8 04/14] lockdep: Make check_prev_add() able to handle external stack_trace Byungchul Park <byungchul.park@lge.com> - 2017-08-07 09:20 +0200
      [tip:locking/core] locking/lockdep: Make check_prev_add() able to  handle external stack_trace tip-bot for Byungchul Park <tipbot@zytor.com> - 2017-08-10 14:30 +0200
    Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-08-09 18:00 +0200
      Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-10 03:00 +0200
        Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-10 05:50 +0200
        Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-10 13:00 +0200
      Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-10 11:40 +0200
        Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Peter Zijlstra <peterz@infradead.org> - 2017-08-10 13:00 +0200
    Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Ingo Molnar <mingo@kernel.org> - 2017-08-10 13:20 +0200
      Re: [PATCH v8 00/14] lockdep: Implement crossrelease feature Byungchul Park <byungchul.park@lge.com> - 2017-08-10 13:50 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1708126 — Re: [PATCH v8 09/14] lockdep: Apply crossrelease to completions

FromByungchul Park <byungchul.park@lge.com>
Date2017-08-10 03:30 +0200
SubjectRe: [PATCH v8 09/14] lockdep: Apply crossrelease to completions
Message-ID<ucKsp-768-7@gated-at.bofh.it>
In reply to#1707252
On Wed, Aug 09, 2017 at 12:24:39PM +0200, Peter Zijlstra wrote:
> On Wed, Aug 09, 2017 at 11:51:07AM +0200, Peter Zijlstra wrote:
> > On Mon, Aug 07, 2017 at 04:12:56PM +0900, Byungchul Park wrote:
> > > +static inline void wait_for_completion(struct completion *x)
> > > +{
> > > +	complete_acquire(x);
> > > +	__wait_for_completion(x);
> > > +	complete_release(x);
> > > +}
> > > +
> > > +static inline void wait_for_completion_io(struct completion *x)
> > > +{
> > > +	complete_acquire(x);
> > > +	__wait_for_completion_io(x);
> > > +	complete_release(x);
> > > +}
> > > +
> > > +static inline int wait_for_completion_interruptible(struct completion *x)
> > > +{
> > > +	int ret;
> > > +	complete_acquire(x);
> > > +	ret = __wait_for_completion_interruptible(x);
> > > +	complete_release(x);
> > > +	return ret;
> > > +}
> > > +
> > > +static inline int wait_for_completion_killable(struct completion *x)
> > > +{
> > > +	int ret;
> > > +	complete_acquire(x);
> > > +	ret = __wait_for_completion_killable(x);
> > > +	complete_release(x);
> > > +	return ret;
> > > +}
> > 
> > I don't understand, why not change __wait_for_common() ?
> 
> That is what is wrong with the below?
> 
> Yes, it adds acquire/release to the timeout variants too, but I don't

Yes, I didn't want to involve them in lockdep play which reports _deadlock_
warning since it's not a dependency causing a deadlock.

> see why we should exclude those, and even if we'd want to do that, it
> would be trivial:
> 
> 	bool timo = (timeout == MAX_SCHEDULE_TIMEOUT);
> 
> 	if (!timo)
> 		complete_acquire(x);
> 
> 	/* ... */
> 
> 	if (!timo)
> 		complete_release(x);

Yes, frankly I wanted to use this.. but skip it.

> But like said, I think we very much want to annotate waits with timeouts
> too. Hitting the max timo doesn't necessarily mean we'll make fwd
> progress, we could be stuck in a loop doing something else again before
> returning to wait.

In that case, it should be detected by other dependencies which makes
problems, not the dependency by wait_for_complete().

> Also, even if we'd make fwd progress, hitting that max timo is still not
> desirable.

It's not desirable but it's not a dependency causing a deadlock, so I did
not want to _deadlock_ warning in that cases.. I didn't want to abuse
lockdep reports..

However, it's OK if you think it's worth warning even in that cases.

Thank you very much,
Byungchul

[toc] | [prev] | [next] | [standalone]

#1708577 — [tip:locking/core] locking/lockdep: Apply crossrelease to completions

Fromtip-bot for Byungchul Park <tipbot@zytor.com>
Date2017-08-10 14:30 +0200
Subject[tip:locking/core] locking/lockdep: Apply crossrelease to completions
Message-ID<ucUL9-5wC-57@gated-at.bofh.it>
In reply to#1705160
Commit-ID:  cd8084f91c02c1afd256a39aa833bff737631304
Gitweb:     http://git.kernel.org/tip/cd8084f91c02c1afd256a39aa833bff737631304
Author:     Byungchul Park <byungchul.park@lge.com>
AuthorDate: Mon, 7 Aug 2017 16:12:56 +0900
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 10 Aug 2017 12:29:10 +0200

locking/lockdep: Apply crossrelease to completions

Although wait_for_completion() and its family can cause deadlock, the
lock correctness validator could not be applied to them until now,
because things like complete() are usually called in a different context
from the waiting context, which violates lockdep's assumption.

Thanks to CONFIG_LOCKDEP_CROSSRELEASE, we can now apply the lockdep
detector to those completion operations. Applied it.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: akpm@linux-foundation.org
Cc: boqun.feng@gmail.com
Cc: kernel-team@lge.com
Cc: kirill@shutemov.name
Cc: npiggin@gmail.com
Cc: walken@google.com
Cc: willy@infradead.org
Link: http://lkml.kernel.org/r/1502089981-21272-10-git-send-email-byungchul.park@lge.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/completion.h | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 kernel/sched/completion.c  | 11 +++++++++++
 lib/Kconfig.debug          |  9 +++++++++
 3 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/include/linux/completion.h b/include/linux/completion.h
index 5d5aaae..9bcebf5 100644
--- a/include/linux/completion.h
+++ b/include/linux/completion.h
@@ -9,6 +9,9 @@
  */
 
 #include <linux/wait.h>
+#ifdef CONFIG_LOCKDEP_COMPLETE
+#include <linux/lockdep.h>
+#endif
 
 /*
  * struct completion - structure used to maintain state for a "completion"
@@ -25,10 +28,50 @@
 struct completion {
 	unsigned int done;
 	wait_queue_head_t wait;
+#ifdef CONFIG_LOCKDEP_COMPLETE
+	struct lockdep_map_cross map;
+#endif
 };
 
+#ifdef CONFIG_LOCKDEP_COMPLETE
+static inline void complete_acquire(struct completion *x)
+{
+	lock_acquire_exclusive((struct lockdep_map *)&x->map, 0, 0, NULL, _RET_IP_);
+}
+
+static inline void complete_release(struct completion *x)
+{
+	lock_release((struct lockdep_map *)&x->map, 0, _RET_IP_);
+}
+
+static inline void complete_release_commit(struct completion *x)
+{
+	lock_commit_crosslock((struct lockdep_map *)&x->map);
+}
+
+#define init_completion(x)						\
+do {									\
+	static struct lock_class_key __key;				\
+	lockdep_init_map_crosslock((struct lockdep_map *)&(x)->map,	\
+			"(complete)" #x,				\
+			&__key, 0);					\
+	__init_completion(x);						\
+} while (0)
+#else
+#define init_completion(x) __init_completion(x)
+static inline void complete_acquire(struct completion *x) {}
+static inline void complete_release(struct completion *x) {}
+static inline void complete_release_commit(struct completion *x) {}
+#endif
+
+#ifdef CONFIG_LOCKDEP_COMPLETE
+#define COMPLETION_INITIALIZER(work) \
+	{ 0, __WAIT_QUEUE_HEAD_INITIALIZER((work).wait), \
+	STATIC_CROSS_LOCKDEP_MAP_INIT("(complete)" #work, &(work)) }
+#else
 #define COMPLETION_INITIALIZER(work) \
 	{ 0, __WAIT_QUEUE_HEAD_INITIALIZER((work).wait) }
+#endif
 
 #define COMPLETION_INITIALIZER_ONSTACK(work) \
 	({ init_completion(&work); work; })
@@ -70,7 +113,7 @@ struct completion {
  * This inline function will initialize a dynamically created completion
  * structure.
  */
-static inline void init_completion(struct completion *x)
+static inline void __init_completion(struct completion *x)
 {
 	x->done = 0;
 	init_waitqueue_head(&x->wait);
diff --git a/kernel/sched/completion.c b/kernel/sched/completion.c
index 13fc5ae..566b6ec 100644
--- a/kernel/sched/completion.c
+++ b/kernel/sched/completion.c
@@ -32,6 +32,12 @@ void complete(struct completion *x)
 	unsigned long flags;
 
 	spin_lock_irqsave(&x->wait.lock, flags);
+
+	/*
+	 * Perform commit of crossrelease here.
+	 */
+	complete_release_commit(x);
+
 	if (x->done != UINT_MAX)
 		x->done++;
 	__wake_up_locked(&x->wait, TASK_NORMAL, 1);
@@ -92,9 +98,14 @@ __wait_for_common(struct completion *x,
 {
 	might_sleep();
 
+	complete_acquire(x);
+
 	spin_lock_irq(&x->wait.lock);
 	timeout = do_wait_for_common(x, action, timeout, state);
 	spin_unlock_irq(&x->wait.lock);
+
+	complete_release(x);
+
 	return timeout;
 }
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c6038f2..ebd40d3 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1162,6 +1162,15 @@ config LOCKDEP_CROSSRELEASE
 	 such as page locks or completions can use the lock correctness
 	 detector, lockdep.
 
+config LOCKDEP_COMPLETE
+	bool "Lock debugging: allow completions to use deadlock detector"
+	depends on PROVE_LOCKING
+	select LOCKDEP_CROSSRELEASE
+	default n
+	help
+	 A deadlock caused by wait_for_completion() and complete() can be
+	 detected by lockdep using crossrelease feature.
+
 config DEBUG_LOCKDEP
 	bool "Lock dependency engine debugging"
 	depends on DEBUG_KERNEL && LOCKDEP

[toc] | [prev] | [next] | [standalone]

#1705161 — [PATCH v8 04/14] lockdep: Make check_prev_add() able to handle external stack_trace

FromByungchul Park <byungchul.park@lge.com>
Date2017-08-07 09:20 +0200
Subject[PATCH v8 04/14] lockdep: Make check_prev_add() able to handle external stack_trace
Message-ID<ubKuv-59V-39@gated-at.bofh.it>
In reply to#1705149
Currently, a space for stack_trace is pinned in check_prev_add(), that
makes us not able to use external stack_trace. The simplest way to
achieve it is to pass an external stack_trace as an argument.

A more suitable solution is to pass a callback additionally along with
a stack_trace so that callers can decide the way to save or whether to
save. Actually crossrelease needs to do other than saving a stack_trace.
So pass a stack_trace and callback to handle it, to check_prev_add().

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 kernel/locking/lockdep.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index b23e930..22a13f9 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1813,20 +1813,13 @@ static inline void inc_chains(void)
  */
 static int
 check_prev_add(struct task_struct *curr, struct held_lock *prev,
-	       struct held_lock *next, int distance, int *stack_saved)
+	       struct held_lock *next, int distance, struct stack_trace *trace,
+	       int (*save)(struct stack_trace *trace))
 {
 	struct lock_list *entry;
 	int ret;
 	struct lock_list this;
 	struct lock_list *uninitialized_var(target_entry);
-	/*
-	 * Static variable, serialized by the graph_lock().
-	 *
-	 * We use this static variable to save the stack trace in case
-	 * we call into this function multiple times due to encountering
-	 * trylocks in the held lock stack.
-	 */
-	static struct stack_trace trace;
 
 	/*
 	 * Prove that the new <prev> -> <next> dependency would not
@@ -1874,11 +1867,8 @@ static inline void inc_chains(void)
 		}
 	}
 
-	if (!*stack_saved) {
-		if (!save_trace(&trace))
-			return 0;
-		*stack_saved = 1;
-	}
+	if (save && !save(trace))
+		return 0;
 
 	/*
 	 * Ok, all validations passed, add the new lock
@@ -1886,14 +1876,14 @@ static inline void inc_chains(void)
 	 */
 	ret = add_lock_to_list(hlock_class(next),
 			       &hlock_class(prev)->locks_after,
-			       next->acquire_ip, distance, &trace);
+			       next->acquire_ip, distance, trace);
 
 	if (!ret)
 		return 0;
 
 	ret = add_lock_to_list(hlock_class(prev),
 			       &hlock_class(next)->locks_before,
-			       next->acquire_ip, distance, &trace);
+			       next->acquire_ip, distance, trace);
 	if (!ret)
 		return 0;
 
@@ -1901,8 +1891,6 @@ static inline void inc_chains(void)
 	 * Debugging printouts:
 	 */
 	if (verbose(hlock_class(prev)) || verbose(hlock_class(next))) {
-		/* We drop graph lock, so another thread can overwrite trace. */
-		*stack_saved = 0;
 		graph_unlock();
 		printk("\n new dependency: ");
 		print_lock_name(hlock_class(prev));
@@ -1926,8 +1914,9 @@ static inline void inc_chains(void)
 check_prevs_add(struct task_struct *curr, struct held_lock *next)
 {
 	int depth = curr->lockdep_depth;
-	int stack_saved = 0;
 	struct held_lock *hlock;
+	struct stack_trace trace;
+	int (*save)(struct stack_trace *trace) = save_trace;
 
 	/*
 	 * Debugging checks.
@@ -1952,9 +1941,18 @@ static inline void inc_chains(void)
 		 * added:
 		 */
 		if (hlock->read != 2 && hlock->check) {
-			if (!check_prev_add(curr, hlock, next,
-						distance, &stack_saved))
+			int ret = check_prev_add(curr, hlock, next,
+						distance, &trace, save);
+			if (!ret)
 				return 0;
+
+			/*
+			 * Stop saving stack_trace if save_trace() was
+			 * called at least once:
+			 */
+			if (save && ret == 2)
+				save = NULL;
+
 			/*
 			 * Stop after the first non-trylock entry,
 			 * as non-trylock entries have added their
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1708575 — [tip:locking/core] locking/lockdep: Make check_prev_add() able to handle external stack_trace

Fromtip-bot for Byungchul Park <tipbot@zytor.com>
Date2017-08-10 14:30 +0200
Subject[tip:locking/core] locking/lockdep: Make check_prev_add() able to handle external stack_trace
Message-ID<ucUL9-5wC-51@gated-at.bofh.it>
In reply to#1705161
Commit-ID:  ce07a9415f266e181a0a33033a5f7138760240a4
Gitweb:     http://git.kernel.org/tip/ce07a9415f266e181a0a33033a5f7138760240a4
Author:     Byungchul Park <byungchul.park@lge.com>
AuthorDate: Mon, 7 Aug 2017 16:12:51 +0900
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 10 Aug 2017 12:29:06 +0200

locking/lockdep: Make check_prev_add() able to handle external stack_trace

Currently, a space for stack_trace is pinned in check_prev_add(), that
makes us not able to use external stack_trace. The simplest way to
achieve it is to pass an external stack_trace as an argument.

A more suitable solution is to pass a callback additionally along with
a stack_trace so that callers can decide the way to save or whether to
save. Actually crossrelease needs to do other than saving a stack_trace.
So pass a stack_trace and callback to handle it, to check_prev_add().

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: akpm@linux-foundation.org
Cc: boqun.feng@gmail.com
Cc: kernel-team@lge.com
Cc: kirill@shutemov.name
Cc: npiggin@gmail.com
Cc: walken@google.com
Cc: willy@infradead.org
Link: http://lkml.kernel.org/r/1502089981-21272-5-git-send-email-byungchul.park@lge.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/lockdep.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 7cf02fa..841828b 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1824,20 +1824,13 @@ check_deadlock(struct task_struct *curr, struct held_lock *next,
  */
 static int
 check_prev_add(struct task_struct *curr, struct held_lock *prev,
-	       struct held_lock *next, int distance, int *stack_saved)
+	       struct held_lock *next, int distance, struct stack_trace *trace,
+	       int (*save)(struct stack_trace *trace))
 {
 	struct lock_list *entry;
 	int ret;
 	struct lock_list this;
 	struct lock_list *uninitialized_var(target_entry);
-	/*
-	 * Static variable, serialized by the graph_lock().
-	 *
-	 * We use this static variable to save the stack trace in case
-	 * we call into this function multiple times due to encountering
-	 * trylocks in the held lock stack.
-	 */
-	static struct stack_trace trace;
 
 	/*
 	 * Prove that the new <prev> -> <next> dependency would not
@@ -1899,11 +1892,8 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
 		return print_bfs_bug(ret);
 
 
-	if (!*stack_saved) {
-		if (!save_trace(&trace))
-			return 0;
-		*stack_saved = 1;
-	}
+	if (save && !save(trace))
+		return 0;
 
 	/*
 	 * Ok, all validations passed, add the new lock
@@ -1911,14 +1901,14 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
 	 */
 	ret = add_lock_to_list(hlock_class(next),
 			       &hlock_class(prev)->locks_after,
-			       next->acquire_ip, distance, &trace);
+			       next->acquire_ip, distance, trace);
 
 	if (!ret)
 		return 0;
 
 	ret = add_lock_to_list(hlock_class(prev),
 			       &hlock_class(next)->locks_before,
-			       next->acquire_ip, distance, &trace);
+			       next->acquire_ip, distance, trace);
 	if (!ret)
 		return 0;
 
@@ -1926,8 +1916,6 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
 	 * Debugging printouts:
 	 */
 	if (verbose(hlock_class(prev)) || verbose(hlock_class(next))) {
-		/* We drop graph lock, so another thread can overwrite trace. */
-		*stack_saved = 0;
 		graph_unlock();
 		printk("\n new dependency: ");
 		print_lock_name(hlock_class(prev));
@@ -1951,8 +1939,9 @@ static int
 check_prevs_add(struct task_struct *curr, struct held_lock *next)
 {
 	int depth = curr->lockdep_depth;
-	int stack_saved = 0;
 	struct held_lock *hlock;
+	struct stack_trace trace;
+	int (*save)(struct stack_trace *trace) = save_trace;
 
 	/*
 	 * Debugging checks.
@@ -1977,9 +1966,18 @@ check_prevs_add(struct task_struct *curr, struct held_lock *next)
 		 * added:
 		 */
 		if (hlock->read != 2 && hlock->check) {
-			if (!check_prev_add(curr, hlock, next,
-						distance, &stack_saved))
+			int ret = check_prev_add(curr, hlock, next,
+						distance, &trace, save);
+			if (!ret)
 				return 0;
+
+			/*
+			 * Stop saving stack_trace if save_trace() was
+			 * called at least once:
+			 */
+			if (save && ret == 2)
+				save = NULL;
+
 			/*
 			 * Stop after the first non-trylock entry,
 			 * as non-trylock entries have added their

[toc] | [prev] | [next] | [standalone]

#1707436

FromPeter Zijlstra <peterz@infradead.org>
Date2017-08-09 18:00 +0200
Message-ID<ucByO-T5-11@gated-at.bofh.it>
In reply to#1705149

Heh, look what it does...


4======================================================
4WARNING: possible circular locking dependency detected
4.13.0-rc2-00317-gadc6764a3adf-dirty #797 Tainted: G        W      
4------------------------------------------------------
4startpar/582 is trying to acquire lock:
c (c(complete)&barr->donec){+.+.}c, at: [<ffffffff8110de4d>] flush_work+0x1fd/0x2c0
4
but task is already holding lock:
c (clockc#3c){+.+.}c, at: [<ffffffff8122e866>] lru_add_drain_all_cpuslocked+0x46/0x1a0
4
which lock already depends on the new lock.

4
the existing dependency chain (in reverse order) is:

-> #4c (clockc#3c){+.+.}c:
       __lock_acquire+0x10a5/0x1100
       lock_acquire+0xea/0x1f0
       __mutex_lock+0x6c/0x960
       mutex_lock_nested+0x1b/0x20
       lru_add_drain_all_cpuslocked+0x46/0x1a0
       lru_add_drain_all+0x13/0x20
       SyS_mlockall+0xb8/0x1c0
       entry_SYSCALL_64_fastpath+0x23/0xc2

-> #3c (ccpu_hotplug_lock.rw_semc){++++}c:
       __lock_acquire+0x10a5/0x1100
       lock_acquire+0xea/0x1f0
       cpus_read_lock+0x2a/0x90
       kmem_cache_create+0x2a/0x1d0
       scsi_init_sense_cache+0xa0/0xc0
       scsi_add_host_with_dma+0x67/0x360
       isci_pci_probe+0x873/0xc90
       local_pci_probe+0x42/0xa0
       work_for_cpu_fn+0x14/0x20
       process_one_work+0x273/0x6b0
       worker_thread+0x21b/0x3f0
       kthread+0x147/0x180
       ret_from_fork+0x2a/0x40

-> #2c (cscsi_sense_cache_mutexc){+.+.}c:
       __lock_acquire+0x10a5/0x1100
       lock_acquire+0xea/0x1f0
       __mutex_lock+0x6c/0x960
       mutex_lock_nested+0x1b/0x20
       scsi_init_sense_cache+0x3d/0xc0
       scsi_add_host_with_dma+0x67/0x360
       isci_pci_probe+0x873/0xc90
       local_pci_probe+0x42/0xa0
       work_for_cpu_fn+0x14/0x20
       process_one_work+0x273/0x6b0
       worker_thread+0x21b/0x3f0
       kthread+0x147/0x180
       ret_from_fork+0x2a/0x40

-> #1c (c(&wfc.work)c){+.+.}c:
       process_one_work+0x244/0x6b0
       worker_thread+0x21b/0x3f0
       kthread+0x147/0x180
       ret_from_fork+0x2a/0x40
       0xffffffffffffffff

-> #0c (c(complete)&barr->donec){+.+.}c:
       check_prev_add+0x3be/0x700
       __lock_acquire+0x10a5/0x1100
       lock_acquire+0xea/0x1f0
       wait_for_completion+0x3b/0x130
       flush_work+0x1fd/0x2c0
       lru_add_drain_all_cpuslocked+0x158/0x1a0
       lru_add_drain_all+0x13/0x20
       SyS_mlockall+0xb8/0x1c0
       entry_SYSCALL_64_fastpath+0x23/0xc2

other info that might help us debug this:

Chain exists of:
  c(complete)&barr->donec --> ccpu_hotplug_lock.rw_semc --> clockc#3c

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(clockc#3c);
                               lock(ccpu_hotplug_lock.rw_semc);
                               lock(clockc#3c);
  lock(c(complete)&barr->donec);

 *** DEADLOCK ***

2 locks held by startpar/582:
 #0: c (ccpu_hotplug_lock.rw_semc){++++}c, at: [<ffffffff8122e9ce>] lru_add_drain_all+0xe/0x20
 #1: c (clockc#3c){+.+.}c, at: [<ffffffff8122e866>] lru_add_drain_all_cpuslocked+0x46/0x1a0

stack backtrace:
dCPU: 23 PID: 582 Comm: startpar Tainted: G        W       4.13.0-rc2-00317-gadc6764a3adf-dirty #797
dHardware name: Intel Corporation S2600GZ/S2600GZ, BIOS SE5C600.86B.02.02.0002.122320131210 12/23/2013
dCall Trace:
d dump_stack+0x86/0xcf
d print_circular_bug+0x203/0x2f0
d check_prev_add+0x3be/0x700
d ? add_lock_to_list.isra.30+0xc0/0xc0
d ? is_bpf_text_address+0x82/0xe0
d ? unwind_get_return_address+0x1f/0x30
d __lock_acquire+0x10a5/0x1100
d ? __lock_acquire+0x10a5/0x1100
d ? add_lock_to_list.isra.30+0xc0/0xc0
d lock_acquire+0xea/0x1f0
d ? flush_work+0x1fd/0x2c0
d wait_for_completion+0x3b/0x130
d ? flush_work+0x1fd/0x2c0
d flush_work+0x1fd/0x2c0
d ? flush_workqueue_prep_pwqs+0x1c0/0x1c0
d ? trace_hardirqs_on+0xd/0x10
d lru_add_drain_all_cpuslocked+0x158/0x1a0
d lru_add_drain_all+0x13/0x20
d SyS_mlockall+0xb8/0x1c0
d entry_SYSCALL_64_fastpath+0x23/0xc2
dRIP: 0033:0x7f818d2e54c7
dRSP: 002b:00007fffcce83798 EFLAGS: 00000246c ORIG_RAX: 0000000000000097
dRAX: ffffffffffffffda RBX: 0000000000000046 RCX: 00007f818d2e54c7
dRDX: 0000000000000000 RSI: 00007fffcce83650 RDI: 0000000000000003
dRBP: 000000000002c010 R08: 0000000000000000 R09: 0000000000000000
dR10: 0000000000000008 R11: 0000000000000246 R12: 000000000002d000
dR13: 000000000002c010 R14: 0000000000001000 R15: 00007f818d599b00

[toc] | [prev] | [next] | [standalone]

#1708110

FromByungchul Park <byungchul.park@lge.com>
Date2017-08-10 03:00 +0200
Message-ID<ucJZo-6H0-11@gated-at.bofh.it>
In reply to#1707436
On Wed, Aug 09, 2017 at 05:50:59PM +0200, Peter Zijlstra wrote:
> 
> 
> Heh, look what it does...

It does not happen in my machine..

I tihink it happens because of "Simplify xhlock ring buffer invalidation"
patch of you.

First of all, could you reverse yours and check if it happens, too?
If not, we have to think the simplification more.

BTW, does your patch consider the possibility that a worker and irqs can
be nested? Is it no problem even in the case?

> 
> 
> 4======================================================
> 4WARNING: possible circular locking dependency detected
> 4.13.0-rc2-00317-gadc6764a3adf-dirty #797 Tainted: G        W      
> 4------------------------------------------------------
> 4startpar/582 is trying to acquire lock:
> c (c(complete)&barr->donec){+.+.}c, at: [<ffffffff8110de4d>] flush_work+0x1fd/0x2c0
> 4
> but task is already holding lock:
> c (clockc#3c){+.+.}c, at: [<ffffffff8122e866>] lru_add_drain_all_cpuslocked+0x46/0x1a0
> 4
> which lock already depends on the new lock.
> 
> 4
> the existing dependency chain (in reverse order) is:
> 
> -> #4c (clockc#3c){+.+.}c:
>        __lock_acquire+0x10a5/0x1100
>        lock_acquire+0xea/0x1f0
>        __mutex_lock+0x6c/0x960
>        mutex_lock_nested+0x1b/0x20
>        lru_add_drain_all_cpuslocked+0x46/0x1a0
>        lru_add_drain_all+0x13/0x20
>        SyS_mlockall+0xb8/0x1c0
>        entry_SYSCALL_64_fastpath+0x23/0xc2
> 
> -> #3c (ccpu_hotplug_lock.rw_semc){++++}c:
>        __lock_acquire+0x10a5/0x1100
>        lock_acquire+0xea/0x1f0
>        cpus_read_lock+0x2a/0x90
>        kmem_cache_create+0x2a/0x1d0
>        scsi_init_sense_cache+0xa0/0xc0
>        scsi_add_host_with_dma+0x67/0x360
>        isci_pci_probe+0x873/0xc90
>        local_pci_probe+0x42/0xa0
>        work_for_cpu_fn+0x14/0x20
>        process_one_work+0x273/0x6b0
>        worker_thread+0x21b/0x3f0
>        kthread+0x147/0x180
>        ret_from_fork+0x2a/0x40
> 
> -> #2c (cscsi_sense_cache_mutexc){+.+.}c:
>        __lock_acquire+0x10a5/0x1100
>        lock_acquire+0xea/0x1f0
>        __mutex_lock+0x6c/0x960
>        mutex_lock_nested+0x1b/0x20
>        scsi_init_sense_cache+0x3d/0xc0
>        scsi_add_host_with_dma+0x67/0x360
>        isci_pci_probe+0x873/0xc90
>        local_pci_probe+0x42/0xa0
>        work_for_cpu_fn+0x14/0x20
>        process_one_work+0x273/0x6b0
>        worker_thread+0x21b/0x3f0
>        kthread+0x147/0x180
>        ret_from_fork+0x2a/0x40
> 
> -> #1c (c(&wfc.work)c){+.+.}c:
>        process_one_work+0x244/0x6b0
>        worker_thread+0x21b/0x3f0
>        kthread+0x147/0x180
>        ret_from_fork+0x2a/0x40
>        0xffffffffffffffff
> 
> -> #0c (c(complete)&barr->donec){+.+.}c:
>        check_prev_add+0x3be/0x700
>        __lock_acquire+0x10a5/0x1100
>        lock_acquire+0xea/0x1f0
>        wait_for_completion+0x3b/0x130
>        flush_work+0x1fd/0x2c0
>        lru_add_drain_all_cpuslocked+0x158/0x1a0
>        lru_add_drain_all+0x13/0x20
>        SyS_mlockall+0xb8/0x1c0
>        entry_SYSCALL_64_fastpath+0x23/0xc2
> 
> other info that might help us debug this:
> 
> Chain exists of:
>   c(complete)&barr->donec --> ccpu_hotplug_lock.rw_semc --> clockc#3c
> 
>  Possible unsafe locking scenario:
> 
>        CPU0                    CPU1
>        ----                    ----
>   lock(clockc#3c);
>                                lock(ccpu_hotplug_lock.rw_semc);
>                                lock(clockc#3c);
>   lock(c(complete)&barr->donec);
> 
>  *** DEADLOCK ***
> 
> 2 locks held by startpar/582:
>  #0: c (ccpu_hotplug_lock.rw_semc){++++}c, at: [<ffffffff8122e9ce>] lru_add_drain_all+0xe/0x20
>  #1: c (clockc#3c){+.+.}c, at: [<ffffffff8122e866>] lru_add_drain_all_cpuslocked+0x46/0x1a0
> 
> stack backtrace:
> dCPU: 23 PID: 582 Comm: startpar Tainted: G        W       4.13.0-rc2-00317-gadc6764a3adf-dirty #797
> dHardware name: Intel Corporation S2600GZ/S2600GZ, BIOS SE5C600.86B.02.02.0002.122320131210 12/23/2013
> dCall Trace:
> d dump_stack+0x86/0xcf
> d print_circular_bug+0x203/0x2f0
> d check_prev_add+0x3be/0x700
> d ? add_lock_to_list.isra.30+0xc0/0xc0
> d ? is_bpf_text_address+0x82/0xe0
> d ? unwind_get_return_address+0x1f/0x30
> d __lock_acquire+0x10a5/0x1100
> d ? __lock_acquire+0x10a5/0x1100
> d ? add_lock_to_list.isra.30+0xc0/0xc0
> d lock_acquire+0xea/0x1f0
> d ? flush_work+0x1fd/0x2c0
> d wait_for_completion+0x3b/0x130
> d ? flush_work+0x1fd/0x2c0
> d flush_work+0x1fd/0x2c0
> d ? flush_workqueue_prep_pwqs+0x1c0/0x1c0
> d ? trace_hardirqs_on+0xd/0x10
> d lru_add_drain_all_cpuslocked+0x158/0x1a0
> d lru_add_drain_all+0x13/0x20
> d SyS_mlockall+0xb8/0x1c0
> d entry_SYSCALL_64_fastpath+0x23/0xc2
> dRIP: 0033:0x7f818d2e54c7
> dRSP: 002b:00007fffcce83798 EFLAGS: 00000246c ORIG_RAX: 0000000000000097
> dRAX: ffffffffffffffda RBX: 0000000000000046 RCX: 00007f818d2e54c7
> dRDX: 0000000000000000 RSI: 00007fffcce83650 RDI: 0000000000000003
> dRBP: 000000000002c010 R08: 0000000000000000 R09: 0000000000000000
> dR10: 0000000000000008 R11: 0000000000000246 R12: 000000000002d000
> dR13: 000000000002c010 R14: 0000000000001000 R15: 00007f818d599b00

[toc] | [prev] | [next] | [standalone]

#1708192

FromByungchul Park <byungchul.park@lge.com>
Date2017-08-10 05:50 +0200
Message-ID<ucMDU-5t-7@gated-at.bofh.it>
In reply to#1708110
On Thu, Aug 10, 2017 at 09:55:56AM +0900, Byungchul Park wrote:
> On Wed, Aug 09, 2017 at 05:50:59PM +0200, Peter Zijlstra wrote:
> > 
> > 
> > Heh, look what it does...
> 
> It does not happen in my machine..
> 
> I tihink it happens because of "Simplify xhlock ring buffer invalidation"
> patch of you.
> 
> First of all, could you reverse yours and check if it happens, too?
> If not, we have to think the simplification more.
> 
> BTW, does your patch consider the possibility that a worker and irqs can
> be nested? Is it no problem even in the case?

In addition, now that each syscall context is isolated by your suggestion
with crossrelease_hist_end() and crossrelease_hist_start(), contexts can
be nested easily. I want to keep my patches unchanged at first and change
code carefully.

> 
> > 
> > 
> > 4======================================================
> > 4WARNING: possible circular locking dependency detected
> > 4.13.0-rc2-00317-gadc6764a3adf-dirty #797 Tainted: G        W      
> > 4------------------------------------------------------
> > 4startpar/582 is trying to acquire lock:
> > c (c(complete)&barr->donec){+.+.}c, at: [<ffffffff8110de4d>] flush_work+0x1fd/0x2c0
> > 4
> > but task is already holding lock:
> > c (clockc#3c){+.+.}c, at: [<ffffffff8122e866>] lru_add_drain_all_cpuslocked+0x46/0x1a0
> > 4
> > which lock already depends on the new lock.
> > 
> > 4
> > the existing dependency chain (in reverse order) is:
> > 
> > -> #4c (clockc#3c){+.+.}c:
> >        __lock_acquire+0x10a5/0x1100
> >        lock_acquire+0xea/0x1f0
> >        __mutex_lock+0x6c/0x960
> >        mutex_lock_nested+0x1b/0x20
> >        lru_add_drain_all_cpuslocked+0x46/0x1a0
> >        lru_add_drain_all+0x13/0x20
> >        SyS_mlockall+0xb8/0x1c0
> >        entry_SYSCALL_64_fastpath+0x23/0xc2
> > 
> > -> #3c (ccpu_hotplug_lock.rw_semc){++++}c:
> >        __lock_acquire+0x10a5/0x1100
> >        lock_acquire+0xea/0x1f0
> >        cpus_read_lock+0x2a/0x90
> >        kmem_cache_create+0x2a/0x1d0
> >        scsi_init_sense_cache+0xa0/0xc0
> >        scsi_add_host_with_dma+0x67/0x360
> >        isci_pci_probe+0x873/0xc90
> >        local_pci_probe+0x42/0xa0
> >        work_for_cpu_fn+0x14/0x20
> >        process_one_work+0x273/0x6b0
> >        worker_thread+0x21b/0x3f0
> >        kthread+0x147/0x180
> >        ret_from_fork+0x2a/0x40
> > 
> > -> #2c (cscsi_sense_cache_mutexc){+.+.}c:
> >        __lock_acquire+0x10a5/0x1100
> >        lock_acquire+0xea/0x1f0
> >        __mutex_lock+0x6c/0x960
> >        mutex_lock_nested+0x1b/0x20
> >        scsi_init_sense_cache+0x3d/0xc0
> >        scsi_add_host_with_dma+0x67/0x360
> >        isci_pci_probe+0x873/0xc90
> >        local_pci_probe+0x42/0xa0
> >        work_for_cpu_fn+0x14/0x20
> >        process_one_work+0x273/0x6b0
> >        worker_thread+0x21b/0x3f0
> >        kthread+0x147/0x180
> >        ret_from_fork+0x2a/0x40
> > 
> > -> #1c (c(&wfc.work)c){+.+.}c:
> >        process_one_work+0x244/0x6b0
> >        worker_thread+0x21b/0x3f0
> >        kthread+0x147/0x180
> >        ret_from_fork+0x2a/0x40
> >        0xffffffffffffffff
> > 
> > -> #0c (c(complete)&barr->donec){+.+.}c:
> >        check_prev_add+0x3be/0x700
> >        __lock_acquire+0x10a5/0x1100
> >        lock_acquire+0xea/0x1f0
> >        wait_for_completion+0x3b/0x130
> >        flush_work+0x1fd/0x2c0
> >        lru_add_drain_all_cpuslocked+0x158/0x1a0
> >        lru_add_drain_all+0x13/0x20
> >        SyS_mlockall+0xb8/0x1c0
> >        entry_SYSCALL_64_fastpath+0x23/0xc2
> > 
> > other info that might help us debug this:
> > 
> > Chain exists of:
> >   c(complete)&barr->donec --> ccpu_hotplug_lock.rw_semc --> clockc#3c
> > 
> >  Possible unsafe locking scenario:
> > 
> >        CPU0                    CPU1
> >        ----                    ----
> >   lock(clockc#3c);
> >                                lock(ccpu_hotplug_lock.rw_semc);
> >                                lock(clockc#3c);
> >   lock(c(complete)&barr->donec);
> > 
> >  *** DEADLOCK ***
> > 
> > 2 locks held by startpar/582:
> >  #0: c (ccpu_hotplug_lock.rw_semc){++++}c, at: [<ffffffff8122e9ce>] lru_add_drain_all+0xe/0x20
> >  #1: c (clockc#3c){+.+.}c, at: [<ffffffff8122e866>] lru_add_drain_all_cpuslocked+0x46/0x1a0
> > 
> > stack backtrace:
> > dCPU: 23 PID: 582 Comm: startpar Tainted: G        W       4.13.0-rc2-00317-gadc6764a3adf-dirty #797
> > dHardware name: Intel Corporation S2600GZ/S2600GZ, BIOS SE5C600.86B.02.02.0002.122320131210 12/23/2013
> > dCall Trace:
> > d dump_stack+0x86/0xcf
> > d print_circular_bug+0x203/0x2f0
> > d check_prev_add+0x3be/0x700
> > d ? add_lock_to_list.isra.30+0xc0/0xc0
> > d ? is_bpf_text_address+0x82/0xe0
> > d ? unwind_get_return_address+0x1f/0x30
> > d __lock_acquire+0x10a5/0x1100
> > d ? __lock_acquire+0x10a5/0x1100
> > d ? add_lock_to_list.isra.30+0xc0/0xc0
> > d lock_acquire+0xea/0x1f0
> > d ? flush_work+0x1fd/0x2c0
> > d wait_for_completion+0x3b/0x130
> > d ? flush_work+0x1fd/0x2c0
> > d flush_work+0x1fd/0x2c0
> > d ? flush_workqueue_prep_pwqs+0x1c0/0x1c0
> > d ? trace_hardirqs_on+0xd/0x10
> > d lru_add_drain_all_cpuslocked+0x158/0x1a0
> > d lru_add_drain_all+0x13/0x20
> > d SyS_mlockall+0xb8/0x1c0
> > d entry_SYSCALL_64_fastpath+0x23/0xc2
> > dRIP: 0033:0x7f818d2e54c7
> > dRSP: 002b:00007fffcce83798 EFLAGS: 00000246c ORIG_RAX: 0000000000000097
> > dRAX: ffffffffffffffda RBX: 0000000000000046 RCX: 00007f818d2e54c7
> > dRDX: 0000000000000000 RSI: 00007fffcce83650 RDI: 0000000000000003
> > dRBP: 000000000002c010 R08: 0000000000000000 R09: 0000000000000000
> > dR10: 0000000000000008 R11: 0000000000000246 R12: 000000000002d000
> > dR13: 000000000002c010 R14: 0000000000001000 R15: 00007f818d599b00

[toc] | [prev] | [next] | [standalone]

#1708436

FromByungchul Park <byungchul.park@lge.com>
Date2017-08-10 13:00 +0200
Message-ID<ucTm2-4w9-15@gated-at.bofh.it>
In reply to#1708110
On Thu, Aug 10, 2017 at 09:55:56AM +0900, Byungchul Park wrote:
> On Wed, Aug 09, 2017 at 05:50:59PM +0200, Peter Zijlstra wrote:
> > 
> > 
> > Heh, look what it does...
> 
> It does not happen in my machine..
> 
> I tihink it happens because of "Simplify xhlock ring buffer invalidation"
> patch of you.

I mis-understood your simplification patch. I think your patch works well
unless overwriting occurs - it doesn't work if overwriting occurs though.

Anyway, if crossrelease and lockdep report the following in a normal
condition, it would be a desirable result.

What do you think about the following report? Positive? Or negative?

> 
> First of all, could you reverse yours and check if it happens, too?
> If not, we have to think the simplification more.
> 
> BTW, does your patch consider the possibility that a worker and irqs can
> be nested? Is it no problem even in the case?
> 
> > 
> > 
> > 4======================================================
> > 4WARNING: possible circular locking dependency detected
> > 4.13.0-rc2-00317-gadc6764a3adf-dirty #797 Tainted: G        W      
> > 4------------------------------------------------------
> > 4startpar/582 is trying to acquire lock:
> > c (c(complete)&barr->donec){+.+.}c, at: [<ffffffff8110de4d>] flush_work+0x1fd/0x2c0
> > 4
> > but task is already holding lock:
> > c (clockc#3c){+.+.}c, at: [<ffffffff8122e866>] lru_add_drain_all_cpuslocked+0x46/0x1a0
> > 4
> > which lock already depends on the new lock.
> > 
> > 4
> > the existing dependency chain (in reverse order) is:
> > 
> > -> #4c (clockc#3c){+.+.}c:
> >        __lock_acquire+0x10a5/0x1100
> >        lock_acquire+0xea/0x1f0
> >        __mutex_lock+0x6c/0x960
> >        mutex_lock_nested+0x1b/0x20
> >        lru_add_drain_all_cpuslocked+0x46/0x1a0
> >        lru_add_drain_all+0x13/0x20
> >        SyS_mlockall+0xb8/0x1c0
> >        entry_SYSCALL_64_fastpath+0x23/0xc2
> > 
> > -> #3c (ccpu_hotplug_lock.rw_semc){++++}c:
> >        __lock_acquire+0x10a5/0x1100
> >        lock_acquire+0xea/0x1f0
> >        cpus_read_lock+0x2a/0x90
> >        kmem_cache_create+0x2a/0x1d0
> >        scsi_init_sense_cache+0xa0/0xc0
> >        scsi_add_host_with_dma+0x67/0x360
> >        isci_pci_probe+0x873/0xc90
> >        local_pci_probe+0x42/0xa0
> >        work_for_cpu_fn+0x14/0x20
> >        process_one_work+0x273/0x6b0
> >        worker_thread+0x21b/0x3f0
> >        kthread+0x147/0x180
> >        ret_from_fork+0x2a/0x40
> > 
> > -> #2c (cscsi_sense_cache_mutexc){+.+.}c:
> >        __lock_acquire+0x10a5/0x1100
> >        lock_acquire+0xea/0x1f0
> >        __mutex_lock+0x6c/0x960
> >        mutex_lock_nested+0x1b/0x20
> >        scsi_init_sense_cache+0x3d/0xc0
> >        scsi_add_host_with_dma+0x67/0x360
> >        isci_pci_probe+0x873/0xc90
> >        local_pci_probe+0x42/0xa0
> >        work_for_cpu_fn+0x14/0x20
> >        process_one_work+0x273/0x6b0
> >        worker_thread+0x21b/0x3f0
> >        kthread+0x147/0x180
> >        ret_from_fork+0x2a/0x40
> > 
> > -> #1c (c(&wfc.work)c){+.+.}c:
> >        process_one_work+0x244/0x6b0
> >        worker_thread+0x21b/0x3f0
> >        kthread+0x147/0x180
> >        ret_from_fork+0x2a/0x40
> >        0xffffffffffffffff
> > 
> > -> #0c (c(complete)&barr->donec){+.+.}c:
> >        check_prev_add+0x3be/0x700
> >        __lock_acquire+0x10a5/0x1100
> >        lock_acquire+0xea/0x1f0
> >        wait_for_completion+0x3b/0x130
> >        flush_work+0x1fd/0x2c0
> >        lru_add_drain_all_cpuslocked+0x158/0x1a0
> >        lru_add_drain_all+0x13/0x20
> >        SyS_mlockall+0xb8/0x1c0
> >        entry_SYSCALL_64_fastpath+0x23/0xc2
> > 
> > other info that might help us debug this:
> > 
> > Chain exists of:
> >   c(complete)&barr->donec --> ccpu_hotplug_lock.rw_semc --> clockc#3c
> > 
> >  Possible unsafe locking scenario:
> > 
> >        CPU0                    CPU1
> >        ----                    ----
> >   lock(clockc#3c);
> >                                lock(ccpu_hotplug_lock.rw_semc);
> >                                lock(clockc#3c);
> >   lock(c(complete)&barr->donec);
> > 
> >  *** DEADLOCK ***
> > 
> > 2 locks held by startpar/582:
> >  #0: c (ccpu_hotplug_lock.rw_semc){++++}c, at: [<ffffffff8122e9ce>] lru_add_drain_all+0xe/0x20
> >  #1: c (clockc#3c){+.+.}c, at: [<ffffffff8122e866>] lru_add_drain_all_cpuslocked+0x46/0x1a0
> > 
> > stack backtrace:
> > dCPU: 23 PID: 582 Comm: startpar Tainted: G        W       4.13.0-rc2-00317-gadc6764a3adf-dirty #797
> > dHardware name: Intel Corporation S2600GZ/S2600GZ, BIOS SE5C600.86B.02.02.0002.122320131210 12/23/2013
> > dCall Trace:
> > d dump_stack+0x86/0xcf
> > d print_circular_bug+0x203/0x2f0
> > d check_prev_add+0x3be/0x700
> > d ? add_lock_to_list.isra.30+0xc0/0xc0
> > d ? is_bpf_text_address+0x82/0xe0
> > d ? unwind_get_return_address+0x1f/0x30
> > d __lock_acquire+0x10a5/0x1100
> > d ? __lock_acquire+0x10a5/0x1100
> > d ? add_lock_to_list.isra.30+0xc0/0xc0
> > d lock_acquire+0xea/0x1f0
> > d ? flush_work+0x1fd/0x2c0
> > d wait_for_completion+0x3b/0x130
> > d ? flush_work+0x1fd/0x2c0
> > d flush_work+0x1fd/0x2c0
> > d ? flush_workqueue_prep_pwqs+0x1c0/0x1c0
> > d ? trace_hardirqs_on+0xd/0x10
> > d lru_add_drain_all_cpuslocked+0x158/0x1a0
> > d lru_add_drain_all+0x13/0x20
> > d SyS_mlockall+0xb8/0x1c0
> > d entry_SYSCALL_64_fastpath+0x23/0xc2
> > dRIP: 0033:0x7f818d2e54c7
> > dRSP: 002b:00007fffcce83798 EFLAGS: 00000246c ORIG_RAX: 0000000000000097
> > dRAX: ffffffffffffffda RBX: 0000000000000046 RCX: 00007f818d2e54c7
> > dRDX: 0000000000000000 RSI: 00007fffcce83650 RDI: 0000000000000003
> > dRBP: 000000000002c010 R08: 0000000000000000 R09: 0000000000000000
> > dR10: 0000000000000008 R11: 0000000000000246 R12: 000000000002d000
> > dR13: 000000000002c010 R14: 0000000000001000 R15: 00007f818d599b00

[toc] | [prev] | [next] | [standalone]

#1708370

FromByungchul Park <byungchul.park@lge.com>
Date2017-08-10 11:40 +0200
Message-ID<ucS6B-3LT-11@gated-at.bofh.it>
In reply to#1707436
On Wed, Aug 09, 2017 at 05:50:59PM +0200, Peter Zijlstra wrote:
> 
> 
> Heh, look what it does...

Wait.. execuse me but.. is it a real problem?

> 
> 
> 4======================================================
> 4WARNING: possible circular locking dependency detected
> 4.13.0-rc2-00317-gadc6764a3adf-dirty #797 Tainted: G        W      
> 4------------------------------------------------------
> 4startpar/582 is trying to acquire lock:
> c (c(complete)&barr->donec){+.+.}c, at: [<ffffffff8110de4d>] flush_work+0x1fd/0x2c0
> 4
> but task is already holding lock:
> c (clockc#3c){+.+.}c, at: [<ffffffff8122e866>] lru_add_drain_all_cpuslocked+0x46/0x1a0
> 4
> which lock already depends on the new lock.
> 
> 4
> the existing dependency chain (in reverse order) is:
> 
> -> #4c (clockc#3c){+.+.}c:
>        __lock_acquire+0x10a5/0x1100
>        lock_acquire+0xea/0x1f0
>        __mutex_lock+0x6c/0x960
>        mutex_lock_nested+0x1b/0x20
>        lru_add_drain_all_cpuslocked+0x46/0x1a0
>        lru_add_drain_all+0x13/0x20
>        SyS_mlockall+0xb8/0x1c0
>        entry_SYSCALL_64_fastpath+0x23/0xc2
> 
> -> #3c (ccpu_hotplug_lock.rw_semc){++++}c:
>        __lock_acquire+0x10a5/0x1100
>        lock_acquire+0xea/0x1f0
>        cpus_read_lock+0x2a/0x90
>        kmem_cache_create+0x2a/0x1d0
>        scsi_init_sense_cache+0xa0/0xc0
>        scsi_add_host_with_dma+0x67/0x360
>        isci_pci_probe+0x873/0xc90
>        local_pci_probe+0x42/0xa0
>        work_for_cpu_fn+0x14/0x20
>        process_one_work+0x273/0x6b0
>        worker_thread+0x21b/0x3f0
>        kthread+0x147/0x180
>        ret_from_fork+0x2a/0x40
> 
> -> #2c (cscsi_sense_cache_mutexc){+.+.}c:
>        __lock_acquire+0x10a5/0x1100
>        lock_acquire+0xea/0x1f0
>        __mutex_lock+0x6c/0x960
>        mutex_lock_nested+0x1b/0x20
>        scsi_init_sense_cache+0x3d/0xc0
>        scsi_add_host_with_dma+0x67/0x360
>        isci_pci_probe+0x873/0xc90
>        local_pci_probe+0x42/0xa0
>        work_for_cpu_fn+0x14/0x20
>        process_one_work+0x273/0x6b0
>        worker_thread+0x21b/0x3f0
>        kthread+0x147/0x180
>        ret_from_fork+0x2a/0x40
> 
> -> #1c (c(&wfc.work)c){+.+.}c:
>        process_one_work+0x244/0x6b0
>        worker_thread+0x21b/0x3f0
>        kthread+0x147/0x180
>        ret_from_fork+0x2a/0x40
>        0xffffffffffffffff
> 
> -> #0c (c(complete)&barr->donec){+.+.}c:
>        check_prev_add+0x3be/0x700
>        __lock_acquire+0x10a5/0x1100
>        lock_acquire+0xea/0x1f0
>        wait_for_completion+0x3b/0x130
>        flush_work+0x1fd/0x2c0
>        lru_add_drain_all_cpuslocked+0x158/0x1a0
>        lru_add_drain_all+0x13/0x20
>        SyS_mlockall+0xb8/0x1c0
>        entry_SYSCALL_64_fastpath+0x23/0xc2
> 
> other info that might help us debug this:
> 
> Chain exists of:
>   c(complete)&barr->donec --> ccpu_hotplug_lock.rw_semc --> clockc#3c
> 
>  Possible unsafe locking scenario:
> 
>        CPU0                    CPU1
>        ----                    ----
>   lock(clockc#3c);
>                                lock(ccpu_hotplug_lock.rw_semc);
>                                lock(clockc#3c);
>   lock(c(complete)&barr->donec);
> 
>  *** DEADLOCK ***
> 
> 2 locks held by startpar/582:
>  #0: c (ccpu_hotplug_lock.rw_semc){++++}c, at: [<ffffffff8122e9ce>] lru_add_drain_all+0xe/0x20
>  #1: c (clockc#3c){+.+.}c, at: [<ffffffff8122e866>] lru_add_drain_all_cpuslocked+0x46/0x1a0
> 
> stack backtrace:
> dCPU: 23 PID: 582 Comm: startpar Tainted: G        W       4.13.0-rc2-00317-gadc6764a3adf-dirty #797
> dHardware name: Intel Corporation S2600GZ/S2600GZ, BIOS SE5C600.86B.02.02.0002.122320131210 12/23/2013
> dCall Trace:
> d dump_stack+0x86/0xcf
> d print_circular_bug+0x203/0x2f0
> d check_prev_add+0x3be/0x700
> d ? add_lock_to_list.isra.30+0xc0/0xc0
> d ? is_bpf_text_address+0x82/0xe0
> d ? unwind_get_return_address+0x1f/0x30
> d __lock_acquire+0x10a5/0x1100
> d ? __lock_acquire+0x10a5/0x1100
> d ? add_lock_to_list.isra.30+0xc0/0xc0
> d lock_acquire+0xea/0x1f0
> d ? flush_work+0x1fd/0x2c0
> d wait_for_completion+0x3b/0x130
> d ? flush_work+0x1fd/0x2c0
> d flush_work+0x1fd/0x2c0
> d ? flush_workqueue_prep_pwqs+0x1c0/0x1c0
> d ? trace_hardirqs_on+0xd/0x10
> d lru_add_drain_all_cpuslocked+0x158/0x1a0
> d lru_add_drain_all+0x13/0x20
> d SyS_mlockall+0xb8/0x1c0
> d entry_SYSCALL_64_fastpath+0x23/0xc2
> dRIP: 0033:0x7f818d2e54c7
> dRSP: 002b:00007fffcce83798 EFLAGS: 00000246c ORIG_RAX: 0000000000000097
> dRAX: ffffffffffffffda RBX: 0000000000000046 RCX: 00007f818d2e54c7
> dRDX: 0000000000000000 RSI: 00007fffcce83650 RDI: 0000000000000003
> dRBP: 000000000002c010 R08: 0000000000000000 R09: 0000000000000000
> dR10: 0000000000000008 R11: 0000000000000246 R12: 000000000002d000
> dR13: 000000000002c010 R14: 0000000000001000 R15: 00007f818d599b00

[toc] | [prev] | [next] | [standalone]

#1708434

FromPeter Zijlstra <peterz@infradead.org>
Date2017-08-10 13:00 +0200
Message-ID<ucTm2-4w9-7@gated-at.bofh.it>
In reply to#1708370
On Thu, Aug 10, 2017 at 06:37:07PM +0900, Byungchul Park wrote:
> On Wed, Aug 09, 2017 at 05:50:59PM +0200, Peter Zijlstra wrote:
> > 
> > 
> > Heh, look what it does...
> 
> Wait.. execuse me but.. is it a real problem?

I've not tried again with my patch removed -- I'm chasing another issue
atm. But note that I'm running this on tip/master which has a bunch of
hotplug lock rework in, and that sequence includes hotplug lock.

[toc] | [prev] | [next] | [standalone]

#1708450

FromIngo Molnar <mingo@kernel.org>
Date2017-08-10 13:20 +0200
Message-ID<ucTFo-4RQ-5@gated-at.bofh.it>
In reply to#1705149
* Byungchul Park <byungchul.park@lge.com> wrote:

> Change from v7
> 	- rebase on latest tip/sched/core (Jul 26 2017)
> 	- apply peterz's suggestions
> 	- simplify code of crossrelease_{hist/soft/hard}_{start/end}
> 	- exclude a patch avoiding redundant links
> 	- exclude a patch already applied onto the base

Ok, it's looking pretty good here now, there's one thing I'd like you to change, 
please remove all the new Kconfig dependencies:

 CONFIG_LOCKDEP_CROSSRELEASE=y
 CONFIG_LOCKDEP_COMPLETE=y

and make it all part of PROVE_LOCKING, like most of the other lock debugging bits.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1708481

FromByungchul Park <byungchul.park@lge.com>
Date2017-08-10 13:50 +0200
Message-ID<ucU8q-51N-25@gated-at.bofh.it>
In reply to#1708450
On Thu, Aug 10, 2017 at 01:10:19PM +0200, Ingo Molnar wrote:
> 
> * Byungchul Park <byungchul.park@lge.com> wrote:
> 
> > Change from v7
> > 	- rebase on latest tip/sched/core (Jul 26 2017)
> > 	- apply peterz's suggestions
> > 	- simplify code of crossrelease_{hist/soft/hard}_{start/end}
> > 	- exclude a patch avoiding redundant links
> > 	- exclude a patch already applied onto the base
> 
> Ok, it's looking pretty good here now, there's one thing I'd like you to change, 
> please remove all the new Kconfig dependencies:
> 
>  CONFIG_LOCKDEP_CROSSRELEASE=y
>  CONFIG_LOCKDEP_COMPLETE=y
> 
> and make it all part of PROVE_LOCKING, like most of the other lock debugging bits.

OK. I will remove them. What about CONFIG_LOCKDEP_PAGELOCK? Should I also
remove it?

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web