On Mon, Feb 17, 2020 at 04:12:46PM +0100, Peter Zijlstra wrote:

On Sun, Feb 16, 2020 at 01:16:36PM +0530, Amol Grover wrote:

quoted

Data is traversed using hlist_for_each_entry_rcu outside an
RCU read-side critical section but under the protection
of either lockdep_lock or with irqs disabled.

Hence, add corresponding lockdep expression to silence false-positive
lockdep warnings, and harden RCU lists. Also add macro for
corresponding lockdep expression.

Two things to note:
- RCU traversals protected under both, irqs disabled and
graph lock, have both the checks in the lockdep expression.
- RCU traversals under the protection of just disabled irqs
don't have a corresponding lockdep expression as it is implicitly
checked for.

Signed-off-by: Amol Grover <redacted>
---
 kernel/locking/lockdep.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 32282e7112d3..696ad5d4daed 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c

@@ -85,6 +85,8 @@ module_param(lock_stat, int, 0644);
  * code to recurse back into the lockdep code...
  */
 static arch_spinlock_t lockdep_lock = (arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED;
+#define graph_lock_held() \
+	arch_spin_is_locked(&lockdep_lock)
 static struct task_struct *lockdep_selftest_task_struct;
 
 static int graph_lock(void)

@@ -1009,7 +1011,7 @@ static bool __check_data_structures(void)
 	/* Check the chain_key of all lock chains. */
 	for (i = 0; i < ARRAY_SIZE(chainhash_table); i++) {
 		head = chainhash_table + i;
-		hlist_for_each_entry_rcu(chain, head, entry) {
+		hlist_for_each_entry_rcu(chain, head, entry, graph_lock_held()) {
 			if (!check_lock_chain_key(chain))
 				return false;
 		}

URGH.. this patch combines two horribles to create a horrific :/

 - spin_is_locked() is an abomination
 - this RCU list stuff is just plain annoying

I'm tempted to do something like:

#define STFU (true)

	hlist_for_each_entry_rcu(chain, head, entry, STFU) {

Paul, are we going a little over-board with this stuff? Do we really
have to annotate all of this?

Could it use hlist_for_each_entry_rcu_notrace() if that's better for this
code? That one does not need the additional condition passed. Though I find
rcu_dereference_raw_nocheck() in that macro a bit odd since it does sparse
checking, where as the rcu_dereference_raw() in hlist_for_each_entry() does
nothing.

And perf can do the same thing if it iss too annoying, like the tracing code
does.

This came up mainly because list_for_each_entry_rcu() does some checking of
it is in a reader section, but it is helpless in its checking when a lock is
held.

thanks,

 - Joel

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees