Re: Should list_entry_rcu use rcu_dereference ?

[Joel Fernandes <joel@joelfernandes.org>]

Hi Paul,

On Mon, May 06, 2019 at 04:54:30PM -0700, Paul E. McKenney wrote:
> > Looking at the list_entry_rcu primitive, I see it does direct READ_ONCE
> > on ptr. That's Ok, but rcu_dereference also does additional lockdep and
> > sparse checking.  Why not call rcu_dereference instead of READ_ONCE? The
> > pointer may be dereference by the caller so IMO makes sense to check.
> > 
> > Here is the definition of list_entry_rcu:
> > /**
> >  * list_entry_rcu - get the struct for this entry
> >  [snip]
> > * This primitive may safely run concurrently with the _rcu list-mutation
> >  * primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock().
> >  */
> > #define list_entry_rcu(ptr, type, member) \
> > 	container_of(READ_ONCE(ptr), type, member)
> > 
> > Also, I was curious why hlist_for_each_entry_rcu() uses rcu_dereference_raw()
> > while __hlist_for_each_rcu)_ uses rcu_dereference(). I feel both should use
> > rcu_dereference to have the lockdep checking. Is this not done due to
> > performance reasons?
> > 
> > thanks!
> > 
> >   - Joel
> 
> The issue is that most of the RCU list macros are generic over the RCU
> read-side flavors.  We could have created _bh and _sched variants of all
> of these, but that seemed like way too much RCU API expansion at the time,
> and still does.  This shows up in the sparse checking as well, so that
> there is just __rcu instead of also being __rcu_bh and __rcu_sched.
> 
> Or are do you have a trick in mind that would allow lockdep checking
> without RCU API expansion?

Sorry it took me a while to reply, I had it in mind to reply and was thinking
about how to do it.

How about something like the following? It does cause some API expansion,
however just one function: rcu_dereference_any(). The purpose of this is to
check if any RCU reader is active at all. I believe after the flavor
consolidation effort, this is a sufficient condition from an RCU perspective.
Having some lockdep checking is better than no lockdep checking so I think it
is good to have. Let me know what you think about the below patch and I can
roll into a proper patch and send it as well (with proper comments).

I was able to trigger the lockdep check by removing the preempt_disable()
calls in ftrace_mod_get_kallsym() and insert some modules (PROVE_LOCKING and
FUNCION_TRACE enabled).

---8<-----------------------

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index e91ec9ddcd30..334c625ef421 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -273,9 +273,12 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  *
  * This primitive may safely run concurrently with the _rcu list-mutation
  * primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock().
+ *
+ * Use rcu_dereference_any() to ensure we are generically within an RCU reader
+ * section (whether sched, bh or regular).
  */
 #define list_entry_rcu(ptr, type, member) \
-	container_of(READ_ONCE(ptr), type, member)
+	container_of(rcu_dereference_any(ptr), type, member)
 
 /*
  * Where are list_empty_rcu() and list_first_entry_rcu()?
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 922bb6848813..7481d93ed9bb 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -223,6 +223,7 @@ int debug_lockdep_rcu_enabled(void);
 int rcu_read_lock_held(void);
 int rcu_read_lock_bh_held(void);
 int rcu_read_lock_sched_held(void);
+int rcu_read_lock_any_held(void);
 
 #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
@@ -243,6 +244,12 @@ static inline int rcu_read_lock_sched_held(void)
 {
 	return !preemptible();
 }
+
+static inline int rcu_read_lock_any_held(void)
+{
+	return !preemptible();
+}
+
 #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
 #ifdef CONFIG_PROVE_RCU
@@ -472,6 +479,10 @@ static inline void rcu_preempt_sleep_check(void) { }
 	__rcu_dereference_check((p), (c) || rcu_read_lock_sched_held(), \
 				__rcu)
 
+#define rcu_dereference_any_check(p, c) \
+	__rcu_dereference_check((p), (c) || rcu_read_lock_any_held(), \
+				__rcu)
+
 /*
  * The tracing infrastructure traces RCU (we want that), but unfortunately
  * some of the RCU checks causes tracing to lock up the system.
@@ -525,6 +536,8 @@ static inline void rcu_preempt_sleep_check(void) { }
  */
 #define rcu_dereference_sched(p) rcu_dereference_sched_check(p, 0)
 
+#define rcu_dereference_any(p) rcu_dereference_any_check(p, 0)
+
 /**
  * rcu_pointer_handoff() - Hand off a pointer from RCU to other mechanism
  * @p: The pointer to hand off
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index c3bf44ba42e5..2dab75d34806 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -298,6 +298,31 @@ int rcu_read_lock_bh_held(void)
 }
 EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
 
+int rcu_read_lock_any_held(void)
+{
+	int lockdep_opinion = 0;
+
+	if (!debug_lockdep_rcu_enabled())
+		return 1;
+	if (!rcu_is_watching())
+		return 0;
+	if (!rcu_lockdep_current_cpu_online())
+		return 0;
+
+	if (lock_is_held(&rcu_lock_map))
+		return 1;
+
+	/* BH flavor */
+	if (in_softirq() || irqs_disabled())
+		return 1;
+
+	/* Sched flavor */
+	if (debug_locks)
+		lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
+	return lockdep_opinion || !preemptible();
+}
+EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
+
 #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
 /**
-- 
2.21.0.1020.gf2820cf01a-goog