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

Groups > linux.kernel > #1722249 > unrolled thread

Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation

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: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-29 11:10 +0200
    [tip:locking/core] locking/lockdep: Untangle xhlock history  save/restore from task independence tip-bot for Peter Zijlstra <tipbot@zytor.com> - 2017-08-29 16:30 +0200
    Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <max.byungchul.park@gmail.com> - 2017-08-29 18:10 +0200
      Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-29 20:50 +0200
    Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-08-30 04:20 +0200
      Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-08-30 09:50 +0200
        Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-30 11:00 +0200
          RE: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation "Byungchul Park" <byungchul.park@lge.com> - 2017-08-30 11:10 +0200
            Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-30 11:20 +0200
              RE: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation "Byungchul Park" <byungchul.park@lge.com> - 2017-08-30 11:40 +0200
            Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-30 11:20 +0200
              RE: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation "Byungchul Park" <byungchul.park@lge.com> - 2017-08-30 11:30 +0200
                Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-08-30 13:30 +0200
                  Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <max.byungchul.park@gmail.com> - 2017-08-30 15:00 +0200
                  Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-08-31 09:30 +0200
                  Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-31 10:10 +0200
                    Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-08-31 10:20 +0200
                      Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-31 10:40 +0200
                        Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-01 04:10 +0200
                          Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-01 11:50 +0200
                            Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-01 12:20 +0200
                              RE: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation 박병철/선임연구원/SW Platform(연)AOT팀(byungchul.park@lge.com)          <byungchul.park@lge.com> - 2017-09-01 14:10 +0200
                              Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-01 14:40 +0200
                                Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <max.byungchul.park@gmail.com> - 2017-09-01 16:00 +0200
                                  Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-01 18:40 +0200
                                    Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-04 03:40 +0200
                                      Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-04 04:10 +0200
                                      Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-04 13:50 +0200
                                        Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-05 02:40 +0200
                                          Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-05 09:10 +0200
                                            Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-05 09:20 +0200
                                              Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-05 11:00 +0200
                                                Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-05 11:40 +0200
                                                  Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-05 12:40 +0200
                                                    Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-05 13:00 +0200
                                                      Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-05 15:50 +0200
                                                        Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-06 02:00 +0200
                                                          Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Boqun Feng <boqun.feng@gmail.com> - 2017-09-06 02:50 +0200
                                                            Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-06 03:40 +0200
                                                              Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-07 02:00 +0200
                                                              Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-07 02:20 +0200
                                                        Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-06 02:50 +0200
                                                    Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-09-05 13:00 +0200
                                                      Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-05 13:30 +0200
                                            Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Byungchul Park <byungchul.park@lge.com> - 2017-09-05 10:40 +0200
                Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation Peter Zijlstra <peterz@infradead.org> - 2017-08-31 10:10 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1724940

On Fri, Sep 01, 2017 at 11:47:47AM +0200, Peter Zijlstra wrote:
> On Fri, Sep 01, 2017 at 11:05:12AM +0900, Byungchul Park wrote:
> > On Thu, Aug 31, 2017 at 10:34:53AM +0200, Peter Zijlstra wrote:
> > > On Thu, Aug 31, 2017 at 05:15:01PM +0900, Byungchul Park wrote:
> > > > It's not important. Ok, check the following, instead:
> > > > 
> > > > context X                 context Y
> > > > ---------                 ---------
> > > >                           wait_for_completion(C)
> > > > acquire(A)
> > > > release(A)
> > > > process_one_work()
> > > >    acquire(B)
> > > >    release(B)
> > > >    work->fn()
> > > >       complete(C)
> > > > 
> > > > We don't need to lose C->A and C->B dependencies unnecessarily.
> > > 
> > > I really can't be arsed about them. Its really only the first few works
> > > that will retain that dependency anyway, even if you were to retain
> > > them.
> > 
> > Wrong.
> > 
> > Every 'work' doing complete() for different classes of completion
> > variable suffers from losing valuable dependencies, every time, not
> > first few ones.
> 
> The moment you overrun the history array its gone. So yes, only the

It would be gone _only_ at the time the history overrun, and then it
will be built again. So, you are wrong.

Let me show you an example: (I hope you also show examples.)

context X                  context Y
---------                  ---------
                           wait_for_completion(D)
while (true)
   acquire(A)
   release(A)
   process_one_work()
      acquire(B)
      release(B)
      work->fn()
         complete(C)
   acquire(D)
   release(D)

When happening an overrun in a 'work', 'A' and 'B' will be gone _only_
at the time, and then 'D', 'A' and 'B' will be queued into the xhlock
*again* from the next loop on, and they can be used to generate useful
dependencies again.

You are being confused now. Acquisitions we are focusing now are not
_stacked_ like hlocks, but _accumulated_ continuously onto the ring
buffer e.i. xhlock array.

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

#1725001

> -----Original Message-----
> From: Byungchul Park [mailto:byungchul.park@lge.com]
> Sent: Friday, September 01, 2017 7:16 PM
> To: Peter Zijlstra
> Cc: mingo@kernel.org; tj@kernel.org; boqun.feng@gmail.com;
> david@fromorbit.com; johannes@sipsolutions.net; oleg@redhat.com; linux-
> kernel@vger.kernel.org; kernel-team@lge.com
> Subject: Re: [PATCH 4/4] lockdep: Fix workqueue crossrelease annotation
> 
> On Fri, Sep 01, 2017 at 11:47:47AM +0200, Peter Zijlstra wrote:
> > On Fri, Sep 01, 2017 at 11:05:12AM +0900, Byungchul Park wrote:
> > > On Thu, Aug 31, 2017 at 10:34:53AM +0200, Peter Zijlstra wrote:
> > > > On Thu, Aug 31, 2017 at 05:15:01PM +0900, Byungchul Park wrote:
> > > > > It's not important. Ok, check the following, instead:
> > > > >
> > > > > context X                 context Y
> > > > > ---------                 ---------
> > > > >                           wait_for_completion(C)
> > > > > acquire(A)
> > > > > release(A)
> > > > > process_one_work()
> > > > >    acquire(B)
> > > > >    release(B)
> > > > >    work->fn()
> > > > >       complete(C)
> > > > >
> > > > > We don't need to lose C->A and C->B dependencies unnecessarily.
> > > >
> > > > I really can't be arsed about them. Its really only the first few
> works
> > > > that will retain that dependency anyway, even if you were to retain
> > > > them.
> > >
> > > Wrong.
> > >
> > > Every 'work' doing complete() for different classes of completion
> > > variable suffers from losing valuable dependencies, every time, not
> > > first few ones.
> >
> > The moment you overrun the history array its gone. So yes, only the
> 
> It would be gone _only_ at the time the history overrun, and then it
> will be built again. So, you are wrong.
> 
> Let me show you an example: (I hope you also show examples.)
> 
> context X                  context Y
> ---------                  ---------
>                            wait_for_completion(D)
> while (true)
>    acquire(A)
>    release(A)
>    process_one_work()
>       acquire(B)
>       release(B)
>       work->fn()
>          complete(C)
>    acquire(D)
>    release(D)
> 
> When happening an overrun in a 'work', 'A' and 'B' will be gone _only_
> at the time, and then 'D', 'A' and 'B' will be queued into the xhlock
> *again* from the next loop on, and they can be used to generate useful
> dependencies again.
> 
> You are being confused now. Acquisitions we are focusing now are not
> _stacked_ like hlocks, but _accumulated_ continuously onto the ring
> buffer e.i. xhlock array.

Agree?

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

#1725020

On Fri, Sep 01, 2017 at 07:16:29PM +0900, Byungchul Park wrote:

> It would be gone _only_ at the time the history overrun, and then it
> will be built again. So, you are wrong.

How will it ever be build again? You only ever spawn the worker thread
_ONCE_, then it runs lots and lots of works.

We _could_ go fix it, but I really don't see it being worth the time and
effort, its a few isolated locks inside the kthread/workqueue code.

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

#1725114

On Fri, Sep 1, 2017 at 9:38 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Sep 01, 2017 at 07:16:29PM +0900, Byungchul Park wrote:
>
>> It would be gone _only_ at the time the history overrun, and then it
>> will be built again. So, you are wrong.

s/it will be built again/the acquisition will be added into the xhlock
array again/

Now, better to understand?

> How will it ever be build again? You only ever spawn the worker thread
> _ONCE_, then it runs lots and lots of works.
>
> We _could_ go fix it, but I really don't see it being worth the time and

We don't need to fix it spending time and effort. Just *revert* all your
wrong patches.

> effort, its a few isolated locks inside the kthread/workqueue code.

Please point out what I am wrong and what you want to say, *with* my
latest example. Doing it with my example would be very helpful to
understand you.

-- 
Thanks,
Byungchul

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

#1725208

On Fri, Sep 01, 2017 at 10:51:48PM +0900, Byungchul Park wrote:
> On Fri, Sep 1, 2017 at 9:38 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Fri, Sep 01, 2017 at 07:16:29PM +0900, Byungchul Park wrote:
> >
> >> It would be gone _only_ at the time the history overrun, and then it
> >> will be built again. So, you are wrong.
> 
> s/it will be built again/the acquisition will be added into the xhlock
> array again/
> 
> Now, better to understand?

No, I still don't get it. How are we ever going to get the workqueue
thread setup code back after its spooled out?

> > How will it ever be build again? You only ever spawn the worker thread
> > _ONCE_, then it runs lots and lots of works.
> >
> > We _could_ go fix it, but I really don't see it being worth the time and
> 
> We don't need to fix it spending time and effort. Just *revert* all your
> wrong patches.

And get tangled up with the workqueue annotation again, no thanks.
Having the first few works see the thread setup isn't worth it.

And your work_id annotation had the same problem.

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

#1725804

On Fri, Sep 01, 2017 at 06:38:52PM +0200, Peter Zijlstra wrote:
> On Fri, Sep 01, 2017 at 10:51:48PM +0900, Byungchul Park wrote:
> > On Fri, Sep 1, 2017 at 9:38 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > On Fri, Sep 01, 2017 at 07:16:29PM +0900, Byungchul Park wrote:
> > >
> > >> It would be gone _only_ at the time the history overrun, and then it
> > >> will be built again. So, you are wrong.
> > 
> > s/it will be built again/the acquisition will be added into the xhlock
> > array again/
> > 
> > Now, better to understand?
> 
> No, I still don't get it. How are we ever going to get the workqueue
> thread setup code back after its spooled out?
> 
> > > How will it ever be build again? You only ever spawn the worker thread
> > > _ONCE_, then it runs lots and lots of works.
> > >
> > > We _could_ go fix it, but I really don't see it being worth the time and
> > 
> > We don't need to fix it spending time and effort. Just *revert* all your
> > wrong patches.
> 
> And get tangled up with the workqueue annotation again, no thanks.
> Having the first few works see the thread setup isn't worth it.
> 
> And your work_id annotation had the same problem.

I keep asking you for an example because I really understand you.

   Fix my problematic example with your patches,

   or,

   Show me a problematic scenario with my original code, you expect.

Whatever, it would be helpful to understand you.

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

#1725809

On Mon, Sep 04, 2017 at 10:30:32AM +0900, Byungchul Park wrote:
> On Fri, Sep 01, 2017 at 06:38:52PM +0200, Peter Zijlstra wrote:
> > On Fri, Sep 01, 2017 at 10:51:48PM +0900, Byungchul Park wrote:
> > > On Fri, Sep 1, 2017 at 9:38 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > > On Fri, Sep 01, 2017 at 07:16:29PM +0900, Byungchul Park wrote:
> > > >
> > > >> It would be gone _only_ at the time the history overrun, and then it
> > > >> will be built again. So, you are wrong.
> > > 
> > > s/it will be built again/the acquisition will be added into the xhlock
> > > array again/
> > > 
> > > Now, better to understand?
> > 
> > No, I still don't get it. How are we ever going to get the workqueue
> > thread setup code back after its spooled out?
> > 
> > > > How will it ever be build again? You only ever spawn the worker thread
> > > > _ONCE_, then it runs lots and lots of works.
> > > >
> > > > We _could_ go fix it, but I really don't see it being worth the time and
> > > 
> > > We don't need to fix it spending time and effort. Just *revert* all your
> > > wrong patches.
> > 
> > And get tangled up with the workqueue annotation again, no thanks.
> > Having the first few works see the thread setup isn't worth it.
> > 
> > And your work_id annotation had the same problem.
> 
> I keep asking you for an example because I really understand you.

I keep asking you for an example because I really want to understand you.

> 
>    Fix my problematic example with your patches,
> 
>    or,
> 
>    Show me a problematic scenario with my original code, you expect.
> 
> Whatever, it would be helpful to understand you.

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

#1726014

On Mon, Sep 04, 2017 at 10:30:32AM +0900, Byungchul Park wrote:
> On Fri, Sep 01, 2017 at 06:38:52PM +0200, Peter Zijlstra wrote:
> > And get tangled up with the workqueue annotation again, no thanks.
> > Having the first few works see the thread setup isn't worth it.
> > 
> > And your work_id annotation had the same problem.
> 
> I keep asking you for an example because I really understand you.
> 
>    Fix my problematic example with your patches,
> 
>    or,
> 
>    Show me a problematic scenario with my original code, you expect.
> 
> Whatever, it would be helpful to understand you.

I _really_ don't understand what you're worried about. Is it the kthread
create and workqueue init or the pool->lock that is released/acquired in
process_one_work()?

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

#1726293

On Mon, Sep 04, 2017 at 01:42:48PM +0200, Peter Zijlstra wrote:
> On Mon, Sep 04, 2017 at 10:30:32AM +0900, Byungchul Park wrote:
> > On Fri, Sep 01, 2017 at 06:38:52PM +0200, Peter Zijlstra wrote:
> > > And get tangled up with the workqueue annotation again, no thanks.
> > > Having the first few works see the thread setup isn't worth it.
> > > 
> > > And your work_id annotation had the same problem.
> > 
> > I keep asking you for an example because I really understand you.
> > 
> >    Fix my problematic example with your patches,
> > 
> >    or,
> > 
> >    Show me a problematic scenario with my original code, you expect.
> > 
> > Whatever, it would be helpful to understand you.
> 
> I _really_ don't understand what you're worried about. Is it the kthread
> create and workqueue init or the pool->lock that is released/acquired in
> process_one_work()?

s/in process_one_work()/in all worker code including setup code/

Original code was already designed to handle real dependencies well. But
you invalidated it _w/o_ any reason, that's why I don't agree with your
patches. Your patches only do avoiding the wq issue now we focus on.

Look at:

 worker thread                         another context
 -------------                         ---------------
                                       wait_for_completion()
       |
       |      (1)
       v
  +---------+
  | Work  A | (2)
  +---------+
       |
       |      (3)
       v
  +---------+
  | Work  B | (4)
  +---------+
       |
       |      (5)
       v
  +---------+
  | Work  C | (6)
  +---------+
       |
       v

We have to consider whole context of the worker to build dependencies
with a crosslock e.g. wait_for_commplete().

Only thing we have to care here is to make all works e.g. (2), (4) and
(6) independent, because workqueue does _concurrency control_. As I said
last year at the very beginning, for works not applied the control e.g.
max_active == 1, we don't need that isolation. I said, it's a future work.

It would have been much easier to communicate with each other if you
*tried* to understand my examples like now or you *tried* to give me one
example at least. You didn't even *try*. Only thing I want to ask you
for is to *try* to understand my opinions on conflicts.

Now, understand what I intended? Still unsufficient?

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

#1726384

On Tue, Sep 05, 2017 at 09:38:45AM +0900, Byungchul Park wrote:
> On Mon, Sep 04, 2017 at 01:42:48PM +0200, Peter Zijlstra wrote:
> > On Mon, Sep 04, 2017 at 10:30:32AM +0900, Byungchul Park wrote:
> > > On Fri, Sep 01, 2017 at 06:38:52PM +0200, Peter Zijlstra wrote:
> > > > And get tangled up with the workqueue annotation again, no thanks.
> > > > Having the first few works see the thread setup isn't worth it.
> > > > 
> > > > And your work_id annotation had the same problem.
> > > 
> > > I keep asking you for an example because I really understand you.
> > > 
> > >    Fix my problematic example with your patches,
> > > 
> > >    or,
> > > 
> > >    Show me a problematic scenario with my original code, you expect.
> > > 
> > > Whatever, it would be helpful to understand you.
> > 
> > I _really_ don't understand what you're worried about. Is it the kthread
> > create and workqueue init or the pool->lock that is released/acquired in
> > process_one_work()?
> 
> s/in process_one_work()/in all worker code including setup code/
> 
> Original code was already designed to handle real dependencies well. But
> you invalidated it _w/o_ any reason, that's why I don't agree with your
> patches.

The reasons:

 - it avoids the interaction with the workqueue annotation
 - it makes each work consistent
 - its not different from what you did with work_id:

    https://lkml.kernel.org/r/1489479542-27030-6-git-send-email-byungchul.park@lge.com

    crossrelease_work_start() vs same_context_xhlock() { if
    (xhlock->work_id == curr->workid) ... }

> Your patches only do avoiding the wq issue now we focus on.
> 
> Look at:
> 
>  worker thread                         another context
>  -------------                         ---------------
>                                        wait_for_completion()
>        |
>        |      (1)
>        v
>   +---------+
>   | Work  A | (2)
>   +---------+
>        |
>        |      (3)
>        v
>   +---------+
>   | Work  B | (4)
>   +---------+
>        |
>        |      (5)
>        v
>   +---------+
>   | Work  C | (6)
>   +---------+
>        |
>        v
> 
> We have to consider whole context of the worker to build dependencies
> with a crosslock e.g. wait_for_commplete().
> 
> Only thing we have to care here is to make all works e.g. (2), (4) and
> (6) independent, because workqueue does _concurrency control_. As I said
> last year at the very beginning, for works not applied the control e.g.
> max_active == 1, we don't need that isolation. I said, it's a future work.
> 
> It would have been much easier to communicate with each other if you
> *tried* to understand my examples like now or you *tried* to give me one
> example at least. You didn't even *try*. Only thing I want to ask you
> for is to *try* to understand my opinions on conflicts.
> 
> Now, understand what I intended? Still unsufficient?

So you worry about max_active==1 ? Or you worry about pool->lock or
about the thread setup? I'm still not sure.

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

#1726399

On Tue, Sep 05, 2017 at 09:08:25AM +0200, Peter Zijlstra wrote:
> So you worry about max_active==1 ? Or you worry about pool->lock or
> about the thread setup? I'm still not sure.

So the thing about pool->lock is that its a leaf lock, we take nothing
inside it. Futhermore its a spinlock and therefore blocking things like
completions or page-lock cannot form a deadlock with it.

It is also fully isolated inside workqueue.c and easy to audit.

This is why I really can't be arsed about it.


And the whole setup stuff isn't properly preserved between works in any
case, only the first few works would ever see that history, so why
bother.


We _could_ save/restore the setup history, by doing a complete copy of
it and restoring that, but that's not what crossrelease did, and I
really don't see the point.

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

#1726557

On Tue, Sep 05, 2017 at 09:19:30AM +0200, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 09:08:25AM +0200, Peter Zijlstra wrote:
> > So you worry about max_active==1 ? Or you worry about pool->lock or
> > about the thread setup? I'm still not sure.
> 
> So the thing about pool->lock is that its a leaf lock, we take nothing

I think the following sentence is a key, I hope...

Leaf locks can also create dependecies with *crosslocks*. These
dependencies are not built between holding locks like typical locks.

> inside it. Futhermore its a spinlock and therefore blocking things like
> completions or page-lock cannot form a deadlock with it.

I agree. Now we should be only interested in blocking things.

> It is also fully isolated inside workqueue.c and easy to audit.
> 
> This is why I really can't be arsed about it.
> 
> And the whole setup stuff isn't properly preserved between works in any
> case, only the first few works would ever see that history, so why
> bother.

As I said in another reply, what about (1), (3) and (5) in my example?

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

#1726587

On Tue, Sep 05, 2017 at 05:57:27PM +0900, Byungchul Park wrote:
> On Tue, Sep 05, 2017 at 09:19:30AM +0200, Peter Zijlstra wrote:
> > On Tue, Sep 05, 2017 at 09:08:25AM +0200, Peter Zijlstra wrote:
> > > So you worry about max_active==1 ? Or you worry about pool->lock or
> > > about the thread setup? I'm still not sure.
> > 
> > So the thing about pool->lock is that its a leaf lock, we take nothing
> 
> I think the following sentence is a key, I hope...
> 
> Leaf locks can also create dependecies with *crosslocks*. These
> dependencies are not built between holding locks like typical locks.

They can create dependencies, but they _cannot_ create deadlocks. So
there's no value in those dependencies.

> > And the whole setup stuff isn't properly preserved between works in any
> > case, only the first few works would ever see that history, so why
> > bother.
> 
> As I said in another reply, what about (1), (3) and (5) in my example?

So for single-threaded workqueues, I'd like to get recursive-read sorted
and then we can make the lockdep_invariant_state() conditional.

Using recurisve-read lock for the wq lockdep_map's has the same effect
as your might thing without having to introduce new magic.

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

#1726602

On Tue, Sep 05, 2017 at 11:36:24AM +0200, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 05:57:27PM +0900, Byungchul Park wrote:
> > On Tue, Sep 05, 2017 at 09:19:30AM +0200, Peter Zijlstra wrote:
> > > On Tue, Sep 05, 2017 at 09:08:25AM +0200, Peter Zijlstra wrote:
> > > > So you worry about max_active==1 ? Or you worry about pool->lock or
> > > > about the thread setup? I'm still not sure.
> > > 
> > > So the thing about pool->lock is that its a leaf lock, we take nothing
> > 
> > I think the following sentence is a key, I hope...
> > 
> > Leaf locks can also create dependecies with *crosslocks*. These
> > dependencies are not built between holding locks like typical locks.
> 
> They can create dependencies, but they _cannot_ create deadlocks. So
> there's no value in those dependencies.

Let me show you a possible scenario with a leaf lock:

                        lock(A)
   lock(A)              wait_for_completion(B)
   unlock(A)            ...
   ...                  unlock(A)
   process_one_work()
      work->func()
         complete(B)

It's a deadlock by a lead lock A and completion B.

> > > And the whole setup stuff isn't properly preserved between works in any
> > > case, only the first few works would ever see that history, so why
> > > bother.
> > 
> > As I said in another reply, what about (1), (3) and (5) in my example?
> 
> So for single-threaded workqueues, I'd like to get recursive-read sorted
> and then we can make the lockdep_invariant_state() conditional.
> 
> Using recurisve-read lock for the wq lockdep_map's has the same effect
> as your might thing without having to introduce new magic.

Recursive-read and the hint I proposed(a.k.a. might) should be used for
their different specific applications. Both meaning and constraints of
them are totally different.

Using a right function semantically is more important than making it
just work, as you know. Wrong?

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

#1726611

On Tue, Sep 05, 2017 at 07:31:44PM +0900, Byungchul Park wrote:
> Recursive-read and the hint I proposed(a.k.a. might) should be used for
> their different specific applications. Both meaning and constraints of
> them are totally different.
> 
> Using a right function semantically is more important than making it
> just work, as you know. Wrong?

For example, _semantically_:

   lock(A) -> recursive-read(A), end in a deadlock, while
   lock(A) -> might(A)         , is like nothing.

   recursive-read(A) -> might(A), is like nothing, while
   might(A) -> recursive-read(A), end in a deadlock.

   And so on...

Of course, in the following cases, the results are same:

   recursive-read(A) -> recursive-read(A), is like nothing, and also
   might(A)          -> might(A)         , is like nothing.

   recursive-read(A) -> lock(A), end in a deadlock, and also
   might(A)          -> lock(A), end in a deadlock.

Futhermore, recursive-read-might() can be used if needed, since their
semantics are orthogonal so they can be used in mixed forms.

I really hope you accept the new semantics... I think current workqueue
code exactly needs the semantics.

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

#1726742

On Tue, Sep 05, 2017 at 07:58:38PM +0900, Byungchul Park wrote:
> On Tue, Sep 05, 2017 at 07:31:44PM +0900, Byungchul Park wrote:
> > Recursive-read and the hint I proposed(a.k.a. might) should be used for
> > their different specific applications. Both meaning and constraints of
> > them are totally different.
> > 
> > Using a right function semantically is more important than making it
> > just work, as you know. Wrong?

> Of course, in the following cases, the results are same:
> 
>    recursive-read(A) -> recursive-read(A), is like nothing, and also
>    might(A)          -> might(A)         , is like nothing.
> 
>    recursive-read(A) -> lock(A), end in a deadlock, and also
>    might(A)          -> lock(A), end in a deadlock.

And these are exactly the cases we need.

> Futhermore, recursive-read-might() can be used if needed, since their
> semantics are orthogonal so they can be used in mixed forms.
> 
> I really hope you accept the new semantics... I think current workqueue
> code exactly needs the semantics.

I really don't want to introduce this extra state if we don't have to.
And as you already noted, this 'might' thing of yours doesn't belong in
the .read argument, since as you say its orthogonal.

recursive-read
wait_for_completion()
			recursive-read
			complete()

is fundamentally not a deadlock, we don't need anything extra.

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

#1727080

On Tue, Sep 05, 2017 at 03:46:43PM +0200, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 07:58:38PM +0900, Byungchul Park wrote:
> > On Tue, Sep 05, 2017 at 07:31:44PM +0900, Byungchul Park wrote:
> > > Recursive-read and the hint I proposed(a.k.a. might) should be used for
> > > their different specific applications. Both meaning and constraints of
> > > them are totally different.
> > > 
> > > Using a right function semantically is more important than making it
> > > just work, as you know. Wrong?
> 
> > Of course, in the following cases, the results are same:
> > 
> >    recursive-read(A) -> recursive-read(A), is like nothing, and also
> >    might(A)          -> might(A)         , is like nothing.
> > 
> >    recursive-read(A) -> lock(A), end in a deadlock, and also
> >    might(A)          -> lock(A), end in a deadlock.
> 
> And these are exactly the cases we need.
> 
> > Futhermore, recursive-read-might() can be used if needed, since their
> > semantics are orthogonal so they can be used in mixed forms.
> > 
> > I really hope you accept the new semantics... I think current workqueue
> > code exactly needs the semantics.
> 
> I really don't want to introduce this extra state if we don't have to.

OK. If the workqueue is only user of the weird lockdep annotations, then
it might be better to defer introducing the extra state until needed.

But, the 'might' thing I introduced would be necessary if more users
want to report deadlocks at the time for crosslocks with speculative
acquisitions like the workqueue does, since the recursive-read thing
would generate false dependencies much more than we want, while the
'might' thing generate them just as many as we want.

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

#1727093

[Multipart message — attachments visible in raw view] — view raw

On Wed, Sep 06, 2017 at 08:52:35AM +0900, Byungchul Park wrote:
> On Tue, Sep 05, 2017 at 03:46:43PM +0200, Peter Zijlstra wrote:
> > On Tue, Sep 05, 2017 at 07:58:38PM +0900, Byungchul Park wrote:
> > > On Tue, Sep 05, 2017 at 07:31:44PM +0900, Byungchul Park wrote:
> > > > Recursive-read and the hint I proposed(a.k.a. might) should be used for
> > > > their different specific applications. Both meaning and constraints of
> > > > them are totally different.
> > > > 
> > > > Using a right function semantically is more important than making it
> > > > just work, as you know. Wrong?
> > 
> > > Of course, in the following cases, the results are same:
> > > 
> > >    recursive-read(A) -> recursive-read(A), is like nothing, and also
> > >    might(A)          -> might(A)         , is like nothing.
> > > 
> > >    recursive-read(A) -> lock(A), end in a deadlock, and also
> > >    might(A)          -> lock(A), end in a deadlock.
> > 
> > And these are exactly the cases we need.
> > 
> > > Futhermore, recursive-read-might() can be used if needed, since their
> > > semantics are orthogonal so they can be used in mixed forms.
> > > 
> > > I really hope you accept the new semantics... I think current workqueue
> > > code exactly needs the semantics.
> > 
> > I really don't want to introduce this extra state if we don't have to.
> 
> OK. If the workqueue is only user of the weird lockdep annotations, then
> it might be better to defer introducing the extra state until needed.
> 
> But, the 'might' thing I introduced would be necessary if more users
> want to report deadlocks at the time for crosslocks with speculative
> acquisitions like the workqueue does, since the recursive-read thing
> would generate false dependencies much more than we want, while the

What do you mean by "false dependencies"? AFAICT, recursive-read could
have dependencies to the following cross commit, for example:

	A(a)
			ARR(a)
			RRR(a)
	WFC(X)
			C(X)

This is a deadlock, no?

In my upcoming v2 for recursive-read support, I'm going to make this
detectable. But please note as crossrelease doesn't have any selftests
as normal lockdep stuffs, I may miss something subtle.

Regards,
Boqun

> 'might' thing generate them just as many as we want.
> 

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

#1727110

On Wed, Sep 06, 2017 at 08:42:11AM +0800, Boqun Feng wrote:
> > OK. If the workqueue is only user of the weird lockdep annotations, then
> > it might be better to defer introducing the extra state until needed.
> > 
> > But, the 'might' thing I introduced would be necessary if more users
> > want to report deadlocks at the time for crosslocks with speculative
> > acquisitions like the workqueue does, since the recursive-read thing
> > would generate false dependencies much more than we want, while the
> 
> What do you mean by "false dependencies"? AFAICT, recursive-read could

All locks used in every work->func() generate false dependencies with
'work' and 'wq', while any flush works are not involved. It's inevitable.

Moreover, it's also possible to generate more false ones between the
pseudo acquisitions, if real acquisitions are used for that speculative
purpose e.i. recursive-read here, which are anyway real ones.

Moreover, it's also possible to generate more false ones between holding
locks and the pseudo ones, of course, the workqueue code is not the case
for now.

Moreover, it's also possible to generate more false ones between the
pseudo ones and crosslocks on commit, once making crossrelease work even
for recursive-read things.

Whatever...

Only thing I want to say is, don't make code just work, but make code
use right ones semantically for its specific application. Otherwise,
we cannot avoid side effects we don't expect. Of course, these side
effects might be not visible at the moment, IOW, generating false
dependencies might be not problems at the moment, but I just want to
avoid not doing something in the right way, if possible. That's all.

> have dependencies to the following cross commit, for example:
> 
> 	A(a)
> 			ARR(a)
> 			RRR(a)
> 	WFC(X)
> 			C(X)
> 
> This is a deadlock, no?

This is obviously a deadlock.

> In my upcoming v2 for recursive-read support, I'm going to make this
> detectable. But please note as crossrelease doesn't have any selftests

I hope you make the recursive-read things work successfully.

> as normal lockdep stuffs, I may miss something subtle.

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

#1727828

On Wed, Sep 06, 2017 at 10:32:54AM +0900, Byungchul Park wrote:
> Moreover, it's also possible to generate more false ones between the
> pseudo acquisitions, if real acquisitions are used for that speculative
> purpose e.i. recursive-read here, which are anyway real ones.

Of course, this problem can be ignored if we *only* use recursive-read
acquisitions for the speculative purpose, though current workqueue
code uses both recursive-read and normal(write) for that.

IOW, as long as we leave the write acquisions for that purpose, this
would still be a problem.

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

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web