Centralize and unify usage of preempt/irq tracepoints
Subsystems (6)
v6 cover Centralize and unify usage of preempt/irq tracepoints
From: "Joel Fernandes (Google)" <[email protected]>
This is the next revision of preempt/irq tracepoint centralization and unified usage across the kernel.
The preempt/irq tracepoints exist but not everything in the kernel is using it. This makes things not work simultaneously (for ex, only either lockdep or irqsoff events can be used at a time). This series is an attempt to solve that, and also results in a nice clean up of kernel in general. Several ifdefs are simpler, and the design is more unified and better. Also as a result of this, we also speeded performance all rcuidle tracepoints since their handling is simpler.
No major changes since v5. Just some spelling errors, small refactoring etc. Also I removed the softirq bug fix from this series and submitted it separately.
I am also writing a tracepoint self-test that runs various tests, along with some documentation. I am planning to submit that separately since it'll take some time to complete them and so that these can go in first. See [1] for a preview of the tests I have so far (its early stage).
v6:
- fixed lockdep_recursion undefined issue, now using lockdep_recursing().
- minor patch clean up
v5:
- Fixed performance issues due to rcu-idle handling
Joel Fernandes (Google) (4):
srcu: Add notrace variant of srcu_dereference trace/irqsoff: Split reset into separate functions
tracepoint: Make rcuidle tracepoint callers use SRCU
tracing: Centralize preemptirq tracepoints and unify their usage
Paul McKenney (1):
srcu: Add notrace variants of srcu_read_{lock,unlock}
include/linux/ftrace.h | 11 +- include/linux/irqflags.h | 11 +- include/linux/lockdep.h | 8 +- include/linux/preempt.h | 2 +- include/linux/srcu.h | 22 +++ include/linux/tracepoint.h | 46 +++++- include/trace/events/preemptirq.h | 23 +-- init/main.c | 5 +- kernel/locking/lockdep.c | 35 ++--- kernel/sched/core.c | 2 +- kernel/trace/Kconfig | 22 ++- kernel/trace/Makefile | 2 +- kernel/trace/trace_irqsoff.c | 235 +++++++++--------------------- kernel/trace/trace_preemptirq.c | 71 +++++++++ kernel/tracepoint.c | 15 +- 15 files changed, 282 insertions(+), 228 deletions(-) create mode 100644 kernel/trace/trace_preemptirq.c
[1] tracepoint-selftests: https://tinyurl.com/tptests-1
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
RFC PATCH v6 1/5 srcu: Add notrace variants of srcu_read_{lock,unlock}
From: Paul McKenney <[email protected]>
This is needed for a future tracepoint patch that uses srcu, and to make sure it doesn't call into lockdep.
tracepoint code already calls notrace variants for rcu_read_lock_sched so this patch does the same for srcu which will be used in a later patch. Keeps it consistent with rcu-sched.
[Joel: Added commit message]
--- include/linux/srcu.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
include/linux/srcu.h +17 -0
@@ -161,6 +161,16 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)return retval; } +/* Used by tracing, cannot be traced and cannot invoke lockdep. */ +static inline notrace int +srcu_read_lock_notrace (struct srcu_struct *sp)__acquires (sp) +{ +int retval; + + retval =__srcu_read_lock (sp); +return retval; +} +/** * srcu_read_unlock - unregister a old reader from an SRCU-protected structure. * @sp: srcu_struct in which to unregister the old reader.
@@ -175,6 +185,13 @@ static inline void srcu_read_unlock(struct srcu_struct *sp, int idx)__srcu_read_unlock (sp, idx); } +/* Used by tracing, cannot be traced and cannot call lockdep. */ +static inline notrace void +srcu_read_unlock_notrace (struct srcu_struct *sp,int idx)__releases (sp) +{ +__srcu_read_unlock (sp, idx); +} +/** * smp_mb__after_srcu_read_unlock - ensure full ordering after srcu_read_unlock *
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
RFC PATCH v6 2/5 srcu: Add notrace variant of srcu_dereference
In reply to Joel FernandesFrom: "Joel Fernandes (Google)" <[email protected]>
In the last patch in this series, we are making lockdep register hooks onto the irq_{disable,enable} tracepoints. These tracepoints use the _rcuidle tracepoint variant. In this series we switch the _rcuidle tracepoint callers to use SRCU instead of sched-RCU. Inorder to dereference the pointer to the probe functions, we could call srcu_dereference, however this API will call back into lockdep to check if the lock is held *before* the lockdep probe hooks have a chance to run and annotate the IRQ enabled/disabled state.
For this reason we need a notrace variant of srcu_dereference since otherwise we get lockdep splats. This patch adds the needed srcu_dereference_notrace variant.
--- include/linux/srcu.h | 5 +++++ 1 file changed, 5 insertions(+)
include/linux/srcu.h +5 -0
@@ -135,6 +135,11 @@ static inline int srcu_read_lock_held(const struct srcu_struct *sp)*/ #define srcu_dereference (p, sp) srcu_dereference_check((p), (sp), 0) +/** +* srcu_dereference_notrace - no tracing and no lockdep calls from here +*/ +#define srcu_dereference_notrace (p, sp) srcu_dereference_check((p), (sp), 1) +/** * srcu_read_lock - register a new reader for an SRCU-protected structure. * @sp: srcu_struct in which to register the new reader.
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
RFC PATCH v6 3/5 trace/irqsoff: Split reset into separate functions
In reply to Joel FernandesFrom: "Joel Fernandes (Google)" <[email protected]>
Split reset functions into seperate functions in preparation of future patches that need to do tracer specific reset.
--- kernel/trace/trace_irqsoff.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)
kernel/trace/trace_irqsoff.c +19 -3
@@ -634,7 +634,7 @@ static int __irqsoff_tracer_init(struct trace_array *tr)return 0 ; } -static void irqsoff_tracer_reset (struct trace_array *tr) +static void __irqsoff_tracer_reset (struct trace_array *tr) {int lat_flag = save_flags &TRACE_ITER_LATENCY_FMT ;int overwrite_flag = save_flags &TRACE_ITER_OVERWRITE ;
@@ -665,6 +665,12 @@ static int irqsoff_tracer_init(struct trace_array *tr)return __irqsoff_tracer_init (tr); } + +static void irqsoff_tracer_reset (struct trace_array *tr) +{ +__irqsoff_tracer_reset (tr); +} +static struct tracer irqsoff_tracer__read_mostly = { .name ="irqsoff" ,
@@ -697,11 +703,16 @@ static int preemptoff_tracer_init(struct trace_array *tr)return __irqsoff_tracer_init (tr); } +static void preemptoff_tracer_reset (struct trace_array *tr) +{ +__irqsoff_tracer_reset (tr); +} +static struct tracer preemptoff_tracer__read_mostly = { .name ="preemptoff" , .init = preemptoff_tracer_init, - .reset = irqsoff_tracer_reset, + .reset = preemptoff_tracer_reset, .start = irqsoff_tracer_start, .stop = irqsoff_tracer_stop, .print_max = true,
@@ -731,11 +742,16 @@ static int preemptirqsoff_tracer_init(struct trace_array *tr)return __irqsoff_tracer_init (tr); } +static void preemptirqsoff_tracer_reset (struct trace_array *tr) +{ +__irqsoff_tracer_reset (tr); +} +static struct tracer preemptirqsoff_tracer__read_mostly = { .name ="preemptirqsoff" , .init = preemptirqsoff_tracer_init, - .reset = irqsoff_tracer_reset, + .reset = preemptirqsoff_tracer_reset, .start = irqsoff_tracer_start, .stop = irqsoff_tracer_stop, .print_max = true,
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
RFC PATCH v6 4/5 tracepoint: Make rcuidle tracepoint callers use SRCU
In reply to Joel FernandesFrom: "Joel Fernandes (Google)" <[email protected]>
In recent tests with IRQ on/off tracepoints, a large performance overhead ~10% is noticed when running hackbench. This is root caused to calls to rcu_irq_enter_irqson and rcu_irq_exit_irqson from the tracepoint code. Following a long discussion on the list [1] about this, we concluded that srcu is a better alternative for use during rcu idle. Although it does involve extra barriers, its lighter than the sched-rcu version which has to do additional RCU calls to notify RCU idle about entry into RCU sections.
In this patch, we change the underlying implementation of the trace_*_rcuidle API to use SRCU. This has shown to improve performance alot for the high frequency irq enable/disable tracepoints.
Test: Tested idle and preempt/irq tracepoints.
Here are some performance numbers:
With a run of the following 30 times on a single core x86 Qemu instance with 1GB memory:
hackbench -g 4 -f 2 -l 3000
Completion times in seconds. CONFIG_PROVE_LOCKING=y.
No patches (without this series)
Mean: 3.048
Median: 3.025
Std Dev: 0.064
With Lockdep using irq tracepoints with RCU implementation:
Mean: 3.451 (-11.66 %)
Median: 3.447 (-12.22%)
Std Dev: 0.049
With Lockdep using irq tracepoints with SRCU implementation (this series):
Mean: 3.020 (I would consider the improvement against the "without
this series" case as just noise).
Median: 3.013
Std Dev: 0.033
[1] https://patchwork.kernel.org/patch/10344297/
--- include/linux/tracepoint.h | 46 +++++++++++++++++++++++++++++++------- kernel/tracepoint.c | 15 ++++++++++++- 2 files changed, 52 insertions(+), 9 deletions(-)
include/linux/tracepoint.h +38 -8
@@ -15,6 +15,7 @@*/ #include <linux/smp.h> +#include <linux/srcu.h> #include <linux/errno.h> #include <linux/types.h> #include <linux/cpumask.h>
@@ -33,6 +34,8 @@ struct trace_eval_map {#define TRACEPOINT_DEFAULT_PRIO 10 +extern struct srcu_struct tracepoint_srcu; +extern int tracepoint_probe_register (struct tracepoint *tp,void *probe,void *data); extern int
@@ -77,6 +80,9 @@ int unregister_tracepoint_module_notifier(struct notifier_block *nb)*/ static inline void tracepoint_synchronize_unregister (void ) { +#ifdef CONFIG_TRACEPOINTS +synchronize_srcu (&tracepoint_srcu); +#endif synchronize_sched (); }
@@ -129,18 +135,38 @@ extern void syscall_unregfunc(void);* as "(void *, void)". The DECLARE_TRACE_NOARGS() will pass in just * "void *data", where as the DECLARE_TRACE() will pass in "void *data, proto". */ -#define __DO_TRACE (tp, proto, args, cond, rcucheck) \ +#define __DO_TRACE (tp, proto, args, cond, rcuidle) \ do { \ struct tracepoint_func *it_func_ptr; \ void *it_func; \ void *__data; \ + int __maybe_unused idx = 0; \ \ if (!(cond)) \ return; \ - if (rcucheck) \ - rcu_irq_enter_irqson(); \ - rcu_read_lock_sched_notrace(); \ - it_func_ptr = rcu_dereference_sched((tp)->funcs); \ + \ +/* \ +* For rcuidle callers, use srcu since sched-rcu \ +* doesn't work from the idle path. \ +*/ \ +if (rcuidle) { \ +if (in_nmi ()) { \ +WARN_ON_ONCE (1 ); \ +return ;/* no srcu from nmi */ \ + } \ + \ + idx =srcu_read_lock_notrace (&tracepoint_srcu); \ + it_func_ptr = \ +srcu_dereference_notrace ((tp)->funcs, \ + &tracepoint_srcu); \ +/* To keep it consistent with !rcuidle path */ \ +preempt_disable_notrace (); \ + }else { \ +rcu_read_lock_sched_notrace (); \ + it_func_ptr = \ +rcu_dereference_sched ((tp)->funcs); \ + } \ + \if (it_func_ptr) { \ do { \ it_func = (it_func_ptr)->func; \
@@ -148,9 +174,13 @@ extern void syscall_unregfunc(void); ((void (*)(proto ))(it_func))(args); \ }while ((++it_func_ptr)->func); \ } \ -rcu_read_unlock_sched_notrace (); \ -if (rcucheck) \ -rcu_irq_exit_irqson (); \ + \ +if (rcuidle) { \ +preempt_enable_notrace (); \ +srcu_read_unlock_notrace (&tracepoint_srcu, idx);\ + }else { \ +rcu_read_unlock_sched_notrace (); \ + } \ }while (0 ) #ifndef MODULE
kernel/tracepoint.c +14 -1
@@ -31,6 +31,9 @@extern struct tracepoint *const __start___tracepoints_ptrs[];extern struct tracepoint *const __stop___tracepoints_ptrs[]; +DEFINE_SRCU (tracepoint_srcu); +EXPORT_SYMBOL_GPL (tracepoint_srcu); +/* Set to 1 to enable tracepoint debug output */ static const int tracepoint_debug;
@@ -67,16 +70,26 @@ static inline void *allocate_probes(int count)return p ==NULL ?NULL : p->probes; } -static void rcu_free_old_probes (struct rcu_head *head) +static void srcu_free_old_probes (struct rcu_head *head) {kfree (container_of(head ,struct tp_probes ,rcu )); } +static void rcu_free_old_probes (struct rcu_head *head) +{ +call_srcu (&tracepoint_srcu, head, srcu_free_old_probes); +} +static inline void release_probes (struct tracepoint_func *old) {if (old) {struct tp_probes *tp_probes =container_of (old, struct tp_probes, probes[0]); +/* +* Tracepoint probes are protected by both sched RCU and SRCU, +* by calling the SRCU callback in the sched RCU callback we +* cover both cases. So lets chain the SRCU and RCU callbacks. +*/ call_rcu_sched (&tp_probes->rcu, rcu_free_old_probes); } }
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
Mathieu Desnoyers
reviewerRe: [PATCH RFC v6 4/5] tracepoint: Make rcuidle tracepoint callers use SRCU
----- On May 7, 2018, at 4:41 PM, Joel Fernandes [email protected] wrote:
[...]
Quoted 6 lines
include/linux/tracepoint.h +2
-0+extern struct srcu_struct tracepoint_srcu; +extern int
tracepoint_probe_register(struct tracepoint *tp, void *probe, void *data); extern int
include/linux/tracepoint.h int unregister_tracepoint_module_notifier(struct · line 87
Quoted 8 lines
include/linux/tracepoint.h +3
-0@@ -77,6 +80,9 @@ int unregister_tracepoint_module_notifier(structnotifier_block *nb) */ static inline void tracepoint_synchronize_unregister (void ) { +#ifdef CONFIG_TRACEPOINTS +synchronize_srcu (&tracepoint_srcu); +#endif synchronize_sched ();
Why is this ifdef needed ?
Thanks,
Mathieu
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [PATCH RFC v6 4/5] tracepoint: Make rcuidle tracepoint callers use SRCU
Quoted 18 lines · Mathieu Desnoyers
----- On May 7, 2018, at 4:41 PM, Joel Fernandes [email protected] wrote:
[...]Quoted 6 lines
include/linux/tracepoint.h +2
-0+extern struct srcu_struct tracepoint_srcu; +extern int
tracepoint_probe_register(struct tracepoint *tp, void *probe, void *data); extern intinclude/linux/tracepoint.h int unregister_tracepoint_module_notifier(struct · line 87
Quoted 8 lines
include/linux/tracepoint.h +3
-0@@ -77,6 +80,9 @@ int unregister_tracepoint_module_notifier(structnotifier_block *nb) */ static inline void tracepoint_synchronize_unregister (void ) { +#ifdef CONFIG_TRACEPOINTS +synchronize_srcu (&tracepoint_srcu); +#endif synchronize_sched ();Why is this ifdef needed ?
tracepoint_srcu is defined in tracepoint.c so if we don't protect usage here, it would cause a build error.
thanks,
- Joel
Mathieu Desnoyers
reviewerRe: [PATCH RFC v6 4/5] tracepoint: Make rcuidle tracepoint callers use SRCU
Quoted 20 lines · ----- On May 7, 2018, at 5:46 PM, Joel Fernandes, Google…
Quoted 18 lines · Mathieu Desnoyers
----- On May 7, 2018, at 4:41 PM, Joel Fernandes [email protected] wrote:
[...]Quoted 6 lines
include/linux/tracepoint.h +2
-0+extern struct srcu_struct tracepoint_srcu; +extern int
tracepoint_probe_register(struct tracepoint *tp, void *probe, void *data); extern intinclude/linux/tracepoint.h int unregister_tracepoint_module_notifier(struct · line 87
Quoted 8 lines
include/linux/tracepoint.h +3
-0@@ -77,6 +80,9 @@ int unregister_tracepoint_module_notifier(structnotifier_block *nb) */ static inline void tracepoint_synchronize_unregister (void ) { +#ifdef CONFIG_TRACEPOINTS +synchronize_srcu (&tracepoint_srcu); +#endif synchronize_sched ();Why is this ifdef needed ?
Quoted 2 lines
tracepoint_srcuis defined intracepoint.cso if we don't protect usage here, it would cause a build error.
Then we should ifdef the entire implementation of tracepoint_synchronize_unregister(). There is no point in issuing synchronize_sched() when code invokes that function on a CONFIG_TRACEPOINTS=n config.
Thanks,
Mathieu
Quoted 4 lines
thanks,
- Joel
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [PATCH RFC v6 4/5] tracepoint: Make rcuidle tracepoint callers use SRCU
Quoted 22 lines · Mathieu Desnoyers
Quoted 20 lines · ----- On May 7, 2018, at 5:46 PM, Joel Fernandes, Google…
Quoted 18 lines · Mathieu Desnoyers
----- On May 7, 2018, at 4:41 PM, Joel Fernandes [email protected] wrote:
[...]Quoted 6 lines
include/linux/tracepoint.h +2
-0+extern struct srcu_struct tracepoint_srcu; +extern int
tracepoint_probe_register(struct tracepoint *tp, void *probe, void *data); extern intinclude/linux/tracepoint.h int unregister_tracepoint_module_notifier(struct · line 87
Quoted 8 lines
include/linux/tracepoint.h +3
-0@@ -77,6 +80,9 @@ int unregister_tracepoint_module_notifier(structnotifier_block *nb) */ static inline void tracepoint_synchronize_unregister (void ) { +#ifdef CONFIG_TRACEPOINTS +synchronize_srcu (&tracepoint_srcu); +#endif synchronize_sched ();Why is this ifdef needed ?
Quoted 6 lines
Quoted 2 lines
tracepoint_srcuis defined intracepoint.cso if we don't protect usage here, it would cause a build error.Then we should ifdef the entire implementation of
tracepoint_synchronize_unregister(). There is no point in issuingsynchronize_sched()when code invokes that function on aCONFIG_TRACEPOINTS=nconfig.
Ok I'll do it this way in the next rev. Could you also share any thoughts on patch 6/6 if you had some time? Development of it has also been complete for some time.
thanks,
- Joel
Re: [PATCH RFC v6 4/5] tracepoint: Make rcuidle tracepoint callers use SRCU
In reply to Joel FernandesQuoted 141 lines · Joel Fernandes
From: "Joel Fernandes (Google)" <[email protected]>
In recent tests with IRQ on/off tracepoints, a large performance overhead ~10% is noticed when running hackbench. This is root caused to calls to
rcu_irq_enter_irqsonandrcu_irq_exit_irqsonfrom the tracepoint code. Following a long discussion on the list [1] about this, we concluded that srcu is a better alternative for use during rcu idle. Although it does involve extra barriers, its lighter than the sched-rcu version which has to do additional RCU calls to notify RCU idle about entry into RCU sections.In this patch, we change the underlying implementation of the trace_*_rcuidle API to use SRCU. This has shown to improve performance alot for the high frequency irq enable/disable tracepoints.
Test: Tested idle and preempt/irq tracepoints.
Here are some performance numbers:
With a run of the following 30 times on a single core x86 Qemu instance with 1GB memory:
hackbench -g 4 -f 2 -l 3000Completion times in seconds.
CONFIG_PROVE_LOCKING=y.No patches (without this series)
Mean: 3.048
Median: 3.025
Std Dev: 0.064With Lockdep using irq tracepoints with RCU implementation:
Mean: 3.451 (-11.66 %)
Median: 3.447 (-12.22%)
Std Dev: 0.049With Lockdep using irq tracepoints with SRCU implementation (this series):
Mean: 3.020 (I would consider the improvement against the "without
this series" case as just noise).
Median: 3.013
Std Dev: 0.033[1] https://patchwork.kernel.org/patch/10344297/
Cc Steven Rostedt <[email protected]>Cc Peter Zilstra <[email protected]>Cc Ingo Molnar <[email protected]>Cc Mathieu Desnoyers <[email protected]>Cc Tom Zanussi <[email protected]>Cc Namhyung Kim <[email protected]>Cc Thomas Glexiner <[email protected]>Cc Boqun Feng <[email protected]>Cc Paul McKenney <[email protected]>Cc Frederic Weisbecker <[email protected]>Cc Randy Dunlap <[email protected]>Cc Masami Hiramatsu <[email protected]>Cc Fenguang Wu <[email protected]>Cc Baohong Liu <[email protected]>Cc Vedang Patel <[email protected]>Signed-off-by Joel Fernandes (Google) <[email protected]>--- include/linux/tracepoint.h | 46 +++++++++++++++++++++++++++++++------- kernel/tracepoint.c | 15 ++++++++++++- 2 files changed, 52 insertions(+), 9 deletions(-)include/linux/tracepoint.h +31
-5@@ -15,6 +15,7 @@*/ #include <linux/smp.h> +#include <linux/srcu.h> #include <linux/errno.h> #include <linux/types.h> #include <linux/cpumask.h> @@ -33,6 +34,8 @@ struct trace_eval_map {#define TRACEPOINT_DEFAULT_PRIO 10 +extern struct srcu_struct tracepoint_srcu; +extern int tracepoint_probe_register (struct tracepoint *tp,void *probe,void *data); extern int@@ -77,6 +80,9 @@ int unregister_tracepoint_module_notifier(struct notifier_block *nb)*/ static inline void tracepoint_synchronize_unregister (void ) { +#ifdef CONFIG_TRACEPOINTS +synchronize_srcu (&tracepoint_srcu); +#endif synchronize_sched (); }@@ -129,18 +135,38 @@ extern void syscall_unregfunc(void);* as "(void *, void)". The DECLARE_TRACE_NOARGS() will pass in just * "void *data", where as the DECLARE_TRACE() will pass in "void *data, proto". */ -#define __DO_TRACE (tp, proto, args, cond, rcucheck) \ +#define __DO_TRACE (tp, proto, args, cond, rcuidle) \ do { \ struct tracepoint_func *it_func_ptr; \ void *it_func; \ void *__data; \ + int __maybe_unused idx = 0; \ \ if (!(cond)) \ return; \ - if (rcucheck) \ - rcu_irq_enter_irqson(); \ - rcu_read_lock_sched_notrace(); \ - it_func_ptr = rcu_dereference_sched((tp)->funcs); \ + \ +/* \ +* For rcuidle callers, use srcu since sched-rcu \ +* doesn't work from the idle path. \ +*/ \ +if (rcuidle) { \ +if (in_nmi ()) { \ +WARN_ON_ONCE (1 ); \ +return ;/* no srcu from nmi */ \ + } \ + \ + idx =srcu_read_lock_notrace (&tracepoint_srcu); \ + it_func_ptr = \ +srcu_dereference_notrace ((tp)->funcs, \ + &tracepoint_srcu); \ +/* To keep it consistent with !rcuidle path */ \ +preempt_disable_notrace (); \ + }else { \ +rcu_read_lock_sched_notrace (); \ + it_func_ptr = \ +rcu_dereference_sched ((tp)->funcs); \ + } \ + \if (it_func_ptr) { \ do { \ it_func = (it_func_ptr)->func; \
include/linux/tracepoint.h extern void syscall_unregfunc(void); · line 178
Quoted 8 lines
include/linux/tracepoint.h +2
-3@@ -148,9 +174,13 @@ extern void syscall_unregfunc(void); ((void (*)(proto ))(it_func))(args); \ }while ((++it_func_ptr)->func); \ } \ -rcu_read_unlock_sched_notrace (); \ -if (rcucheck) \ -rcu_irq_exit_irqson (); \ + \ +if (rcuidle) { \
Don't we also need an in_nmi() check here in order to avoid unbalanced srcu_read_unlock_notrace() calls?
Thanx, Paul
Quoted 53 lines
include/linux/tracepoint.h +5
-0+preempt_enable_notrace (); \ +srcu_read_unlock_notrace (&tracepoint_srcu, idx);\ + }else { \ +rcu_read_unlock_sched_notrace (); \ + } \ }while (0 ) #ifndef MODULEkernel/tracepoint.c +14
-1@@ -31,6 +31,9 @@extern struct tracepoint *const __start___tracepoints_ptrs[];extern struct tracepoint *const __stop___tracepoints_ptrs[]; +DEFINE_SRCU (tracepoint_srcu); +EXPORT_SYMBOL_GPL (tracepoint_srcu); +/* Set to 1 to enable tracepoint debug output */ static const int tracepoint_debug;@@ -67,16 +70,26 @@ static inline void *allocate_probes(int count)return p ==NULL ?NULL : p->probes; } -static void rcu_free_old_probes (struct rcu_head *head) +static void srcu_free_old_probes (struct rcu_head *head) {kfree (container_of(head ,struct tp_probes ,rcu )); } +static void rcu_free_old_probes (struct rcu_head *head) +{ +call_srcu (&tracepoint_srcu, head, srcu_free_old_probes); +} +static inline void release_probes (struct tracepoint_func *old) {if (old) {struct tp_probes *tp_probes =container_of (old, struct tp_probes, probes[0]); +/* +* Tracepoint probes are protected by both sched RCU and SRCU, +* by calling the SRCU callback in the sched RCU callback we +* cover both cases. So lets chain the SRCU and RCU callbacks. +*/ call_rcu_sched (&tp_probes->rcu, rcu_free_old_probes); } }Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
Mathieu Desnoyers
reviewerRe: [PATCH RFC v6 4/5] tracepoint: Make rcuidle tracepoint callers use SRCU
Quoted 146 lines · ----- On May 7, 2018, at 5:08 PM, Paul E. McKenney [email protected]…
Quoted 145 lines · Joel Fernandes
From: "Joel Fernandes (Google)" <[email protected]>
In recent tests with IRQ on/off tracepoints, a large performance overhead ~10% is noticed when running hackbench. This is root caused to calls to
rcu_irq_enter_irqsonandrcu_irq_exit_irqsonfrom the tracepoint code. Following a long discussion on the list [1] about this, we concluded that srcu is a better alternative for use during rcu idle. Although it does involve extra barriers, its lighter than the sched-rcu version which has to do additional RCU calls to notify RCU idle about entry into RCU sections.In this patch, we change the underlying implementation of the trace_*_rcuidle API to use SRCU. This has shown to improve performance alot for the high frequency irq enable/disable tracepoints.
Test: Tested idle and preempt/irq tracepoints.
Here are some performance numbers:
With a run of the following 30 times on a single core x86 Qemu instance with 1GB memory:
hackbench -g 4 -f 2 -l 3000Completion times in seconds.
CONFIG_PROVE_LOCKING=y.No patches (without this series)
Mean: 3.048
Median: 3.025
Std Dev: 0.064With Lockdep using irq tracepoints with RCU implementation:
Mean: 3.451 (-11.66 %)
Median: 3.447 (-12.22%)
Std Dev: 0.049With Lockdep using irq tracepoints with SRCU implementation (this series):
Mean: 3.020 (I would consider the improvement against the "without
this series" case as just noise).
Median: 3.013
Std Dev: 0.033[1] https://patchwork.kernel.org/patch/10344297/
Cc Steven Rostedt <[email protected]>Cc Peter Zilstra <[email protected]>Cc Ingo Molnar <[email protected]>Cc Mathieu Desnoyers <[email protected]>Cc Tom Zanussi <[email protected]>Cc Namhyung Kim <[email protected]>Cc Thomas Glexiner <[email protected]>Cc Boqun Feng <[email protected]>Cc Paul McKenney <[email protected]>Cc Frederic Weisbecker <[email protected]>Cc Randy Dunlap <[email protected]>Cc Masami Hiramatsu <[email protected]>Cc Fenguang Wu <[email protected]>Cc Baohong Liu <[email protected]>Cc Vedang Patel <[email protected]>Signed-off-by Joel Fernandes (Google) <[email protected]>--- include/linux/tracepoint.h | 46 +++++++++++++++++++++++++++++++------- kernel/tracepoint.c | 15 ++++++++++++- 2 files changed, 52 insertions(+), 9 deletions(-)include/linux/tracepoint.h +31
-5@@ -15,6 +15,7 @@*/ #include <linux/smp.h> +#include <linux/srcu.h> #include <linux/errno.h> #include <linux/types.h> #include <linux/cpumask.h> @@ -33,6 +34,8 @@ struct trace_eval_map {#define TRACEPOINT_DEFAULT_PRIO 10 +extern struct srcu_struct tracepoint_srcu; +extern int tracepoint_probe_register (struct tracepoint *tp,void *probe,void *data); extern int@@ -77,6 +80,9 @@ int unregister_tracepoint_module_notifier(structnotifier_block *nb) */ static inline void tracepoint_synchronize_unregister (void ) { +#ifdef CONFIG_TRACEPOINTS +synchronize_srcu (&tracepoint_srcu); +#endif synchronize_sched (); }@@ -129,18 +135,38 @@ extern void syscall_unregfunc(void);* as "(void *, void)". The DECLARE_TRACE_NOARGS() will pass in just * "void *data", where as the DECLARE_TRACE() will pass in "void *data, proto". */ -#define __DO_TRACE (tp, proto, args, cond, rcucheck) \ +#define __DO_TRACE (tp, proto, args, cond, rcuidle) \ do { \ struct tracepoint_func *it_func_ptr; \ void *it_func; \ void *__data; \ + int __maybe_unused idx = 0; \ \ if (!(cond)) \ return; \ - if (rcucheck) \ - rcu_irq_enter_irqson(); \ - rcu_read_lock_sched_notrace(); \ - it_func_ptr = rcu_dereference_sched((tp)->funcs); \ + \ +/* \ +* For rcuidle callers, use srcu since sched-rcu \ +* doesn't work from the idle path. \ +*/ \ +if (rcuidle) { \ +if (in_nmi ()) { \ +WARN_ON_ONCE (1 ); \ +return ;/* no srcu from nmi */ \ + } \ + \ + idx =srcu_read_lock_notrace (&tracepoint_srcu); \ + it_func_ptr = \ +srcu_dereference_notrace ((tp)->funcs, \ + &tracepoint_srcu); \ +/* To keep it consistent with !rcuidle path */ \ +preempt_disable_notrace (); \ + }else { \ +rcu_read_lock_sched_notrace (); \ + it_func_ptr = \ +rcu_dereference_sched ((tp)->funcs); \ + } \ + \if (it_func_ptr) { \ do { \ it_func = (it_func_ptr)->func; \@@ -148,9 +174,13 @@ extern void syscall_unregfunc(void); ((void (*)(proto ))(it_func))(args); \ }while ((++it_func_ptr)->func); \ } \
Quoted 8 lines
include/linux/tracepoint.h extern void syscall_unregfunc(void); · line 178
Quoted 5 lines
include/linux/tracepoint.h +2
-3-rcu_read_unlock_sched_notrace (); \ -if (rcucheck) \ -rcu_irq_exit_irqson (); \ + \ +if (rcuidle) { \Don't we also need an
in_nmi()check here in order to avoid unbalancedsrcu_read_unlock_notrace()calls?
AFAIU the "return;" in the if (in_nmi()) branch above takes care of never executing the following code. diff appears to be a bit confused by the preprocessor macros, but in reality this is all part of the same static inline function.
Thanks,
Mathieu
Quoted 55 lines
Thanx, Paul
Quoted 52 lines
include/linux/tracepoint.h +5
-0+preempt_enable_notrace (); \ +srcu_read_unlock_notrace (&tracepoint_srcu, idx);\ + }else { \ +rcu_read_unlock_sched_notrace (); \ + } \ }while (0 ) #ifndef MODULEkernel/tracepoint.c +14
-2@@ -31,6 +31,9 @@extern struct tracepoint *const __start___tracepoints_ptrs[];extern struct tracepoint *const __stop___tracepoints_ptrs[]; +DEFINE_SRCU (tracepoint_srcu); +EXPORT_SYMBOL_GPL (tracepoint_srcu); +/* Set to 1 to enable tracepoint debug output */ static const int tracepoint_debug;@@ -67,16 +70,26 @@ static inline void *allocate_probes(int count)return p ==NULL ?NULL : p->probes; } -static void rcu_free_old_probes (struct rcu_head *head) +static void srcu_free_old_probes (struct rcu_head *head) {kfree (container_of(head ,struct tp_probes ,rcu )); } +static void rcu_free_old_probes (struct rcu_head *head) +{ +call_srcu (&tracepoint_srcu, head, srcu_free_old_probes); +} +static inline void release_probes (struct tracepoint_func *old) {if (old) {struct tp_probes *tp_probes =container_of (old, struct tp_probes, probes[0]); +/* +* Tracepoint probes are protected by both sched RCU and SRCU, +* by calling the SRCU callback in the sched RCU callback we +* cover both cases. So lets chain the SRCU and RCU callbacks. +*/ call_rcu_sched (&tp_probes->rcu, rcu_free_old_probes); } } --2.17.0.441.gb46fe60e1d-goog
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [PATCH RFC v6 4/5] tracepoint: Make rcuidle tracepoint callers use SRCU
In reply to Paul E. McKenneyQuoted 15 lines · Paul E. McKenney
Quoted 14 lines · Joel Fernandes
From: "Joel Fernandes (Google)" <[email protected]>
In recent tests with IRQ on/off tracepoints, a large performance overhead ~10% is noticed when running hackbench. This is root caused to calls to
rcu_irq_enter_irqsonandrcu_irq_exit_irqsonfrom the tracepoint code. Following a long discussion on the list [1] about this, we concluded that srcu is a better alternative for use during rcu idle. Although it does involve extra barriers, its lighter than the sched-rcu version which has to do additional RCU calls to notify RCU idle about entry into RCU sections.In this patch, we change the underlying implementation of the trace_*_rcuidle API to use SRCU. This has shown to improve performance alot for the high frequency irq enable/disable tracepoints.
[...]
Quoted 85 lines
Quoted 85 lines
Signed-off-by Joel Fernandes (Google) <[email protected]>--- include/linux/tracepoint.h | 46 +++++++++++++++++++++++++++++++------- kernel/tracepoint.c | 15 ++++++++++++- 2 files changed, 52 insertions(+), 9 deletions(-)include/linux/tracepoint.h +31
-5@@ -15,6 +15,7 @@*/ #include <linux/smp.h> +#include <linux/srcu.h> #include <linux/errno.h> #include <linux/types.h> #include <linux/cpumask.h> @@ -33,6 +34,8 @@ struct trace_eval_map {#define TRACEPOINT_DEFAULT_PRIO 10 +extern struct srcu_struct tracepoint_srcu; +extern int tracepoint_probe_register (struct tracepoint *tp,void *probe,void *data); extern int@@ -77,6 +80,9 @@ int unregister_tracepoint_module_notifier(struct notifier_block *nb)*/ static inline void tracepoint_synchronize_unregister (void ) { +#ifdef CONFIG_TRACEPOINTS +synchronize_srcu (&tracepoint_srcu); +#endif synchronize_sched (); }@@ -129,18 +135,38 @@ extern void syscall_unregfunc(void);* as "(void *, void)". The DECLARE_TRACE_NOARGS() will pass in just * "void *data", where as the DECLARE_TRACE() will pass in "void *data, proto". */ -#define __DO_TRACE (tp, proto, args, cond, rcucheck) \ +#define __DO_TRACE (tp, proto, args, cond, rcuidle) \ do { \ struct tracepoint_func *it_func_ptr; \ void *it_func; \ void *__data; \ + int __maybe_unused idx = 0; \ \ if (!(cond)) \ return; \ - if (rcucheck) \ - rcu_irq_enter_irqson(); \ - rcu_read_lock_sched_notrace(); \ - it_func_ptr = rcu_dereference_sched((tp)->funcs); \ + \ +/* \ +* For rcuidle callers, use srcu since sched-rcu \ +* doesn't work from the idle path. \ +*/ \ +if (rcuidle) { \ +if (in_nmi ()) { \ +WARN_ON_ONCE (1 ); \ +return ;/* no srcu from nmi */ \ + } \ + \ + idx =srcu_read_lock_notrace (&tracepoint_srcu); \ + it_func_ptr = \ +srcu_dereference_notrace ((tp)->funcs, \ + &tracepoint_srcu); \ +/* To keep it consistent with !rcuidle path */ \ +preempt_disable_notrace (); \ + }else { \ +rcu_read_lock_sched_notrace (); \ + it_func_ptr = \ +rcu_dereference_sched ((tp)->funcs); \ + } \ + \if (it_func_ptr) { \ do { \ it_func = (it_func_ptr)->func; \@@ -148,9 +174,13 @@ extern void syscall_unregfunc(void); ((void (*)(proto ))(it_func))(args); \ }while ((++it_func_ptr)->func); \ } \
Quoted 8 lines
include/linux/tracepoint.h extern void syscall_unregfunc(void); · line 178
Quoted 5 lines
include/linux/tracepoint.h +2
-3-rcu_read_unlock_sched_notrace (); \ -if (rcucheck) \ -rcu_irq_exit_irqson (); \ + \ +if (rcuidle) { \Don't we also need an
in_nmi()check here in order to avoid unbalancedsrcu_read_unlock_notrace()calls?
The in_nmi() in the lock path should take care of making sure its balanced.
The diff the way its formatted appears confusing as Mathieu pointed.
thanks,
- Joel
Re: [PATCH RFC v6 4/5] tracepoint: Make rcuidle tracepoint callers use SRCU
Quoted 111 lines · Joel Fernandes
Quoted 15 lines · Paul E. McKenney
Quoted 14 lines · Joel Fernandes
From: "Joel Fernandes (Google)" <[email protected]>
In recent tests with IRQ on/off tracepoints, a large performance overhead ~10% is noticed when running hackbench. This is root caused to calls to
rcu_irq_enter_irqsonandrcu_irq_exit_irqsonfrom the tracepoint code. Following a long discussion on the list [1] about this, we concluded that srcu is a better alternative for use during rcu idle. Although it does involve extra barriers, its lighter than the sched-rcu version which has to do additional RCU calls to notify RCU idle about entry into RCU sections.In this patch, we change the underlying implementation of the trace_*_rcuidle API to use SRCU. This has shown to improve performance alot for the high frequency irq enable/disable tracepoints.
[...]
Quoted 85 lines
Quoted 85 lines
Signed-off-by Joel Fernandes (Google) <[email protected]>--- include/linux/tracepoint.h | 46 +++++++++++++++++++++++++++++++------- kernel/tracepoint.c | 15 ++++++++++++- 2 files changed, 52 insertions(+), 9 deletions(-)include/linux/tracepoint.h +31
-5@@ -15,6 +15,7 @@*/ #include <linux/smp.h> +#include <linux/srcu.h> #include <linux/errno.h> #include <linux/types.h> #include <linux/cpumask.h> @@ -33,6 +34,8 @@ struct trace_eval_map {#define TRACEPOINT_DEFAULT_PRIO 10 +extern struct srcu_struct tracepoint_srcu; +extern int tracepoint_probe_register (struct tracepoint *tp,void *probe,void *data); extern int@@ -77,6 +80,9 @@ int unregister_tracepoint_module_notifier(struct notifier_block *nb)*/ static inline void tracepoint_synchronize_unregister (void ) { +#ifdef CONFIG_TRACEPOINTS +synchronize_srcu (&tracepoint_srcu); +#endif synchronize_sched (); }@@ -129,18 +135,38 @@ extern void syscall_unregfunc(void);* as "(void *, void)". The DECLARE_TRACE_NOARGS() will pass in just * "void *data", where as the DECLARE_TRACE() will pass in "void *data, proto". */ -#define __DO_TRACE (tp, proto, args, cond, rcucheck) \ +#define __DO_TRACE (tp, proto, args, cond, rcuidle) \ do { \ struct tracepoint_func *it_func_ptr; \ void *it_func; \ void *__data; \ + int __maybe_unused idx = 0; \ \ if (!(cond)) \ return; \ - if (rcucheck) \ - rcu_irq_enter_irqson(); \ - rcu_read_lock_sched_notrace(); \ - it_func_ptr = rcu_dereference_sched((tp)->funcs); \ + \ +/* \ +* For rcuidle callers, use srcu since sched-rcu \ +* doesn't work from the idle path. \ +*/ \ +if (rcuidle) { \ +if (in_nmi ()) { \ +WARN_ON_ONCE (1 ); \ +return ;/* no srcu from nmi */ \ + } \ + \ + idx =srcu_read_lock_notrace (&tracepoint_srcu); \ + it_func_ptr = \ +srcu_dereference_notrace ((tp)->funcs, \ + &tracepoint_srcu); \ +/* To keep it consistent with !rcuidle path */ \ +preempt_disable_notrace (); \ + }else { \ +rcu_read_lock_sched_notrace (); \ + it_func_ptr = \ +rcu_dereference_sched ((tp)->funcs); \ + } \ + \if (it_func_ptr) { \ do { \ it_func = (it_func_ptr)->func; \@@ -148,9 +174,13 @@ extern void syscall_unregfunc(void); ((void (*)(proto ))(it_func))(args); \ }while ((++it_func_ptr)->func); \ } \Quoted 8 lines
include/linux/tracepoint.h extern void syscall_unregfunc(void); · line 178
Quoted 5 lines
include/linux/tracepoint.h +2
-3-rcu_read_unlock_sched_notrace (); \ -if (rcucheck) \ -rcu_irq_exit_irqson (); \ + \ +if (rcuidle) { \Don't we also need an
in_nmi()check here in order to avoid unbalancedsrcu_read_unlock_notrace()calls?
Quoted 3 lines
The
in_nmi()in the lock path should take care of making sure its balanced.The diff the way its formatted appears confusing as Mathieu pointed.
Ah, right, I was for some reason thinking that the two hunks of the diff were two separate macros. Apologies for my confusion!
Thanx, Paul
Re: [PATCH RFC v6 4/5] tracepoint: Make rcuidle tracepoint callers use SRCU
Quoted 125 lines · Paul E. McKenney
Quoted 6 lines · Joel Fernandes
Quoted 5 lines · Paul E. McKenney
Quoted 4 lines · Joel Fernandes
From: "Joel Fernandes (Google)" <[email protected]>
In recent tests with IRQ on/off tracepoints, a large performance overhead ~10% is noticed when running hackbench. This is root
caused to
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
calls to
rcu_irq_enter_irqsonandrcu_irq_exit_irqsonfrom the tracepoint code. Following a long discussion on the list [1]about this,
Quoted 1 line
Quoted 1 line
Quoted 1 line
we concluded that srcu is a better alternative for use during rcu
idle.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Although it does involve extra barriers, its lighter than the
sched-rcu
Quoted 1 line
Quoted 1 line
Quoted 1 line
version which has to do additional RCU calls to notify RCU idle
about
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
entry into RCU sections.
In this patch, we change the underlying implementation of the trace_*_rcuidle API to use SRCU. This has shown to improve
performance
Quoted 5 lines
Quoted 1 line
Quoted 1 line
alot for the high frequency irq enable/disable tracepoints.
[...]
Quoted 3 lines
Quoted 3 lines
Signed-off-by Joel Fernandes (Google) <[email protected]>--- include/linux/tracepoint.h | 46+++++++++++++++++++++++++++++++-------
Quoted 4 lines
Quoted 4 lines
Review
Quoted 4 lines
kernel/tracepoint.c | 15 ++++++++++++- 2 files changed, 52 insertions(+), 9 deletions(-)(quoted diff) +0
-0
b/include/linux/tracepoint.hQuoted 13 lines
Quoted 13 lines
Quoted 13 lines
index
c94f466d57ef..f56f290cf8eb100644include/linux/tracepoint.h +1
-0@@ -15,6 +15,7 @@*/ #include <linux/smp.h> +#include <linux/srcu.h> #include <linux/errno.h> #include <linux/types.h> #include <linux/cpumask.h> @@ -33,6 +34,8 @@ struct trace_eval_map {Quoted 6 lines
Quoted 6 lines
include/linux/tracepoint.h struct trace_eval_map {
Quoted 6 lines
include/linux/tracepoint.h +2
-0#define TRACEPOINT_DEFAULT_PRIO 10 +extern struct srcu_struct tracepoint_srcu; + extern int tracepoint_probe_register(struct tracepoint *tp, void *probe,void *data);
Quoted 2 lines
Quoted 2 lines
include/linux/tracepoint.h int
Quoted 2 lines
include/linux/tracepoint.h +0
-0extern int@@ -77,6 +80,9 @@ int
unregister_tracepoint_module_notifier(struct notifier_block *nb)Quoted 11 lines
Quoted 11 lines
include/linux/tracepoint.h extern void syscall_unregfunc(void); · line 135
Quoted 11 lines
include/linux/tracepoint.h +3
-0*/ static inline void tracepoint_synchronize_unregister (void ) { +#ifdef CONFIG_TRACEPOINTS +synchronize_srcu (&tracepoint_srcu); +#endif synchronize_sched (); }@@ -129,18 +135,38 @@ extern void syscall_unregfunc(void);* as "(void *, void)". The DECLARE_TRACE_NOARGS() will pass in just
Quoted 1 line
Quoted 1 line
include/linux/tracepoint.h extern void syscall_unregfunc(void); · line 136
Quoted 1 line
include/linux/tracepoint.h +0
-0* "void *data", where as the DECLARE_TRACE() will pass in "void *data, proto".
Quoted 44 lines
Quoted 44 lines
Quoted 44 lines
include/linux/tracepoint.h +25
-5*/ -#define __DO_TRACE (tp, proto, args, cond, rcucheck) \ +#define __DO_TRACE (tp, proto, args, cond, rcuidle) \ do { \ struct tracepoint_func *it_func_ptr; \ void *it_func; \ void *__data; \ + int __maybe_unused idx = 0; \ \ if (!(cond)) \ return; \ - if (rcucheck) \ - rcu_irq_enter_irqson(); \ - rcu_read_lock_sched_notrace(); \ - it_func_ptr = rcu_dereference_sched((tp)->funcs); \ + \ +/* \ +* For rcuidle callers, use srcu since sched-rcu \ +* doesn't work from the idle path. \ +*/ \ +if (rcuidle) { \ +if (in_nmi ()) { \ +WARN_ON_ONCE (1 ); \ +return ;/* no srcu from nmi */ \ + } \ + \ + idx =srcu_read_lock_notrace (&tracepoint_srcu); \ + it_func_ptr = \ +srcu_dereference_notrace ((tp)->funcs, \ + &tracepoint_srcu); \ +/* To keep it consistent with !rcuidle path */ \ +preempt_disable_notrace (); \ + }else { \ +rcu_read_lock_sched_notrace (); \ + it_func_ptr = \ +rcu_dereference_sched ((tp)->funcs); \ + } \ + \if (it_func_ptr) { \ do { \ it_func = (it_func_ptr)->func; \@@ -148,9 +174,13 @@ extern void syscall_unregfunc(void); ((void (*)(proto ))(it_func))(args); \ }while ((++it_func_ptr)->func); \Quoted 8 lines
Quoted 8 lines
include/linux/tracepoint.h extern void syscall_unregfunc(void); · line 178
Quoted 6 lines
include/linux/tracepoint.h +2
-3} \ -rcu_read_unlock_sched_notrace (); \ -if (rcucheck) \ -rcu_irq_exit_irqson (); \ + \ +if (rcuidle) { \Don't we also need an
in_nmi()check here in order to avoidunbalanced
Quoted 2 lines
Quoted 1 line
srcu_read_unlock_notrace()calls?
Quoted 8 lines
Quoted 1 line
The
in_nmi()in the lock path should take care of making sure itsbalanced.
Quoted 2 lines
The diff the way its formatted appears confusing as Mathieu pointed.
Ah, right, I was for some reason thinking that the two hunks of the diff were two separate macros. Apologies for my confusion!
No worries! Thanks for the great idea to use in_nmi in the first place.
- Joel
Quoted 1 line
Thanx, Paul
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.
RFC PATCH v6 5/5 tracing: Centralize preemptirq tracepoints and unify their usage
In reply to Joel FernandesFrom: "Joel Fernandes (Google)" <[email protected]>
This patch detaches the preemptirq tracepoints from the tracers and keeps it separate.
Advantages:
* Lockdep and irqsoff event can now run in parallel since they no longer have their own calls.
* This unifies the usecase of adding hooks to an irqsoff and irqson event, and a preemptoff and preempton event.
3 users of the events exist:
- Lockdep
- irqsoff and preemptoff tracers
- irqs and preempt trace events
The unification cleans up several ifdefs and makes the code in preempt tracer and irqsoff tracers simpler. It gets rid of all the horrific ifdeferry around PROVE_LOCKING and makes configuration of the different users of the tracepoints more easy and understandable. It also gets rid of the time_* function calls from the lockdep hooks used to call into the preemptirq tracer which is not needed anymore. The negative delta in lines of code in this patch is quite large too.
In the patch we introduce a new CONFIG option PREEMPTIRQ_TRACEPOINTS as a single point for registering probes onto the tracepoints. With this,
the web of config options for preempt/irq toggle tracepoints and its users becomes:
PREEMPT_TRACER PREEMPTIRQ_EVENTS IRQSOFF_TRACER PROVE_LOCKING
| | \ | |
\ (selects) / \ \ (selects) /
TRACE_PREEMPT_TOGGLE ----> TRACE_IRQFLAGS
\ /
\ (depends on) / PREEMPTIRQ_TRACEPOINTS
One note, I have to check for lockdep recursion in the code that calls the trace events API and bail out if we're in lockdep recursion protection to prevent something like the following case: a spin_lock is taken. Then lockdep_acquired is called. That does a raw_local_irq_save and then sets lockdep_recursion, and then calls __lockdep_acquired. In this function, a call to get_lock_stats happens which calls preempt_disable, which calls trace IRQS off somewhere which enters my tracepoint code and sets the tracing_irq_cpu flag to prevent recursion. This flag is then never cleared causing lockdep paths to never be entered and thus causing splats and other bad things.
--- include/linux/ftrace.h | 11 +- include/linux/irqflags.h | 11 +- include/linux/lockdep.h | 8 +- include/linux/preempt.h | 2 +- include/trace/events/preemptirq.h | 23 ++-- init/main.c | 5 +- kernel/locking/lockdep.c | 35 ++--- kernel/sched/core.c | 2 +- kernel/trace/Kconfig | 22 ++- kernel/trace/Makefile | 2 +- kernel/trace/trace_irqsoff.c | 213 ++++++++---------------------- kernel/trace/trace_preemptirq.c | 71 ++++++++++ 12 files changed, 189 insertions(+), 216 deletions(-) create mode 100644 kernel/trace/trace_preemptirq.c
include/linux/ftrace.h +1 -10
@@ -709,16 +709,7 @@ static inline unsigned long get_lock_parent_ip(void)return CALLER_ADDR2 ; } -#ifdef CONFIG_IRQSOFF_TRACER -extern void time_hardirqs_on (unsigned long a0,unsigned long a1); -extern void time_hardirqs_off (unsigned long a0,unsigned long a1); -#else -static inline void time_hardirqs_on (unsigned long a0,unsigned long a1) { } -static inline void time_hardirqs_off (unsigned long a0,unsigned long a1) { } -#endif - -#if defined(CONFIG_PREEMPT_TRACER ) || \ - (defined(CONFIG_DEBUG_PREEMPT ) && defined(CONFIG_PREEMPTIRQ_EVENTS )) +#ifdef CONFIG_TRACE_PREEMPT_TOGGLE extern void trace_preempt_on (unsigned long a0,unsigned long a1);extern void trace_preempt_off (unsigned long a0,unsigned long a1);#else
include/linux/irqflags.h +8 -3
@@ -15,9 +15,16 @@#include <linux/typecheck.h> #include <asm/irqflags.h> -#ifdef CONFIG_TRACE_IRQFLAGS +/* Currently trace_softirqs_on/off is used only by lockdep */ +#ifdef CONFIG_PROVE_LOCKING extern void trace_softirqs_on (unsigned long ip);extern void trace_softirqs_off (unsigned long ip); +#else +# define trace_softirqs_on (ip) do { } while (0) +# define trace_softirqs_off (ip) do { } while (0) +#endif + +#ifdef CONFIG_TRACE_IRQFLAGS extern void trace_hardirqs_on (void );extern void trace_hardirqs_off (void );# define trace_hardirq_context (p) ((p)->hardirq_context)
@@ -43,8 +50,6 @@ do { \#else # define trace_hardirqs_on () do { } while (0)# define trace_hardirqs_off () do { } while (0) -# define trace_softirqs_on (ip) do { } while (0) -# define trace_softirqs_off (ip) do { } while (0)# define trace_hardirq_context (p) 0# define trace_softirq_context (p) 0# define trace_hardirqs_enabled (p) 0
include/linux/lockdep.h +5 -3
@@ -266,7 +266,8 @@ struct held_lock {/* * Initialization, self-test and debugging-output methods: */ -extern void lockdep_info (void ); +extern void lockdep_init (void ); +extern void lockdep_init_early (void );extern void lockdep_reset (void );extern void lockdep_reset_lock (struct lockdep_map *lock);extern void lockdep_free_key_range (void *start,unsigned long size);
@@ -406,7 +407,8 @@ static inline void lockdep_on(void)# define lock_downgrade (l, i) do { } while (0)# define lock_set_class (l, n, k, s, i) do { } while (0)# define lock_set_subclass (l, s, i) do { } while (0) -# define lockdep_info () do { } while (0) +# define lockdep_init () do { } while (0) +# define lockdep_init_early () do { } while (0)# define lockdep_init_map (lock, name, key, sub) \ do { (void)(name); (void)(key); } while (0)# define lockdep_set_class (lock, key) do { (void)(key); } while (0)
@@ -532,7 +534,7 @@ do { \#endif /* CONFIG_LOCKDEP */ -#ifdef CONFIG_TRACE_IRQFLAGS +#ifdef CONFIG_PROVE_LOCKING extern void print_irqtrace_events (struct task_struct *curr);#else static inline void print_irqtrace_events (struct task_struct *curr)
include/linux/preempt.h +1 -1
@@ -150,7 +150,7 @@*/ #define in_atomic_preempt_off () (preempt_count() != PREEMPT_DISABLE_OFFSET) -#if defined(CONFIG_DEBUG_PREEMPT ) || defined(CONFIG_PREEMPT_TRACER ) +#if defined(CONFIG_DEBUG_PREEMPT ) || defined(CONFIG_TRACE_PREEMPT_TOGGLE )extern void preempt_count_add (int val);extern void preempt_count_sub (int val);#define preempt_count_dec_and_test () \
include/trace/events/preemptirq.h +14 -9
@@ -1,4 +1,4 @@ -#ifdef CONFIG_PREEMPTIRQ_EVENTS +#ifdef CONFIG_PREEMPTIRQ_TRACEPOINTS #undef TRACE_SYSTEM#define TRACE_SYSTEM preemptirq
@@ -32,7 +32,7 @@ DECLARE_EVENT_CLASS(preemptirq_template, (void *)((unsigned long)(_stext) + __entry->parent_offs)) ); -#ifndef CONFIG_PROVE_LOCKING +#ifdef CONFIG_TRACE_IRQFLAGS DEFINE_EVENT (preemptirq_template, irq_disable, TP_PROTO(unsigned long ip, unsigned long parent_ip),TP_ARGS (ip, parent_ip));
@@ -40,9 +40,14 @@ DEFINE_EVENT(preemptirq_template, irq_disable,DEFINE_EVENT (preemptirq_template, irq_enable, TP_PROTO(unsigned long ip, unsigned long parent_ip),TP_ARGS (ip, parent_ip)); +#else +#define trace_irq_enable (...) +#define trace_irq_disable (...) +#define trace_irq_enable_rcuidle (...) +#define trace_irq_disable_rcuidle (...)#endif -#ifdef CONFIG_DEBUG_PREEMPT +#ifdef CONFIG_TRACE_PREEMPT_TOGGLE DEFINE_EVENT (preemptirq_template, preempt_disable, TP_PROTO(unsigned long ip, unsigned long parent_ip),TP_ARGS (ip, parent_ip));
@@ -50,22 +55,22 @@ DEFINE_EVENT(preemptirq_template, preempt_disable,DEFINE_EVENT (preemptirq_template, preempt_enable, TP_PROTO(unsigned long ip, unsigned long parent_ip),TP_ARGS (ip, parent_ip)); +#else +#define trace_preempt_enable (...) +#define trace_preempt_disable (...) +#define trace_preempt_enable_rcuidle (...) +#define trace_preempt_disable_rcuidle (...)#endif #endif /* _TRACE_PREEMPTIRQ_H */ #include <trace/define_trace.h> -#endif /* !CONFIG_PREEMPTIRQ_EVENTS */ - -#if !defined(CONFIG_PREEMPTIRQ_EVENTS ) || defined(CONFIG_PROVE_LOCKING ) +#else /* !CONFIG_PREEMPTIRQ_TRACEPOINTS */ #define trace_irq_enable (...)#define trace_irq_disable (...)#define trace_irq_enable_rcuidle (...)#define trace_irq_disable_rcuidle (...) -#endif - -#if !defined(CONFIG_PREEMPTIRQ_EVENTS ) || !defined(CONFIG_DEBUG_PREEMPT )#define trace_preempt_enable (...)#define trace_preempt_disable (...)#define trace_preempt_enable_rcuidle (...)
init/main.c +4 -1
@@ -630,6 +630,9 @@ asmlinkage __visible void __init start_kernel(void)call_function_init ();WARN (!irqs_disabled (),"Interrupts were enabled early\n" ); early_boot_irqs_disabled = false; + +lockdep_init_early (); +local_irq_enable ();kmem_cache_init_late ();
@@ -644,7 +647,7 @@ asmlinkage __visible void __init start_kernel(void)panic ("Too many boot %s vars at `%s'" , panic_later, panic_param); -lockdep_info (); +lockdep_init ();/* * Need to run this when irqs are enabled, because it wants
kernel/locking/lockdep.c +14 -21
@@ -55,6 +55,7 @@#include "lockdep_internals.h" +#include <trace/events/preemptirq.h> #define CREATE_TRACE_POINTS #include <trace/events/lock.h>
@@ -2841,10 +2842,9 @@ static void __trace_hardirqs_on_caller(unsigned long ip)debug_atomic_inc (hardirqs_on_events); } -__visible void trace_hardirqs_on_caller(unsigned long ip) +static void lockdep_hardirqs_on (void *none,unsigned long ignore, +unsigned long ip) { -time_hardirqs_on (CALLER_ADDR0 , ip); -if (unlikely (!debug_locks || current->lockdep_recursion))return ;
@@ -2883,23 +2883,15 @@ __visible void trace_hardirqs_on_caller(unsigned long ip)__trace_hardirqs_on_caller (ip); current->lockdep_recursion =0 ; } -EXPORT_SYMBOL (trace_hardirqs_on_caller); - -void trace_hardirqs_on (void ) -{ -trace_hardirqs_on_caller (CALLER_ADDR0 ); -} -EXPORT_SYMBOL (trace_hardirqs_on);/* * Hardirqs were disabled: */ -__visible void trace_hardirqs_off_caller(unsigned long ip) +static void lockdep_hardirqs_off (void *none,unsigned long ignore, +unsigned long ip) {struct task_struct *curr = current; -time_hardirqs_off (CALLER_ADDR0 , ip); -if (unlikely (!debug_locks || current->lockdep_recursion))return ;
@@ -2921,13 +2913,6 @@ __visible void trace_hardirqs_off_caller(unsigned long ip) }else debug_atomic_inc (redundant_hardirqs_off); } -EXPORT_SYMBOL (trace_hardirqs_off_caller); - -void trace_hardirqs_off (void ) -{ -trace_hardirqs_off_caller (CALLER_ADDR0 ); -} -EXPORT_SYMBOL (trace_hardirqs_off);/* * Softirqs will be enabled:
@@ -4334,7 +4319,15 @@ void lockdep_reset_lock(struct lockdep_map *lock)raw_local_irq_restore (flags); } -void __init lockdep_info (void ) +void __init lockdep_init_early (void ) +{ +#ifdef CONFIG_PROVE_LOCKING +register_trace_prio_irq_disable (lockdep_hardirqs_off,NULL ,INT_MAX ); +register_trace_prio_irq_enable (lockdep_hardirqs_on,NULL ,INT_MIN ); +#endif +} + +void __init lockdep_init (void ) {printk ("Lock dependency validator: Copyright (c) 2006 Red Hat, Inc., Ingo Molnar\n" );
kernel/sched/core.c +1 -1
@@ -3128,7 +3128,7 @@ u64 scheduler_tick_max_deferment(void)#endif #if defined(CONFIG_PREEMPT ) && (defined(CONFIG_DEBUG_PREEMPT ) || \ - defined(CONFIG_PREEMPT_TRACER )) + defined(CONFIG_TRACE_PREEMPT_TOGGLE ))/* * If the value passed in is equal to the current preempt count * then we just disabled preemption. Start timing the latency.
kernel/trace/Kconfig +17 -5
@@ -82,6 +82,15 @@ config RING_BUFFER_ALLOW_SWAP Allow the use of ring_buffer_swap_cpu. Adds a very slight overhead to tracing when enabled. +config PREEMPTIRQ_TRACEPOINTS + bool + depends on TRACE_PREEMPT_TOGGLE || TRACE_IRQFLAGS + select TRACING + default y + help + Create preempt/irq toggle tracepoints if needed, so that other parts + of the kernel can use them to generate or add hooks to them. + # All tracer options should select GENERIC_TRACER. For those options that are # enabled by all tracers (context switch and event tracer) they select TRACING. # This allows those options to appear when no other tracer is selected. But the
@@ -159,18 +168,20 @@ config FUNCTION_GRAPH_TRACER the return value. This is done by setting the current return address on the current task structure into a stack of calls. +config TRACE_PREEMPT_TOGGLE + bool + help + Enables hooks which will be called when preemption is first disabled, + and last enabled. config PREEMPTIRQ_EVENTS bool "Enable trace events for preempt and irq disable/enable" select TRACE_IRQFLAGS - depends on DEBUG_PREEMPT || !PROVE_LOCKING - depends on TRACING + select TRACE_PREEMPT_TOGGLE if PREEMPT + select GENERIC_TRACER default n help Enable tracing of disable and enable events for preemption and irqs. - For tracing preempt disable/enable events, DEBUG_PREEMPT must be - enabled. For tracing irq disable/enable events, PROVE_LOCKING must - be disabled. config IRQSOFF_TRACER bool "Interrupts-off Latency Tracer"
@@ -207,6 +218,7 @@ config PREEMPT_TRACER select RING_BUFFER_ALLOW_SWAP select TRACER_SNAPSHOT select TRACER_SNAPSHOT_PER_CPU_SWAP + select TRACE_PREEMPT_TOGGLE help This option measures the time spent in preemption-off critical sections, with microsecond accuracy.
kernel/trace/Makefile +1 -1
@@ -35,7 +35,7 @@ obj-$(CONFIG_TRACING) += trace_printk.o obj-$(CONFIG_TRACING_MAP) += tracing_map.o obj-$(CONFIG_CONTEXT_SWITCH_TRACER) += trace_sched_switch.o obj-$(CONFIG_FUNCTION_TRACER) += trace_functions.o -obj-$(CONFIG_PREEMPTIRQ_EVENTS) += trace_irqsoff.o +obj-$(CONFIG_PREEMPTIRQ_TRACEPOINTS) += trace_preemptirq.o obj-$(CONFIG_IRQSOFF_TRACER) += trace_irqsoff.o obj-$(CONFIG_PREEMPT_TRACER) += trace_irqsoff.o obj-$(CONFIG_SCHED_TRACER) += trace_sched_wakeup.o
kernel/trace/trace_irqsoff.c +52 -161
279 lines not on this page · raw message
kernel/trace/trace_preemptirq.c new file +71 -0
71 lines not on this page · raw message
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog