locking/lockdep: A few cleanups

PATCH New Frederic Weisbecker 2 participants 5 messages Dec 28, 2018 – Jan 21, 2019 Locking primitives

lore

Share & tools

lore

Frederic Weisbecker

author

2018-12-28 05:02 UTC #0 lore raw

cover locking/lockdep: A few cleanups

Just a few simplification and code cleanup.

Frederic Weisbecker (2):
locking/lockdep: Simplify mark_held_locks() locking/lockdep: Provide enum lock_usage_bit mask names

 kernel/locking/lockdep.c           | 54 +++++++++++++-------------------------
 kernel/locking/lockdep_internals.h |  4 +++
 2 files changed, 22 insertions(+), 36 deletions(-)

Signature · 1 line

2.7.4

Frederic Weisbecker

author

2018-12-28 05:02 UTC #3 lore raw

PATCH 2/2 locking/lockdep: Provide enum lock_usage_bit mask names

It makes the code more self-explanatory and tells throughout the code what magic number refers to:

* state (Hardirq/Softirq)
* direction (used in or enabled above state)
* read or write

We can even remove some comments that were compensating for the lack of those constant names.

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Ingo Molnar <[email protected]>

---
 kernel/locking/lockdep.c           | 33 +++++++++++----------------------
 kernel/locking/lockdep_internals.h |  4 ++++
 2 files changed, 15 insertions(+), 22 deletions(-)

kernel/locking/lockdep.c +11 ~~-22~~

index 118a554..115d43e 100644
@@ -1624,29 +1624,18 @@ static const char *state_rnames[] = {
 
 static inline const char *state_name(enum lock_usage_bit bit)
 {
-	return (bit & 1) ? state_rnames[bit >> 2] : state_names[bit >> 2];
+	return (bit & LOCK_USAGE_READ_MASK) ? state_rnames[bit >> 2] : state_names[bit >> 2];
 }
 
 static int exclusive_bit(int new_bit)
 {
-	/*
-	 * USED_IN
-	 * USED_IN_READ
-	 * ENABLED
-	 * ENABLED_READ
-	 *
-	 * bit 0 - write/read
-	 * bit 1 - used_in/enabled
-	 * bit 2+  state
-	 */
-
-	int state = new_bit & ~3;
-	int dir = new_bit & 2;
+	int state = new_bit & LOCK_USAGE_STATE_MASK;
+	int dir = new_bit & LOCK_USAGE_DIR_MASK;
 
 	/*
 	 * keep state, bit flip the direction and strip read.
 	 */
-	return state | (dir ^ 2);
+	return state | (dir ^ LOCK_USAGE_DIR_MASK);
 }
 
 static int check_irq_usage(struct task_struct *curr, struct held_lock *prev,

@@ -2662,8 +2651,8 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this,
 		enum lock_usage_bit new_bit)
 {
 	int excl_bit = exclusive_bit(new_bit);
-	int read = new_bit & 1;
-	int dir = new_bit & 2;
+	int read = new_bit & LOCK_USAGE_READ_MASK;
+	int dir = new_bit & LOCK_USAGE_DIR_MASK;
 
 	/*
 	 * mark USED_IN has to look forwards -- to ensure no dependency

@@ -2687,19 +2676,19 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this,
 	 * states.
 	 */
 	if ((!read || !dir || STRICT_READ_CHECKS) &&
-			!usage(curr, this, excl_bit, state_name(new_bit & ~1)))
+			!usage(curr, this, excl_bit, state_name(new_bit & ~LOCK_USAGE_READ_MASK)))
 		return 0;
 
 	/*
 	 * Check for read in write conflicts
 	 */
 	if (!read) {
-		if (!valid_state(curr, this, new_bit, excl_bit + 1))
+		if (!valid_state(curr, this, new_bit, excl_bit + LOCK_USAGE_READ_MASK))
 			return 0;
 
 		if (STRICT_READ_CHECKS &&
-			!usage(curr, this, excl_bit + 1,
-				state_name(new_bit + 1)))
+			!usage(curr, this, excl_bit + LOCK_USAGE_READ_MASK,
+				state_name(new_bit + LOCK_USAGE_READ_MASK)))
 			return 0;
 	}

@@ -2723,7 +2712,7 @@ mark_held_locks(struct task_struct *curr, enum lock_usage_bit base_bit)
 		hlock = curr->held_locks + i;
 
 		if (hlock->read)
-			hlock_bit += 1; /* READ */
+			hlock_bit += LOCK_USAGE_READ_MASK;
 
 		BUG_ON(hlock_bit >= LOCK_USAGE_STATES);

kernel/locking/lockdep_internals.h +4 -0

index 88c847a..2ebb9d0 100644
@@ -22,6 +22,10 @@ enum lock_usage_bit {
 	LOCK_USAGE_STATES
 };
 
+#define LOCK_USAGE_READ_MASK 1
+#define LOCK_USAGE_DIR_MASK  2
+#define LOCK_USAGE_STATE_MASK (~(LOCK_USAGE_READ_MASK | LOCK_USAGE_DIR_MASK))
+
 /*
  * Usage-state bitmasks:
  */

Signature · 1 line

2.7.4

Frederic Weisbecker

author

2018-12-28 05:02 UTC #1 lore raw

PATCH 1/2 locking/lockdep: Simplify mark_held_locks()

In reply to Frederic Weisbecker

The enum mark_type appears a bit artificial here. We can directly pass the base enum lock_usage_bit value to mark_held_locks(). All we need then is to add the read index for each lock if necessary. It makes the code clearer.

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Ingo Molnar <[email protected]>

---
 kernel/locking/lockdep.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

kernel/locking/lockdep.c +8 ~~-15~~

index 5c837a5..118a554 100644
@@ -2709,35 +2709,28 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this,
 	return 1;
 }
 
-enum mark_type {
-#define LOCKDEP_STATE(__STATE)	__STATE,
-#include "lockdep_states.h"
-#undef LOCKDEP_STATE
-};
-
 /*
  * Mark all held locks with a usage bit:
  */
 static int
-mark_held_locks(struct task_struct *curr, enum mark_type mark)
+mark_held_locks(struct task_struct *curr, enum lock_usage_bit base_bit)
 {
-	enum lock_usage_bit usage_bit;
 	struct held_lock *hlock;
 	int i;
 
 	for (i = 0; i < curr->lockdep_depth; i++) {
+		enum lock_usage_bit hlock_bit = base_bit;
 		hlock = curr->held_locks + i;
 
-		usage_bit = 2 + (mark << 2); /* ENABLED */
 		if (hlock->read)
-			usage_bit += 1; /* READ */
+			hlock_bit += 1; /* READ */
 
-		BUG_ON(usage_bit >= LOCK_USAGE_STATES);
+		BUG_ON(hlock_bit >= LOCK_USAGE_STATES);
 
 		if (!hlock->check)
 			continue;
 
-		if (!mark_lock(curr, hlock, usage_bit))
+		if (!mark_lock(curr, hlock, hlock_bit))
 			return 0;
 	}

@@ -2758,7 +2751,7 @@ static void __trace_hardirqs_on_caller(unsigned long ip)
 	 * We are going to turn hardirqs on, so set the
 	 * usage bit for all held locks:
 	 */
-	if (!mark_held_locks(curr, HARDIRQ))
+	if (!mark_held_locks(curr, LOCK_ENABLED_HARDIRQ))
 		return;
 	/*
 	 * If we have softirqs enabled, then set the usage

@@ -2766,7 +2759,7 @@ static void __trace_hardirqs_on_caller(unsigned long ip)
 	 * this bit from being set before)
 	 */
 	if (curr->softirqs_enabled)
-		if (!mark_held_locks(curr, SOFTIRQ))
+		if (!mark_held_locks(curr, LOCK_ENABLED_SOFTIRQ))
 			return;
 
 	curr->hardirq_enable_ip = ip;

@@ -2880,7 +2873,7 @@ void trace_softirqs_on(unsigned long ip)
 	 * enabled too:
 	 */
 	if (curr->hardirqs_enabled)
-		mark_held_locks(curr, SOFTIRQ);
+		mark_held_locks(curr, LOCK_ENABLED_SOFTIRQ);
 	current->lockdep_recursion = 0;
 }

Signature · 1 line

2.7.4

tip-bot for Frederic Weisbecker

merge bot

2019-01-21 11:31 UTC #2 lore raw

[tip:locking/core] locking/lockdep: Simplify mark_held_locks()

locking/lockdep: Simplify mark_held_locks()

Commit-ID:  436a49ae7b693161c4fdf98b575ef16243dc2dfa
Gitweb:     https://git.kernel.org/tip/436a49ae7b693161c4fdf98b575ef16243dc2dfa
Author:     Frederic Weisbecker <[email protected]>
AuthorDate: Fri, 28 Dec 2018 06:02:00 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 21 Jan 2019 11:18:54 +0100

locking/lockdep: Simplify mark_held_locks()

Signed-off-by Frederic Weisbecker <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Paul E. McKenney <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Will Deacon <[email protected]>

Link https://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/locking/lockdep.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

kernel/locking/lockdep.c +8 ~~-15~~

index e805fe3bf87f..1dcd8341e35b 100644
@@ -2709,35 +2709,28 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this,
 	return 1;
 }
 
-enum mark_type {
-#define LOCKDEP_STATE(__STATE)	__STATE,
-#include "lockdep_states.h"
-#undef LOCKDEP_STATE
-};
-
 /*
  * Mark all held locks with a usage bit:
  */
 static int
-mark_held_locks(struct task_struct *curr, enum mark_type mark)
+mark_held_locks(struct task_struct *curr, enum lock_usage_bit base_bit)
 {
-	enum lock_usage_bit usage_bit;
 	struct held_lock *hlock;
 	int i;
 
 	for (i = 0; i < curr->lockdep_depth; i++) {
+		enum lock_usage_bit hlock_bit = base_bit;
 		hlock = curr->held_locks + i;
 
-		usage_bit = 2 + (mark << 2); /* ENABLED */
 		if (hlock->read)
-			usage_bit += 1; /* READ */
+			hlock_bit += 1; /* READ */
 
-		BUG_ON(usage_bit >= LOCK_USAGE_STATES);
+		BUG_ON(hlock_bit >= LOCK_USAGE_STATES);
 
 		if (!hlock->check)
 			continue;
 
-		if (!mark_lock(curr, hlock, usage_bit))
+		if (!mark_lock(curr, hlock, hlock_bit))
 			return 0;
 	}

@@ -2758,7 +2751,7 @@ static void __trace_hardirqs_on_caller(unsigned long ip)
 	 * We are going to turn hardirqs on, so set the
 	 * usage bit for all held locks:
 	 */
-	if (!mark_held_locks(curr, HARDIRQ))
+	if (!mark_held_locks(curr, LOCK_ENABLED_HARDIRQ))
 		return;
 	/*
 	 * If we have softirqs enabled, then set the usage

@@ -2766,7 +2759,7 @@ static void __trace_hardirqs_on_caller(unsigned long ip)
 	 * this bit from being set before)
 	 */
 	if (curr->softirqs_enabled)
-		if (!mark_held_locks(curr, SOFTIRQ))
+		if (!mark_held_locks(curr, LOCK_ENABLED_SOFTIRQ))
 			return;
 
 	curr->hardirq_enable_ip = ip;

@@ -2880,7 +2873,7 @@ void trace_softirqs_on(unsigned long ip)
 	 * enabled too:
 	 */
 	if (curr->hardirqs_enabled)
-		mark_held_locks(curr, SOFTIRQ);
+		mark_held_locks(curr, LOCK_ENABLED_SOFTIRQ);
 	current->lockdep_recursion = 0;
 }

tip-bot for Frederic Weisbecker

merge bot

2019-01-21 11:32 UTC #4 lore raw

[tip:locking/core] locking/lockdep: Provide enum lock_usage_bit mask names

In reply to Frederic Weisbecker

locking/lockdep: Provide enum lock_usage_bit mask names

Commit-ID:  bba2a8f1f974a45ca6ceaf688b2be7bc1c418a2f
Gitweb:     https://git.kernel.org/tip/bba2a8f1f974a45ca6ceaf688b2be7bc1c418a2f
Author:     Frederic Weisbecker <[email protected]>
AuthorDate: Fri, 28 Dec 2018 06:02:01 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 21 Jan 2019 11:18:56 +0100

locking/lockdep: Provide enum lock_usage_bit mask names

It makes the code more self-explanatory and tells throughout the code what magic number refers to:

- state (Hardirq/Softirq)
- direction (used in or enabled above state)
- read or write

We can even remove some comments that were compensating for the lack of those constant names.

Signed-off-by Frederic Weisbecker <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Paul E. McKenney <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Will Deacon <[email protected]>

Link https://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/locking/lockdep.c           | 33 +++++++++++----------------------
 kernel/locking/lockdep_internals.h |  4 ++++
 2 files changed, 15 insertions(+), 22 deletions(-)

kernel/locking/lockdep.c +11 ~~-22~~

index 1dcd8341e35b..608f74ed8bb9 100644
@@ -1624,29 +1624,18 @@ static const char *state_rnames[] = {
 
 static inline const char *state_name(enum lock_usage_bit bit)
 {
-	return (bit & 1) ? state_rnames[bit >> 2] : state_names[bit >> 2];
+	return (bit & LOCK_USAGE_READ_MASK) ? state_rnames[bit >> 2] : state_names[bit >> 2];
 }
 
 static int exclusive_bit(int new_bit)
 {
-	/*
-	 * USED_IN
-	 * USED_IN_READ
-	 * ENABLED
-	 * ENABLED_READ
-	 *
-	 * bit 0 - write/read
-	 * bit 1 - used_in/enabled
-	 * bit 2+  state
-	 */
-
-	int state = new_bit & ~3;
-	int dir = new_bit & 2;
+	int state = new_bit & LOCK_USAGE_STATE_MASK;
+	int dir = new_bit & LOCK_USAGE_DIR_MASK;
 
 	/*
 	 * keep state, bit flip the direction and strip read.
 	 */
-	return state | (dir ^ 2);
+	return state | (dir ^ LOCK_USAGE_DIR_MASK);
 }
 
 static int check_irq_usage(struct task_struct *curr, struct held_lock *prev,

@@ -2662,8 +2651,8 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this,
 		enum lock_usage_bit new_bit)
 {
 	int excl_bit = exclusive_bit(new_bit);
-	int read = new_bit & 1;
-	int dir = new_bit & 2;
+	int read = new_bit & LOCK_USAGE_READ_MASK;
+	int dir = new_bit & LOCK_USAGE_DIR_MASK;
 
 	/*
 	 * mark USED_IN has to look forwards -- to ensure no dependency

@@ -2687,19 +2676,19 @@ mark_lock_irq(struct task_struct *curr, struct held_lock *this,
 	 * states.
 	 */
 	if ((!read || !dir || STRICT_READ_CHECKS) &&
-			!usage(curr, this, excl_bit, state_name(new_bit & ~1)))
+			!usage(curr, this, excl_bit, state_name(new_bit & ~LOCK_USAGE_READ_MASK)))
 		return 0;
 
 	/*
 	 * Check for read in write conflicts
 	 */
 	if (!read) {
-		if (!valid_state(curr, this, new_bit, excl_bit + 1))
+		if (!valid_state(curr, this, new_bit, excl_bit + LOCK_USAGE_READ_MASK))
 			return 0;
 
 		if (STRICT_READ_CHECKS &&
-			!usage(curr, this, excl_bit + 1,
-				state_name(new_bit + 1)))
+			!usage(curr, this, excl_bit + LOCK_USAGE_READ_MASK,
+				state_name(new_bit + LOCK_USAGE_READ_MASK)))
 			return 0;
 	}

@@ -2723,7 +2712,7 @@ mark_held_locks(struct task_struct *curr, enum lock_usage_bit base_bit)
 		hlock = curr->held_locks + i;
 
 		if (hlock->read)
-			hlock_bit += 1; /* READ */
+			hlock_bit += LOCK_USAGE_READ_MASK;
 
 		BUG_ON(hlock_bit >= LOCK_USAGE_STATES);

kernel/locking/lockdep_internals.h +4 -0

index 88c847a41c8a..2ebb9d0ea91c 100644
@@ -22,6 +22,10 @@ enum lock_usage_bit {
 	LOCK_USAGE_STATES
 };
 
+#define LOCK_USAGE_READ_MASK 1
+#define LOCK_USAGE_DIR_MASK  2
+#define LOCK_USAGE_STATE_MASK (~(LOCK_USAGE_READ_MASK | LOCK_USAGE_DIR_MASK))
+
 /*
  * Usage-state bitmasks:
  */