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