On Wed, 2018-08-22 at 18:15 +0900, Byungchul Park wrote:
Fair point.thread 1 thread 2 (wq thread)I see. Now, I understand what you are talking about.
common_case = false;
-> schedule(), wait for mutex
-> deadlock - we can't make any forward progress here.
Ok. I didn't know what you are talking about but now I got it.great.
You are talking about who knows whether common_case is true or not,Yes.
so we must aggresively consider the case the wait_for_completion()
so far hasn't been called but may be called in the future.
I think it's a problem of how aggressively we need to check dependencies.I don't think you could consider a report from "aggressive reporting" to
If we choose the aggressive option, then we could get reported
aggressively but could not avoid aggresive false positives either.
I don't want to strongly argue that because it's a problem of policy.
be a false positive. It's clearly possible that this happens, and once
you go to a workqueue you basically don't really know your sequencing
and timing any more.
Just, I would consider only waits that actually happened anyway. OfOk, I thought you were arguing we shouldn't revert it :)
course, we gotta consider the waits definitely even if any actual
deadlock doesn't happen since detecting potantial problem is more
important than doing on actual ones as you said.
So no big objection to check dependencies aggressively.
Here we don't have a deadlock, but without the revert we will also notYou misunderstand me. The commit should be reverted or acquire/release
pairs should be added in both flush functions.
I don't know whether to revert or just add the pairs in the flush
functions, because I can't say I understand what the third part of the
Anyway the annotation should be placed in the path whereYes, it should be called regardless of whether we actually wait or not,
wait_for_completion() might be called.
i.e. before most of the checking in the functions.
My issue #3 that I outlined is related - we'd like to have lockdep
understand that if this work was on the WQ it might deadlock, but we
don't have a way to get the WQ unless the work is scheduled - and in the
case that it is scheduled we might already hitting the deadlock
(depending on the order of execution though I guess).
Absolutly true. You can find my opinion about that inOk, you know what, I'm going to read this now ... hang on........
Documentation/locking/crossrelease.txt which has been removed because
crossrelease is strong at detecting *potential* deadlock problems.
I see. You were trying to solve the problem of completions/context
transfers in lockdep.
Given the revert of crossrelease though, we can't actually revert your
patch anyway, and looking at it again I see what you mean by the "name"
So yeah, we should only re-add the two acquire/release pairs. I guess
I'll make a patch for that, replacing my patch 2.
I'm intrigued by the crossrelease - but that's a separate topic.