Messages in this thread | | | Subject | Re: [PATCH 1/2] workqueue: skip lockdep wq dependency in cancel_work_sync() | From | Johannes Berg <> | Date | Wed, 22 Aug 2018 11:42:43 +0200 |
| |
On Wed, 2018-08-22 at 18:15 +0900, Byungchul Park wrote: > > > thread 1 thread 2 (wq thread) > > > > common_case = false; > > queue_work(&my_wq, &work); > > mutex_lock(&mutex); > > > > flush_workqueue(&my_wq); > > work_function() > > -> mutex_lock(&mutex); > > -> schedule(), wait for mutex > > wait_for_completion() > > > > -> deadlock - we can't make any forward progress here. > > I see. Now, I understand what you are talking about. > > if (common_case) > schedule_and_wait_for_something_that_takes_a_long_time() > > should be: > > if (common_case) > schedule_and_wait_long_time_so_that_the_work_to_finish()
Fair point.
> 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, > so we must aggresively consider the case the wait_for_completion() > so far hasn't been called but may be called in the future.
Yes.
> I think it's a problem of how aggressively we need to check dependencies. > 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.
I don't think you could consider a report from "aggressive reporting" to 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. Of > 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 not > > You misunderstand me. The commit should be reverted or acquire/release > pairs should be added in both flush functions.
Ok, I thought you were arguing we shouldn't revert it :)
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 patch does.
> Anyway the annotation should be placed in the path where > wait_for_completion() might be called.
Yes, it should be called regardless of whether we actually wait or not, 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 in > Documentation/locking/crossrelease.txt which has been removed because > crossrelease is strong at detecting *potential* deadlock problems.
Ok, you know what, I'm going to read this now ... hang on........
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" now ...
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.
johannes
| |