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
