lkml.org 
[lkml]   [2016]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 2/3] tracing: Add trace_irqsoff tracepoints
From
Date
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

\
 
 \ /
  Last update: 2016-09-17 09:58    [W:0.776 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site