lockdep: Introduce wait-type checks

(quoted diff) +1 -2

@@ -276,8 +292,14 @@ extern void lockdep_on(void);
  * to lockdep:
  */
 
-extern void lockdep_init_map(struct lockdep_map *lock, const char *name,
-			     struct lock_class_key *key, int subclass);
+extern void lockdep_init_map_wait(struct lockdep_map *lock, const char *name,

kernel/locking/lockdep.c +21 -0

+static int check_context(struct task_struct *curr, struct held_lock *next)
+{
+	short next_inner = hlock_class(next)->wait_type_inner;
+	short next_outer = hlock_class(next)->wait_type_outer;
+	short curr_inner = LD_WAIT_MAX;
+	int depth;
+
+	if (!curr->lockdep_depth || !next_inner)
+		return 0;
+
+	if (!next_outer)
+		next_outer = next_inner;
+
+	for (depth = 0; depth < curr->lockdep_depth; depth++) {
+		struct held_lock *prev = curr->held_locks + depth;
+		short prev_inner = hlock_class(prev)->wait_type_inner;
+
+		if (prev_inner) {
+			/*
+			 * we can have a bigger inner than a previous one
+			 * when outer is smaller than inner, as with RCU.

This is really minor, but it seems you can simplify it a little bit. We do not really need curr_inner, the main loop can do

	for (...) {
		...

		if (prev_inner && prev_inner < next_outer)
			return print_lock_invalid_wait_context(...);
	}

	return 0;

Off-topic question... I can't understand the "int check" argument of lock_acquire(). First of all, __lock_acquire() does

	if (!prove_locking)
		check = 1;

Doesn't this mean lock_acquire_*() do not depend on CONFIG_PROVE_LOCKING? IOW, can't we do

	--- x/include/linux/lockdep.h
	+++ x/include/linux/lockdep.h
	@@ -479,15 +479,9 @@ static inline void print_irqtrace_events
	  * on the per lock-class debug mode:
	  */
	 
	-#ifdef CONFIG_PROVE_LOCKING
	- #define lock_acquire_exclusive(l, s, t, n, i)		lock_acquire(l, s, t, 0, 2, n, i)
	- #define lock_acquire_shared(l, s, t, n, i)		lock_acquire(l, s, t, 1, 2, n, i)
	- #define lock_acquire_shared_recursive(l, s, t, n, i)	lock_acquire(l, s, t, 2, 2, n, i)

	-#else
	- #define lock_acquire_exclusive(l, s, t, n, i)		lock_acquire(l, s, t, 0, 1, n, i)
	- #define lock_acquire_shared(l, s, t, n, i)		lock_acquire(l, s, t, 1, 1, n, i)
	- #define lock_acquire_shared_recursive(l, s, t, n, i)	lock_acquire(l, s, t, 2, 1, n, i)
	-#endif
	+#define lock_acquire_exclusive(l, s, t, n, i)		lock_acquire(l, s, t, 0, 2, n, i)
	+#define lock_acquire_shared(l, s, t, n, i)		lock_acquire(l, s, t, 1, 2, n, i)
	+#define lock_acquire_shared_recursive(l, s, t, n, i)	lock_acquire(l, s, t, 2, 2, n, i)

But what I really can't understans is what "check == 0" means? It seems that in fact it can be 1 or 2? Or, iow, "check == 0" is actually equivalent to "check == 1" ?

Quoted 4 lines · Oleg Nesterov

-#ifdef CONFIG_PROVE_LOCKING
- #define lock_acquire_exclusive(l, s, t, n, i) lock_acquire(l, s, t, 0, 2, n, i)
- #define lock_acquire_shared(l, s, t, n, i) lock_acquire(l, s, t, 1, 2, n, i)
- #define lock_acquire_shared_recursive(l, s, t, n, i) lock_acquire(l, s, t, 2, 2, n, i)

Quoted 8 lines

-#else
- #define lock_acquire_exclusive(l, s, t, n, i) lock_acquire(l, s, t, 0, 1, n, i)
- #define lock_acquire_shared(l, s, t, n, i) lock_acquire(l, s, t, 1, 1, n, i)
- #define lock_acquire_shared_recursive(l, s, t, n, i) lock_acquire(l, s, t, 2, 1, n, i)
-#endif
+#define lock_acquire_exclusive(l, s, t, n, i) lock_acquire(l, s, t, 0, 2, n, i)
+#define lock_acquire_shared(l, s, t, n, i) lock_acquire(l, s, t, 1, 2, n, i)
+#define lock_acquire_shared_recursive(l, s, t, n, i) lock_acquire(l, s, t, 2, 2, n, i)

I suppose we could; note however that the if (!prove_locking) logic was added later.

Quoted 3 lines

But what I really can't understans is what "check == 0" means? It seems that in fact it can be 1 or 2? Or, iow, "check == 0" is actually equivalent to "check == 1" ?

Hmm indeed, the comment in lockdep.h says 0 means no checks at all, but the code doesn't actually appear to work like that. I'm not sure it ever did or not, I'd have to go dig through history.

That said, given the current state it certainly looks like we can remove the check argument.

But this connects to lockdep_no_validate. Not sure I understand what this class should actually do, but consider this code:

	DEFINE_MUTEX(m1);
	DEFINE_MUTEX(m2);
	DEFINE_MUTEX(mx);

	void lockdep_should_complain(void)
	{
		lockdep_set_novalidate_class(&mx);

		// m1 -> mx -> m2
		mutex_lock(&m1);
		mutex_lock(&mx);
		mutex_lock(&m2);
		mutex_unlock(&m2);
		mutex_unlock(&mx);
		mutex_unlock(&m1);


		// m2 -> m1 ; should trigger the warning
		mutex_lock(&m2);
		mutex_lock(&m1);
		mutex_unlock(&m1);
		mutex_unlock(&m2);
	}

lockdep doesn't not detect the trivial possible deadlock.

The patch below seems to work but most probably it is not right, and I forgot everything (not too much) I knew about lockdep internals.

Oleg.

Quoted 5 lines · Oleg Nesterov

kernel/locking/lockdep.c check_prevs_add(struct task_struct *curr, struct held_lock *next) · line 1946

Quoted 9 lines

kernel/locking/lockdep.c +2 -1

@@ -1939,7 +1939,8 @@ check_prevs_add(struct task_struct *curr, struct held_lock *next)
 		 * Only non-recursive-read entries get new dependencies
 		 * added:
 		 */
-		if (hlock->read != 2) {
+		if (hlock->read != 2 &&
+		    hlock->instance->key != &__lockdep_no_validate__) {
 			if (!check_prev_add(curr, hlock, next,
 						distance, trylock_loop))
 				return 0;

Hmm, you are quite right indeed;

although I would write it like:

  if (hlock->read != 2 && hlock->check == 2)

because the __lockdep_no_validate__ thing forces the ->check value to 1.

Quoted 18 lines · Peter Zijlstra

Quoted 5 lines · Oleg Nesterov

kernel/locking/lockdep.c check_prevs_add(struct task_struct *curr, struct held_lock *next) · line 1946

Quoted 9 lines

kernel/locking/lockdep.c +2 -1

@@ -1939,7 +1939,8 @@ check_prevs_add(struct task_struct *curr, struct held_lock *next)
 		 * Only non-recursive-read entries get new dependencies
 		 * added:
 		 */
-		if (hlock->read != 2) {
+		if (hlock->read != 2 &&
+		    hlock->instance->key != &__lockdep_no_validate__) {
 			if (!check_prev_add(curr, hlock, next,
 						distance, trylock_loop))
 				return 0;

Hmm, you are quite right indeed;

Thanks!

Quoted 4 lines

although I would write it like:

  if (hlock->read != 2 && hlock->check == 2)

Quoted 1 line

because the __lockdep_no_validate__ thing forces the ->check value to 1.

Agreed, hlock->check == 2 looks better. But this connects to another patch I sent which removes hlock->check...

OK, I'll wait for review on that patch, then resend this one with ->check or __lockdep_no_validate__ depending on the result.

But with or without this change the following code

		static DEFINE_MUTEX(m1);
		static DEFINE_MUTEX(mx);

		lockdep_set_novalidate_class(&mx);

		// m1 -> mx
		mutex_lock(&m1);
		mutex_lock(&mx);
		mutex_unlock(&mx);
		mutex_unlock(&m1);

		// mx -> m1 ; should trigger the warning ???
		mutex_lock(&mx);
		mutex_lock(&m1);
		mutex_unlock(&m1);
		mutex_unlock(&mx);

doesn't trigger the warning too. This is correct because lockdep_set_novalidate_class() means, well, no-validate. The question is: do we really want to avoid all validations?

Why lockdep_set_novalidate_class() was added? Unlees I missed something the problem is that (say) __driver_attach() can take the "same" lock twice, drivers/base/ lacks annotations.

Perhaps we should change the meaning of lockdep_set_novalidate_class? (perhaps with rename). What do you think about the patch below?

With this patch __lockdep_no_validate__ means "automatically nested",

although I have to remind I can hardly understand the code I am trying to change ;)

Quoted 19 lines · Oleg Nesterov

But with or without this change the following code

		static DEFINE_MUTEX(m1);
		static DEFINE_MUTEX(mx);

		lockdep_set_novalidate_class(&mx);

		// m1 -> mx
		mutex_lock(&m1);
		mutex_lock(&mx);
		mutex_unlock(&mx);
		mutex_unlock(&m1);

		// mx -> m1 ; should trigger the warning ???
		mutex_lock(&mx);
		mutex_lock(&m1);
		mutex_unlock(&m1);
		mutex_unlock(&mx);

Quoted 3 lines

doesn't trigger the warning too. This is correct because lockdep_set_novalidate_class() means, well, no-validate. The question is: do we really want to avoid all validations?

Good question.

Quoted 3 lines

Why lockdep_set_novalidate_class() was added? Unlees I missed something the problem is that (say) __driver_attach() can take the "same" lock twice, drivers/base/ lacks annotations.

Indeed, the driver model locking always slips my mind but yes its creative. Alan Stern seems to have a good grasp on it though.

Quoted 4 lines

Perhaps we should change the meaning of lockdep_set_novalidate_class? (perhaps with rename). What do you think about the patch below?

With this patch __lockdep_no_validate__ means "automatically nested",

Yes, I suppose that might work, it would allow some validation.

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines · Oleg Nesterov

Perhaps we should change the meaning of lockdep_set_novalidate_class? (perhaps with rename). What do you think about the patch below?

With this patch __lockdep_no_validate__ means "automatically nested",

Yes, I suppose that might work, it would allow some validation.

I haven't seen the patch, but I'm not so sure it will work. Suppose we have two devices, D1 and D2, and some other mutex, M. Then the locking pattern:

	lock(D1);
	lock(M);
	unlock(M);
	unlock(D1);

generally should not conflict with:

	lock(M);
	lock(D2);
	unlock(D2);
	unlock(M);

Quoted 21 lines · Alan Stern

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines · Oleg Nesterov

Perhaps we should change the meaning of lockdep_set_novalidate_class? (perhaps with rename). What do you think about the patch below?

With this patch __lockdep_no_validate__ means "automatically nested",

Yes, I suppose that might work, it would allow some validation.

I haven't seen the patch, but I'm not so sure it will work. Suppose we have two devices, D1 and D2, and some other mutex, M. Then the locking pattern:

	lock(D1);
	lock(M);
	unlock(M);
	unlock(D1);

Quoted 6 lines

generally should not conflict with:

	lock(M);
	lock(D2);
	unlock(D2);
	unlock(M);

Yes, sure. This change assumes that the only problem in drivers/base is dev->parent->mutex / dev->mutex dependency. If the locking is even more "broken" (wrt lockdep), we can't replace lockdep_set_novalidate_class() with lockdep_set_auto_nested().

And, otoh, with this change lockdep can miss the real problems too, for example:

	func1(dev)
	{
		device_lock(dev->parent);
		mutex_lock(MUTEX);
		device_lock(dev);
		...
	}

	func2(dev)
	{
		device_lock(dev);
		mutex_lock(MUTEX);
		...
	}

lockdep will only notice dev -> MUTEX dependency.

I booted the kernel (under kvm) with this change and there is nothing in dmesg, but of course this is not the real testing.

So do you think that dev->mutex should not be validated at all ?

Just in case... Of course, if we actually add auto_nested we should not use a single class unless dev->mutex will be the only user.

Quoted 4 lines · Oleg Nesterov

Yes, sure. This change assumes that the only problem in drivers/base is dev->parent->mutex / dev->mutex dependency. If the locking is even more "broken" (wrt lockdep), we can't replace lockdep_set_novalidate_class() with lockdep_set_auto_nested().

I suspect it is even more "broken". But I can't point to specific examples.

...

My guess is that if your change is deployed widely, there will be reports of violations. That's only a guess.

Still, you could go ahead and try it, just to see what happens.

Also, take a look at commit 356c05d58af0. It's a similar situation (not exactly the same).

I tried the below but filed to see my vmlinux shrink, maybe I'm just not building the right kernel, or otherwise GCC is stupid.

Quoted 4 lines · Peter Zijlstra

I tried the below but filed to see my vmlinux shrink, maybe I'm just not building the right kernel, or otherwise GCC is stupid.

include/linux/lockdep.h static inline int lockdep_match_key(stru · line 337

Quoted 9 lines

include/linux/lockdep.h +6 -3

-extern void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
-			 int trylock, int read, int check,
-			 struct lockdep_map *nest_lock, unsigned long ip);
+struct lockdep_acquire_flags {
+	unsigned long subclass : 3;
+	unsigned long trylock  : 1;
+	unsigned long read     : 2;
+	unsigned long check    : 1;
+};

As it's only 7 bits here, could we pack them into a char ?

Quoted 16 lines · Dave Jones

Quoted 13 lines · Peter Zijlstra

I tried the below but filed to see my vmlinux shrink, maybe I'm just not building the right kernel, or otherwise GCC is stupid.

include/linux/lockdep.h +6 -3

-extern void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
-			 int trylock, int read, int check,
-			 struct lockdep_map *nest_lock, unsigned long ip);
+struct lockdep_acquire_flags {
+	unsigned long subclass : 3;
+	unsigned long trylock  : 1;
+	unsigned long read     : 2;
+	unsigned long check    : 1;
+};

Quoted 1 line

As it's only 7 bits here, could we pack them into a char ?

Do you mean __attribute__((packed)) ?

On Tue, 21 Jan 2014 19:43:10 +0100
Oleg Nesterov <[email protected]> wrote:

Quoted 1 line

Do you mean __attribute__((packed)) ?

I think Dave means:

	unsigned char subclass	: 3;
	unsigned char trylock	: 1;
	unsigned char read	: 2;
	unsigned char check	: 1;

But I agreed that the code looks simpler with bitfields, so perhaps this patch is better.

Quoted 3 lines · Oleg Nesterov

But I agreed that the code looks simpler with bitfields, so perhaps this patch is better.

Besides, I guess the major offender is rcu...

Paul, can't we do something like below? Saves 19.5 kilobytes,

	-       5255131 2974376 10125312        18354819        1181283 vmlinux
	+	5235227 2970344 10125312        18330883        117b503 vmlinux

probably we can also uninline rcu_lockdep_assert()...

Oleg.
---

include/linux/rcupdate.h +21 -8

index 2eef290..58f7a97 100644
@@ -310,18 +310,34 @@ static inline bool rcu_lockdep_current_cpu_online(void)
 }
 #endif /* #else #if defined(CONFIG_HOTPLUG_CPU) && defined(CONFIG_PROVE_RCU) */
 
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-
-static inline void rcu_lock_acquire(struct lockdep_map *map)
+static inline void __rcu_lock_acquire(struct lockdep_map *map, unsigned long ip)
 {
-	lock_acquire(map, 0, 0, 2, 0, NULL, _THIS_IP_);
+	lock_acquire(map, 0, 0, 2, 0, NULL, ip);
 }
 
-static inline void rcu_lock_release(struct lockdep_map *map)
+static inline void __rcu_lock_release(struct lockdep_map *map, unsigned long ip)
 {
 	lock_release(map, 1, _THIS_IP_);
 }
 
+#if defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PROVE_RCU)
+extern void rcu_lock_acquire(void);
+extern void rcu_lock_release(void);
+extern void rcu_lock_acquire_bh(void);
+extern void rcu_lock_release_bh(void);
+extern void rcu_lock_acquire_sched(void);
+extern void rcu_lock_release_sched(void);
+#else
+#define rcu_lock_acquire()		do { } while (0)
+#define rcu_lock_release()		do { } while (0)
+#define rcu_lock_acquire_bh()		do { } while (0)
+#define rcu_lock_release_bh()		do { } while (0)
+#define rcu_lock_acquire_sched()	do { } while (0)
+#define rcu_lock_release_sched()	do { } while (0)
+#endif
+
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+
 extern struct lockdep_map rcu_lock_map;
 extern struct lockdep_map rcu_bh_lock_map;
 extern struct lockdep_map rcu_sched_lock_map;

@@ -419,9 +435,6 @@ static inline int rcu_read_lock_sched_held(void)
 
 #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
-# define rcu_lock_acquire(a)		do { } while (0)
-# define rcu_lock_release(a)		do { } while (0)
-
 static inline int rcu_read_lock_held(void)
 {
 	return 1;

include/linux/rcupdate.h +6 -12

 }
 
 /*

@@ -790,11 +801,9 @@ static inline void rcu_read_lock(void)
  */
 static inline void rcu_read_unlock(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock() used illegally while idle");
-	rcu_lock_release(&rcu_lock_map);
-	__release(RCU);
+	rcu_lock_release();
 	__rcu_read_unlock();
+	__release(RCU);
 }
 
 /**

@@ -816,11 +825,9 @@ static inline void rcu_read_unlock(void)
  */
 static inline void rcu_read_lock_bh(void)
 {
-	local_bh_disable();
 	__acquire(RCU_BH);
-	rcu_lock_acquire(&rcu_bh_lock_map);
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock_bh() used illegally while idle");
+	local_bh_disable();
+	rcu_lock_acquire_bh();
 }
 
 /*

@@ -830,11 +837,9 @@ static inline void rcu_read_lock_bh(void)
  */
 static inline void rcu_read_unlock_bh(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock_bh() used illegally while idle");
-	rcu_lock_release(&rcu_bh_lock_map);
-	__release(RCU_BH);
+	rcu_lock_release_bh();
 	local_bh_enable();
+	__release(RCU_BH);
 }
 
 /**

include/linux/rcupdate.h +3 -5

 }
 
 /*

@@ -873,18 +878,16 @@ static inline notrace void rcu_read_lock_sched_notrace(void)
  */
 static inline void rcu_read_unlock_sched(void)
 {
-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_unlock_sched() used illegally while idle");
-	rcu_lock_release(&rcu_sched_lock_map);
-	__release(RCU_SCHED);
+	rcu_lock_release_sched();
 	preempt_enable();
+	__release(RCU_SCHED);
 }
 
 /* Used by lockdep and tracing: cannot be traced, cannot call lockdep. */
 static inline notrace void rcu_read_unlock_sched_notrace(void)
 {
-	__release(RCU_SCHED);
 	preempt_enable_notrace();
+	__release(RCU_SCHED);
 }
 
 /**

include/linux/srcu.h +0 -0

index 9b058ee..9b0f568 100644

include/linux/srcu.h +1 -1

 	return retval;
 }
 

@@ -233,7 +233,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
 static inline void srcu_read_unlock(struct srcu_struct *sp, int idx)
 	__releases(sp)
 {
-	rcu_lock_release(&(sp)->dep_map);
+	__rcu_lock_release(&(sp)->dep_map, _THIS_IP_);
 	__srcu_read_unlock(sp, idx);
 }
 

kernel/rcu/update.c +43 -0

index a3596c8..19ff915 100644
@@ -333,4 +333,47 @@ static int __init check_cpu_stall_init(void)
 }
 early_initcall(check_cpu_stall_init);
 
+#if defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PROVE_RCU)
+
+static void ck_rcu_is_watching(const char *message)
+{
+	rcu_lockdep_assert(rcu_is_watching(), message);
+}
+
+void rcu_lock_acquire(void)
+{
+	__rcu_lock_acquire(&rcu_lock_map, _RET_IP_);
+	ck_rcu_is_watching("rcu_read_lock() used illegally while idle");
+}
+
+void rcu_lock_release(void)
+{
+	ck_rcu_is_watching("rcu_read_unlock() used illegally while idle");
+	__rcu_lock_release(&rcu_lock_map, _RET_IP_);
+}
+
+void rcu_lock_acquire_bh(void)
+{
+	__rcu_lock_acquire(&rcu_bh_lock_map, _RET_IP_);
+	ck_rcu_is_watching("rcu_read_lock_bh() used illegally while idle");
+}
+
+void rcu_lock_release_bh(void)
+{
+	ck_rcu_is_watching("rcu_read_unlock_bh() used illegally while idle");
+	__rcu_lock_release(&rcu_bh_lock_map, _RET_IP_);
+}
+void rcu_lock_acquire_sched(void)
+{
+	__rcu_lock_acquire(&rcu_sched_lock_map, _RET_IP_);
+	ck_rcu_is_watching("rcu_read_lock_sched() used illegally while idle");
+}
+
+void rcu_lock_release_sched(void)
+{
+	ck_rcu_is_watching("rcu_read_unlock_sched() used illegally while idle");
+	__rcu_lock_release(&rcu_sched_lock_map, _RET_IP_);
+}
+#endif
+
 #endif /* #ifdef CONFIG_RCU_STALL_COMMON */

Quoted 7 lines · Oleg Nesterov

Quoted 3 lines · Oleg Nesterov

But I agreed that the code looks simpler with bitfields, so perhaps this patch is better.

Besides, I guess the major offender is rcu...

Quoted 6 lines

Paul, can't we do something like below? Saves 19.5 kilobytes,

	-       5255131 2974376 10125312        18354819        1181283 vmlinux
	+	5235227 2970344 10125312        18330883        117b503 vmlinux

probably we can also uninline rcu_lockdep_assert()...

Looks mostly plausible, some questions inline below.

Quoted 5 lines

static inline void rcu_read_lock(void)
{

include/linux/rcupdate.h +0 -2

-	__rcu_read_lock();
 	__acquire(RCU);
-	rcu_lock_acquire(&rcu_lock_map);

include/linux/rcupdate.h static inline void rcu_preempt_sleep_check(void) · line 784

Quoted 4 lines

include/linux/rcupdate.h +2 -2

-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock() used illegally while idle");
+	__rcu_read_lock();
+	rcu_lock_acquire();

Not sure why __rcu_read_lock() needs to be in any particular order with respect to the sparse __acquire(RCU), but should work either way. Same question about the other reorderings of similar statements.

Quoted 4 lines

static inline void rcu_read_lock_sched(void)
{

include/linux/rcupdate.h +0 -1

-	preempt_disable();
 	__acquire(RCU_SCHED);

include/linux/rcupdate.h static inline void rcu_read_unlock_bh(void) · line 864

Quoted 5 lines

include/linux/rcupdate.h +2 -1

-	rcu_lock_acquire(&rcu_sched_lock_map);
+	preempt_disable();
+	rcu_lock_acquire_sched();
 	rcu_lockdep_assert(rcu_is_watching(),
 			   "rcu_read_lock_sched() used illegally while idle");

The above pair of lines (rcu_lockdep_assert()) should also be removed, correct?

include/linux/rcupdate.h static inline void rcu_read_lock_sched(void) · line 871

Quoted 7 lines

include/linux/rcupdate.h +1 -1

@@ -862,8 +867,8 @@ static inline void rcu_read_lock_sched(void)
 /* Used by lockdep and tracing: cannot be traced, cannot call lockdep. */
 static inline notrace void rcu_read_lock_sched_notrace(void)
 {
-	preempt_disable_notrace();
 	__acquire(RCU_SCHED);
+	preempt_disable_notrace();

I cannot help repeating myself on this one... ;-)

Why the change in order?

Quoted 17 lines

kernel/rcu/update.c +11 -0

@@ -333,4 +333,47 @@ static int __init check_cpu_stall_init(void)
 }
 early_initcall(check_cpu_stall_init);
 
+#if defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PROVE_RCU)
+
+static void ck_rcu_is_watching(const char *message)
+{
+	rcu_lockdep_assert(rcu_is_watching(), message);
+}
+
+void rcu_lock_acquire(void)
+{
+	__rcu_lock_acquire(&rcu_lock_map, _RET_IP_);
+	ck_rcu_is_watching("rcu_read_lock() used illegally while idle");

kernel/rcu/update.c static int __init check_cpu_stall_init(void) · line 353

Quoted 8 lines

kernel/rcu/update.c +7 -0

+}
+
+void rcu_lock_release(void)
+{
+	ck_rcu_is_watching("rcu_read_unlock() used illegally while idle");
+	__rcu_lock_release(&rcu_lock_map, _RET_IP_);
+}
...

Quoted 9 lines · Paul E. McKenney

Quoted 7 lines · Oleg Nesterov

Quoted 3 lines · Oleg Nesterov

But I agreed that the code looks simpler with bitfields, so perhaps this patch is better.

Besides, I guess the major offender is rcu...

Quoted 8 lines

Quoted 6 lines

Paul, can't we do something like below? Saves 19.5 kilobytes,

	-       5255131 2974376 10125312        18354819        1181283 vmlinux
	+	5235227 2970344 10125312        18330883        117b503 vmlinux

probably we can also uninline rcu_lockdep_assert()...

Looks mostly plausible, some questions inline below.

Thanks!

Quoted 5 lines

Quoted 5 lines

static inline void rcu_read_lock(void)
{

include/linux/rcupdate.h +0 -2

-	__rcu_read_lock();
 	__acquire(RCU);
-	rcu_lock_acquire(&rcu_lock_map);

Quoted 8 lines

include/linux/rcupdate.h static inline void rcu_preempt_sleep_check(void) · line 784

Quoted 4 lines

include/linux/rcupdate.h +2 -2

-	rcu_lockdep_assert(rcu_is_watching(),
-			   "rcu_read_lock() used illegally while idle");
+	__rcu_read_lock();
+	rcu_lock_acquire();

Not sure why __rcu_read_lock() needs to be in any particular order with respect to the sparse __acquire(RCU), but should work either way. Same question about the other reorderings of similar statements.

I did this unconsciously and for no reason, will revert this accidental change.

Quoted 4 lines

Quoted 4 lines

static inline void rcu_read_lock_sched(void)
{

include/linux/rcupdate.h +0 -1

-	preempt_disable();
 	__acquire(RCU_SCHED);

Quoted 8 lines

include/linux/rcupdate.h static inline void rcu_read_unlock_bh(void) · line 864

Quoted 5 lines

include/linux/rcupdate.h +2 -1

-	rcu_lock_acquire(&rcu_sched_lock_map);
+	preempt_disable();
+	rcu_lock_acquire_sched();
 	rcu_lockdep_assert(rcu_is_watching(),
 			   "rcu_read_lock_sched() used illegally while idle");

The above pair of lines (rcu_lockdep_assert()) should also be removed, correct?

yes, sure, thanks,

Quoted 11 lines

include/linux/rcupdate.h static inline void rcu_read_lock_sched(void) · line 871

Quoted 7 lines

include/linux/rcupdate.h +1 -1

@@ -862,8 +867,8 @@ static inline void rcu_read_lock_sched(void)
 /* Used by lockdep and tracing: cannot be traced, cannot call lockdep. */
 static inline notrace void rcu_read_lock_sched_notrace(void)
 {
-	preempt_disable_notrace();
 	__acquire(RCU_SCHED);
+	preempt_disable_notrace();

I cannot help repeating myself on this one... ;-)

Why the change in order?

see above ;)

Quoted 19 lines

Quoted 19 lines

kernel/rcu/update.c +13 -0

@@ -333,4 +333,47 @@ static int __init check_cpu_stall_init(void)
 }
 early_initcall(check_cpu_stall_init);
 
+#if defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PROVE_RCU)
+
+static void ck_rcu_is_watching(const char *message)
+{
+	rcu_lockdep_assert(rcu_is_watching(), message);
+}
+
+void rcu_lock_acquire(void)
+{
+	__rcu_lock_acquire(&rcu_lock_map, _RET_IP_);
+	ck_rcu_is_watching("rcu_read_lock() used illegally while idle");
+}
+

Quoted 6 lines

kernel/rcu/update.c static int __init check_cpu_stall_init(void) · line 353

Quoted 6 lines

kernel/rcu/update.c +5 -0

+void rcu_lock_release(void)
+{
+	ck_rcu_is_watching("rcu_read_unlock() used illegally while idle");
+	__rcu_lock_release(&rcu_lock_map, _RET_IP_);
+}
...

Also, this all should be exported. And I think cleanuped somehow.

Quoted 2 lines · Oleg Nesterov

Also, this all should be exported. And I think cleanuped somehow.

I would be happy to take a patch with the above issues fixed.

Quoted 8 lines · Alan Stern

Quoted 4 lines · Oleg Nesterov

Yes, sure. This change assumes that the only problem in drivers/base is dev->parent->mutex / dev->mutex dependency. If the locking is even more "broken" (wrt lockdep), we can't replace lockdep_set_novalidate_class() with lockdep_set_auto_nested().

Quoted 7 lines

I suspect it is even more "broken". But I can't point to specific examples.

...

My guess is that if your change is deployed widely, there will be reports of violations. That's only a guess.

OK, lets (try to) do this later. Let me send the changes which I hope should be fine in any case.

Quoted 1 line

Still, you could go ahead and try it, just to see what happens.

Yes, perhaps it makes sense at least to test this change and see what happens... We will see.

Quoted 2 lines

Also, take a look at commit 356c05d58af0. It's a similar situation (not exactly the same).

At first glance, can't __ATTR_IGNORE_LOCKDEP() use no_validate too ? (ignoring the fact checkpatch.pl won't be happy). This can simplify the code, it seems.

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Alan Stern

Also, take a look at commit 356c05d58af0. It's a similar situation (not exactly the same).

Quoted 3 lines

At first glance, can't __ATTR_IGNORE_LOCKDEP() use no_validate too ? (ignoring the fact checkpatch.pl won't be happy). This can simplify the code, it seems.

Well, the macro itself doesn't specify the lockdep class. That happens implicitly in sysfs_get_active(), in the call to rwsem_acquire_read(). However, it ought to be possible to change the code so that when ignore_lockdep(sd) returns nonzero, we end up using no_validate.

Quoted 8 lines

Quoted 3 lines

At first glance, can't __ATTR_IGNORE_LOCKDEP() use no_validate too ? (ignoring the fact checkpatch.pl won't be happy). This can simplify the code, it seems.

Well, the macro itself doesn't specify the lockdep class. That happens implicitly in sysfs_get_active(), in the call to rwsem_acquire_read(). However, it ought to be possible to change the code so that when ignore_lockdep(sd) returns nonzero, we end up using no_validate.

sysfs_dirent_init_lockdep() can check ->ignore_lockdep and do lockdep_set_novalidate_class(). This way sysfs_ignore_lockdep() can go away.

I guess we could even change __ATTR_IGNORE_LOCKDEP() to initialize ->key = __lockdep_no_validate__ and kill ->ignore_lockdep.

Quoted 3 lines

But what I really can't understans is what "check == 0" means? It seems that in fact it can be 1 or 2? Or, iow, "check == 0" is actually equivalent to "check == 1" ?

Hmm indeed, the comment in lockdep.h says 0 means no checks at all, but the code doesn't actually appear to work like that. I'm not sure it ever did or not, I'd have to go dig through history.

That said, given the current state it certainly looks like we can remove the check argument.

Ingo?

Quoted 8 lines · Peter Zijlstra

Quoted 3 lines

But what I really can't understans is what "check == 0" means? It seems that in fact it can be 1 or 2? Or, iow, "check == 0" is actually equivalent to "check == 1" ?

Hmm indeed, the comment in lockdep.h says 0 means no checks at all, but the code doesn't actually appear to work like that. I'm not sure it ever did or not, I'd have to go dig through history.

Quoted 4 lines

That said, given the current state it certainly looks like we can remove the check argument.

Ingo?

Agreed.

Quoted 7 lines · Oleg Nesterov

include/linux/lockdep.h +0 -0

index 92b1bfc..13bd13d 100644
@@ -252,7 +252,6 @@ struct held_lock {
 	unsigned int trylock:1;						/* 16 bits */
 

include/linux/lockdep.h struct held_lock { · line 256

Quoted 4 lines

include/linux/lockdep.h +0 -1

 	unsigned int read:2;        /* see lock_acquire() comment */
-	unsigned int check:2;       /* see lock_acquire() comment */
 	unsigned int hardirqs_off:1;
 	unsigned int references:11;					/* 32 bits */

The 'bits' comments seem to be crap, even before your patch.

The "int check" argument of lock_acquire() and held_lock->check are misleading and unneeded. This is only used as a boolean, 2 denotes "true", everything else is "false". And this boolean is always equal to prove_locking.

The only exception is __lockdep_no_validate__ which should make this condition "false" in validate_chain().

Quoted 8 lines · Oleg Nesterov

The "int check" argument of lock_acquire() and held_lock->check are misleading and unneeded. This is only used as a boolean, 2 denotes "true", everything else is "false". And this boolean is always equal to prove_locking.

The only exception is __lockdep_no_validate__ which should make this condition "false" in validate_chain().

And I missed mark_irqflags(),

Quoted 4 lines

kernel/locking/lockdep.c +0 -0

@@ -3136,7 +3130,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 	hlock->holdtime_stamp = lockstat_clock();
 #endif
 

kernel/locking/lockdep.c static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass, · line 3134

Quoted 3 lines

kernel/locking/lockdep.c +1 -1

-	if (check == 2 && !mark_irqflags(curr, hlock))
+	if (prove_locking && !mark_irqflags(curr, hlock))
 		return 0;

This change is not right, at least it is not equivalent.

And I just realized that rcu_lock_acquire() does lock_acquire(check => 1). Probably we can mark rcu_lock_map's as __lockdep_no_validate__.

Anything else I missed?

Quoted 3 lines · Oleg Nesterov

And I just realized that rcu_lock_acquire() does lock_acquire(check => 1). Probably we can mark rcu_lock_map's as __lockdep_no_validate__.

Can't, RCU needs its own classes. Otherwise it cannot tell which version of the RCU read lock its holding at just that moment.

At the very least we can reduce check to a single bit.

Quoted 22 lines · Oleg Nesterov

Quoted 8 lines · Oleg Nesterov

The "int check" argument of lock_acquire() and held_lock->check are misleading and unneeded. This is only used as a boolean, 2 denotes "true", everything else is "false". And this boolean is always equal to prove_locking.

The only exception is __lockdep_no_validate__ which should make this condition "false" in validate_chain().

And I missed mark_irqflags(),

kernel/locking/lockdep.c static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass, · line 3134

Quoted 7 lines

kernel/locking/lockdep.c +1 -1

@@ -3136,7 +3130,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
 	hlock->holdtime_stamp = lockstat_clock();
 #endif
 
-	if (check == 2 && !mark_irqflags(curr, hlock))
+	if (prove_locking && !mark_irqflags(curr, hlock))
 		return 0;

This change is not right, at least it is not equivalent.

Quoted 2 lines

And I just realized that rcu_lock_acquire() does lock_acquire(check => 1). Probably we can mark rcu_lock_map's as __lockdep_no_validate__.

Can't, RCU needs its own classes. Otherwise it cannot tell which version of the RCU read lock its holding at just that moment.

Quoted 1 line

Anything else I missed?

Nothing springs to mind, but then, I totally missed the RCU thing too.

At the very least we can reduce check to a single bit.

But could someone please explain me what should lockdep_no_validate actually do? 1704f47b5 "lockdep: Add novalidate class for dev->mutex conversion" doesn't tell which kind of warnings it tries to avoid, and it looks buggy (see another email from me).

Quoted 4 lines · Oleg Nesterov

But could someone please explain me what should lockdep_no_validate actually do? 1704f47b5 "lockdep: Add novalidate class for dev->mutex conversion" doesn't tell which kind of warnings it tries to avoid, and it looks buggy (see another email from me).

It should disable lock-order checks for the locks that set that as class. So we can still do the simple checks like detect if we free memory with a held lock in and make sure we do not return to userspace with a held lock; but the class should not participate in the lock graph.

Subject: lockdep: Introduce wait-type checks
From: Peter Zijlstra <[email protected]>
Date: Tue, 19 Nov 2013 21:45:48 +0100

This patch extends lockdep to validate lock wait-type context.

Quoted 5 lines · Peter Zijlstra

Subject: lockdep: Introduce wait-type checks
From: Peter Zijlstra <[email protected]>
Date: Tue, 19 Nov 2013 21:45:48 +0100

This patch extends lockdep to validate lock wait-type context.

ooh, a new toy.

*boom*

nitpick: Why is the backtrace printed twice ?

Quoted 5 lines · Peter Zijlstra

Subject: lockdep: Introduce wait-type checks
From: Peter Zijlstra <[email protected]>
Date: Tue, 19 Nov 2013 21:45:48 +0100

This patch extends lockdep to validate lock wait-type context.

ooh, a new toy.

*boom*

[    0.298629] =============================
[    0.298732] [ BUG: Invalid wait context ]
[    0.298834] 3.13.0-rc7+ #15 Not tainted
[    0.298935] -----------------------------
[    0.299038] swapper/0/1 is trying to lock:
[    0.299135]  (&n->list_lock){......}-{3:3}, at: [<ffffffff816dea54>] get_partial_node.isra.49+0x4d/0x228
[    0.299453] 
stack backtrace:
[    0.299608] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.13.0-rc7+ #15 
[    0.299983]  0000000000000001 ffff880243f37a00 ffffffff816dfe5b 0000000000000014
[    0.300302]  ffff880243f37a78 ffffffff8109f1f7 0000000000000000 ffff880243f37a78
[    0.300611]  0000000000000046 ffffffff81189ae3 ffffffff00000000 0000000000000046
[    0.300927] Call Trace:
[    0.301028]  [<ffffffff816dfe5b>] dump_stack+0x4e/0x7a
[    0.301128]  [<ffffffff8109f1f7>] __lock_acquire.isra.28+0x3d7/0xd80
[    0.301238]  [<ffffffff81189ae3>] ? deactivate_slab+0x3c3/0x740
[    0.301345]  [<ffffffff810a030d>] lock_acquire+0x8d/0x120
[    0.302971]  [<ffffffff816dea54>] ? get_partial_node.isra.49+0x4d/0x228
[    0.303077]  [<ffffffff816e9e3b>] _raw_spin_lock+0x3b/0x50
[    0.303183]  [<ffffffff816dea54>] ? get_partial_node.isra.49+0x4d/0x228
[    0.303290]  [<ffffffff816dea54>] get_partial_node.isra.49+0x4d/0x228
[    0.303397]  [<ffffffff810cd482>] ? __module_text_address+0x12/0x60
[    0.303502]  [<ffffffff810d35ff>] ? is_module_text_address+0x2f/0x50
[    0.303610]  [<ffffffff81074548>] ? __kernel_text_address+0x58/0x80
[    0.303717]  [<ffffffff816dedfc>] __slab_alloc+0x1cd/0x562
[    0.303821]  [<ffffffff812e86ff>] ? alloc_cpumask_var_node+0x1f/0x90
[    0.303929]  [<ffffffff8118ab6a>] kmem_cache_alloc_node_trace+0xda/0x290
[    0.304037]  [<ffffffff812e86ff>] ? alloc_cpumask_var_node+0x1f/0x90
[    0.304145]  [<ffffffff812e86ff>] alloc_cpumask_var_node+0x1f/0x90
[    0.304250]  [<ffffffff812e879e>] alloc_cpumask_var+0xe/0x10
[    0.304357]  [<ffffffff81030990>] __assign_irq_vector+0x40/0x340
[    0.304462]  [<ffffffff810324a1>] __create_irqs+0x151/0x210
[    0.304567]  [<ffffffff810325a2>] create_irq+0x22/0x30
[    0.304674]  [<ffffffff815ab25d>] dmar_set_interrupt+0x2d/0xd0
[    0.304784]  [<ffffffff81d6a532>] enable_drhd_fault_handling+0x24/0x66
[    0.304890]  [<ffffffff81d6bc7f>] irq_remap_enable_fault_handling+0x26/0x30
[    0.304999]  [<ffffffff81d2f3e0>] bsp_end_local_APIC_setup+0x18/0x1a
[    0.305106]  [<ffffffff81d2d44a>] native_smp_prepare_cpus+0x35c/0x3d3
[    0.305215]  [<ffffffff81d20f93>] kernel_init_freeable+0x124/0x26c
[    0.305321]  [<ffffffff816d6d4e>] ? kernel_init+0xe/0x130
[    0.305427]  [<ffffffff816d6d40>] ? rest_init+0xd0/0xd0

[    0.305529]  [<ffffffff816d6d4e>] kernel_init+0xe/0x130
[    0.305627]  [<ffffffff816f23ac>] ret_from_fork+0x7c/0xb0
[    0.305731]  [<ffffffff816d6d40>] ? rest_init+0xd0/0xd0
[    0.305836] 
other info that might help us debug this:
[    0.305993] 1 lock held by swapper/0/1:
[    0.306093]  #0:  (vector_lock){......}-{2:2}, at: [<ffffffff8103245c>] __create_irqs+0x10c/0x210
[    0.306444]