Date:   Wed, 22 Aug 2018 11:42:43 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Byungchul Park <byungchul.park@....com>
Cc:     Tejun Heo <tj@...nel.org>, Lai Jiangshan <jiangshanlai@...il.com>,
        linux-kernel@...r.kernel.org, linux-wireless@...r.kernel.org,
        kernel-team@....com
Subject: Re: [PATCH 1/2] workqueue: skip lockdep wq dependency in
 cancel_work_sync()

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

Powered by blists - more mailing lists