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

Groups > linux.kernel > #1722249 > unrolled thread

Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-29 11:10 +0200
    [tip:locking/core] locking/lockdep: Untangle xhlock history  save/restore from task independence tip-bot for Peter Zijlstra <tipbot@zytor.com> - 2017-08-29 16:30 +0200
    Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <max.byungchul.park@gmail.com> - 2017-08-29 18:10 +0200
      Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-29 20:50 +0200
    Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-08-30 04:20 +0200
      Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-08-30 09:50 +0200
        Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-30 11:00 +0200
          RE: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation "Byungchul Park" <byungchul.park@lge.com> - 2017-08-30 11:10 +0200
            Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-30 11:20 +0200
              RE: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation "Byungchul Park" <byungchul.park@lge.com> - 2017-08-30 11:40 +0200
            Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-30 11:20 +0200
              RE: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation "Byungchul Park" <byungchul.park@lge.com> - 2017-08-30 11:30 +0200
                Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-08-30 13:30 +0200
                  Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <max.byungchul.park@gmail.com> - 2017-08-30 15:00 +0200
                  Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-08-31 09:30 +0200
                  Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-31 10:10 +0200
                    Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-08-31 10:20 +0200
                      Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-31 10:40 +0200
                        Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-01 04:10 +0200
                          Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-01 11:50 +0200
                            Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-01 12:20 +0200
                              RE: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation 박병철/선임연구원/SW Platform(연)AOT팀(byungchul.park@lge.com)          <byungchul.park@lge.com> - 2017-09-01 14:10 +0200
                              Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-01 14:40 +0200
                                Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <max.byungchul.park@gmail.com> - 2017-09-01 16:00 +0200
                                  Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-01 18:40 +0200
                                    Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-04 03:40 +0200
                                      Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-04 04:10 +0200
                                      Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-04 13:50 +0200
                                        Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-05 02:40 +0200
                                          Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-05 09:10 +0200
                                            Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-05 09:20 +0200
                                              Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-05 11:00 +0200
                                                Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-05 11:40 +0200
                                                  Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-05 12:40 +0200
                                                    Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-05 13:00 +0200
                                                      Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-05 15:50 +0200
                                                        Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-06 02:00 +0200
                                                          Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Boqun Feng <boqun.feng@gmail.com> - 2017-09-06 02:50 +0200
                                                            Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-06 03:40 +0200
                                                              Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-07 02:00 +0200
                                                              Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-07 02:20 +0200
                                                        Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-06 02:50 +0200
                                                    Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-05 13:00 +0200
                                                      Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-05 13:30 +0200
                                            Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-05 10:40 +0200
                Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-31 10:10 +0200

Page 1 of 3  [1] 2 3  Next page →

#1722249 — Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation

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) <peterz@infradead.org>
---
 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);
 

[toc] | [next] | [standalone]

#1722535 — [tip:locking/core] locking/lockdep: Untangle xhlock history save/restore from task independence

Commit-ID:  f52be5708076b75a045ac52c6fef3fffb8300525
Gitweb:     http://git.kernel.org/tip/f52be5708076b75a045ac52c6fef3fffb8300525
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Tue, 29 Aug 2017 10:59:39 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 29 Aug 2017 15:14:38 +0200

locking/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) <peterz@infradead.org>
Cc: Byungchul Park <byungchul.park@lge.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: boqun.feng@gmail.com
Cc: david@fromorbit.com
Cc: johannes@sipsolutions.net
Cc: kernel-team@lge.com
Cc: oleg@redhat.com
Cc: tj@kernel.org
Link: http://lkml.kernel.org/r/20170829085939.ggmb6xiohw67micb@hirez.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 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 9bc050b..5fdd93b 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 78bb713..bfa8e0b 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 f73ca59..44c8d0d 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 c033189..ab3c0dc 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);
 

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

#1722585

On Tue, Aug 29, 2017 at 5:59 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> 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.

I rather have explained why it's not, more times than you did, and you
have not read my explanation. Anyway, I am seriously curious about
why. Of course, I remember you said "PROC is special", but not _why_.
I really want to know _why_ PROC(=each work) should be handled
differently from others. Please show me an example except wq case
where you just tried to avoid problems than fix them.

> You seem to still think it can be used like the soft/hard-irq ones, this
> is fundamentally not so.

I wonder why, seriously.

>
> 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) <peterz@infradead.org>
> ---
>  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);
>



-- 
Thanks,
Byungchul

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

#1722694

On Wed, Aug 30, 2017 at 01:02:39AM +0900, Byungchul Park wrote:
> On Tue, Aug 29, 2017 at 5:59 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > 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.
> 
> I rather have explained why it's not, more times than you did, and you
> have not read my explanation. Anyway, I am seriously curious about
> why. Of course, I remember you said "PROC is special", but not _why_.
> I really want to know _why_ PROC(=each work) should be handled
> differently from others.

It is a question of need. We don't need more.

At points where we hold no locks and know we don't depend on prior
state, we can simply throw away history to get what we want, independent
execution.

We don't loose anything by it and its simpler. It is not much different
from that work_id thing you had previously.

And its fairly fundamental, every site where we know prior state is
irrelevant, we must not hold any locks, because at that point you
explicitly throw away dependencies (like we do for the wq thing now).

> Please show me an example except wq case
> where you just tried to avoid problems than fix them.

wq isn't broken, the annotations there are fine. The interaction between
the existing annotations and crossrelease are unfortunate but that
doesn't mean they're no good.

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

#1722972

On Tue, Aug 29, 2017 at 10:59:39AM +0200, Peter Zijlstra wrote:
> 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.

Much better to me than the patch you did previously, but, see blow.

> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index c0331891dec1..ab3c0dc8c7ed 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -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);

This is what I am always curious about. It would be ok if you agree with
removing this work-around after fixing acquire things in wq. But, you
keep to say this is essencial.

You should focus on what dependencies actually are, than saparating
contexts unnecessarily. Of course, we have to do it for each work, _BUT_
not between outside of work and each work since there might be
dependencies between them certainly.

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

#1723076

On Wed, Aug 30, 2017 at 11:09:53AM +0900, Byungchul Park wrote:
> On Tue, Aug 29, 2017 at 10:59:39AM +0200, Peter Zijlstra wrote:
> > 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.
> 
> Much better to me than the patch you did previously, but, see blow.
> 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index c0331891dec1..ab3c0dc8c7ed 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -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);
> 
> This is what I am always curious about. It would be ok if you agree with
> removing this work-around after fixing acquire things in wq. But, you
> keep to say this is essencial.
> 
> You should focus on what dependencies actually are, than saparating
> contexts unnecessarily. Of course, we have to do it for each work, _BUT_
> not between outside of work and each work since there might be
> dependencies between them certainly.

You have never answered it. I'm curious about your answer. If you can't,
I think you have to revert all your patches. All yours are wrong.

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

#1723126

On Wed, Aug 30, 2017 at 04:41:17PM +0900, Byungchul Park wrote:
> On Wed, Aug 30, 2017 at 11:09:53AM +0900, Byungchul Park wrote:

> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > > index c0331891dec1..ab3c0dc8c7ed 100644
> > > --- a/kernel/workqueue.c
> > > +++ b/kernel/workqueue.c
> > > @@ -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);
> > 
> > This is what I am always curious about. It would be ok if you agree with
> > removing this work-around after fixing acquire things in wq. But, you
> > keep to say this is essencial.
> > 
> > You should focus on what dependencies actually are, than saparating
> > contexts unnecessarily. Of course, we have to do it for each work, _BUT_
> > not between outside of work and each work since there might be
> > dependencies between them certainly.
> 
> You have never answered it. I'm curious about your answer. If you can't,
> I think you have to revert all your patches. All yours are wrong.

Because I don't understand what you're on about. And my patches actually
work.

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

#1723137

> -----Original Message-----
> From: Peter Zijlstra [mailto:peterz@infradead.org]
> Sent: Wednesday, August 30, 2017 5:54 PM
> 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
> 
> On Wed, Aug 30, 2017 at 04:41:17PM +0900, Byungchul Park wrote:
> > On Wed, Aug 30, 2017 at 11:09:53AM +0900, Byungchul Park wrote:
> 
> > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > > ---
> > > > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > > > index c0331891dec1..ab3c0dc8c7ed 100644
> > > > --- a/kernel/workqueue.c
> > > > +++ b/kernel/workqueue.c
> > > > @@ -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);
> > >
> > > This is what I am always curious about. It would be ok if you agree
> with
> > > removing this work-around after fixing acquire things in wq. But, you
> > > keep to say this is essencial.
> > >
> > > You should focus on what dependencies actually are, than saparating
> > > contexts unnecessarily. Of course, we have to do it for each work,
> _BUT_
> > > not between outside of work and each work since there might be
> > > dependencies between them certainly.
> >
> > You have never answered it. I'm curious about your answer. If you can't,
> > I think you have to revert all your patches. All yours are wrong.
> 
> Because I don't understand what you're on about. And my patches actually
> work.

My point is that we inevitably lose valuable dependencies by yours. That's
why I've endlessly asked you 'do you have any reason you try those patches?'
a ton of times. And you have never answered it.

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

#1723143

On Wed, Aug 30, 2017 at 11:12:23AM +0200, Peter Zijlstra wrote:
> On Wed, Aug 30, 2017 at 06:01:59PM +0900, Byungchul Park wrote:
> > My point is that we inevitably lose valuable dependencies by yours. That's
> > why I've endlessly asked you 'do you have any reason you try those patches?'
> > a ton of times. And you have never answered it.
> 
> The only dependencies that are lost are those between the first work and
> the setup of the workqueue thread.
> 
> And there obviously _should_ not be any dependencies between those. A
> work should not depend on the setup of the thread.

Furthermore, the save/restore can't preserve those dependencies. The
moment a work exhausts xhlocks[] they are gone. So by assuming the first
work _will_ exhaust the history there is effectively nothing lost.

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

#1723155

> -----Original Message-----
> From: Peter Zijlstra [mailto:peterz@infradead.org]
> Sent: Wednesday, August 30, 2017 6:14 PM
> 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
> 
> On Wed, Aug 30, 2017 at 11:12:23AM +0200, Peter Zijlstra wrote:
> > On Wed, Aug 30, 2017 at 06:01:59PM +0900, Byungchul Park wrote:
> > > My point is that we inevitably lose valuable dependencies by yours.
> That's
> > > why I've endlessly asked you 'do you have any reason you try those
> patches?'
> > > a ton of times. And you have never answered it.
> >
> > The only dependencies that are lost are those between the first work and
> > the setup of the workqueue thread.
> >
> > And there obviously _should_ not be any dependencies between those. A
> > work should not depend on the setup of the thread.
> 
> Furthermore, the save/restore can't preserve those dependencies. The
> moment a work exhausts xhlocks[] they are gone. So by assuming the first

They are gone _one time_ only once it has been overwritten, and
Recovered at next turn, with original code. But you made it
un-recoverable even at the next time and lose all valuable
dependencies unconditionally.

> work _will_ exhaust the history there is effectively nothing lost.

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

#1723144

On Wed, Aug 30, 2017 at 06:01:59PM +0900, Byungchul Park wrote:
> My point is that we inevitably lose valuable dependencies by yours. That's
> why I've endlessly asked you 'do you have any reason you try those patches?'
> a ton of times. And you have never answered it.

The only dependencies that are lost are those between the first work and
the setup of the workqueue thread.

And there obviously _should_ not be any dependencies between those. A
work should not depend on the setup of the thread.

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

#1723146

> -----Original Message-----
> From: Peter Zijlstra [mailto:peterz@infradead.org]
> Sent: Wednesday, August 30, 2017 6:12 PM
> 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
> 
> On Wed, Aug 30, 2017 at 06:01:59PM +0900, Byungchul Park wrote:
> > My point is that we inevitably lose valuable dependencies by yours.
> That's
> > why I've endlessly asked you 'do you have any reason you try those
> patches?'
> > a ton of times. And you have never answered it.
> 
> The only dependencies that are lost are those between the first work and
> the setup of the workqueue thread.
> 
> And there obviously _should_ not be any dependencies between those. A

100% right. Since there obviously should not be any, it would be better
to check them. So I've endlessly asked you 'do you have any reason removing
the opportunity for that check?'. Overhead? Logical problem? Or want to
believe workqueue setup code perfect forever? I mean, is it a problem if we
check them?

> work should not depend on the setup of the thread.

100% right.

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

#1723212

On Wed, Aug 30, 2017 at 06:24:39PM +0900, Byungchul Park wrote:
> > -----Original Message-----
> > From: Peter Zijlstra [mailto:peterz@infradead.org]
> > Sent: Wednesday, August 30, 2017 6:12 PM
> > 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
> > 
> > On Wed, Aug 30, 2017 at 06:01:59PM +0900, Byungchul Park wrote:
> > > My point is that we inevitably lose valuable dependencies by yours.
> > That's
> > > why I've endlessly asked you 'do you have any reason you try those
> > patches?'
> > > a ton of times. And you have never answered it.
> > 
> > The only dependencies that are lost are those between the first work and
> > the setup of the workqueue thread.
> > 
> > And there obviously _should_ not be any dependencies between those. A
> 
> 100% right. Since there obviously should not be any, it would be better
> to check them. So I've endlessly asked you 'do you have any reason removing
> the opportunity for that check?'. Overhead? Logical problem? Or want to
> believe workqueue setup code perfect forever? I mean, is it a problem if we
> check them?
> 
> > work should not depend on the setup of the thread.
> 
> 100% right.

For example - I'm giving you the same example repeatedly:

context X                 context Y
---------                 ---------
                          wait_for_completion(C)
acquire(A)
process_one_work()
   acquire(B)
   work->fn()
      complete(C)

Please check C->A and C->B.

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

#1723282

On Wed, Aug 30, 2017 at 8:25 PM, Byungchul Park <byungchul.park@lge.com> wrote:
> On Wed, Aug 30, 2017 at 06:24:39PM +0900, Byungchul Park wrote:
>> > -----Original Message-----
>> > From: Peter Zijlstra [mailto:peterz@infradead.org]
>> > Sent: Wednesday, August 30, 2017 6:12 PM
>> > 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
>> >
>> > On Wed, Aug 30, 2017 at 06:01:59PM +0900, Byungchul Park wrote:
>> > > My point is that we inevitably lose valuable dependencies by yours.
>> > That's
>> > > why I've endlessly asked you 'do you have any reason you try those
>> > patches?'
>> > > a ton of times. And you have never answered it.
>> >
>> > The only dependencies that are lost are those between the first work and
>> > the setup of the workqueue thread.
>> >
>> > And there obviously _should_ not be any dependencies between those. A
>>
>> 100% right. Since there obviously should not be any, it would be better
>> to check them. So I've endlessly asked you 'do you have any reason removing
>> the opportunity for that check?'. Overhead? Logical problem? Or want to
>> believe workqueue setup code perfect forever? I mean, is it a problem if we
>> check them?
>>
>> > work should not depend on the setup of the thread.
>>
>> 100% right.
>
> For example - I'm giving you the same example repeatedly:
>
> context X                 context Y
> ---------                 ---------
>                           wait_for_completion(C)
> acquire(A)
> process_one_work()
>    acquire(B)
>    work->fn()
>       complete(C)
>
> Please check C->A and C->B.

s/check/let lockdep check/


-- 
Thanks,
Byungchul

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

#1723902

On Wed, Aug 30, 2017 at 08:25:46PM +0900, Byungchul Park wrote:
> On Wed, Aug 30, 2017 at 06:24:39PM +0900, Byungchul Park wrote:
> > > -----Original Message-----
> > > From: Peter Zijlstra [mailto:peterz@infradead.org]
> > > Sent: Wednesday, August 30, 2017 6:12 PM
> > > 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
> > > 
> > > On Wed, Aug 30, 2017 at 06:01:59PM +0900, Byungchul Park wrote:
> > > > My point is that we inevitably lose valuable dependencies by yours.
> > > That's
> > > > why I've endlessly asked you 'do you have any reason you try those
> > > patches?'
> > > > a ton of times. And you have never answered it.
> > > 
> > > The only dependencies that are lost are those between the first work and
> > > the setup of the workqueue thread.
> > > 
> > > And there obviously _should_ not be any dependencies between those. A
> > 
> > 100% right. Since there obviously should not be any, it would be better
> > to check them. So I've endlessly asked you 'do you have any reason removing
> > the opportunity for that check?'. Overhead? Logical problem? Or want to
> > believe workqueue setup code perfect forever? I mean, is it a problem if we
> > check them?
> > 
> > > work should not depend on the setup of the thread.
> > 
> > 100% right.
> 
> For example - I'm giving you the same example repeatedly:
> 
> context X                 context Y
> ---------                 ---------
>                           wait_for_completion(C)
> acquire(A)
> process_one_work()
>    acquire(B)
>    work->fn()
>       complete(C)
> 
> Please let lockdep check C->A and C->B.

You always stop answering whenever I ask you for opinion with this
example. I'm really curious. Could you let me know your opinion about
this example?

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

#1723965

On Wed, Aug 30, 2017 at 08:25:46PM +0900, Byungchul Park wrote:

> For example - I'm giving you the same example repeatedly:
> 
> context X                 context Y
> ---------                 ---------
>                           wait_for_completion(C)
> acquire(A)
> process_one_work()
>    acquire(B)
>    work->fn()
>       complete(C)
> 
> Please check C->A and C->B.

Is there a caller of procesS_one_work() that holds a lock?

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

#1723979

On Thu, Aug 31, 2017 at 10:04:42AM +0200, Peter Zijlstra wrote:
> On Wed, Aug 30, 2017 at 08:25:46PM +0900, Byungchul Park wrote:
> 
> > For example - I'm giving you the same example repeatedly:
> > 
> > context X                 context Y
> > ---------                 ---------
> >                           wait_for_completion(C)
> > acquire(A)
> > process_one_work()
> >    acquire(B)
> >    work->fn()
> >       complete(C)
> > 
> > Please check C->A and C->B.
> 
> Is there a caller of procesS_one_work() that holds a lock?

It's not important. Ok, check the following, instead:

context X                 context Y
---------                 ---------
                          wait_for_completion(C)
acquire(A)
release(A)
process_one_work()
   acquire(B)
   release(B)
   work->fn()
      complete(C)

We don't need to lose C->A and C->B dependencies unnecessarily.

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

#1724004

On Thu, Aug 31, 2017 at 05:15:01PM +0900, Byungchul Park wrote:
> It's not important. Ok, check the following, instead:
> 
> context X                 context Y
> ---------                 ---------
>                           wait_for_completion(C)
> acquire(A)
> release(A)
> process_one_work()
>    acquire(B)
>    release(B)
>    work->fn()
>       complete(C)
> 
> We don't need to lose C->A and C->B dependencies unnecessarily.

I really can't be arsed about them. Its really only the first few works
that will retain that dependency anyway, even if you were to retain
them.

All of that is contained in kernel/kthread and kernel/workqueue and can
be audited if needed. Its a very limited amount of code.

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

#1724742

On Thu, Aug 31, 2017 at 10:34:53AM +0200, Peter Zijlstra wrote:
> On Thu, Aug 31, 2017 at 05:15:01PM +0900, Byungchul Park wrote:
> > It's not important. Ok, check the following, instead:
> > 
> > context X                 context Y
> > ---------                 ---------
> >                           wait_for_completion(C)
> > acquire(A)
> > release(A)
> > process_one_work()
> >    acquire(B)
> >    release(B)
> >    work->fn()
> >       complete(C)
> > 
> > We don't need to lose C->A and C->B dependencies unnecessarily.
> 
> I really can't be arsed about them. Its really only the first few works
> that will retain that dependency anyway, even if you were to retain
> them.

Wrong.

Every 'work' doing complete() for different classes of completion
variable suffers from losing valuable dependencies, every time, not
first few ones.

Remind we are talking about dependencies wrt cross-lock, not between
_holding_ locks. If you invalidate xhlock whenever work->fn(), we cannot
build dependencies like C->A and C->B every time. Right?

> All of that is contained in kernel/kthread and kernel/workqueue and can
> be audited if needed. Its a very limited amount of code.

I mean, doing it automatically w/o additional overhead is better than
considering the limited amount of code manually every time changing
kernel code. Do as you please.

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

#1724921

On Fri, Sep 01, 2017 at 11:05:12AM +0900, Byungchul Park wrote:
> On Thu, Aug 31, 2017 at 10:34:53AM +0200, Peter Zijlstra wrote:
> > On Thu, Aug 31, 2017 at 05:15:01PM +0900, Byungchul Park wrote:
> > > It's not important. Ok, check the following, instead:
> > > 
> > > context X                 context Y
> > > ---------                 ---------
> > >                           wait_for_completion(C)
> > > acquire(A)
> > > release(A)
> > > process_one_work()
> > >    acquire(B)
> > >    release(B)
> > >    work->fn()
> > >       complete(C)
> > > 
> > > We don't need to lose C->A and C->B dependencies unnecessarily.
> > 
> > I really can't be arsed about them. Its really only the first few works
> > that will retain that dependency anyway, even if you were to retain
> > them.
> 
> Wrong.
> 
> Every 'work' doing complete() for different classes of completion
> variable suffers from losing valuable dependencies, every time, not
> first few ones.

The moment you overrun the history array its gone. So yes, only the
first few works will ever see them

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

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web