Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1720059 > unrolled thread

Re: [RFC] workqueue: remove manual lockdep uses to detect deadlocks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC] workqueue: remove manual lockdep uses to detect deadlocks Tejun Heo <tj@kernel.org> - 2017-08-25 15:40 +0200
    Re: [RFC] workqueue: remove manual lockdep uses to detect deadlocks Byungchul Park <max.byungchul.park@gmail.com> - 2017-08-25 17:50 +0200
      Re: [RFC] workqueue: remove manual lockdep uses to detect deadlocks Peter Zijlstra <peterz@infradead.org> - 2017-08-29 21:00 +0200
        Re: [RFC] workqueue: remove manual lockdep uses to detect deadlocks Byungchul Park <byungchul.park@lge.com> - 2017-08-30 04:00 +0200
          Re: [RFC] workqueue: remove manual lockdep uses to detect deadlocks Peter Zijlstra <peterz@infradead.org> - 2017-08-30 08:30 +0200
    Re: [RFC] workqueue: remove manual lockdep uses to detect deadlocks Byungchul Park <byungchul.park@lge.com> - 2017-08-29 02:30 +0200

#1720059 — Re: [RFC] workqueue: remove manual lockdep uses to detect deadlocks

On Fri, Aug 25, 2017 at 05:41:03PM +0900, Byungchul Park wrote:
> Hello all,
> 
> This is _RFC_.
> 
> I want to request for comments about if it's reasonable conceptually. If
> yes, I want to resend after working it more carefully.
> 
> Could you let me know your opinions about this?
> 
> ----->8-----
> From 448360c343477fff63df766544eec4620657a59e Mon Sep 17 00:00:00 2001
> From: Byungchul Park <byungchul.park@lge.com>
> Date: Fri, 25 Aug 2017 17:35:07 +0900
> Subject: [RFC] workqueue: remove manual lockdep uses to detect deadlocks
> 
> We introduced the following commit to detect deadlocks caused by
> wait_for_completion() in flush_{workqueue, work}() and other locks. But
> now LOCKDEP_COMPLETIONS is introduced, such works are automatically done
> by LOCKDEP_COMPLETIONS. So it doesn't have to be done manually anymore.
> Removed it.

I'm not following lockdep development, so can't really comment but if
you're saying that wq can retain the same level of protection while
not having explicit annotations, conceptually, it's of course great.
However, how would it distinguish things like flushing another work
item on a workqueue w/ max_active of 1?

Thanks.

-- 
tejun

[toc] | [next] | [standalone]

#1720196

On Fri, Aug 25, 2017 at 10:34 PM, Tejun Heo <tj@kernel.org> wrote:
> On Fri, Aug 25, 2017 at 05:41:03PM +0900, Byungchul Park wrote:
>> Hello all,
>>
>> This is _RFC_.
>>
>> I want to request for comments about if it's reasonable conceptually. If
>> yes, I want to resend after working it more carefully.
>>
>> Could you let me know your opinions about this?
>>
>> ----->8-----
>> From 448360c343477fff63df766544eec4620657a59e Mon Sep 17 00:00:00 2001
>> From: Byungchul Park <byungchul.park@lge.com>
>> Date: Fri, 25 Aug 2017 17:35:07 +0900
>> Subject: [RFC] workqueue: remove manual lockdep uses to detect deadlocks
>>
>> We introduced the following commit to detect deadlocks caused by
>> wait_for_completion() in flush_{workqueue, work}() and other locks. But
>> now LOCKDEP_COMPLETIONS is introduced, such works are automatically done
>> by LOCKDEP_COMPLETIONS. So it doesn't have to be done manually anymore.
>> Removed it.
>
> I'm not following lockdep development, so can't really comment but if
> you're saying that wq can retain the same level of protection while
> not having explicit annotations, conceptually, it's of course great.

Well.. I don't think it's the same level currently. But, I can make it with some
modification. I expect the wq code to become much simpler.

> However, how would it distinguish things like flushing another work

I think it must be distinguished with what it actually waits for, e.i.
completion
variables instead of work or wq. I will make it next week and let you know.

> item on a workqueue w/ max_active of 1?

I will answer it wrt max_active == 1 next week. I need to review wq code.

-- 
Thanks,
Byungchul

[toc] | [prev] | [next] | [standalone]

#1722698

On Sat, Aug 26, 2017 at 12:49:26AM +0900, Byungchul Park wrote:
> > However, how would it distinguish things like flushing another work
> 
> I think it must be distinguished with what it actually waits for, e.i.
> completion
> variables instead of work or wq. I will make it next week and let you know.

So no. The existing annotations are strictly better than relying on
cross-release.

As you know the problem with cross-release is that it is timing
dependent. You need to actually observe the problematic sequence before
it can warn, and only the whole instance->class mapping saves us from
actually hitting the deadlock.

Cross-release can result in deadlocks without warnings. If you were to
run:

	mutex_lock(A);
					mutex_lock(A);
					complete(C);
	wait_for_completion(C);

You'd deadlock without issue. Only if we observe this:

	mutex_lock(A);
	wait_for_completion(C);
					mutex_lock(A);
					complete(C);

Where we acquire A after wait_for_completion() but before complete()
will we observe the deadlock.

The same would be true for using cross-release for workqueues as well,
something like:

					W:
					mutex_lock(A)

	mutex_lock(A)
	flush_work(W)

would go unreported whereas the current workqueue annotation will
generate a splat.


This does not mean cross-release isn't worth it, its better than nothing,
but its strictly weaker than traditional annotations.

So where a traditional annotation is possible, we should use them.

[toc] | [prev] | [next] | [standalone]

#1722966

On Tue, Aug 29, 2017 at 08:57:27PM +0200, Peter Zijlstra wrote:
> On Sat, Aug 26, 2017 at 12:49:26AM +0900, Byungchul Park wrote:
> > > However, how would it distinguish things like flushing another work
> > 
> > I think it must be distinguished with what it actually waits for, e.i.
> > completion
> > variables instead of work or wq. I will make it next week and let you know.
> 
> So no. The existing annotations are strictly better than relying on
> cross-release.

Thank you for exaplanation but, as I already said, this is why I said
"I don't think it's the same level currently. But, I can make it with
some modification." to TJ:

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1479560.html

And also I mentioned we might need the current code inevitably but, the
existing annotations are never good and why here:

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1480173.html

> As you know the problem with cross-release is that it is timing
> dependent. You need to actually observe the problematic sequence before
> it can warn, and only the whole instance->class mapping saves us from
> actually hitting the deadlock.

Of course.

> The same would be true for using cross-release for workqueues as well,
> something like:
> 
> 					W:
> 					mutex_lock(A)
> 
> 	mutex_lock(A)
> 	flush_work(W)
> 
> would go unreported whereas the current workqueue annotation will
> generate a splat.

Of course.

That's why I said we need to work on it. But it should be modified so
that the wq code becomes more clear instead of abusing weird acquire()s.

[toc] | [prev] | [next] | [standalone]

#1723059

On Wed, Aug 30, 2017 at 10:53:39AM +0900, Byungchul Park wrote:
> On Tue, Aug 29, 2017 at 08:57:27PM +0200, Peter Zijlstra wrote:
> > On Sat, Aug 26, 2017 at 12:49:26AM +0900, Byungchul Park wrote:
> > > > However, how would it distinguish things like flushing another work
> > > 
> > > I think it must be distinguished with what it actually waits for, e.i.
> > > completion
> > > variables instead of work or wq. I will make it next week and let you know.
> > 
> > So no. The existing annotations are strictly better than relying on
> > cross-release.
> 
> Thank you for exaplanation but, as I already said, this is why I said
> "I don't think it's the same level currently. But, I can make it with
> some modification." to TJ:
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1479560.html
> 
> And also I mentioned we might need the current code inevitably but, the
> existing annotations are never good and why here:
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1480173.html

I can read the words, but have no idea what you're trying to say.

[toc] | [prev] | [next] | [standalone]

#1722079

On Fri, Aug 25, 2017 at 06:34:43AM -0700, Tejun Heo wrote:
> On Fri, Aug 25, 2017 at 05:41:03PM +0900, Byungchul Park wrote:
> > Hello all,
> > 
> > This is _RFC_.
> > 
> > I want to request for comments about if it's reasonable conceptually. If
> > yes, I want to resend after working it more carefully.
> > 
> > Could you let me know your opinions about this?
> > 
> > ----->8-----
> > From 448360c343477fff63df766544eec4620657a59e Mon Sep 17 00:00:00 2001
> > From: Byungchul Park <byungchul.park@lge.com>
> > Date: Fri, 25 Aug 2017 17:35:07 +0900
> > Subject: [RFC] workqueue: remove manual lockdep uses to detect deadlocks
> > 
> > We introduced the following commit to detect deadlocks caused by
> > wait_for_completion() in flush_{workqueue, work}() and other locks. But
> > now LOCKDEP_COMPLETIONS is introduced, such works are automatically done
> > by LOCKDEP_COMPLETIONS. So it doesn't have to be done manually anymore.
> > Removed it.
> 
> I'm not following lockdep development, so can't really comment but if
> you're saying that wq can retain the same level of protection while
> not having explicit annotations, conceptually, it's of course great.
> However, how would it distinguish things like flushing another work
> item on a workqueue w/ max_active of 1?

Do you mean the following?

process_one_work()
   acquire(W1) <---------+- distinguishable?
   work->fn()            |
      flush_work(W2)     |
         acquire(W2) <---+
         release(W2)
   release(W1)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web