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

Groups > linux.kernel > #1726313 > unrolled thread

[PATCH 3/3] lockdep: Remove unnecessary acquisitions wrt workqueue flush

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

  [PATCH 3/3] lockdep: Remove unnecessary acquisitions wrt workqueue flush Byungchul Park <byungchul.park@lge.com> - 2017-09-05 04:30 +0200
    Re: [PATCH 3/3] lockdep: Remove unnecessary acquisitions wrt  workqueue flush Peter Zijlstra <peterz@infradead.org> - 2017-09-05 09:30 +0200
      RE: [PATCH 3/3] lockdep: Remove unnecessary acquisitions wrt  workqueue flush 박병철/선임연구원/SW Platform(연)AOT팀(byungchul.park@lge.com)          <byungchul.park@lge.com> - 2017-09-05 09:40 +0200
        Re: [PATCH 3/3] lockdep: Remove unnecessary acquisitions wrt  workqueue flush Peter Zijlstra <peterz@infradead.org> - 2017-09-05 10:20 +0200
        Re: [PATCH 3/3] lockdep: Remove unnecessary acquisitions wrt  workqueue flush Byungchul Park <byungchul.park@lge.com> - 2017-09-05 10:30 +0200

#1726313 — [PATCH 3/3] lockdep: Remove unnecessary acquisitions wrt workqueue flush

Workqueue added manual acquisitions to catch deadlock cases. Now
crossrelease was introduced, some of those are redundant because
crossrelease-enabled wait_for_completeion() also does it. Removed it.

Also, lock_map_acquire() in process_one_work() is too strong for
that purpose. lock_map_acquire_might() is enough. Replaced it.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 kernel/workqueue.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index ab3c0dc..8b728d1 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2091,8 +2091,8 @@ static void process_one_work(struct worker *worker, struct work_struct *work)
 
 	spin_unlock_irq(&pool->lock);
 
-	lock_map_acquire(&pwq->wq->lockdep_map);
-	lock_map_acquire(&lockdep_map);
+	lock_map_acquire_might(&pwq->wq->lockdep_map);
+	lock_map_acquire_might(&lockdep_map);
 	/*
 	 * Strictly speaking we should mark the invariant state without holding
 	 * any locks, that is, before these two lock_map_acquire()'s.
@@ -2504,7 +2504,7 @@ static void insert_wq_barrier(struct pool_workqueue *pwq,
 	 */
 	lockdep_init_map_crosslock((struct lockdep_map *)&barr->done.map,
 				   "(complete)wq_barr::done",
-				   target->lockdep_map.key, 1);
+				   target->lockdep_map.key, 0);
 	__init_completion(&barr->done);
 	barr->task = current;
 
@@ -2611,16 +2611,17 @@ void flush_workqueue(struct workqueue_struct *wq)
 	struct wq_flusher this_flusher = {
 		.list = LIST_HEAD_INIT(this_flusher.list),
 		.flush_color = -1,
-		.done = COMPLETION_INITIALIZER_ONSTACK(this_flusher.done),
 	};
 	int next_color;
 
+	lockdep_init_map_crosslock((struct lockdep_map *)&this_flusher.done.map,
+				   "(complete)wq_flusher::done",
+				   wq->lockdep_map.key, 0);
+	__init_completion(&this_flusher.done);
+
 	if (WARN_ON(!wq_online))
 		return;
 
-	lock_map_acquire(&wq->lockdep_map);
-	lock_map_release(&wq->lockdep_map);
-
 	mutex_lock(&wq->mutex);
 
 	/*
@@ -2883,9 +2884,6 @@ bool flush_work(struct work_struct *work)
 	if (WARN_ON(!wq_online))
 		return false;
 
-	lock_map_acquire(&work->lockdep_map);
-	lock_map_release(&work->lockdep_map);
-
 	if (start_flush_work(work, &barr)) {
 		wait_for_completion(&barr.done);
 		destroy_work_on_stack(&barr.work);
-- 
1.9.1

[toc] | [next] | [standalone]

#1726465 — Re: [PATCH 3/3] lockdep: Remove unnecessary acquisitions wrt workqueue flush

On Tue, Sep 05, 2017 at 11:29:14AM +0900, Byungchul Park wrote:

> Also, lock_map_acquire() in process_one_work() is too strong for
> that purpose. lock_map_acquire_might() is enough. Replaced it.

NAK!! traditional annotations are superior to cross-release. They are
not timing dependent.

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

#1726479 — RE: [PATCH 3/3] lockdep: Remove unnecessary acquisitions wrt workqueue flush

> -----Original Message-----
> From: Peter Zijlstra [mailto:peterz@infradead.org]
> Sent: Tuesday, September 05, 2017 4:26 PM
> To: Byungchul Park
> Cc: tj@kernel.org; johannes.berg@intel.com; mingo@kernel.org;
> tglx@linutronix.de; oleg@redhat.com; david@fromorbit.com; linux-
> kernel@vger.kernel.org; kernel-team@lge.com
> Subject: Re: [PATCH 3/3] lockdep: Remove unnecessary acquisitions wrt
> workqueue flush
> 
> On Tue, Sep 05, 2017 at 11:29:14AM +0900, Byungchul Park wrote:
> 
> > Also, lock_map_acquire() in process_one_work() is too strong for that
> > purpose. lock_map_acquire_might() is enough. Replaced it.
> 
> NAK!! traditional annotations are superior to cross-release. They are not
> timing dependent.

You seem to mis-understand this. This also make them timing independent.
I also agree that we need timing independent report in workqueue code.
That's actually why I propose this patch.

I just tried to do it in a right way.

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

#1726537 — Re: [PATCH 3/3] lockdep: Remove unnecessary acquisitions wrt workqueue flush

On Tue, Sep 05, 2017 at 04:36:18PM +0900, �ں�ö/���ӿ�����/SW Platform(��)AOT��(byungchul.park@lge.com) wrote:
> > -----Original Message-----
> > From: Peter Zijlstra [mailto:peterz@infradead.org]
> > Sent: Tuesday, September 05, 2017 4:26 PM
> > To: Byungchul Park
> > Cc: tj@kernel.org; johannes.berg@intel.com; mingo@kernel.org;
> > tglx@linutronix.de; oleg@redhat.com; david@fromorbit.com; linux-
> > kernel@vger.kernel.org; kernel-team@lge.com
> > Subject: Re: [PATCH 3/3] lockdep: Remove unnecessary acquisitions wrt
> > workqueue flush
> > 
> > On Tue, Sep 05, 2017 at 11:29:14AM +0900, Byungchul Park wrote:
> > 
> > > Also, lock_map_acquire() in process_one_work() is too strong for that
> > > purpose. lock_map_acquire_might() is enough. Replaced it.
> > 
> > NAK!! traditional annotations are superior to cross-release. They are not
> > timing dependent.
> 
> You seem to mis-understand this. This also make them timing independent.
> I also agree that we need timing independent report in workqueue code.
> That's actually why I propose this patch.

Then clearly it needs comments and changelog to explain how it does
things.

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

#1726542 — Re: [PATCH 3/3] lockdep: Remove unnecessary acquisitions wrt workqueue flush

On Tue, Sep 05, 2017 at 04:36:18PM +0900, �ں�ö/���ӿ�����/SW Platform(��)AOT��(byungchul.park@lge.com) wrote:
> > -----Original Message-----
> > From: Peter Zijlstra [mailto:peterz@infradead.org]
> > Sent: Tuesday, September 05, 2017 4:26 PM
> > To: Byungchul Park
> > Cc: tj@kernel.org; johannes.berg@intel.com; mingo@kernel.org;
> > tglx@linutronix.de; oleg@redhat.com; david@fromorbit.com; linux-
> > kernel@vger.kernel.org; kernel-team@lge.com
> > Subject: Re: [PATCH 3/3] lockdep: Remove unnecessary acquisitions wrt
> > workqueue flush
> > 
> > On Tue, Sep 05, 2017 at 11:29:14AM +0900, Byungchul Park wrote:
> > 
> > > Also, lock_map_acquire() in process_one_work() is too strong for that
> > > purpose. lock_map_acquire_might() is enough. Replaced it.
> > 
> > NAK!! traditional annotations are superior to cross-release. They are not
> > timing dependent.
> 
> You seem to mis-understand this. This also make them timing independent.
> I also agree that we need timing independent report in workqueue code.
> That's actually why I propose this patch.
> 
> I just tried to do it in a right way.

Adding insufficient comments seems to lead to mis-understand what
lock_map_acquire_might() does. I will enhance it. (And might need to
rename it to a better one as you pointed out in another reply.)

I introduced it to give a hint to lockdep that "It's not actual
acquisition but pseudo one, informing that the context might be the
commit context" so that lockdep can report warnings at the real time
as current code does.

This hint is useful to report run time deadlocks, timing independently,
but not need to be considered on commit. And the hint should be relaxed
as far as possible. I just did that.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web