Should list_entry_rcu use rcu_dereference ?

Should list_entry_rcu use rcu_dereference ?

[Joel Fernandes]

Hi,
Sorry if this is a silly question.

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

https://cdn-images-1.medium.com/max/1600/0*XTYvSNj_rT2UZwfm.png

Re: Should list_entry_rcu use rcu_dereference ?

[Paul E. McKenney]

On Sat, May 04, 2019 at 02:59:44PM -0400, Joel Fernandes wrote:
> Hi,
> Sorry if this is a silly question.

Not at all silly!

> 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
> 
> https://cdn-images-1.medium.com/max/1600/0*XTYvSNj_rT2UZwfm.png

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?

							Thanx, Paul

Re: Should list_entry_rcu use rcu_dereference ?

[Joel Fernandes]

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

Re: Should list_entry_rcu use rcu_dereference ?

[Paul E. McKenney]

On Sun, May 12, 2019 at 11:30:17PM -0400, Joel Fernandes wrote:
> 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().

You do also have rcu_read_lock_any_held() and rcu_dereference_any_check(),
but yes, fairly modest as API expansion goes.

>                                                   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).

The big question is "what would use these?".  Although it would be good
to replace uses of rcu_dereference_raw(), many of those are driven by a
desire to support both RCU and non-RCU use cases, where in the non-RCU
variant the user supplies the lock.

So would these actually see enough use to make their addition worthwhile?
If so, which uses are you thinking in terms of?

Sorry to be so skeptical, but it has not been that long since Linus
read me the riot act over RCU complexity.  ;-)

							Thanx, Paul

> ---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
> 

Re: Should list_entry_rcu use rcu_dereference ?

[Joel Fernandes]

On Tue, May 14, 2019 at 03:20:18PM -0700, Paul E. McKenney wrote:
> On Sun, May 12, 2019 at 11:30:17PM -0400, Joel Fernandes wrote:
> > 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().
> 
> You do also have rcu_read_lock_any_held() and rcu_dereference_any_check(),
> but yes, fairly modest as API expansion goes.
> >                                                   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).
> 
> The big question is "what would use these?".  Although it would be good
> to replace uses of rcu_dereference_raw(), many of those are driven by a
> desire to support both RCU and non-RCU use cases, where in the non-RCU
> variant the user supplies the lock.
> 
> So would these actually see enough use to make their addition worthwhile?
> If so, which uses are you thinking in terms of?

Yes true. Actually my initial intent was to add some lockdep checking for
RCU-list traversal via list_for_each_entry_rcu(). Since the pointers are
traversed without any lockdep checking due to not going through the
rcu_dereference API family.

You are right the patch I shared is complicated, and I could just keep the
rcu_read_lock_any_held() and use that in list_for_each_entry_rcu for that
matter (and in any other list RCU APIs doing list traversals). In fact I
could probably also call this new API as: lockdep_assert_rcu_held().

Also I noticed there is a rcu_lockdep_assert() mentioned in the
Requirements.html part of the documentation, but didn't find such a
definition in the kernel sources. So we should probably also update that ;-)

From Requirements.html:
	A given function might wish to check for RCU-related preconditions
	upon entry, before using any other RCU API.
	The <tt>rcu_lockdep_assert()</tt> does this job,
	asserting the expression in kernels having lockdep enabled
	and doing nothing otherwise.

> Sorry to be so skeptical, but it has not been that long since Linus
> read me the riot act over RCU complexity.  ;-)

No problem, perfectly legit skepticism ;-)

thanks,

 - Joel

> 
> 							Thanx, Paul
> 
> > ---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
> > 

Re: Should list_entry_rcu use rcu_dereference ?

[Joel Fernandes]

On Fri, May 24, 2019 at 04:07:37AM -0400, Joel Fernandes wrote:
[snip]
> > > 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).
> > 
> > The big question is "what would use these?".  Although it would be good
> > to replace uses of rcu_dereference_raw(), many of those are driven by a
> > desire to support both RCU and non-RCU use cases, where in the non-RCU
> > variant the user supplies the lock.
> > 
> > So would these actually see enough use to make their addition worthwhile?
> > If so, which uses are you thinking in terms of?
> 
> Yes true. Actually my initial intent was to add some lockdep checking for
> RCU-list traversal via list_for_each_entry_rcu(). Since the pointers are
> traversed without any lockdep checking due to not going through the
> rcu_dereference API family.
> 
> You are right the patch I shared is complicated, and I could just keep the
> rcu_read_lock_any_held() and use that in list_for_each_entry_rcu for that
> matter (and in any other list RCU APIs doing list traversals). In fact I
> could probably also call this new API as: lockdep_assert_rcu_held().
> 
> Also I noticed there is a rcu_lockdep_assert() mentioned in the
> Requirements.html part of the documentation, but didn't find such a
> definition in the kernel sources. So we should probably also update that ;-)

And I see rcu_lockdep_assert() got converted to RCU_LOCKDEP_WARN in
https://lore.kernel.org/patchwork/patch/580641/

I will toy with the idea of a lockdep_assert_rcu_held() which checks for
consolidated RCU reader sections and use that for list RCU and such, and we
can discuss more on the subsequent RFC.

thanks!


> 
> From Requirements.html:
> 	A given function might wish to check for RCU-related preconditions
> 	upon entry, before using any other RCU API.
> 	The <tt>rcu_lockdep_assert()</tt> does this job,
> 	asserting the expression in kernels having lockdep enabled
> 	and doing nothing otherwise.
> 
> > Sorry to be so skeptical, but it has not been that long since Linus
> > read me the riot act over RCU complexity.  ;-)
> 
> No problem, perfectly legit skepticism ;-)
> 
> thanks,
> 
>  - Joel
> 
> > 
> > 							Thanx, Paul
> > 
> > > ---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
> > > 

Re: Should list_entry_rcu use rcu_dereference ?

[Paul E. McKenney]

On Fri, May 24, 2019 at 12:50:09PM -0400, Joel Fernandes wrote:
> On Fri, May 24, 2019 at 04:07:37AM -0400, Joel Fernandes wrote:
> [snip]
> > > > 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).
> > > 
> > > The big question is "what would use these?".  Although it would be good
> > > to replace uses of rcu_dereference_raw(), many of those are driven by a
> > > desire to support both RCU and non-RCU use cases, where in the non-RCU
> > > variant the user supplies the lock.
> > > 
> > > So would these actually see enough use to make their addition worthwhile?
> > > If so, which uses are you thinking in terms of?
> > 
> > Yes true. Actually my initial intent was to add some lockdep checking for
> > RCU-list traversal via list_for_each_entry_rcu(). Since the pointers are
> > traversed without any lockdep checking due to not going through the
> > rcu_dereference API family.
> > 
> > You are right the patch I shared is complicated, and I could just keep the
> > rcu_read_lock_any_held() and use that in list_for_each_entry_rcu for that
> > matter (and in any other list RCU APIs doing list traversals). In fact I
> > could probably also call this new API as: lockdep_assert_rcu_held().
> > 
> > Also I noticed there is a rcu_lockdep_assert() mentioned in the
> > Requirements.html part of the documentation, but didn't find such a
> > definition in the kernel sources. So we should probably also update that ;-)
> 
> And I see rcu_lockdep_assert() got converted to RCU_LOCKDEP_WARN in
> https://lore.kernel.org/patchwork/patch/580641/
> 
> I will toy with the idea of a lockdep_assert_rcu_held() which checks for
> consolidated RCU reader sections and use that for list RCU and such, and we
> can discuss more on the subsequent RFC.

Interesting...

RCU is now out of sync with the other lockdep assertions, but at Ingo
Molnar's request a few years back.  ;-)

							Thanx, Paul

> thanks!
> 
> 
> > 
> > From Requirements.html:
> > 	A given function might wish to check for RCU-related preconditions
> > 	upon entry, before using any other RCU API.
> > 	The <tt>rcu_lockdep_assert()</tt> does this job,
> > 	asserting the expression in kernels having lockdep enabled
> > 	and doing nothing otherwise.
> > 
> > > Sorry to be so skeptical, but it has not been that long since Linus
> > > read me the riot act over RCU complexity.  ;-)
> > 
> > No problem, perfectly legit skepticism ;-)
> > 
> > thanks,
> > 
> >  - Joel
> > 
> > > 
> > > 							Thanx, Paul
> > > 
> > > > ---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
> > > > 
>