Re: [PATCH -tip] kprobes: Lock rcu_read_lock() while searching kprobe

[Paul E. McKenney]

On Sat, Dec 07, 2019 at 07:08:42PM -0500, Joel Fernandes wrote:
> On Thu, Dec 05, 2019 at 07:11:51PM -0800, Paul E. McKenney wrote:
> > On Thu, Dec 05, 2019 at 08:11:37PM -0500, Joel Fernandes wrote:
> > > On Wed, Dec 04, 2019 at 08:12:39AM -0800, Paul E. McKenney wrote:
> > > > On Wed, Dec 04, 2019 at 11:05:50AM +0100, Ingo Molnar wrote:
> > > > > 
> > > > > * Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> > > > > 
> > > > > > >  * This list-traversal primitive may safely run concurrently with
> > > > > > >  * the _rcu list-mutation primitives such as hlist_add_head_rcu()
> > > > > > >  * as long as the traversal is guarded by rcu_read_lock().
> > > > > > >  */
> > > > > > > #define hlist_for_each_entry_rcu(pos, head, member, cond...)            \
> > > > > > > 
> > > > > > > is actively harmful. Why is it there?
> > > > > > 
> > > > > > For cases where common code might be invoked both from the reader
> > > > > > (with RCU protection) and from the updater (protected by some
> > > > > > lock).  This common code can then use the optional argument to
> > > > > > hlist_for_each_entry_rcu() to truthfully tell lockdep that it might be
> > > > > > called with either form of protection in place.
> > > > > > 
> > > > > > This also combines with the __rcu tag used to mark RCU-protected
> > > > > > pointers, in which case sparse complains when a non-RCU API is applied
> > > > > > to these pointers, to get back to your earlier question about use of
> > > > > > hlist_for_each_entry_rcu() within the update-side lock.
> > > > > > 
> > > > > > But what are you seeing as actively harmful about all of this?
> > > > > > What should we be doing instead?
> > > > > 
> > > > > Yeah, so basically in the write-locked path hlist_for_each_entry() 
> > > > > generates (slightly) more efficient code than hlist_for_each_entry_rcu(), 
> > > > > correct?
> > > > 
> > > > Potentially yes, if the READ_ONCE() constrains the compiler.  Or not,
> > > > depending of course on the compiler and the surrounding code.
> > > > 
> > > > > Also, the principle of passing warning flags around is problematic - but 
> > > > > I can see the point in this specific case.
> > > > 
> > > > Would it help to add an hlist_for_each_entry_protected() that expected
> > > > RCU-protected pointers and write-side protection, analogous to
> > > > rcu_dereference_protected()?  Or would that expansion of the RCU API
> > > > outweigh any benefits?
> > > 
> > > Personally, I like keeping the same API and using the optional argument like
> > > we did thus preventing too many APIs / new APIs.
> > 
> > Would you be willing to put together a prototype patch so that people
> > can see exactly how it would look?
> 
> Hi Paul,
> 
> I was referring to the same API we have at the moment (that is
> hlist_for_each_entry_rcu() with the additional cond parameter). I was saying
> let us keep that and not add a hlist_for_each_entry_protected() instead, so
> as to not proliferate the number of APIs.
> 
> Or did I miss the point?

This would work for me.  The only concern would be inefficiency, but we
have heard from people saying that the unnecessary inefficiency is only
on code paths that they do not care about, so we should be good.

							Thanx, Paul