[PATCH 0/2] Update lockdep doc and add section about annotations

[PATCH 0/2] Update lockdep doc and add section about annotations

[Juri Lelli]

Hi,

a couple of patches to lockdep docs.

First one is a small fix for that fact that Peter made documentation
stale (as usual :P).

Second one is an RFC. I thought that adding some info about lockdep
asserts might help spread adoption even wider (mostly regarding
lockdep_pin_lock stuff, which is pretty new and maybe got unnoticed
outside of the scheduler?).

Best,

- Juri 

Juri Lelli (2):
  Documentation/locking/lockdep: update info about states
  Documentation/locking/lockdep: Add section about available annotations

 Documentation/locking/lockdep-design.txt | 51 ++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

-- 
2.14.3

[PATCH 1/2] Documentation/locking/lockdep: update info about states

[Juri Lelli]

Commit d92a8cfcb37e ("locking/lockdep: Rework FS_RECLAIM annotation") removed
reclaim_fs lockdep STATE.

Reflect the change in documentation.

While we are at it, also clarify formula to calculate number of state
bits.

Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: linux-kernel@vger.kernel.org
Cc: linux-doc@vger.kernel.org
---
 Documentation/locking/lockdep-design.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/locking/lockdep-design.txt b/Documentation/locking/lockdep-design.txt
index 9de1c158d44c..e341c2f34e68 100644
--- a/Documentation/locking/lockdep-design.txt
+++ b/Documentation/locking/lockdep-design.txt
@@ -27,7 +27,8 @@ lock-class.
 State
 -----
 
-The validator tracks lock-class usage history into 4n + 1 separate state bits:
+The validator tracks lock-class usage history into 4 * nSTATEs + 1 separate
+state bits:
 
 - 'ever held in STATE context'
 - 'ever held as readlock in STATE context'
@@ -37,7 +38,6 @@ The validator tracks lock-class usage history into 4n + 1 separate state bits:
 Where STATE can be either one of (kernel/locking/lockdep_states.h)
  - hardirq
  - softirq
- - reclaim_fs
 
 - 'ever used'                                       [ == !unused        ]
 
-- 
2.14.3

[RFC PATCH 2/2] Documentation/locking/lockdep: Add section about available annotations

[Juri Lelli]

Add section about annotations that can be used to perform additional runtime
checking of locking correctness: assert that certain locks are held and
prevent accidental unlocking.

Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: linux-kernel@vger.kernel.org
Cc: linux-doc@vger.kernel.org
---
 Documentation/locking/lockdep-design.txt | 47 ++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/Documentation/locking/lockdep-design.txt b/Documentation/locking/lockdep-design.txt
index e341c2f34e68..74347a24efc7 100644
--- a/Documentation/locking/lockdep-design.txt
+++ b/Documentation/locking/lockdep-design.txt
@@ -169,6 +169,53 @@ Note: When changing code to use the _nested() primitives, be careful and
 check really thoroughly that the hierarchy is correctly mapped; otherwise
 you can get false positives or false negatives.
 
+Annotations
+-----------
+
+Two constructs can be used to annotate and check where and if certain locks
+must be held: lockdep_assert_held*(&lock) and lockdep_*pin_lock(&lock).
+
+As the name suggests, lockdep_assert_held* family of macros assert that a
+particular lock is held at a certain time (and generate a WARN otherwise).
+This annotation is largely used all over the kernel, e.g. kernel/sched/
+core.c
+
+  void update_rq_clock(struct rq *rq)
+  {
+  	s64 delta;
+
+  	lockdep_assert_held(&rq->lock);
+	[...]
+  }
+
+where holding rq->lock is required to safely update a rq's clock.
+
+The other family of macros is lockdep_*pin_lock, which is admittedly only
+used for rq->lock ATM. Despite their limited adoption these annotations
+generate a WARN if the lock of interest is "accidentally" unlocked. This turns
+out to be especially helpful to debug code with callbacks, where an upper
+layer assumes a lock remains taken, but a lower layer thinks it can maybe drop
+and reacquire the lock ("unwittingly" introducing races). lockdep_pin_lock
+returns a 'struct pin_cookie' that is then used by lockdep_unpin_lock to check
+that nobody tampered with the lock, e.g. kernel/sched/sched.h
+
+  static inline void rq_pin_lock(struct rq *rq, struct rq_flags *rf)
+  {
+  	rf->cookie = lockdep_pin_lock(&rq->lock);
+	[...]
+  }
+
+  static inline void rq_unpin_lock(struct rq *rq, struct rq_flags *rf)
+  {
+  	[...]
+  	lockdep_unpin_lock(&rq->lock, rf->cookie);
+  }
+
+While comments about locking requirements might provide useful information,
+the runtime checks performed by annotations are invaluable when debugging
+locking problems and they carry the same level of details when inspecting
+code.  Always prefer annotations when in doubt!
+
 Proof of 100% correctness:
 --------------------------
 
-- 
2.14.3

Re: [PATCH 0/2] Update lockdep doc and add section about annotations

[Peter Zijlstra]

On Tue, Feb 13, 2018 at 07:55:17PM +0100, Juri Lelli wrote:
> Hi,
> 
> a couple of patches to lockdep docs.
> 
> First one is a small fix for that fact that Peter made documentation
> stale (as usual :P).
> 
> Second one is an RFC. I thought that adding some info about lockdep
> asserts might help spread adoption even wider (mostly regarding
> lockdep_pin_lock stuff, which is pretty new and maybe got unnoticed
> outside of the scheduler?).

Thanks for writing that Juri!

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

[tip:locking/core] Documentation/locking/lockdep: Update info about states

[tip-bot for Juri Lelli]

Commit-ID:  e5684bbfc3f03480d6ba2150f133630fb510d3eb
Gitweb:     https://git.kernel.org/tip/e5684bbfc3f03480d6ba2150f133630fb510d3eb
Author:     Juri Lelli <juri.lelli@redhat.com>
AuthorDate: Tue, 13 Feb 2018 19:55:18 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 14 Feb 2018 12:01:22 +0100

Documentation/locking/lockdep: Update info about states

Commit:

  d92a8cfcb37e ("locking/lockdep: Rework FS_RECLAIM annotation")

removed the 'reclaim_fs' lockdep STATE.

Reflect the change in the documentation.

While we are at it, also clarify the formula to calculate the number of state bits.

Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-doc@vger.kernel.org
Link: http://lkml.kernel.org/r/20180213185519.18186-2-juri.lelli@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 Documentation/locking/lockdep-design.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/locking/lockdep-design.txt b/Documentation/locking/lockdep-design.txt
index 9de1c15..e341c2f 100644
--- a/Documentation/locking/lockdep-design.txt
+++ b/Documentation/locking/lockdep-design.txt
@@ -27,7 +27,8 @@ lock-class.
 State
 -----
 
-The validator tracks lock-class usage history into 4n + 1 separate state bits:
+The validator tracks lock-class usage history into 4 * nSTATEs + 1 separate
+state bits:
 
 - 'ever held in STATE context'
 - 'ever held as readlock in STATE context'
@@ -37,7 +38,6 @@ The validator tracks lock-class usage history into 4n + 1 separate state bits:
 Where STATE can be either one of (kernel/locking/lockdep_states.h)
  - hardirq
  - softirq
- - reclaim_fs
 
 - 'ever used'                                       [ == !unused        ]
 

[tip:locking/core] Documentation/locking/lockdep: Add section about available annotations

[tip-bot for Juri Lelli]

Commit-ID:  a1ea544fe0911492b9f8d101bcbf46cc8c47fbc5
Gitweb:     https://git.kernel.org/tip/a1ea544fe0911492b9f8d101bcbf46cc8c47fbc5
Author:     Juri Lelli <juri.lelli@redhat.com>
AuthorDate: Tue, 13 Feb 2018 19:55:19 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 14 Feb 2018 12:01:22 +0100

Documentation/locking/lockdep: Add section about available annotations

Add section about annotations that can be used to perform additional runtime
checking of locking correctness: assert that certain locks are held and
prevent accidental unlocking.

Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-doc@vger.kernel.org
Link: http://lkml.kernel.org/r/20180213185519.18186-3-juri.lelli@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 Documentation/locking/lockdep-design.txt | 47 ++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/Documentation/locking/lockdep-design.txt b/Documentation/locking/lockdep-design.txt
index e341c2f..49f58a0 100644
--- a/Documentation/locking/lockdep-design.txt
+++ b/Documentation/locking/lockdep-design.txt
@@ -169,6 +169,53 @@ Note: When changing code to use the _nested() primitives, be careful and
 check really thoroughly that the hierarchy is correctly mapped; otherwise
 you can get false positives or false negatives.
 
+Annotations
+-----------
+
+Two constructs can be used to annotate and check where and if certain locks
+must be held: lockdep_assert_held*(&lock) and lockdep_*pin_lock(&lock).
+
+As the name suggests, lockdep_assert_held* family of macros assert that a
+particular lock is held at a certain time (and generate a WARN() otherwise).
+This annotation is largely used all over the kernel, e.g. kernel/sched/
+core.c
+
+  void update_rq_clock(struct rq *rq)
+  {
+	s64 delta;
+
+	lockdep_assert_held(&rq->lock);
+	[...]
+  }
+
+where holding rq->lock is required to safely update a rq's clock.
+
+The other family of macros is lockdep_*pin_lock(), which is admittedly only
+used for rq->lock ATM. Despite their limited adoption these annotations
+generate a WARN() if the lock of interest is "accidentally" unlocked. This turns
+out to be especially helpful to debug code with callbacks, where an upper
+layer assumes a lock remains taken, but a lower layer thinks it can maybe drop
+and reacquire the lock ("unwittingly" introducing races). lockdep_pin_lock()
+returns a 'struct pin_cookie' that is then used by lockdep_unpin_lock() to check
+that nobody tampered with the lock, e.g. kernel/sched/sched.h
+
+  static inline void rq_pin_lock(struct rq *rq, struct rq_flags *rf)
+  {
+	rf->cookie = lockdep_pin_lock(&rq->lock);
+	[...]
+  }
+
+  static inline void rq_unpin_lock(struct rq *rq, struct rq_flags *rf)
+  {
+	[...]
+	lockdep_unpin_lock(&rq->lock, rf->cookie);
+  }
+
+While comments about locking requirements might provide useful information,
+the runtime checks performed by annotations are invaluable when debugging
+locking problems and they carry the same level of details when inspecting
+code.  Always prefer annotations when in doubt!
+
 Proof of 100% correctness:
 --------------------------