Hi Binoy,
Some minor nitpicking.
On 08/29/2016 08:55 AM, Binoy Jayan wrote:
+DECLARE_EVENT_CLASS(latency_template,
- TP_PROTO(int ltype, int cpu, cycles_t latency),
- TP_ARGS(ltype, cpu, latency),
- TP_STRUCT__entry(
__field(int, ltype)__field(int, cpu)__field(cycles_t, latency)- ),
- TP_fast_assign(
__entry->ltype = ltype;__entry->cpu = cpu;__entry->latency = latency;- ),
- TP_printk("ltype=%d, cpu=%d, latency=%lu",
__entry->ltype, __entry->cpu, (unsigned long) __entry->latency)+);
As Steven already pointed out, the cpu field is available in all traces. It's one of the predefined fields (see kernel/trace/trace_events.c/trace_define_generic_fields()). Just drop it.
+DEFINE_EVENT(latency_template, latency_preempt,
TP_PROTO(int ltype, int cpu, cycles_t latency),TP_ARGS(ltype, cpu, latency));+#endif /* _TRACE_HIST_H */
+/* This part must be outside protection */ +#include <trace/define_trace.h> diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c index 03cdff8..d4f2b25 100644 --- a/kernel/trace/trace_irqsoff.c +++ b/kernel/trace/trace_irqsoff.c @@ -13,14 +13,27 @@ #include <linux/uaccess.h> #include <linux/module.h> #include <linux/ftrace.h> +#include <linux/percpu-defs.h>
+#include <trace/events/sched.h>
#include "trace.h"
+#define CREATE_TRACE_POINTS +#include <trace/events/latency.h>
static struct trace_array *irqsoff_trace __read_mostly; static int tracer_enabled __read_mostly;
-static DEFINE_PER_CPU(int, tracing_cpu); +enum latency_type {
TS_IRQ,TS_PREEMPT,TS_CRITTIME,TS_MAX+};
Come to think of it, the TS_ prefix doesn't really make sense, maybe LT_ would be more consistent (short hand for latency type).
+static DEFINE_PER_CPU(int, tracing_cpu); +static DEFINE_PER_CPU(cycle_t, lat_ts[TS_MAX]); static DEFINE_RAW_SPINLOCK(max_trace_lock);
enum { @@ -419,9 +432,19 @@ stop_critical_timing(unsigned long ip, unsigned long parent_ip) atomic_dec(&data->disabled); }
+static inline void latency_trace(enum latency_type type) +{
- trace_latency_preempt(type, raw_smp_processor_id(),
(cycle_t) trace_clock_local() - this_cpu_read(lat_ts[type]));+}
empty line here
/* start and stop critical timings used to for stoppage (in idle) */ void start_critical_timings(void) {
- if (trace_latency_preempt_enabled())
this_cpu_write(lat_ts[TS_CRITTIME],(cycle_t) trace_clock_local());
Introduce a inline function like the latency_trace() function. No need to open code this logic 3 times.
if (preempt_trace() || irq_trace()) start_critical_timing(CALLER_ADDR0, CALLER_ADDR1); } @@ -431,6 +454,10 @@ void stop_critical_timings(void) { if (preempt_trace() || irq_trace()) stop_critical_timing(CALLER_ADDR0, CALLER_ADDR1);
- if (trace_latency_preempt_enabled())
latency_trace(TS_CRITTIME);} EXPORT_SYMBOL_GPL(stop_critical_timings);
@@ -438,6 +465,9 @@ EXPORT_SYMBOL_GPL(stop_critical_timings); #ifdef CONFIG_PROVE_LOCKING void time_hardirqs_on(unsigned long a0, unsigned long a1) {
- if (trace_latency_preempt_enabled()) {
latency_trace(TS_IRQ);- }
Please follow the style of the rest of the file. Just drop the brackets.
if (!preempt_trace() && irq_trace()) stop_critical_timing(a0, a1); } @@ -446,6 +476,11 @@ void time_hardirqs_off(unsigned long a0, unsigned long a1) { if (!preempt_trace() && irq_trace()) start_critical_timing(a0, a1);
- if (trace_latency_preempt_enabled()) {
this_cpu_write(lat_ts[TS_IRQ],(cycle_t) trace_clock_local());- }
Same thing.
}
#else /* !CONFIG_PROVE_LOCKING */ @@ -503,6 +538,9 @@ EXPORT_SYMBOL(trace_hardirqs_off_caller); #ifdef CONFIG_PREEMPT_TRACER void trace_preempt_on(unsigned long a0, unsigned long a1) {
- if (trace_latency_preempt_enabled())
latency_trace(TS_PREEMPT);- if (preempt_trace() && !irq_trace()) stop_critical_timing(a0, a1);
} @@ -511,6 +549,11 @@ void trace_preempt_off(unsigned long a0, unsigned long a1) { if (preempt_trace() && !irq_trace()) start_critical_timing(a0, a1);
- if (trace_latency_preempt_enabled()) {
this_cpu_write(lat_ts[TS_PREEMPT],(cycle_t) trace_clock_local());- }
And here too.
} #endif /* CONFIG_PREEMPT_TRACER */
cheers, daniel