From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753909AbdHWKU4 (ORCPT ); Wed, 23 Aug 2017 06:20:56 -0400 Received: from bombadil.infradead.org ([65.50.211.133]:49924 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753837AbdHWKUz (ORCPT ); Wed, 23 Aug 2017 06:20:55 -0400 Date: Wed, 23 Aug 2017 12:20:48 +0200 From: Peter Zijlstra To: Byungchul Park Cc: Dave Chinner , mingo@kernel.org, linux-kernel@vger.kernel.org, kernel-team@lge.com, Arnaldo Carvalho de Melo , Tejun Heo , Boqun Feng Subject: Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Message-ID: <20170823102048.5a6s5dbynyuv3bba@hirez.programming.kicks-ass.net> References: <1502960261-16206-1-git-send-email-byungchul.park@lge.com> <20170821154600.asyzqs2zg6w6o4pg@hirez.programming.kicks-ass.net> <20170822054603.GT10621@dastard> <20170822090603.vufl5ncoequ5ag6x@hirez.programming.kicks-ass.net> <20170822092236.GI20323@X58A-UD3R> <20170822093703.vuuxvxaytna5im33@hirez.programming.kicks-ass.net> <20170823021217.GB3108@X58A-UD3R> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170823021217.GB3108@X58A-UD3R> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Aug 23, 2017 at 11:12:17AM +0900, Byungchul Park wrote: > > > We have to detect dependecies if it exists, even in the following case: > > > > > > oooooooiiiiiiiiiiiiiiiiiii......... > > > |<- range for commit ->| > > > > > > where > > > o: acquisition outside of each work, > > > i: acquisition inside of each work, > > > > > > With yours, we can never detect dependecies wrt 'o'. > > > > There really shouldn't be any o's when you call > > There can be any o's. Yes, but they _must_ be irrelevant, see below. > > crossrelease_hist_start(XHLOCK_PROC), it should denote the bottom of a > > No, I don't think so. It can be either the bottom or not. Nope, wrong, _must_ be bottom. > hist_start() and hist_end() is only for special contexts which need roll > back on exit e.g. irq, work and so on. Normal kernel context should work > well w/o hist_start() or hist_end(). The (soft) IRQ ones, yes, the PROC one is special though. > > context, see: > > > > https://lkml.kernel.org/r/20170301104328.GD6515@twins.programming.kicks-ass.net > > Actually, I don't agree with that. > > > And in that respect you placed the calls wrongly in process_one_work(), > > Why is it wrong? It's intended. Could you tell me why? The purpose of the PROC thing is to annotate _independent_ execution, like work's. Either they should _all_ depend on a held lock, or they should not depend on it at all. Either way this means we should start(PROC) before taking any locks. Similar with history, independence means to not depend on prior state, so per definition we should not have history at this point. So by this reasoning the workqueue annotation should be: crossrelease_hist_start(XHLOCK_PROC); lock_map_acquire(wq->lockdep_map); lock_map_acquire(lockdep_map); work->func(work); /* go hard */ lock_map_release(lockdep_map); lock_map_release(wq->lockdep_map); crossrelease_hist_end(XHLOCK_PROC); This way _all_ works are guaranteed the same context, and not only those that didn't overflow the history stack. Now, it so happens we have an unfortunate interaction with the flush_work{,queue}() annotations. The read-recursive thing would work if lockdep were fixed, however I don't think there is a possible deadlock between flush_work() and complete() (except for single-threaded workqueues) So until we fix lockdep for read-recursive things, I don't think its a problem if we (ab)use your wrong placement to kill the interaction between these two annotations. I'll send some patches..