From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752992AbdH2PwK (ORCPT ); Tue, 29 Aug 2017 11:52:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43354 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751729AbdH2PwJ (ORCPT ); Tue, 29 Aug 2017 11:52:09 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 979F0C04BD2C Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=oleg@redhat.com Date: Tue, 29 Aug 2017 17:52:05 +0200 From: Oleg Nesterov To: Peter Zijlstra Cc: Byungchul Park , mingo@kernel.org, linux-kernel@vger.kernel.org, kernel-team@lge.com, Arnaldo Carvalho de Melo , Dave Chinner , Tejun Heo , johannes@sipsolutions.net Subject: Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Message-ID: <20170829155205.GA17290@redhat.com> References: <20170822051438.GD20323@X58A-UD3R> <20170822075238.uyfmhgxeal2bwcdg@hirez.programming.kicks-ass.net> <20170822085100.GH20323@X58A-UD3R> <20170822092141.fjmr74xhfid7vu7h@hirez.programming.kicks-ass.net> <20170822093337.GJ20323@X58A-UD3R> <20170822100840.eababgjcu76iois5@hirez.programming.kicks-ass.net> <20170822134922.m2g6kqsqo2eojrg7@hirez.programming.kicks-ass.net> <20170822144602.uh5jzkkchvdgzs3s@hirez.programming.kicks-ass.net> <20170823163903.GA12973@redhat.com> <20170823174714.in4mv7uc3rdheygg@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170823174714.in4mv7uc3rdheygg@hirez.programming.kicks-ass.net> User-Agent: Mutt/1.5.24 (2015-08-30) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 29 Aug 2017 15:52:09 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Peter, sorry for delay, didn't have a chance to return to this discussion... On 08/23, Peter Zijlstra wrote: > > > > It was added by Oleg in commit: > > > > > > a67da70dc095 ("workqueues: lockdep annotations for flush_work()") > > > > No, these annotations were moved later into start_flush, iiuc... > > > > This > > > > lock_map_acquire(&work->lockdep_map); > > lock_map_release(&work->lockdep_map); > > > > was added by another commit 0976dfc1d0cd80a4e9dfaf87bd8744612bde475a > > "workqueue: Catch more locking problems with flush_work()", and at > > first glance it is fine. > > Those are fine and are indeed the flush_work() vs work inversion. > > The two straight forward annotations are: > > flush_work(work) process_one_work(wq, work) > A(work) A(work) > R(work) work->func(work); > R(work) > > Which catches: > > Task-1: work: > > mutex_lock(&A); mutex_lock(&A); > flush_work(work); Yes, yes, this is clear. But if we ignore the multithreaded workqueues, in this particular case we could rely on A(wq)/R(wq) in start_flush() and process_one_work(). The problem is that start_flush_work() does not do acquire/release unconditionally, it does this only if it is going to wait, and I am not sure this is right... Plus process_one_work() does lock_map_acquire_read(), I don't really understand this too. > And the analogous: > > flush_workqueue(wq) process_one_work(wq, work) > A(wq) A(wq) > R(wq) work->func(work); > R(wq) > > > The thing I puzzled over was flush_work() (really start_flush_work()) > doing: > > if (pwq->wq->saved_max_active == 1 || pwq->wq->rescuer) > lock_map_acquire(&pwq->wq->lockdep_map); > else > lock_map_acquire_read(&pwq->wq->lockdep_map); > lock_map_release(&pwq->wq->lockdep_map); > > Why does flush_work() care about the wq->lockdep_map? > > The answer is because, for single-threaded workqueues, doing > flush_work() from a work is a potential deadlock: Yes, but the simple answer is that flush_work() doesn't really differ from flush_workqueue() in this respect? If nothing else, if some WORK is the last queued work on WQ, then flush_work(WORK) is the same thing as flush_workqueuw(WQ), more or less. Again, I am talking about single-threaded workqueues. > workqueue-thread: > > work-n: > flush_work(work-n+1); > > work-n+1: > > > Will not be going anywhere fast.. Or another example, lock(LOCK); flush_work(WORK); unlock(LOCK); workqueue-thread: another_pending_work: LOCK(LOCK); UNLOCK(LOCK); WORK: In this case we do not care about WORK->lockdep_map, but taking the wq->lockdep_map from flush_work() (if single-threaded) allows to report the deadlock. Again, this is just like flush_workqueue(). Oleg.