mbox series

[RFC,0/5] Remove some notrace RCU APIs

Message ID 20190524234933.5133-1-joel@joelfernandes.org
Headers show
Series Remove some notrace RCU APIs | expand

Message

Joel Fernandes May 24, 2019, 11:49 p.m. UTC
The series removes users of the following APIs, and the APIs themselves, since
the regular non - _notrace variants don't do any tracing anyway.
 * hlist_for_each_entry_rcu_notrace
 * rcu_dereference_raw_notrace

Joel Fernandes (Google) (5):
powerpc: Use regular rcu_dereference_raw API
trace: Use regular rcu_dereference_raw API
hashtable: Use the regular hlist_for_each_entry_rcu API
rculist: Remove hlist_for_each_entry_rcu_notrace since no users
rcu: Remove rcu_dereference_raw_notrace since no users

.clang-format                                 |  1 -
.../RCU/Design/Requirements/Requirements.html |  6 +++---
arch/powerpc/include/asm/kvm_book3s_64.h      |  2 +-
include/linux/hashtable.h                     |  2 +-
include/linux/rculist.h                       | 20 -------------------
include/linux/rcupdate.h                      |  9 ---------
kernel/trace/ftrace.c                         |  4 ++--
kernel/trace/ftrace_internal.h                |  8 ++++----
kernel/trace/trace.c                          |  4 ++--
9 files changed, 13 insertions(+), 43 deletions(-)

--
2.22.0.rc1.257.g3120a18244-goog

Comments

Steven Rostedt May 25, 2019, 3:24 a.m. UTC | #1
On Fri, 24 May 2019 19:49:28 -0400
"Joel Fernandes (Google)" <joel@joelfernandes.org> wrote:

> The series removes users of the following APIs, and the APIs themselves, since
> the regular non - _notrace variants don't do any tracing anyway.
>  * hlist_for_each_entry_rcu_notrace
>  * rcu_dereference_raw_notrace
> 

I guess the difference between the _raw_notrace and just _raw variants
is that _notrace ones do a rcu_check_sparse(). Don't we want to keep
that check?

-- Steve
Joel Fernandes May 25, 2019, 8:14 a.m. UTC | #2
On Fri, May 24, 2019 at 11:24:58PM -0400, Steven Rostedt wrote:
> On Fri, 24 May 2019 19:49:28 -0400
> "Joel Fernandes (Google)" <joel@joelfernandes.org> wrote:
> 
> > The series removes users of the following APIs, and the APIs themselves, since
> > the regular non - _notrace variants don't do any tracing anyway.
> >  * hlist_for_each_entry_rcu_notrace
> >  * rcu_dereference_raw_notrace
> > 
> 
> I guess the difference between the _raw_notrace and just _raw variants
> is that _notrace ones do a rcu_check_sparse(). Don't we want to keep
> that check?

This is true.

Since the users of _raw_notrace are very few, is it worth keeping this API
just for sparse checking? The API naming is also confusing. I was expecting
_raw_notrace to do fewer checks than _raw, instead of more. Honestly, I just
want to nuke _raw_notrace as done in this series and later we can introduce a
sparse checking version of _raw if need-be. The other option could be to
always do sparse checking for _raw however that used to be the case and got
changed in http://lists.infradead.org/pipermail/linux-afs/2016-July/001016.html

thanks a lot,

 - Joel

> 
> -- Steve
Steven Rostedt May 25, 2019, 11:08 a.m. UTC | #3
On Sat, 25 May 2019 04:14:44 -0400
Joel Fernandes <joel@joelfernandes.org> wrote:

> > I guess the difference between the _raw_notrace and just _raw variants
> > is that _notrace ones do a rcu_check_sparse(). Don't we want to keep
> > that check?  
> 
> This is true.
> 
> Since the users of _raw_notrace are very few, is it worth keeping this API
> just for sparse checking? The API naming is also confusing. I was expecting
> _raw_notrace to do fewer checks than _raw, instead of more. Honestly, I just
> want to nuke _raw_notrace as done in this series and later we can introduce a
> sparse checking version of _raw if need-be. The other option could be to
> always do sparse checking for _raw however that used to be the case and got
> changed in http://lists.infradead.org/pipermail/linux-afs/2016-July/001016.html

What if we just rename _raw to _raw_nocheck, and _raw_notrace to _raw ?

-- Steve
Joel Fernandes May 25, 2019, 2:19 p.m. UTC | #4
On Sat, May 25, 2019 at 07:08:26AM -0400, Steven Rostedt wrote:
> On Sat, 25 May 2019 04:14:44 -0400
> Joel Fernandes <joel@joelfernandes.org> wrote:
> 
> > > I guess the difference between the _raw_notrace and just _raw variants
> > > is that _notrace ones do a rcu_check_sparse(). Don't we want to keep
> > > that check?  
> > 
> > This is true.
> > 
> > Since the users of _raw_notrace are very few, is it worth keeping this API
> > just for sparse checking? The API naming is also confusing. I was expecting
> > _raw_notrace to do fewer checks than _raw, instead of more. Honestly, I just
> > want to nuke _raw_notrace as done in this series and later we can introduce a
> > sparse checking version of _raw if need-be. The other option could be to
> > always do sparse checking for _raw however that used to be the case and got
> > changed in http://lists.infradead.org/pipermail/linux-afs/2016-July/001016.html
> 
> What if we just rename _raw to _raw_nocheck, and _raw_notrace to _raw ?

That would also mean changing 160 usages of _raw to _raw_nocheck in the
kernel :-/.

The tracing usage of _raw_notrace is only like 2 or 3 users. Can we just call
rcu_check_sparse directly in the calling code for those and eliminate the APIs?

I wonder what Paul thinks about the matter as well.

thanks, Steven!
Paul E. McKenney May 25, 2019, 3:50 p.m. UTC | #5
On Sat, May 25, 2019 at 10:19:54AM -0400, Joel Fernandes wrote:
> On Sat, May 25, 2019 at 07:08:26AM -0400, Steven Rostedt wrote:
> > On Sat, 25 May 2019 04:14:44 -0400
> > Joel Fernandes <joel@joelfernandes.org> wrote:
> > 
> > > > I guess the difference between the _raw_notrace and just _raw variants
> > > > is that _notrace ones do a rcu_check_sparse(). Don't we want to keep
> > > > that check?  
> > > 
> > > This is true.
> > > 
> > > Since the users of _raw_notrace are very few, is it worth keeping this API
> > > just for sparse checking? The API naming is also confusing. I was expecting
> > > _raw_notrace to do fewer checks than _raw, instead of more. Honestly, I just
> > > want to nuke _raw_notrace as done in this series and later we can introduce a
> > > sparse checking version of _raw if need-be. The other option could be to
> > > always do sparse checking for _raw however that used to be the case and got
> > > changed in http://lists.infradead.org/pipermail/linux-afs/2016-July/001016.html
> > 
> > What if we just rename _raw to _raw_nocheck, and _raw_notrace to _raw ?
> 
> That would also mean changing 160 usages of _raw to _raw_nocheck in the
> kernel :-/.
> 
> The tracing usage of _raw_notrace is only like 2 or 3 users. Can we just call
> rcu_check_sparse directly in the calling code for those and eliminate the APIs?
> 
> I wonder what Paul thinks about the matter as well.

My thought is that it is likely that a goodly number of the current uses
of _raw should really be some form of _check, with lockdep expressions
spelled out.  Not that working out what exactly those lockdep expressions
should be is necessarily a trivial undertaking.  ;-)

That aside, if we are going to change the name of an API that is
used 160 places throughout the tree, we would need to have a pretty
good justification.  Without such a justification, it will just look
like pointless churn to the various developers and maintainers on the
receiving end of the patches.

							Thanx, Paul

> thanks, Steven!
>
Joel Fernandes May 25, 2019, 6:14 p.m. UTC | #6
On Sat, May 25, 2019 at 08:50:35AM -0700, Paul E. McKenney wrote:
> On Sat, May 25, 2019 at 10:19:54AM -0400, Joel Fernandes wrote:
> > On Sat, May 25, 2019 at 07:08:26AM -0400, Steven Rostedt wrote:
> > > On Sat, 25 May 2019 04:14:44 -0400
> > > Joel Fernandes <joel@joelfernandes.org> wrote:
> > > 
> > > > > I guess the difference between the _raw_notrace and just _raw variants
> > > > > is that _notrace ones do a rcu_check_sparse(). Don't we want to keep
> > > > > that check?  
> > > > 
> > > > This is true.
> > > > 
> > > > Since the users of _raw_notrace are very few, is it worth keeping this API
> > > > just for sparse checking? The API naming is also confusing. I was expecting
> > > > _raw_notrace to do fewer checks than _raw, instead of more. Honestly, I just
> > > > want to nuke _raw_notrace as done in this series and later we can introduce a
> > > > sparse checking version of _raw if need-be. The other option could be to
> > > > always do sparse checking for _raw however that used to be the case and got
> > > > changed in http://lists.infradead.org/pipermail/linux-afs/2016-July/001016.html
> > > 
> > > What if we just rename _raw to _raw_nocheck, and _raw_notrace to _raw ?
> > 
> > That would also mean changing 160 usages of _raw to _raw_nocheck in the
> > kernel :-/.
> > 
> > The tracing usage of _raw_notrace is only like 2 or 3 users. Can we just call
> > rcu_check_sparse directly in the calling code for those and eliminate the APIs?
> > 
> > I wonder what Paul thinks about the matter as well.
> 
> My thought is that it is likely that a goodly number of the current uses
> of _raw should really be some form of _check, with lockdep expressions
> spelled out.  Not that working out what exactly those lockdep expressions
> should be is necessarily a trivial undertaking.  ;-)

Yes, currently where I am a bit stuck is the rcu_dereference_raw()
cannot possibly know what SRCU domain it is under, so lockdep cannot check if
an SRCU lock is held without the user also passing along the SRCU domain. I
am trying to change lockdep to see if it can check if *any* srcu domain lock
is held (regardless of which one) and complain if none are. This is at least
better than no check at all.

However, I think it gets tricky for mutexes. If you have something like:
mutex_lock(some_mutex);
p = rcu_dereference_raw(gp);
mutex_unlock(some_mutex);

This might be a perfectly valid invocation of _raw, however my checks (patch
is still cooking) trigger a lockdep warning becase _raw cannot know that this
is Ok. lockdep thinks it is not in a reader section. This then gets into the
territory of a new rcu_derference_raw_protected(gp, assert_held(some_mutex))
which sucks because its yet another API. To circumvent this issue, can we
just have callers of rcu_dereference_raw ensure that they call
rcu_read_lock() if they are protecting dereferences by a mutex? That would
make things a lot easier and also may be Ok since rcu_read_lock is quite
cheap.

> That aside, if we are going to change the name of an API that is
> used 160 places throughout the tree, we would need to have a pretty
> good justification.  Without such a justification, it will just look
> like pointless churn to the various developers and maintainers on the
> receiving end of the patches.

Actually, the API name change is not something I want to do, it is Steven
suggestion. My suggestion is let us just delete _raw_notrace and just use the
_raw API for tracing, since _raw doesn't do any tracing anyway. Steve pointed
that _raw_notrace does sparse checking unlike _raw, but I think that isn't an
issue since _raw doesn't do such checking at the moment anyway.. (if possible
check my cover letter again for details/motivation of this series).

thanks!

 - Joel

> 							Thanx, Paul
> 
> > thanks, Steven!
> > 
>
Joel Fernandes May 25, 2019, 6:18 p.m. UTC | #7
On Sat, May 25, 2019 at 02:14:07PM -0400, Joel Fernandes wrote:
[snip]
> > That aside, if we are going to change the name of an API that is
> > used 160 places throughout the tree, we would need to have a pretty
> > good justification.  Without such a justification, it will just look
> > like pointless churn to the various developers and maintainers on the
> > receiving end of the patches.
> 
> Actually, the API name change is not something I want to do, it is Steven
> suggestion. My suggestion is let us just delete _raw_notrace and just use the
> _raw API for tracing, since _raw doesn't do any tracing anyway. Steve pointed
> that _raw_notrace does sparse checking unlike _raw, but I think that isn't an
> issue since _raw doesn't do such checking at the moment anyway.. (if possible
> check my cover letter again for details/motivation of this series).

Come to think of it, if we/I succeed in adding lockdep checking in _raw, then
we can just keep the current APIs and not delete anything. And we can have
_raw_notrace skip the lockdep checks. The sparse check question would still
be an open one though, since _raw doesn't do sparse checks at the moment
unlike _raw_notrace as Steve pointed.

Thanks,

 - Joel
Paul E. McKenney May 28, 2019, 12:24 p.m. UTC | #8
On Sat, May 25, 2019 at 02:14:07PM -0400, Joel Fernandes wrote:
> On Sat, May 25, 2019 at 08:50:35AM -0700, Paul E. McKenney wrote:
> > On Sat, May 25, 2019 at 10:19:54AM -0400, Joel Fernandes wrote:
> > > On Sat, May 25, 2019 at 07:08:26AM -0400, Steven Rostedt wrote:
> > > > On Sat, 25 May 2019 04:14:44 -0400
> > > > Joel Fernandes <joel@joelfernandes.org> wrote:
> > > > 
> > > > > > I guess the difference between the _raw_notrace and just _raw variants
> > > > > > is that _notrace ones do a rcu_check_sparse(). Don't we want to keep
> > > > > > that check?  
> > > > > 
> > > > > This is true.
> > > > > 
> > > > > Since the users of _raw_notrace are very few, is it worth keeping this API
> > > > > just for sparse checking? The API naming is also confusing. I was expecting
> > > > > _raw_notrace to do fewer checks than _raw, instead of more. Honestly, I just
> > > > > want to nuke _raw_notrace as done in this series and later we can introduce a
> > > > > sparse checking version of _raw if need-be. The other option could be to
> > > > > always do sparse checking for _raw however that used to be the case and got
> > > > > changed in http://lists.infradead.org/pipermail/linux-afs/2016-July/001016.html
> > > > 
> > > > What if we just rename _raw to _raw_nocheck, and _raw_notrace to _raw ?
> > > 
> > > That would also mean changing 160 usages of _raw to _raw_nocheck in the
> > > kernel :-/.
> > > 
> > > The tracing usage of _raw_notrace is only like 2 or 3 users. Can we just call
> > > rcu_check_sparse directly in the calling code for those and eliminate the APIs?
> > > 
> > > I wonder what Paul thinks about the matter as well.
> > 
> > My thought is that it is likely that a goodly number of the current uses
> > of _raw should really be some form of _check, with lockdep expressions
> > spelled out.  Not that working out what exactly those lockdep expressions
> > should be is necessarily a trivial undertaking.  ;-)
> 
> Yes, currently where I am a bit stuck is the rcu_dereference_raw()
> cannot possibly know what SRCU domain it is under, so lockdep cannot check if
> an SRCU lock is held without the user also passing along the SRCU domain. I
> am trying to change lockdep to see if it can check if *any* srcu domain lock
> is held (regardless of which one) and complain if none are. This is at least
> better than no check at all.
> 
> However, I think it gets tricky for mutexes. If you have something like:
> mutex_lock(some_mutex);
> p = rcu_dereference_raw(gp);
> mutex_unlock(some_mutex);
> 
> This might be a perfectly valid invocation of _raw, however my checks (patch
> is still cooking) trigger a lockdep warning becase _raw cannot know that this
> is Ok. lockdep thinks it is not in a reader section. This then gets into the
> territory of a new rcu_derference_raw_protected(gp, assert_held(some_mutex))
> which sucks because its yet another API. To circumvent this issue, can we
> just have callers of rcu_dereference_raw ensure that they call
> rcu_read_lock() if they are protecting dereferences by a mutex? That would
> make things a lot easier and also may be Ok since rcu_read_lock is quite
> cheap.

Why not just rcu_dereference_protected(lockdep_is_held(some_mutex))?
The API is already there, and no need for spurious readers.

> > That aside, if we are going to change the name of an API that is
> > used 160 places throughout the tree, we would need to have a pretty
> > good justification.  Without such a justification, it will just look
> > like pointless churn to the various developers and maintainers on the
> > receiving end of the patches.
> 
> Actually, the API name change is not something I want to do, it is Steven
> suggestion. My suggestion is let us just delete _raw_notrace and just use the
> _raw API for tracing, since _raw doesn't do any tracing anyway. Steve pointed
> that _raw_notrace does sparse checking unlike _raw, but I think that isn't an
> issue since _raw doesn't do such checking at the moment anyway.. (if possible
> check my cover letter again for details/motivation of this series).

Understood, but regardless of who suggested it, if we are to go through
with it, good justification will be required.  ;-)

							Thanx, Paul

> thanks!
> 
>  - Joel
> 
> > 							Thanx, Paul
> > 
> > > thanks, Steven!
> > > 
> > 
>
Joel Fernandes May 28, 2019, 7 p.m. UTC | #9
On Tue, May 28, 2019 at 05:24:47AM -0700, Paul E. McKenney wrote:
> On Sat, May 25, 2019 at 02:14:07PM -0400, Joel Fernandes wrote:
> > On Sat, May 25, 2019 at 08:50:35AM -0700, Paul E. McKenney wrote:
> > > On Sat, May 25, 2019 at 10:19:54AM -0400, Joel Fernandes wrote:
> > > > On Sat, May 25, 2019 at 07:08:26AM -0400, Steven Rostedt wrote:
> > > > > On Sat, 25 May 2019 04:14:44 -0400
> > > > > Joel Fernandes <joel@joelfernandes.org> wrote:
> > > > > 
> > > > > > > I guess the difference between the _raw_notrace and just _raw variants
> > > > > > > is that _notrace ones do a rcu_check_sparse(). Don't we want to keep
> > > > > > > that check?  
> > > > > > 
> > > > > > This is true.
> > > > > > 
> > > > > > Since the users of _raw_notrace are very few, is it worth keeping this API
> > > > > > just for sparse checking? The API naming is also confusing. I was expecting
> > > > > > _raw_notrace to do fewer checks than _raw, instead of more. Honestly, I just
> > > > > > want to nuke _raw_notrace as done in this series and later we can introduce a
> > > > > > sparse checking version of _raw if need-be. The other option could be to
> > > > > > always do sparse checking for _raw however that used to be the case and got
> > > > > > changed in http://lists.infradead.org/pipermail/linux-afs/2016-July/001016.html
> > > > > 
> > > > > What if we just rename _raw to _raw_nocheck, and _raw_notrace to _raw ?
> > > > 
> > > > That would also mean changing 160 usages of _raw to _raw_nocheck in the
> > > > kernel :-/.
> > > > 
> > > > The tracing usage of _raw_notrace is only like 2 or 3 users. Can we just call
> > > > rcu_check_sparse directly in the calling code for those and eliminate the APIs?
> > > > 
> > > > I wonder what Paul thinks about the matter as well.
> > > 
> > > My thought is that it is likely that a goodly number of the current uses
> > > of _raw should really be some form of _check, with lockdep expressions
> > > spelled out.  Not that working out what exactly those lockdep expressions
> > > should be is necessarily a trivial undertaking.  ;-)
> > 
> > Yes, currently where I am a bit stuck is the rcu_dereference_raw()
> > cannot possibly know what SRCU domain it is under, so lockdep cannot check if
> > an SRCU lock is held without the user also passing along the SRCU domain. I
> > am trying to change lockdep to see if it can check if *any* srcu domain lock
> > is held (regardless of which one) and complain if none are. This is at least
> > better than no check at all.
> > 
> > However, I think it gets tricky for mutexes. If you have something like:
> > mutex_lock(some_mutex);
> > p = rcu_dereference_raw(gp);
> > mutex_unlock(some_mutex);
> > 
> > This might be a perfectly valid invocation of _raw, however my checks (patch
> > is still cooking) trigger a lockdep warning becase _raw cannot know that this
> > is Ok. lockdep thinks it is not in a reader section. This then gets into the
> > territory of a new rcu_derference_raw_protected(gp, assert_held(some_mutex))
> > which sucks because its yet another API. To circumvent this issue, can we
> > just have callers of rcu_dereference_raw ensure that they call
> > rcu_read_lock() if they are protecting dereferences by a mutex? That would
> > make things a lot easier and also may be Ok since rcu_read_lock is quite
> > cheap.
> 
> Why not just rcu_dereference_protected(lockdep_is_held(some_mutex))?
> The API is already there, and no need for spurious readers.

Hmm, so I gave a bad example, here is a better example:

fib_get_table calls hlist_for_each_entry_rcu()
hlist_for_each_entry_rcu calls rcu_dereference_raw().

This is perfectly Ok to be called under rtnl_mutex. However rcu_dererence_raw
in hlist_for_each_entry_rcu has no way of knowing that the rtnl_mutex held is
sufficient for the protection since it is not directly called by the caller.

I am almost sure I saw other examples of rcu_dereference_raw being called
this way as well.

I was trying to make an "automatic" lockdep check for all this, but it is
quite hard to do so without passing down lockdep experessions down a call
chain thus complicating all such callchains.

Further I don't think code can trivially be converted from
rcu_dereference_raw to rcu_dereference_protected even if the protection being
offered is known, since the former does not do sparse checking and the latter
might trigger false sparse checks in case the pointer in concern is protected
both by RCU and non-RCU methods. I believe this is why you removed sparse
checking from rcu_dereference_raw as well:

http://lists.infradead.org/pipermail/linux-afs/2016-July/001016.html

> > > That aside, if we are going to change the name of an API that is
> > > used 160 places throughout the tree, we would need to have a pretty
> > > good justification.  Without such a justification, it will just look
> > > like pointless churn to the various developers and maintainers on the
> > > receiving end of the patches.
> > 
> > Actually, the API name change is not something I want to do, it is Steven
> > suggestion. My suggestion is let us just delete _raw_notrace and just use the
> > _raw API for tracing, since _raw doesn't do any tracing anyway. Steve pointed
> > that _raw_notrace does sparse checking unlike _raw, but I think that isn't an
> > issue since _raw doesn't do such checking at the moment anyway.. (if possible
> > check my cover letter again for details/motivation of this series).
> 
> Understood, but regardless of who suggested it, if we are to go through
> with it, good justification will be required.  ;-)

Ok ;-). About the names of the APIs, I thought of leaving rcu_dereference_raw
and its callers intact, and just rename:

 * hlist_for_each_entry_rcu_notrace
 * rcu_dereference_raw_notrace

to:
 * hlist_for_each_entry_rcu_sparse
 * rcu_dereference_raw_sparse

The _sparse would stand for "sparse checking". However I am open to better
names..

Such renaming would avoid confusion and keep the fact about sparse checking
less ambiguous.

thanks!

 - Joel
Paul E. McKenney May 28, 2019, 8 p.m. UTC | #10
On Tue, May 28, 2019 at 03:00:07PM -0400, Joel Fernandes wrote:
> On Tue, May 28, 2019 at 05:24:47AM -0700, Paul E. McKenney wrote:
> > On Sat, May 25, 2019 at 02:14:07PM -0400, Joel Fernandes wrote:
> > > On Sat, May 25, 2019 at 08:50:35AM -0700, Paul E. McKenney wrote:
> > > > On Sat, May 25, 2019 at 10:19:54AM -0400, Joel Fernandes wrote:
> > > > > On Sat, May 25, 2019 at 07:08:26AM -0400, Steven Rostedt wrote:
> > > > > > On Sat, 25 May 2019 04:14:44 -0400
> > > > > > Joel Fernandes <joel@joelfernandes.org> wrote:
> > > > > > 
> > > > > > > > I guess the difference between the _raw_notrace and just _raw variants
> > > > > > > > is that _notrace ones do a rcu_check_sparse(). Don't we want to keep
> > > > > > > > that check?  
> > > > > > > 
> > > > > > > This is true.
> > > > > > > 
> > > > > > > Since the users of _raw_notrace are very few, is it worth keeping this API
> > > > > > > just for sparse checking? The API naming is also confusing. I was expecting
> > > > > > > _raw_notrace to do fewer checks than _raw, instead of more. Honestly, I just
> > > > > > > want to nuke _raw_notrace as done in this series and later we can introduce a
> > > > > > > sparse checking version of _raw if need-be. The other option could be to
> > > > > > > always do sparse checking for _raw however that used to be the case and got
> > > > > > > changed in http://lists.infradead.org/pipermail/linux-afs/2016-July/001016.html
> > > > > > 
> > > > > > What if we just rename _raw to _raw_nocheck, and _raw_notrace to _raw ?
> > > > > 
> > > > > That would also mean changing 160 usages of _raw to _raw_nocheck in the
> > > > > kernel :-/.
> > > > > 
> > > > > The tracing usage of _raw_notrace is only like 2 or 3 users. Can we just call
> > > > > rcu_check_sparse directly in the calling code for those and eliminate the APIs?
> > > > > 
> > > > > I wonder what Paul thinks about the matter as well.
> > > > 
> > > > My thought is that it is likely that a goodly number of the current uses
> > > > of _raw should really be some form of _check, with lockdep expressions
> > > > spelled out.  Not that working out what exactly those lockdep expressions
> > > > should be is necessarily a trivial undertaking.  ;-)
> > > 
> > > Yes, currently where I am a bit stuck is the rcu_dereference_raw()
> > > cannot possibly know what SRCU domain it is under, so lockdep cannot check if
> > > an SRCU lock is held without the user also passing along the SRCU domain. I
> > > am trying to change lockdep to see if it can check if *any* srcu domain lock
> > > is held (regardless of which one) and complain if none are. This is at least
> > > better than no check at all.
> > > 
> > > However, I think it gets tricky for mutexes. If you have something like:
> > > mutex_lock(some_mutex);
> > > p = rcu_dereference_raw(gp);
> > > mutex_unlock(some_mutex);
> > > 
> > > This might be a perfectly valid invocation of _raw, however my checks (patch
> > > is still cooking) trigger a lockdep warning becase _raw cannot know that this
> > > is Ok. lockdep thinks it is not in a reader section. This then gets into the
> > > territory of a new rcu_derference_raw_protected(gp, assert_held(some_mutex))
> > > which sucks because its yet another API. To circumvent this issue, can we
> > > just have callers of rcu_dereference_raw ensure that they call
> > > rcu_read_lock() if they are protecting dereferences by a mutex? That would
> > > make things a lot easier and also may be Ok since rcu_read_lock is quite
> > > cheap.
> > 
> > Why not just rcu_dereference_protected(lockdep_is_held(some_mutex))?
> > The API is already there, and no need for spurious readers.
> 
> Hmm, so I gave a bad example, here is a better example:
> 
> fib_get_table calls hlist_for_each_entry_rcu()
> hlist_for_each_entry_rcu calls rcu_dereference_raw().
> 
> This is perfectly Ok to be called under rtnl_mutex. However rcu_dererence_raw
> in hlist_for_each_entry_rcu has no way of knowing that the rtnl_mutex held is
> sufficient for the protection since it is not directly called by the caller.

Agreed, and this just happens to be one of the use cases that led to
rcu_dereference_raw().  The calling code (in this case, FIB) simply has
no idea what the synchronization strategy might be.

> I am almost sure I saw other examples of rcu_dereference_raw being called
> this way as well.

And I am OK with this sort of use case.  The ones I am less happy with
are the ones where there really is a lockdep expression that could be
constructed.

> I was trying to make an "automatic" lockdep check for all this, but it is
> quite hard to do so without passing down lockdep experessions down a call
> chain thus complicating all such callchains.

Understood!  Not an easy task.

> Further I don't think code can trivially be converted from
> rcu_dereference_raw to rcu_dereference_protected even if the protection being
> offered is known, since the former does not do sparse checking and the latter
> might trigger false sparse checks in case the pointer in concern is protected
> both by RCU and non-RCU methods. I believe this is why you removed sparse
> checking from rcu_dereference_raw as well:
> 
> http://lists.infradead.org/pipermail/linux-afs/2016-July/001016.html

Good point!

> > > > That aside, if we are going to change the name of an API that is
> > > > used 160 places throughout the tree, we would need to have a pretty
> > > > good justification.  Without such a justification, it will just look
> > > > like pointless churn to the various developers and maintainers on the
> > > > receiving end of the patches.
> > > 
> > > Actually, the API name change is not something I want to do, it is Steven
> > > suggestion. My suggestion is let us just delete _raw_notrace and just use the
> > > _raw API for tracing, since _raw doesn't do any tracing anyway. Steve pointed
> > > that _raw_notrace does sparse checking unlike _raw, but I think that isn't an
> > > issue since _raw doesn't do such checking at the moment anyway.. (if possible
> > > check my cover letter again for details/motivation of this series).
> > 
> > Understood, but regardless of who suggested it, if we are to go through
> > with it, good justification will be required.  ;-)
> 
> Ok ;-). About the names of the APIs, I thought of leaving rcu_dereference_raw
> and its callers intact, and just rename:
> 
>  * hlist_for_each_entry_rcu_notrace
>  * rcu_dereference_raw_notrace
> 
> to:
>  * hlist_for_each_entry_rcu_sparse
>  * rcu_dereference_raw_sparse
> 
> The _sparse would stand for "sparse checking". However I am open to better
> names..
> 
> Such renaming would avoid confusion and keep the fact about sparse checking
> less ambiguous.

Let's give people a few days to propose different names, and if nothing
compelling, those names look good.  There are not very many of them, so
the penalty for having to rename is quite low.

							Thanx, Paul