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:

quoted

On Sat, May 25, 2019 at 10:19:54AM -0400, Joel Fernandes wrote:

quoted

On Sat, May 25, 2019 at 07:08:26AM -0400, Steven Rostedt wrote:

quoted

On Sat, 25 May 2019 04:14:44 -0400
Joel Fernandes [off-list ref] wrote:

quoted

quoted

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.

quoted

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

quoted

							Thanx, Paul

quoted

thanks, Steven!