Date: Wed, 6 Sep 2017 10:32:54 +0900
From: Byungchul Park <byungchul.park@lge.com>
To: Boqun Feng <boqun.feng@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
        Byungchul Park <max.byungchul.park@gmail.com>,
        Ingo Molnar <mingo@kernel.org>, Tejun Heo <tj@kernel.org>,
        david@fromorbit.com, Johannes Berg <johannes@sipsolutions.net>,
        oleg@redhat.com,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        kernel-team@lge.com
Subject: Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation
Message-ID: <20170906013254.GB3240@X58A-UD3R>
References: <20170905003844.GO3240@X58A-UD3R>
 <20170905070825.tovfkqvxpwosh5oa@hirez.programming.kicks-ass.net>
 <20170905071930.h6t2f4guvmswibnv@hirez.programming.kicks-ass.net>
 <20170905085727.GV3240@X58A-UD3R>
 <20170905093624.zlwhvg32ahkpnamk@hirez.programming.kicks-ass.net>
 <20170905103144.GW3240@X58A-UD3R>
 <20170905105838.GX3240@X58A-UD3R>
 <20170905134643.mbjjphn2obwkzpzx@hirez.programming.kicks-ass.net>
 <20170905235235.GZ3240@X58A-UD3R>
 <20170906004211.GT11771@tardis>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170906004211.GT11771@tardis>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2128
Lines: 53

On Wed, Sep 06, 2017 at 08:42:11AM +0800, Boqun Feng wrote:
> > OK. If the workqueue is only user of the weird lockdep annotations, then
> > it might be better to defer introducing the extra state until needed.
> > 
> > But, the 'might' thing I introduced would be necessary if more users
> > want to report deadlocks at the time for crosslocks with speculative
> > acquisitions like the workqueue does, since the recursive-read thing
> > would generate false dependencies much more than we want, while the
> 
> What do you mean by "false dependencies"? AFAICT, recursive-read could

All locks used in every work->func() generate false dependencies with
'work' and 'wq', while any flush works are not involved. It's inevitable.

Moreover, it's also possible to generate more false ones between the
pseudo acquisitions, if real acquisitions are used for that speculative
purpose e.i. recursive-read here, which are anyway real ones.

Moreover, it's also possible to generate more false ones between holding
locks and the pseudo ones, of course, the workqueue code is not the case
for now.

Moreover, it's also possible to generate more false ones between the
pseudo ones and crosslocks on commit, once making crossrelease work even
for recursive-read things.

Whatever...

Only thing I want to say is, don't make code just work, but make code
use right ones semantically for its specific application. Otherwise,
we cannot avoid side effects we don't expect. Of course, these side
effects might be not visible at the moment, IOW, generating false
dependencies might be not problems at the moment, but I just want to
avoid not doing something in the right way, if possible. That's all.

> have dependencies to the following cross commit, for example:
> 
> 	A(a)
> 			ARR(a)
> 			RRR(a)
> 	WFC(X)
> 			C(X)
> 
> This is a deadlock, no?

This is obviously a deadlock.

> In my upcoming v2 for recursive-read support, I'm going to make this
> detectable. But please note as crossrelease doesn't have any selftests

I hope you make the recursive-read things work successfully.

> as normal lockdep stuffs, I may miss something subtle.