On Wed, Aug 23, 2017 at 11:43:23AM +0900, Byungchul Park wrote: > On Tue, Aug 22, 2017 at 03:49:22PM +0200, Peter Zijlstra wrote: > > On Tue, Aug 22, 2017 at 12:08:40PM +0200, Peter Zijlstra wrote: > > > > > > > > I meant: > > > > > > > > > > > > mutex_lock(&A) > > > > > > <work> > > > > > > lockdep_map_acquire_read(&work) > > > > > > mutex_lock(&A) > > > > > > > > > > > > lockdep_map_acquire(&work) > > > > > > flush_work(&work) > > > > > > > > > > > > I mean it can still be detected with a read acquisition in work. > > > > > > Am I wrong? > > > > > > > > > > Think so, although there's something weird with read locks that I keep > > > > > forgetting. But I'm not sure it'll actually solve the problem. But I > > > > > can > > > > > > > > I mean, read acquisitions are nothing but ones allowing read ones to be > > > > re-acquired legally, IOW, we want to check entrance of flush_work() and > > > > works, not between works. That's why I suggested to use read ones in > > > > work > > > > in that case. > > > > > > Does seem to work. > > > > So I think we'll end up hitting a lockdep deficiency and not trigger the > > splat on flush_work(), see also: > > > > https://lwn.net/Articles/332801/ > > > > lock_map_acquire_read() is a read-recursive and will not in fact create > > any dependencies because of this issue. > > > > In specific, check_prev_add() has: > > > > if (next->read == 2 || prev->read == 2) > > return 1; > > > > This means that for: > > > > lock_map_acquire_read(W)(2) > > down_write(A) (0) > > > > down_write(A) (0) > > wait_for_completion(C) (0) > > > > lock_map_acquire_read(W)(2) > > complete(C) (0) > > > > All the (2) effectively go away and 'solve' our current issue, but: > > > > lock_map_acquire_read(W)(2) > > mutex_lock(A) (0) > > > > mutex_lock(A) (0) > > lock_map_acquire(W) (0) > > > > as per flush_work() will not in fact trigger anymore either. > > It should be triggered. Lockdep code should be fixed so that it does. > > > See also the below locking-selftest changes. > > > > > > Now, this means I also have to consider the existing > > lock_map_acquire_read() users and if they really wanted to be recursive > > or not. When I change lock_map_acquire_read() to use > > lock_acquire_shared() this annotation no longer suffices and the splat > > comes back. > > > > > > Also, the acquire_read() annotation will (obviously) no longer work to > > cure this problem when we switch to normal read (1), because then the > > generated chain: > > > > W(1) -> A(0) -> C(0) -> W(1) > > Please explain what W/A/C stand for.
I eventually found them in your words. Let me read this again.
