On Thu, Jul 12, 2018 at 09:35:12AM -0400, Steven Rostedt wrote:
On Wed, 11 Jul 2018 21:28:25 -0700 Joel Fernandes joel@joelfernandes.org wrote:
On Wed, Jul 11, 2018 at 11:21:20PM -0400, Steven Rostedt wrote:
On Wed, 11 Jul 2018 13:52:49 -0700 Joel Fernandes joel@joelfernandes.org wrote:
#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \ extern struct tracepoint __tracepoint_##name; \ static inline void trace_##name(proto) \ { \ if (static_key_false(&__tracepoint_##name.key)) \ __DO_TRACE(&__tracepoint_##name, \ TP_PROTO(data_proto), \ TP_ARGS(data_args), \ TP_CONDITION(cond), 0); \ if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) { \ rcu_read_lock_sched_notrace(); \ rcu_dereference_sched(__tracepoint_##name.funcs);\ rcu_read_unlock_sched_notrace(); \ } \ }
Because lockdep would only trigger warnings when the tracepoint was enabled and used in a place it shouldn't be, we added the above IS_ENABLED(CONFIG_LOCKDEP) part to test regardless if the the tracepoint was enabled or not. Because we do this, we don't need to have the test in the __DO_TRACE() code itself. That means we can clean up the code as per Peter's suggestion.
Sounds good, I'm Ok with making this change.
Just to clarify, are you proposing to change the rcu_dereference_sched to rcu_dereference_raw in both __DECLARE_TRACE and __DO_TRACE?
No, just in __DO_TRACE(). The rcu_dereference_sched() above in __DECLARE_TRACE() in the if (IS_ENABLED(CONFIG_LOCKDEP) block is required to show the warnings if trace_##name() is used wrong, and is the reason we can use rcu_dereference_raw() in __DO_TRACE() in the first place ;-)
This brings up another point. We should probably add to __DECLARE_TRACE_RCU() this:
#ifndef MODULE #define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args) \ static inline void trace_##name##_rcuidle(proto) \ { \ if (static_key_false(&__tracepoint_##name.key)) \ __DO_TRACE(&__tracepoint_##name, \ TP_PROTO(data_proto), \ TP_ARGS(data_args), \ TP_CONDITION(cond), 1); \
if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) { \int idx; \idx = srcu_read_lock_notrace(&tracepoint_srcu); \srcu_dereference_notrace(__tracepoint_##name.funcs, \&tracepoint_srcu); \srcu_read_unlock_notrace(&tracepoint_srcu, idx); \} \#else
So that lockdep works with trace_##name##__rcuidle() when the trace event is not enabled.
But that should be a separate patch and not part of this series. I may write that up tomorrow.
Yes, that sounds good to me and would be good to add the safe guard there. But you meant srcu_dereference above, not srcu_dereference_notrace right?
We don't need to trace them. I believe that the "srcu_*_notrace" still performs the lockdep checks. That's what we want. If they don't then we should not use notrace. But I believe they still do lockdep.
AFAICT, _notrace doesn't call into lockdep or tracing (there's also a comment that says so):
/** * srcu_dereference_notrace - no tracing and no lockdep calls from here */
So then, we should use the regular variant for this additional check you're suggesting.
thanks,
- Joel
-- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html