From: Joel Fernandes <joel@joelfernandes.org>
To: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: rcu@vger.kernel.org
Subject: Re: Should list_entry_rcu use rcu_dereference ?
Date: Fri, 24 May 2019 12:50:09 -0400 [thread overview]
Message-ID: <20190524165009.GB197789@google.com> (raw)
In-Reply-To: <20190524080737.GA176324@google.com>
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
> > >
next prev parent reply other threads:[~2019-05-24 16:50 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-04 18:59 Should list_entry_rcu use rcu_dereference ? Joel Fernandes
2019-05-06 23:54 ` Paul E. McKenney
2019-05-13 3:30 ` Joel Fernandes
2019-05-14 22:20 ` Paul E. McKenney
2019-05-24 8:07 ` Joel Fernandes
2019-05-24 16:50 ` Joel Fernandes [this message]
2019-05-24 17:33 ` Paul E. McKenney
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190524165009.GB197789@google.com \
--to=joel@joelfernandes.org \
--cc=paulmck@linux.ibm.com \
--cc=rcu@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).