Messages in this thread | | | Date | Tue, 22 Aug 2017 16:46:02 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING |
| |
On Tue, Aug 22, 2017 at 03:49:22PM +0200, Peter Zijlstra wrote: > 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) > > spells deadlock, since W isn't allowed to recurse. > > > /me goes dig through commit: > > e159489baa71 ("workqueue: relax lockdep annotation on flush_work()") > > to figure out wth the existing users really want.
Yep, they really want recursive, the pattern there is one work flushing another on the same workqueue, which ends up being:
Work W1: Work W2: Task:
AR(Q) AR(Q) M(A) A(W1) A(W2) flush_workqueue(Q) flush_work(W2) M(A) A(Q) A(W2) R(W2) R(Q) R(W2) R(Q) AR(Q) R(Q) R(W1) R(Q)
should spell deadlock (AQ-QA), and W1 takes Q recursively.
I am however slightly puzzled by the need of flush_work() to take Q, what deadlock potential is there?
Task: Work-W1: Work-W2:
M(A) AR(Q) AR(Q) flush_work(W1) A(W1) A(W2) A(W1) M(A) R(W1) AR(Q) R(Q)
Spells deadlock on AQ-QA, but why? Why is flush_work() linked to any lock taken inside random other works. If we can get rid of flush_work()'s usage of Q, we can drop the recursive nature.
It was added by Oleg in commit:
a67da70dc095 ("workqueues: lockdep annotations for flush_work()")
Which has a distinct lack of Changelog. However, that is still very much the old workqueue code, where I think the annotation makes sense because that was a single thread running the works consecutively. But I don't see it making sense for the current workqueue that runs works concurrently.
TJ, Oleg, can we agree flush_work() no longer needs the dependency on Q?
Also, TJ, what protects @pwq in start_flush_work() at the time of lock_map_*() ?
Also^2, TJ, what's the purpose of using atomic_long_t for work->data? All it ever seems to do is atomic_long_read() and atomic_long_set(), neither of which provides anything stronger than READ_ONCE()/WRITE_ONCE() respectively.
| |