linux-kernel - Re: [PATCH v3 2/3] tracing: Add trace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fe9ddd6d-e36b-e43b-9d2c-9d09f2aed9de@bmw-carit.de>
Date:   Mon, 29 Aug 2016 11:43:50 +0200
From:   Daniel Wagner <daniel.wagner@...-carit.de>
To:     Binoy Jayan <binoy.jayan@...aro.org>,
        "Steven Rostedt (Red Hat)" <rostedt@...dmis.org>
CC:     Ingo Molnar <mingo@...hat.com>, Arnd Bergmann <arnd@...db.de>,
        <linaro-kernel@...ts.linaro.org>, Carsten Emde <C.Emde@...dl.org>,
        <linux-kernel@...r.kernel.org>,
        Masami <masami.hiramatsu@...aro.org>
Subject: Re: [PATCH v3 2/3] tracing: Add trace_irqsoff tracepoints

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