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

Groups > linux.kernel > #1713681 > unrolled thread

[PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-17 11:00 +0200
    [PATCH v3 2/3] lockdep: Reword title of LOCKDEP_CROSSRELEASE config Byungchul Park <byungchul.park@lge.com> - 2017-08-17 11:00 +0200
      [tip:locking/core] locking/lockdep: Reword title of  LOCKDEP_CROSSRELEASE config tip-bot for Byungchul Park <tipbot@zytor.com> - 2017-08-17 12:30 +0200
    [PATCH v3 3/3] lockdep: Rename LOCKDEP_COMPLETE config Byungchul Park <byungchul.park@lge.com> - 2017-08-17 11:00 +0200
      [tip:locking/core] locking/lockdep: Rename CONFIG_LOCKDEP_COMPLETE  to CONFIG_LOCKDEP_COMPLETIONS tip-bot for Byungchul Park <tipbot@zytor.com> - 2017-08-17 12:30 +0200
    [tip:locking/core] locking/lockdep: Make  CONFIG_LOCKDEP_CROSSRELEASE part of CONFIG_PROVE_LOCKING tip-bot for Byungchul Park <tipbot@zytor.com> - 2017-08-17 12:30 +0200
      Re: [tip:locking/core] locking/lockdep: Make  CONFIG_LOCKDEP_CROSSRELEASE part of CONFIG_PROVE_LOCKING Ingo Molnar <mingo@kernel.org> - 2017-08-17 12:50 +0200
        Re: [tip:locking/core] locking/lockdep: Make  CONFIG_LOCKDEP_CROSSRELEASE part of CONFIG_PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-18 07:40 +0200
    Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-21 17:50 +0200
      Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-22 07:20 +0200
        Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 10:00 +0200
          Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-22 11:00 +0200
            Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 11:30 +0200
              Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-22 11:40 +0200
                Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 12:10 +0200
                  Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 16:00 +0200
                    Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 16:50 +0200
                      Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 17:20 +0200
                      Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Oleg Nesterov <oleg@redhat.com> - 2017-08-22 18:00 +0200
                        Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 18:40 +0200
                      Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Oleg Nesterov <oleg@redhat.com> - 2017-08-23 18:40 +0200
                        Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-23 19:50 +0200
                          Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-24 08:20 +0200
                            Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-24 09:40 +0200
                              Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-24 10:20 +0200
                                Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-25 03:20 +0200
                    Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-23 04:50 +0200
                      Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-23 08:40 +0200
                      Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-23 12:30 +0200
                        Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-24 07:10 +0200
      Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Dave Chinner <david@fromorbit.com> - 2017-08-22 07:50 +0200
        Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 11:10 +0200
          Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-22 11:30 +0200
            Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 11:40 +0200
              Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-22 11:50 +0200
              Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-23 04:20 +0200
                Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-23 08:10 +0200
                Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-23 12:30 +0200
                  Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-24 04:10 +0200
                    Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-24 09:40 +0200
          Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Dave Chinner <david@fromorbit.com> - 2017-08-22 23:20 +0200
          Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-23 04:40 +0200
            Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-23 08:20 +0200
            Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Peter Zijlstra <peterz@infradead.org> - 2017-08-23 12:50 +0200
              Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-24 07:10 +0200
        Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all  part of PROVE_LOCKING Byungchul Park <byungchul.park@lge.com> - 2017-08-23 04:00 +0200

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

#1718516 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

Peter, I am all confused and I am still trying to understand your email.
In particular, because I no longer understand the lockdep annotations in
workqueue.c, it turns out I forgot everything...

On 08/22, Peter Zijlstra wrote:
>
> I am however slightly puzzled by the need of flush_work() to take Q,
> what deadlock potential is there?

Do you really mean flush_work()? Or start_flush_work() ?

> It was added by Oleg in commit:
>
>   a67da70dc095 ("workqueues: lockdep annotations for flush_work()")

No, these annotations were moved later into start_flush, iiuc...

This

	lock_map_acquire(&work->lockdep_map);
	lock_map_release(&work->lockdep_map);

was added by another commit 0976dfc1d0cd80a4e9dfaf87bd8744612bde475a
"workqueue: Catch more locking problems with flush_work()", and at
first glance it is fine.

At the same time, I have a vague feeling that (perhaps) we can remove
these 2 annotations if we change process_one_work() and start_flush_work(),
not sure...

Oleg.

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

#1718554 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Wed, Aug 23, 2017 at 06:39:03PM +0200, Oleg Nesterov wrote:
> Peter, I am all confused and I am still trying to understand your email.
> In particular, because I no longer understand the lockdep annotations in
> workqueue.c, it turns out I forgot everything...

Yeah, that happens :/

> On 08/22, Peter Zijlstra wrote:
> >
> > I am however slightly puzzled by the need of flush_work() to take Q,
> > what deadlock potential is there?
> 
> Do you really mean flush_work()? Or start_flush_work() ?

Same thing, start_flush_work() has exactly one caller: flush_work().

> > It was added by Oleg in commit:
> >
> >   a67da70dc095 ("workqueues: lockdep annotations for flush_work()")
> 
> No, these annotations were moved later into start_flush, iiuc...
> 
> This
> 
> 	lock_map_acquire(&work->lockdep_map);
> 	lock_map_release(&work->lockdep_map);
> 
> was added by another commit 0976dfc1d0cd80a4e9dfaf87bd8744612bde475a
> "workqueue: Catch more locking problems with flush_work()", and at
> first glance it is fine.

Those are fine and are indeed the flush_work() vs work inversion.

The two straight forward annotations are:

flush_work(work)	process_one_work(wq, work)
  A(work)		  A(work)
  R(work)		  work->func(work);
			  R(work)

Which catches:

Task-1:			work:

  mutex_lock(&A);	mutex_lock(&A);
  flush_work(work);


And the analogous:

flush_workqueue(wq)	process_one_work(wq, work)
  A(wq)			  A(wq)
  R(wq)			  work->func(work);
			  R(wq)


The thing I puzzled over was flush_work() (really start_flush_work())
doing:

        if (pwq->wq->saved_max_active == 1 || pwq->wq->rescuer)
                lock_map_acquire(&pwq->wq->lockdep_map);
        else
                lock_map_acquire_read(&pwq->wq->lockdep_map);
        lock_map_release(&pwq->wq->lockdep_map);

Why does flush_work() care about the wq->lockdep_map?

The answer is because, for single-threaded workqueues, doing
flush_work() from a work is a potential deadlock:

workqueue-thread:

	work-n:
	  flush_work(work-n+1);

	work-n+1:


Will not be going anywhere fast..

And by taking the wq->lockdep_map from flush_work(), but _only_ when
single-threaded (or rescuer, see other emails), and by doing that, it
forces a recursive lock deadlock message like:

  process_one_work(wq, work)
   A(wq)
   A(work)

   work->func(work)
     flush_work(work2)
       A(work2)
       R(work2)

       A(wq) <-- recursive lock deadlock




Make sense?

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

#1718862 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Wed, Aug 23, 2017 at 07:47:14PM +0200, Peter Zijlstra wrote:
> Those are fine and are indeed the flush_work() vs work inversion.
> 
> The two straight forward annotations are:
> 
> flush_work(work)	process_one_work(wq, work)
>   A(work)		  A(work)
>   R(work)		  work->func(work);
> 			  R(work)
> 
> Which catches:
> 
> Task-1:			work:
> 
>   mutex_lock(&A);	mutex_lock(&A);
>   flush_work(work);

I'm not sure but, with LOCKDEP_COMPLETE enabled, this issue would
automatically be covered w/o additional A(work)/R(work). Right?

A(work)/R(work) seem to be used for preventing wait_for_completion()
in flush_work() from waiting for the completion forever because of the
work using mutex_lock(&A). Am I understanding correctly?

If yes, we can use just LOCKDEP_COMPLETE for that purpose.

> And the analogous:
> 
> flush_workqueue(wq)	process_one_work(wq, work)
>   A(wq)			  A(wq)
>   R(wq)			  work->func(work);
>				  (wq)

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

#1718920 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Thu, Aug 24, 2017 at 03:11:53PM +0900, Byungchul Park wrote:
> On Wed, Aug 23, 2017 at 07:47:14PM +0200, Peter Zijlstra wrote:
> > Those are fine and are indeed the flush_work() vs work inversion.
> > 
> > The two straight forward annotations are:
> > 
> > flush_work(work)	process_one_work(wq, work)
> >   A(work)		  A(work)
> >   R(work)		  work->func(work);
> > 			  R(work)
> > 
> > Which catches:
> > 
> > Task-1:			work:
> > 
> >   mutex_lock(&A);	mutex_lock(&A);
> >   flush_work(work);
> 
> I'm not sure but, with LOCKDEP_COMPLETE enabled, this issue would
> automatically be covered w/o additional A(work)/R(work). Right?
> 
> A(work)/R(work) seem to be used for preventing wait_for_completion()
> in flush_work() from waiting for the completion forever because of the
> work using mutex_lock(&A). Am I understanding correctly?
> 
> If yes, we can use just LOCKDEP_COMPLETE for that purpose.

I'm not familiar with workqueue but, the manual lockdep_map_acquire() in
workqueue code seems to be introduced to do what LOCKDEP_COMPLETE does
for wait_for_completion() and complete().

Wrong?

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

#1718957 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Thu, Aug 24, 2017 at 04:37:13PM +0900, Byungchul Park wrote:
> On Thu, Aug 24, 2017 at 03:11:53PM +0900, Byungchul Park wrote:
> > On Wed, Aug 23, 2017 at 07:47:14PM +0200, Peter Zijlstra wrote:
> > > Those are fine and are indeed the flush_work() vs work inversion.
> > > 
> > > The two straight forward annotations are:
> > > 
> > > flush_work(work)	process_one_work(wq, work)
> > >   A(work)		  A(work)
> > >   R(work)		  work->func(work);
> > > 			  R(work)
> > > 
> > > Which catches:
> > > 
> > > Task-1:			work:
> > > 
> > >   mutex_lock(&A);	mutex_lock(&A);
> > >   flush_work(work);
> > 
> > I'm not sure but, with LOCKDEP_COMPLETE enabled, this issue would
> > automatically be covered w/o additional A(work)/R(work). Right?
> > 
> > A(work)/R(work) seem to be used for preventing wait_for_completion()
> > in flush_work() from waiting for the completion forever because of the
> > work using mutex_lock(&A). Am I understanding correctly?
> > 
> > If yes, we can use just LOCKDEP_COMPLETE for that purpose.
> 
> I'm not familiar with workqueue but, the manual lockdep_map_acquire() in
> workqueue code seems to be introduced to do what LOCKDEP_COMPLETE does
> for wait_for_completion() and complete().
> 
> Wrong?

As I understand how workqueue code works more, thanks to Peterz, I get
convinced. What they want to detect with acquire(w/wq) is a deadlock
caused by wait_for_completion() mixed with typical locks.

We have to detect it with _variables_ which it actually waits for, i.e.
completion variable, neither _work_ nor _workqueue_ which we are
currently using.

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

#1719694 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Thu, Aug 24, 2017 at 05:11:01PM +0900, Byungchul Park wrote:
> On Thu, Aug 24, 2017 at 04:37:13PM +0900, Byungchul Park wrote:
> > On Thu, Aug 24, 2017 at 03:11:53PM +0900, Byungchul Park wrote:
> > > On Wed, Aug 23, 2017 at 07:47:14PM +0200, Peter Zijlstra wrote:
> > > > Those are fine and are indeed the flush_work() vs work inversion.
> > > > 
> > > > The two straight forward annotations are:
> > > > 
> > > > flush_work(work)	process_one_work(wq, work)
> > > >   A(work)		  A(work)
> > > >   R(work)		  work->func(work);
> > > > 			  R(work)
> > > > 
> > > > Which catches:
> > > > 
> > > > Task-1:			work:
> > > > 
> > > >   mutex_lock(&A);	mutex_lock(&A);
> > > >   flush_work(work);
> > > 
> > > I'm not sure but, with LOCKDEP_COMPLETE enabled, this issue would
> > > automatically be covered w/o additional A(work)/R(work). Right?
> > > 
> > > A(work)/R(work) seem to be used for preventing wait_for_completion()
> > > in flush_work() from waiting for the completion forever because of the
> > > work using mutex_lock(&A). Am I understanding correctly?
> > > 
> > > If yes, we can use just LOCKDEP_COMPLETE for that purpose.
> > 
> > I'm not familiar with workqueue but, the manual lockdep_map_acquire() in
> > workqueue code seems to be introduced to do what LOCKDEP_COMPLETE does
> > for wait_for_completion() and complete().
> > 
> > Wrong?
> 
> As I understand how workqueue code works more, thanks to Peterz, I get
> convinced. What they want to detect with acquire(w/wq) is a deadlock
> caused by wait_for_completion() mixed with typical locks.
> 
> We have to detect it with _variables_ which it actually waits for, i.e.
> completion variable, neither _work_ nor _workqueue_ which we are
> currently using.

Please read this more _carefully_.

I would be sorry if wrong,

_BUT_,

it could be a key to solve the issue of workqueue in right way, IIUC.

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

#1717991 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Tue, Aug 22, 2017 at 03:49:22PM +0200, Peter Zijlstra wrote:
> On Tue, Aug 22, 2017 at 12:08:40PM +0200, Peter Zijlstra wrote:
> 
> > > > > I meant:
> > > > > 
> > > > >  	mutex_lock(&A)
> > > > >  				<work>
> > > > >  				lockdep_map_acquire_read(&work)
> > > > >  				mutex_lock(&A)
> > > > > 
> > > > >  	lockdep_map_acquire(&work)
> > > > >  	flush_work(&work)
> > > > > 
> > > > > I mean it can still be detected with a read acquisition in work.
> > > > > Am I wrong?
> > > > 
> > > > Think so, although there's something weird with read locks that I keep
> > > > forgetting. But I'm not sure it'll actually solve the problem. But I can
> > > 
> > > I mean, read acquisitions are nothing but ones allowing read ones to be
> > > re-acquired legally, IOW, we want to check entrance of flush_work() and
> > > works, not between works. That's why I suggested to use read ones in work
> > > in that case.
> > 
> > Does seem to work.
> 
> So I think we'll end up hitting a lockdep deficiency and not trigger the
> splat on flush_work(), see also:
> 
>   https://lwn.net/Articles/332801/
> 
> lock_map_acquire_read() is a read-recursive and will not in fact create
> any dependencies because of this issue.
> 
> In specific, check_prev_add() has:
> 
> 	if (next->read == 2 || prev->read == 2)
> 		return 1;
> 
> This means that for:
> 
> 	lock_map_acquire_read(W)(2)
> 	down_write(A)		(0)
> 
> 			down_write(A)		(0)
> 			wait_for_completion(C)	(0)
> 
> 					lock_map_acquire_read(W)(2)
> 					complete(C)		(0)
> 
> All the (2) effectively go away and 'solve' our current issue, but:
> 
> 	lock_map_acquire_read(W)(2)
> 	mutex_lock(A)		(0)
> 
> 			mutex_lock(A)		(0)
> 			lock_map_acquire(W)	(0)
> 
> as per flush_work() will not in fact trigger anymore either.

It should be triggered. Lockdep code should be fixed so that it does.

> See also the below locking-selftest changes.
> 
> 
> Now, this means I also have to consider the existing
> lock_map_acquire_read() users and if they really wanted to be recursive
> or not. When I change lock_map_acquire_read() to use
> lock_acquire_shared() this annotation no longer suffices and the splat
> comes back.
> 
> 
> Also, the acquire_read() annotation will (obviously) no longer work to
> cure this problem when we switch to normal read (1), because then the
> generated chain:
> 
> 	W(1) -> A(0) -> C(0) -> W(1)

Please explain what W/A/C stand for.

> 
> spells deadlock, since W isn't allowed to recurse.
> 
> 
> /me goes dig through commit:
> 
>   e159489baa71 ("workqueue: relax lockdep annotation on flush_work()")
> 
> to figure out wth the existing users really want.
> 
> 
> [    0.000000] ----------------------------------------------------------------------------
> [    0.000000]                                  | spin |wlock |rlock |mutex | wsem | rsem |
> [    0.000000]   --------------------------------------------------------------------------
> 
> [    0.000000]   --------------------------------------------------------------------------
> [    0.000000]               recursive read-lock:             |  ok  |             |  ok  |
> [    0.000000]            recursive read-lock #2:             |  ok  |             |  ok  |
> [    0.000000]             mixed read-write-lock:             |  ok  |             |  ok  |
> [    0.000000]             mixed write-read-lock:             |  ok  |             |  ok  |
> [    0.000000]   mixed read-lock/lock-write ABBA:             |FAILED|             |  ok  |
> [    0.000000]    mixed read-lock/lock-read ABBA:             |  ok  |             |  ok  |
> [    0.000000]  mixed write-lock/lock-write ABBA:             |  ok  |             |  ok  |
> [    0.000000]   --------------------------------------------------------------------------
> 
> ---
>  lib/locking-selftest.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 116 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c
> index 6f2b135dc5e8..b99d365cf399 100644
> --- a/lib/locking-selftest.c
> +++ b/lib/locking-selftest.c
> @@ -363,6 +363,103 @@ static void rsem_AA3(void)
>  }
>  
>  /*
> + * read_lock(A)
> + * spin_lock(B)
> + *		spin_lock(B)
> + *		write_lock(A)
> + */
> +static void rlock_ABBA1(void)
> +{
> +	RL(X1);
> +	L(Y1);
> +	U(Y1);
> +	RU(X1);
> +
> +	L(Y1);
> +	WL(X1);
> +	WU(X1);
> +	U(Y1); // should fail
> +}
> +
> +static void rwsem_ABBA1(void)
> +{
> +	RSL(X1);
> +	ML(Y1);
> +	MU(Y1);
> +	RSU(X1);
> +
> +	ML(Y1);
> +	WSL(X1);
> +	WSU(X1);
> +	MU(Y1); // should fail
> +}
> +
> +/*
> + * read_lock(A)
> + * spin_lock(B)
> + *		spin_lock(B)
> + *		read_lock(A)
> + */
> +static void rlock_ABBA2(void)
> +{
> +	RL(X1);
> +	L(Y1);
> +	U(Y1);
> +	RU(X1);
> +
> +	L(Y1);
> +	RL(X1);
> +	RU(X1);
> +	U(Y1); // should NOT fail
> +}
> +
> +static void rwsem_ABBA2(void)
> +{
> +	RSL(X1);
> +	ML(Y1);
> +	MU(Y1);
> +	RSU(X1);
> +
> +	ML(Y1);
> +	RSL(X1);
> +	RSU(X1);
> +	MU(Y1); // should fail
> +}
> +
> +
> +/*
> + * write_lock(A)
> + * spin_lock(B)
> + *		spin_lock(B)
> + *		write_lock(A)
> + */
> +static void rlock_ABBA3(void)
> +{
> +	WL(X1);
> +	L(Y1);
> +	U(Y1);
> +	WU(X1);
> +
> +	L(Y1);
> +	WL(X1);
> +	WU(X1);
> +	U(Y1); // should fail
> +}
> +
> +static void rwsem_ABBA3(void)
> +{
> +	WSL(X1);
> +	ML(Y1);
> +	MU(Y1);
> +	WSU(X1);
> +
> +	ML(Y1);
> +	WSL(X1);
> +	WSU(X1);
> +	MU(Y1); // should fail
> +}
> +
> +/*
>   * ABBA deadlock:
>   */
>  
> @@ -1057,7 +1154,7 @@ static void dotest(void (*testcase_fn)(void), int expected, int lockclass_mask)
>  		unexpected_testcase_failures++;
>  		pr_cont("FAILED|");
>  
> -		dump_stack();
> +//		dump_stack();
>  	} else {
>  		testcase_successes++;
>  		pr_cont("  ok  |");
> @@ -1933,6 +2030,24 @@ void locking_selftest(void)
>  	dotest(rsem_AA3, FAILURE, LOCKTYPE_RWSEM);
>  	pr_cont("\n");
>  
> +	print_testname("mixed read-lock/lock-write ABBA");
> +	pr_cont("             |");
> +	dotest(rlock_ABBA1, FAILURE, LOCKTYPE_RWLOCK);
> +	pr_cont("             |");
> +	dotest(rwsem_ABBA1, FAILURE, LOCKTYPE_RWSEM);
> +
> +	print_testname("mixed read-lock/lock-read ABBA");
> +	pr_cont("             |");
> +	dotest(rlock_ABBA2, SUCCESS, LOCKTYPE_RWLOCK);
> +	pr_cont("             |");
> +	dotest(rwsem_ABBA2, FAILURE, LOCKTYPE_RWSEM);
> +
> +	print_testname("mixed write-lock/lock-write ABBA");
> +	pr_cont("             |");
> +	dotest(rlock_ABBA3, FAILURE, LOCKTYPE_RWLOCK);
> +	pr_cont("             |");
> +	dotest(rwsem_ABBA3, FAILURE, LOCKTYPE_RWSEM);
> +
>  	printk("  --------------------------------------------------------------------------\n");
>  
>  	/*

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

#1718078 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Wed, Aug 23, 2017 at 11:43:23AM +0900, Byungchul Park wrote:
> On Tue, Aug 22, 2017 at 03:49:22PM +0200, Peter Zijlstra wrote:
> > On Tue, Aug 22, 2017 at 12:08:40PM +0200, Peter Zijlstra wrote:
> > 
> > > > > > I meant:
> > > > > > 
> > > > > >  	mutex_lock(&A)
> > > > > >  				<work>
> > > > > >  				lockdep_map_acquire_read(&work)
> > > > > >  				mutex_lock(&A)
> > > > > > 
> > > > > >  	lockdep_map_acquire(&work)
> > > > > >  	flush_work(&work)
> > > > > > 
> > > > > > I mean it can still be detected with a read acquisition in work.
> > > > > > Am I wrong?
> > > > > 
> > > > > Think so, although there's something weird with read locks that I keep
> > > > > forgetting. But I'm not sure it'll actually solve the problem. But I can
> > > > 
> > > > I mean, read acquisitions are nothing but ones allowing read ones to be
> > > > re-acquired legally, IOW, we want to check entrance of flush_work() and
> > > > works, not between works. That's why I suggested to use read ones in work
> > > > in that case.
> > > 
> > > Does seem to work.
> > 
> > So I think we'll end up hitting a lockdep deficiency and not trigger the
> > splat on flush_work(), see also:
> > 
> >   https://lwn.net/Articles/332801/
> > 
> > lock_map_acquire_read() is a read-recursive and will not in fact create
> > any dependencies because of this issue.
> > 
> > In specific, check_prev_add() has:
> > 
> > 	if (next->read == 2 || prev->read == 2)
> > 		return 1;
> > 
> > This means that for:
> > 
> > 	lock_map_acquire_read(W)(2)
> > 	down_write(A)		(0)
> > 
> > 			down_write(A)		(0)
> > 			wait_for_completion(C)	(0)
> > 
> > 					lock_map_acquire_read(W)(2)
> > 					complete(C)		(0)
> > 
> > All the (2) effectively go away and 'solve' our current issue, but:
> > 
> > 	lock_map_acquire_read(W)(2)
> > 	mutex_lock(A)		(0)
> > 
> > 			mutex_lock(A)		(0)
> > 			lock_map_acquire(W)	(0)
> > 
> > as per flush_work() will not in fact trigger anymore either.
> 
> It should be triggered. Lockdep code should be fixed so that it does.
> 
> > See also the below locking-selftest changes.
> > 
> > 
> > Now, this means I also have to consider the existing
> > lock_map_acquire_read() users and if they really wanted to be recursive
> > or not. When I change lock_map_acquire_read() to use
> > lock_acquire_shared() this annotation no longer suffices and the splat
> > comes back.
> > 
> > 
> > Also, the acquire_read() annotation will (obviously) no longer work to
> > cure this problem when we switch to normal read (1), because then the
> > generated chain:
> > 
> > 	W(1) -> A(0) -> C(0) -> W(1)
> 
> Please explain what W/A/C stand for.

I eventually found them in your words. Let me read this again.

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

#1718228 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Wed, Aug 23, 2017 at 11:43:23AM +0900, Byungchul Park wrote:
> On Tue, Aug 22, 2017 at 03:49:22PM +0200, Peter Zijlstra wrote:

> > So I think we'll end up hitting a lockdep deficiency and not trigger the
> > splat on flush_work(), see also:
> > 
> >   https://lwn.net/Articles/332801/
> > 
> > lock_map_acquire_read() is a read-recursive and will not in fact create
> > any dependencies because of this issue.
> > 
> > In specific, check_prev_add() has:
> > 
> > 	if (next->read == 2 || prev->read == 2)
> > 		return 1;
> > 
> > This means that for:
> > 
> > 	lock_map_acquire_read(W)(2)
> > 	down_write(A)		(0)
> > 
> > 			down_write(A)		(0)
> > 			wait_for_completion(C)	(0)
> > 
> > 					lock_map_acquire_read(W)(2)
> > 					complete(C)		(0)
> > 
> > All the (2) effectively go away and 'solve' our current issue, but:
> > 
> > 	lock_map_acquire_read(W)(2)
> > 	mutex_lock(A)		(0)
> > 
> > 			mutex_lock(A)		(0)
> > 			lock_map_acquire(W)	(0)
> > 
> > as per flush_work() will not in fact trigger anymore either.
> 
> It should be triggered. Lockdep code should be fixed so that it does.

Yeah, its just something we never got around to. Once every so often I
get reminded of it, like now. But then it sits on the todo list and
never quite happens.

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

#1718837 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Wed, Aug 23, 2017 at 12:26:52PM +0200, Peter Zijlstra wrote:
> On Wed, Aug 23, 2017 at 11:43:23AM +0900, Byungchul Park wrote:
> > On Tue, Aug 22, 2017 at 03:49:22PM +0200, Peter Zijlstra wrote:
> 
> > > So I think we'll end up hitting a lockdep deficiency and not trigger the
> > > splat on flush_work(), see also:
> > > 
> > >   https://lwn.net/Articles/332801/
> > > 
> > > lock_map_acquire_read() is a read-recursive and will not in fact create
> > > any dependencies because of this issue.
> > > 
> > > In specific, check_prev_add() has:
> > > 
> > > 	if (next->read == 2 || prev->read == 2)
> > > 		return 1;
> > > 
> > > This means that for:
> > > 
> > > 	lock_map_acquire_read(W)(2)
> > > 	down_write(A)		(0)
> > > 
> > > 			down_write(A)		(0)
> > > 			wait_for_completion(C)	(0)
> > > 
> > > 					lock_map_acquire_read(W)(2)
> > > 					complete(C)		(0)
> > > 
> > > All the (2) effectively go away and 'solve' our current issue, but:
> > > 
> > > 	lock_map_acquire_read(W)(2)
> > > 	mutex_lock(A)		(0)
> > > 
> > > 			mutex_lock(A)		(0)
> > > 			lock_map_acquire(W)	(0)
> > > 
> > > as per flush_work() will not in fact trigger anymore either.
> > 
> > It should be triggered. Lockdep code should be fixed so that it does.
> 
> Yeah, its just something we never got around to. Once every so often I
> get reminded of it, like now. But then it sits on the todo list and
> never quite happens.

I want to try it.

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

#1717039 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Mon, Aug 21, 2017 at 05:46:00PM +0200, Peter Zijlstra wrote:
> 
> 
> Booting the very latest -tip on my test machine gets me the below splat.
> 
> Dave, TJ, FYI, lockdep grew annotations for completions; it remembers
> which locks were taken before we complete() and checks none of those are
> held while we wait_for_completion().
> 
> That is, something like:
> 
> 					mutex_lock(&A);
> 					mutex_unlock(&A);
> 					complete(&C);
> 
> 	mutex_lock(&A);
> 	wait_for_completion(&C);

Ok, that seems reasonable...

> Is now considered a problem. Note that in order for C to link to A it
> needs to have observed the complete() thread acquiring it after a
> wait_for_completion(), something like:
> 
> 	wait_for_completion(&C)
> 					mutex_lock(&A);
> 					mutex_unlock(&A);
> 					complete(&C);

Sure.

> 
> That is, only locks observed taken between C's wait_for_completion() and
> it's complete() are considered.
> 
> Now given the above observance rule and the fact that the below report
> is from the complete, the thing that happened appears to be:
> 
> 
> 	lockdep_map_acquire(&work->lockdep_map)
> 	down_write(&A)
> 
> 			down_write(&A)
> 			wait_for_completion(&C)
> 
> 					lockdep_map_acquire(&work_lockdep_map);
> 					complete(&C)
> 
> Which lockdep then puked over because both sides saw the same work
> class.

Let me get this straight, first. If we:

	1. take a lock in a work queue context; and
	2. in a separate context, hold that lock while we wait for a
	completion; and
	3. Run the completion from the original workqueue where we
	/once/ held that lock

lockdep will barf?

IIUC, that's a problem because XFS does this all over the place.
Let me pull the trace apart in reverse order to explain why XFS is
going to throw endless false positives on this:

> [   39.159708] -> #0 ((&ioend->io_work)){+.+.}:
> [   39.166126]        process_one_work+0x244/0x6b0
> [   39.171184]        worker_thread+0x48/0x3f0
> [   39.175845]        kthread+0x147/0x180
> [   39.180020]        ret_from_fork+0x2a/0x40
> [   39.184593]        0xffffffffffffffff

That's a data IO completion, which will take an inode lock if we
have to run a transaction to update inode size or convert an
unwritten extent.

> [   39.100611] -> #1 (&xfs_nondir_ilock_class){++++}:
> [   39.107612]        __lock_acquire+0x10a1/0x1100
> [   39.112660]        lock_acquire+0xea/0x1f0
> [   39.117224]        down_write_nested+0x26/0x60
> [   39.122184]        xfs_ilock+0x166/0x220
> [   39.126563]        __xfs_setfilesize+0x30/0x200
> [   39.131612]        xfs_setfilesize_ioend+0x7f/0xb0
> [   39.136958]        xfs_end_io+0x49/0xf0
> [   39.141240]        process_one_work+0x273/0x6b0
> [   39.146288]        worker_thread+0x48/0x3f0
> [   39.150960]        kthread+0x147/0x180
> [   39.155146]        ret_from_fork+0x2a/0x40

That's the data IO completion locking the inode inside a transaction
to update the inode size inside a workqueue.


> [   38.962397] -> #2 ((complete)&bp->b_iowait){+.+.}:
> [   38.969401]        __lock_acquire+0x10a1/0x1100
> [   38.974460]        lock_acquire+0xea/0x1f0
> [   38.979036]        wait_for_completion+0x3b/0x130
> [   38.984285]        xfs_buf_submit_wait+0xb2/0x590
> [   38.989535]        _xfs_buf_read+0x23/0x30
> [   38.994108]        xfs_buf_read_map+0x14f/0x320
> [   38.999169]        xfs_trans_read_buf_map+0x170/0x5d0
> [   39.004812]        xfs_read_agf+0x97/0x1d0
> [   39.009386]        xfs_alloc_read_agf+0x60/0x240
> [   39.014541]        xfs_alloc_fix_freelist+0x32a/0x3d0
> [   39.020180]        xfs_free_extent_fix_freelist+0x6b/0xa0
> [   39.026206]        xfs_free_extent+0x48/0x120
> [   39.031068]        xfs_trans_free_extent+0x57/0x200
> [   39.036512]        xfs_extent_free_finish_item+0x26/0x40
> [   39.042442]        xfs_defer_finish+0x174/0x770
> [   39.047494]        xfs_itruncate_extents+0x126/0x470
> [   39.053035]        xfs_setattr_size+0x2a1/0x310
> [   39.058093]        xfs_vn_setattr_size+0x57/0x160
> [   39.063342]        xfs_vn_setattr+0x50/0x70
> [   39.068015]        notify_change+0x2ee/0x420
> [   39.072785]        do_truncate+0x5d/0x90
> [   39.077165]        path_openat+0xab2/0xc00
> [   39.081737]        do_filp_open+0x8a/0xf0
> [   39.086213]        do_sys_open+0x123/0x200
> [   39.090787]        SyS_open+0x1e/0x20
> [   39.094876]        entry_SYSCALL_64_fastpath+0x23/0xc2

And that's waiting for a metadata read IO to complete during a
truncate transaction. This has no direct connection to the inode at
all - it's a allocation group header that we have to lock to do
block allocation. The completion it is waiting on doesn't even run
through the same workqueue as the ioends - ioends go through
mp->m_data_workqueue or mp->m_unwritten_workqueue, metadata buffers
go through mp->m_buf_workqueue or mp->m_log_workqueue.

So from that perspective, an ioend blocked on an inode lock should
not ever prevent metadata buffer completions from being run. Hence
I'd call this a false positive at best, though I think it really
indicates a bug in the new lockdep code because it isn't
discriminating between different workqueue contexts properly.

Even if I ignore the fact that buffer completions are run on
different workqueues, there seems to be a bigger problem with this
sort of completion checking.

That is, the trace looks plausible because we are definitely hold an
inode locked deep inside a truncate operation where the completion
if flagged.  Indeed, some transactions that would flag like this
could be holding up to 5 inodes locked and have tens of other
metadata objects locked. There are potentially tens (maybe even
hundreds) of different paths into this IO wait point, and all have
different combinations of objects locked when it triggers. So
there's massive scope for potential deadlocks....

.... and so we must have some way of avoiding this whole class of
problems that lockdep is unaware of.

Indeed, I don't think we can get stuck here because of the IO
locking rules we have specifically to avoid this sort of issue.
Having IO in flight and running completions during a truncate
operation on the same inode is a Realy Bad Thing to be doing. It's
actually a corruption and/or data exposure vector, neither of which
make users happy, and so fileystems tend to have designed in
mechanisms for avoiding these problems.

In the case of XFS, it's the data IO serialisation model that
prevents such problems. i.e. the inode locks we hold at this point
in the truncate process (i.e. the XFS_IOLOCK a.k.a i_rwsem) prevent
new IO from being run, and we don't start the truncate until we've
waited for all in progress IO to complete.  Hence while the truncate
runs and blocks on metadata IO completions, no data IO can be in
progress on that inode, so there is no completions being run on that
inode in workqueues.

And therefore the IO completion deadlock path reported by lockdep
can not actually be executed during a truncate, and so it's a false
positive.

Back to the drawing board, I guess....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

#1717205 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Tue, Aug 22, 2017 at 03:46:03PM +1000, Dave Chinner wrote:
> On Mon, Aug 21, 2017 at 05:46:00PM +0200, Peter Zijlstra wrote:

> Let me get this straight, first. If we:
> 
> 	1. take a lock in a work queue context; and
> 	2. in a separate context, hold that lock while we wait for a
> 	completion; and
> 	3. Run the completion from the original workqueue where we
> 	/once/ held that lock
> 
> lockdep will barf?
> 
> IIUC, that's a problem because XFS does this all over the place.
> Let me pull the trace apart in reverse order to explain why XFS is
> going to throw endless false positives on this:

Yeah, and I agree that's not right or desired. Just trying to figure out
how to go about fixing that.

Because, with possible exception for the single threaded workqueues,
there is no actual deadlock there unless its the very same work
instance. Its the classic instance vs class thing.

And splitting up work classes is going to be a nightmare too.

> > [   39.159708] -> #0 ((&ioend->io_work)){+.+.}:
> > [   39.166126]        process_one_work+0x244/0x6b0
> > [   39.171184]        worker_thread+0x48/0x3f0
> > [   39.175845]        kthread+0x147/0x180
> > [   39.180020]        ret_from_fork+0x2a/0x40
> > [   39.184593]        0xffffffffffffffff
> 
> That's a data IO completion, which will take an inode lock if we
> have to run a transaction to update inode size or convert an
> unwritten extent.
> 
> > [   39.100611] -> #1 (&xfs_nondir_ilock_class){++++}:
> > [   39.107612]        __lock_acquire+0x10a1/0x1100
> > [   39.112660]        lock_acquire+0xea/0x1f0
> > [   39.117224]        down_write_nested+0x26/0x60
> > [   39.122184]        xfs_ilock+0x166/0x220
> > [   39.126563]        __xfs_setfilesize+0x30/0x200
> > [   39.131612]        xfs_setfilesize_ioend+0x7f/0xb0
> > [   39.136958]        xfs_end_io+0x49/0xf0
> > [   39.141240]        process_one_work+0x273/0x6b0
> > [   39.146288]        worker_thread+0x48/0x3f0
> > [   39.150960]        kthread+0x147/0x180
> > [   39.155146]        ret_from_fork+0x2a/0x40
> 
> That's the data IO completion locking the inode inside a transaction
> to update the inode size inside a workqueue.
> 
> 
> > [   38.962397] -> #2 ((complete)&bp->b_iowait){+.+.}:
> > [   38.969401]        __lock_acquire+0x10a1/0x1100
> > [   38.974460]        lock_acquire+0xea/0x1f0
> > [   38.979036]        wait_for_completion+0x3b/0x130
> > [   38.984285]        xfs_buf_submit_wait+0xb2/0x590
> > [   38.989535]        _xfs_buf_read+0x23/0x30
> > [   38.994108]        xfs_buf_read_map+0x14f/0x320
> > [   38.999169]        xfs_trans_read_buf_map+0x170/0x5d0
> > [   39.004812]        xfs_read_agf+0x97/0x1d0
> > [   39.009386]        xfs_alloc_read_agf+0x60/0x240
> > [   39.014541]        xfs_alloc_fix_freelist+0x32a/0x3d0
> > [   39.020180]        xfs_free_extent_fix_freelist+0x6b/0xa0
> > [   39.026206]        xfs_free_extent+0x48/0x120
> > [   39.031068]        xfs_trans_free_extent+0x57/0x200
> > [   39.036512]        xfs_extent_free_finish_item+0x26/0x40
> > [   39.042442]        xfs_defer_finish+0x174/0x770
> > [   39.047494]        xfs_itruncate_extents+0x126/0x470
> > [   39.053035]        xfs_setattr_size+0x2a1/0x310
> > [   39.058093]        xfs_vn_setattr_size+0x57/0x160
> > [   39.063342]        xfs_vn_setattr+0x50/0x70
> > [   39.068015]        notify_change+0x2ee/0x420
> > [   39.072785]        do_truncate+0x5d/0x90
> > [   39.077165]        path_openat+0xab2/0xc00
> > [   39.081737]        do_filp_open+0x8a/0xf0
> > [   39.086213]        do_sys_open+0x123/0x200
> > [   39.090787]        SyS_open+0x1e/0x20
> > [   39.094876]        entry_SYSCALL_64_fastpath+0x23/0xc2
> 
> And that's waiting for a metadata read IO to complete during a
> truncate transaction. This has no direct connection to the inode at
> all - it's a allocation group header that we have to lock to do
> block allocation. The completion it is waiting on doesn't even run
> through the same workqueue as the ioends - ioends go through
> mp->m_data_workqueue or mp->m_unwritten_workqueue, metadata buffers
> go through mp->m_buf_workqueue or mp->m_log_workqueue.
> 
> So from that perspective, an ioend blocked on an inode lock should
> not ever prevent metadata buffer completions from being run. Hence
> I'd call this a false positive at best, though I think it really
> indicates a bug in the new lockdep code because it isn't
> discriminating between different workqueue contexts properly.

Agreed. The interaction with workqueues is buggered.

> Even if I ignore the fact that buffer completions are run on
> different workqueues, there seems to be a bigger problem with this
> sort of completion checking.
> 
> That is, the trace looks plausible because we are definitely hold an
> inode locked deep inside a truncate operation where the completion
> if flagged.  Indeed, some transactions that would flag like this
> could be holding up to 5 inodes locked and have tens of other
> metadata objects locked. There are potentially tens (maybe even
> hundreds) of different paths into this IO wait point, and all have
> different combinations of objects locked when it triggers. So
> there's massive scope for potential deadlocks....
> 
> .... and so we must have some way of avoiding this whole class of
> problems that lockdep is unaware of.


So I did the below little hack, which basically wipes the entire lock
history when we start a work and thereby disregards/looses the
dependency on the work 'lock'.

It makes my test box able to boot and build a kernel on XFS, so while I
see what you're saying (I think), it doesn't appear to instantly show.

Should I run xfstests or something to further verify things are OK? Does
that need a scratch partition (I keep forgetting how to run that stuff
:/).

---
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 66011c9f5df3..de91cdce9460 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4756,10 +4756,14 @@ void crossrelease_hist_start(enum xhlock_context_t c)
 {
 	struct task_struct *cur = current;
 
-	if (cur->xhlocks) {
-		cur->xhlock_idx_hist[c] = cur->xhlock_idx;
-		cur->hist_id_save[c] = cur->hist_id;
-	}
+	if (!cur->xhlocks)
+		return;
+
+	if (c == XHLOCK_PROC)
+		invalidate_xhlock(&xhlock(cur->xhlock_idx));
+
+	cur->xhlock_idx_hist[c] = cur->xhlock_idx;
+	cur->hist_id_save[c] = cur->hist_id;
 }
 
 void crossrelease_hist_end(enum xhlock_context_t c)

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

#1717217 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Tue, Aug 22, 2017 at 11:06:03AM +0200, Peter Zijlstra wrote:
> So I did the below little hack, which basically wipes the entire lock
> history when we start a work and thereby disregards/looses the
> dependency on the work 'lock'.
> 
> It makes my test box able to boot and build a kernel on XFS, so while I
> see what you're saying (I think), it doesn't appear to instantly show.
> 
> Should I run xfstests or something to further verify things are OK? Does
> that need a scratch partition (I keep forgetting how to run that stuff
> :/).
> 
> ---
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 66011c9f5df3..de91cdce9460 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -4756,10 +4756,14 @@ void crossrelease_hist_start(enum xhlock_context_t c)
>  {
>  	struct task_struct *cur = current;
>  
> -	if (cur->xhlocks) {
> -		cur->xhlock_idx_hist[c] = cur->xhlock_idx;
> -		cur->hist_id_save[c] = cur->hist_id;
> -	}
> +	if (!cur->xhlocks)
> +		return;
> +
> +	if (c == XHLOCK_PROC)
> +		invalidate_xhlock(&xhlock(cur->xhlock_idx));

We have to detect dependecies if it exists, even in the following case:

oooooooiiiiiiiiiiiiiiiiiii.........
  |<- range for commit ->|

  where
  o: acquisition outside of each work,
  i: acquisition inside of each work,

With yours, we can never detect dependecies wrt 'o'.

We have to remove false dependencies if we established them incorrectly.

I will also think it more.

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

#1717223 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Tue, Aug 22, 2017 at 06:22:36PM +0900, Byungchul Park wrote:
> On Tue, Aug 22, 2017 at 11:06:03AM +0200, Peter Zijlstra wrote:
> > So I did the below little hack, which basically wipes the entire lock
> > history when we start a work and thereby disregards/looses the
> > dependency on the work 'lock'.
> > 
> > It makes my test box able to boot and build a kernel on XFS, so while I
> > see what you're saying (I think), it doesn't appear to instantly show.
> > 
> > Should I run xfstests or something to further verify things are OK? Does
> > that need a scratch partition (I keep forgetting how to run that stuff
> > :/).
> > 
> > ---
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index 66011c9f5df3..de91cdce9460 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -4756,10 +4756,14 @@ void crossrelease_hist_start(enum xhlock_context_t c)
> >  {
> >  	struct task_struct *cur = current;
> >  
> > -	if (cur->xhlocks) {
> > -		cur->xhlock_idx_hist[c] = cur->xhlock_idx;
> > -		cur->hist_id_save[c] = cur->hist_id;
> > -	}
> > +	if (!cur->xhlocks)
> > +		return;
> > +
> > +	if (c == XHLOCK_PROC)
> > +		invalidate_xhlock(&xhlock(cur->xhlock_idx));
> 
> We have to detect dependecies if it exists, even in the following case:
> 
> oooooooiiiiiiiiiiiiiiiiiii.........
>   |<- range for commit ->|
> 
>   where
>   o: acquisition outside of each work,
>   i: acquisition inside of each work,
> 
> With yours, we can never detect dependecies wrt 'o'.

There really shouldn't be any o's when you call
crossrelease_hist_start(XHLOCK_PROC), it should denote the bottom of a
context, see:

  https://lkml.kernel.org/r/20170301104328.GD6515@twins.programming.kicks-ass.net

And in that respect you placed the calls wrongly in process_one_work(),
except it now works out nicely to disregard these fake locks.

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

#1717227 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

TJ, afaict the workqueue stuff doesn't in fact use signals, right?

Should we do the below? That is, the only reason it appears to use
INTERRUPTIBLE is to avoid increasing the loadavg, and we've introduced
IDLE for that a while back:

  80ed87c8a9ca ("sched/wait: Introduce TASK_NOLOAD and TASK_IDLE")

---
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index f128b3becfe1..737a079480e8 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2249,7 +2249,7 @@ static int worker_thread(void *__worker)
 	 * event.
 	 */
 	worker_enter_idle(worker);
-	__set_current_state(TASK_INTERRUPTIBLE);
+	__set_current_state(TASK_IDLE);
 	spin_unlock_irq(&pool->lock);
 	schedule();
 	goto woke_up;
@@ -2291,7 +2291,7 @@ static int rescuer_thread(void *__rescuer)
 	 */
 	rescuer->task->flags |= PF_WQ_WORKER;
 repeat:
-	set_current_state(TASK_INTERRUPTIBLE);
+	set_current_state(TASK_IDLE);
 
 	/*
 	 * By the time the rescuer is requested to stop, the workqueue

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

#1717979 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Tue, Aug 22, 2017 at 11:37:03AM +0200, Peter Zijlstra wrote:
> On Tue, Aug 22, 2017 at 06:22:36PM +0900, Byungchul Park wrote:
> > On Tue, Aug 22, 2017 at 11:06:03AM +0200, Peter Zijlstra wrote:
> > > So I did the below little hack, which basically wipes the entire lock
> > > history when we start a work and thereby disregards/looses the
> > > dependency on the work 'lock'.
> > > 
> > > It makes my test box able to boot and build a kernel on XFS, so while I
> > > see what you're saying (I think), it doesn't appear to instantly show.
> > > 
> > > Should I run xfstests or something to further verify things are OK? Does
> > > that need a scratch partition (I keep forgetting how to run that stuff
> > > :/).
> > > 
> > > ---
> > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > > index 66011c9f5df3..de91cdce9460 100644
> > > --- a/kernel/locking/lockdep.c
> > > +++ b/kernel/locking/lockdep.c
> > > @@ -4756,10 +4756,14 @@ void crossrelease_hist_start(enum xhlock_context_t c)
> > >  {
> > >  	struct task_struct *cur = current;
> > >  
> > > -	if (cur->xhlocks) {
> > > -		cur->xhlock_idx_hist[c] = cur->xhlock_idx;
> > > -		cur->hist_id_save[c] = cur->hist_id;
> > > -	}
> > > +	if (!cur->xhlocks)
> > > +		return;
> > > +
> > > +	if (c == XHLOCK_PROC)
> > > +		invalidate_xhlock(&xhlock(cur->xhlock_idx));
> > 
> > We have to detect dependecies if it exists, even in the following case:
> > 
> > oooooooiiiiiiiiiiiiiiiiiii.........
> >   |<- range for commit ->|
> > 
> >   where
> >   o: acquisition outside of each work,
> >   i: acquisition inside of each work,
> > 
> > With yours, we can never detect dependecies wrt 'o'.
> 
> There really shouldn't be any o's when you call

There can be any o's.

> crossrelease_hist_start(XHLOCK_PROC), it should denote the bottom of a

No, I don't think so. It can be either the bottom or not.

hist_start() and hist_end() is only for special contexts which need roll
back on exit e.g. irq, work and so on. Normal kernel context should work
well w/o hist_start() or hist_end().

> context, see:
> 
>   https://lkml.kernel.org/r/20170301104328.GD6515@twins.programming.kicks-ass.net

Actually, I don't agree with that.

> And in that respect you placed the calls wrongly in process_one_work(),

Why is it wrong? It's intended. Could you tell me why?

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

#1718059 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Wed, Aug 23, 2017 at 11:12:17AM +0900, Byungchul Park wrote:
> > > We have to detect dependecies if it exists, even in the following case:
> > > 
> > > oooooooiiiiiiiiiiiiiiiiiii.........
> > >   |<- range for commit ->|
> > > 
> > >   where
> > >   o: acquisition outside of each work,
> > >   i: acquisition inside of each work,
> > > 
> > > With yours, we can never detect dependecies wrt 'o'.
> > 
> > There really shouldn't be any o's when you call
> 
> There can be any o's.

I meant it's very possible for 'o's to exist. And we have to, of course,
consider them wrt dependencies. No reason we should ignore them.

> > crossrelease_hist_start(XHLOCK_PROC), it should denote the bottom of a

Honestly, I prefer another naming like XHLOCK_WORK or XHLOCK_SYSCALL over
XHLOCK_PROC, since the functions are for special contexts e.g. works.
But I thought XHLOCK_PROC is not bad because XHLOCK_WORK and
XHLOCK_SYSCALL does never conflict to each other and they anyway run as
normal process contexts.

Remind they are for special contexts.

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

#1718227 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Wed, Aug 23, 2017 at 11:12:17AM +0900, Byungchul Park wrote:

> > > We have to detect dependecies if it exists, even in the following case:
> > > 
> > > oooooooiiiiiiiiiiiiiiiiiii.........
> > >   |<- range for commit ->|
> > > 
> > >   where
> > >   o: acquisition outside of each work,
> > >   i: acquisition inside of each work,
> > > 
> > > With yours, we can never detect dependecies wrt 'o'.
> > 
> > There really shouldn't be any o's when you call
> 
> There can be any o's.

Yes, but they _must_ be irrelevant, see below.

> > crossrelease_hist_start(XHLOCK_PROC), it should denote the bottom of a
> 
> No, I don't think so. It can be either the bottom or not.

Nope, wrong, _must_ be bottom.

> hist_start() and hist_end() is only for special contexts which need roll
> back on exit e.g. irq, work and so on. Normal kernel context should work
> well w/o hist_start() or hist_end().

The (soft) IRQ ones, yes, the PROC one is special though.

> > context, see:
> > 
> >   https://lkml.kernel.org/r/20170301104328.GD6515@twins.programming.kicks-ass.net
> 
> Actually, I don't agree with that.
> 
> > And in that respect you placed the calls wrongly in process_one_work(),
> 
> Why is it wrong? It's intended. Could you tell me why?

The purpose of the PROC thing is to annotate _independent_ execution,
like work's.

Either they should _all_ depend on a held lock, or they should not
depend on it at all. Either way this means we should start(PROC) before
taking any locks.

Similar with history, independence means to not depend on prior state,
so per definition we should not have history at this point.

So by this reasoning the workqueue annotation should be:


	crossrelease_hist_start(XHLOCK_PROC);

	lock_map_acquire(wq->lockdep_map);
	lock_map_acquire(lockdep_map);

	work->func(work); /* go hard */

	lock_map_release(lockdep_map);
	lock_map_release(wq->lockdep_map);

	crossrelease_hist_end(XHLOCK_PROC);


This way _all_ works are guaranteed the same context, and not only those
that didn't overflow the history stack.

Now, it so happens we have an unfortunate interaction with the
flush_work{,queue}() annotations. The read-recursive thing would work if
lockdep were fixed, however I don't think there is a possible deadlock
between flush_work() and complete() (except for single-threaded
workqueues)

So until we fix lockdep for read-recursive things, I don't think its a
problem if we (ab)use your wrong placement to kill the interaction
between these two annotations.

I'll send some patches..

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

#1718776 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Wed, Aug 23, 2017 at 12:20:48PM +0200, Peter Zijlstra wrote:
> On Wed, Aug 23, 2017 at 11:12:17AM +0900, Byungchul Park wrote:
> 
> > > > We have to detect dependecies if it exists, even in the following case:
> > > > 
> > > > oooooooiiiiiiiiiiiiiiiiiii.........
> > > >   |<- range for commit ->|
> > > > 
> > > >   where
> > > >   o: acquisition outside of each work,
> > > >   i: acquisition inside of each work,
> > > > 
> > > > With yours, we can never detect dependecies wrt 'o'.
> > > 
> > > There really shouldn't be any o's when you call
> > 
> > There can be any o's.
> 
> Yes, but they _must_ be irrelevant, see below.

No, they can be relevant, see below.

> > > crossrelease_hist_start(XHLOCK_PROC), it should denote the bottom of a
> > 
> > No, I don't think so. It can be either the bottom or not.
> 
> Nope, wrong, _must_ be bottom.

No, wrong, it can be either one.

> > hist_start() and hist_end() is only for special contexts which need roll
> > back on exit e.g. irq, work and so on. Normal kernel context should work
> > well w/o hist_start() or hist_end().
> 
> The (soft) IRQ ones, yes, the PROC one is special though.
> 
> > > context, see:
> > > 
> > >   https://lkml.kernel.org/r/20170301104328.GD6515@twins.programming.kicks-ass.net
> > 
> > Actually, I don't agree with that.
> > 
> > > And in that respect you placed the calls wrongly in process_one_work(),
> > 
> > Why is it wrong? It's intended. Could you tell me why?
> 
> The purpose of the PROC thing is to annotate _independent_ execution,
> like work's.

The purpose of the PROC thing is to annotate independent execution of
work _itself_.

'o' in my examplel can be relevant and should be relevant with each
execution, that is, 'i' in my example.

> Either they should _all_ depend on a held lock, or they should not
> depend on it at all. Either way this means we should start(PROC) before
> taking any locks.

No, as I said, we don't have to start(PROC) for normal kernel contexts
before taking any locks. They should be able to do it w/o start(PROC).
That is necessary only for spectial contexts such as work.

> Similar with history, independence means to not depend on prior state,

Independence means to not depend on each other context e.i. work. IOW,
we don't have to make dependencies between 'o's and 'i's indenpendent.
The reason is simple. It's becasue the dependencies can exist.

> so per definition we should not have history at this point.
> 
> So by this reasoning the workqueue annotation should be:
> 
> 
> 	crossrelease_hist_start(XHLOCK_PROC);
> 
> 	lock_map_acquire(wq->lockdep_map);
> 	lock_map_acquire(lockdep_map);
> 
> 	work->func(work); /* go hard */
> 
> 	lock_map_release(lockdep_map);
> 	lock_map_release(wq->lockdep_map);
> 
> 	crossrelease_hist_end(XHLOCK_PROC);
>

No matter whether the lock_map_acquire()s is 'o' or 'i', it works
depending on what we want. If we want to make each lock_map_acquire()
here independent, then we should choose your way. But if we should not,
then we should keep my way unchaged.

> This way _all_ works are guaranteed the same context, and not only those

I don't understand what you intended here. This way all works become
independent each other.

> Now, it so happens we have an unfortunate interaction with the
> flush_work{,queue}() annotations. The read-recursive thing would work if
> lockdep were fixed, however I don't think there is a possible deadlock
> between flush_work() and complete() (except for single-threaded
> workqueues)

I got it. I want to work and make the read-recursive thing in lockdep
work soon.

> So until we fix lockdep for read-recursive things, I don't think its a
> problem if we (ab)use your wrong placement to kill the interaction
> between these two annotations.

I think you get confused now about start(PROC)/end(PROC). Or am I
confused wrt what you intend?

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

#1718923 — Re: [PATCH v3 1/3] lockdep: Make LOCKDEP_CROSSRELEASE configs all part of PROVE_LOCKING

On Thu, Aug 24, 2017 at 11:02:36AM +0900, Byungchul Park wrote:
> On Wed, Aug 23, 2017 at 12:20:48PM +0200, Peter Zijlstra wrote:
> > On Wed, Aug 23, 2017 at 11:12:17AM +0900, Byungchul Park wrote:
> > 
> > > > > We have to detect dependecies if it exists, even in the following case:
> > > > > 
> > > > > oooooooiiiiiiiiiiiiiiiiiii.........
> > > > >   |<- range for commit ->|
> > > > > 
> > > > >   where
> > > > >   o: acquisition outside of each work,
> > > > >   i: acquisition inside of each work,
> > > > > 
> > > > > With yours, we can never detect dependecies wrt 'o'.
> > > > 
> > > > There really shouldn't be any o's when you call
> > > 
> > > There can be any o's.
> > 
> > Yes, but they _must_ be irrelevant, see below.
> 
> No, they can be relevant, see below.

I meant we have to detect problems like, just for example:

A worker:

   acquire(A)
   process_one_work()
      acquire(B)

      crossrelease_hist_start(PROC)
      work_x->func()
         acquire(C)
         release(C)
         complete(D)
      crossrelease_hist_end(PROC)

      release(B)
   release(A)

A task:

   acquire(A)
   acquire(B)
   initiate work_x
   wait_for_completion(D)

In this case, I want to detect a problem by creating true dependencies,
which are:

A -> B
B -> C
D -> A // You are currently trying to invalidate this unnecessarily.
D -> B // You are currently trying to invalidate this unnecessarily.
D -> C

in the worker,

B -> D

in the task.

Crossrelease should detect the problem with the following chain:

A -> B -> D -> A

or

B -> D -> B

So, please keep my original code unchanged conceptially.

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

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

Back to top | Article view | linux.kernel

csiph-web