linux-kernel - Re: [PATCH V6 06/20] trace/osnoise: Allow multiple instances of the same tracer

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]

Date:   Wed, 27 Oct 2021 22:50:51 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Daniel Bristot de Oliveira <bristot@...nel.org>
Cc:     Tao Zhou <tao.zhou@...ux.dev>, Ingo Molnar <mingo@...hat.com>,
        Tom Zanussi <zanussi@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Clark Williams <williams@...hat.com>,
        John Kacur <jkacur@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        linux-rt-users@...r.kernel.org, linux-trace-devel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V6 06/20] trace/osnoise: Allow multiple instances of the
 same tracer

On Wed, 27 Oct 2021 00:06:17 +0200
Daniel Bristot de Oliveira <bristot@...nel.org> wrote:

>  static int osnoise_tracer_init(struct trace_array *tr)
>  {
> -
> -	/* Only allow one instance to enable this */
> -	if (osnoise_has_registered_instances())
> +	/*
> +	 * Only allow osnoise tracer if timerlat tracer is not running
> +	 * already.
> +	 */
> +	if (osnoise_data.timerlat_tracer)
>  		return -EBUSY;
>  

This fails to build when timerlat is not enabled:

/work/git/linux-trace.git/kernel/trace/trace_osnoise.c: In function ‘osnoise_tracer_init’:
/work/git/linux-trace.git/kernel/trace/trace_osnoise.c:2161:18: error: ‘struct osnoise_data’ has no member named ‘timerlat_tracer’
 2161 |  if (osnoise_data.timerlat_tracer)
      |                  ^
make[3]: *** [/work/git/linux-trace.git/scripts/Makefile.build:277: kernel/trace/trace_osnoise.o] Error 1


Also, I hate all the #ifdef muckery in this file. What you need is:

static struct osnoise_data {
	u64	sample_period;		/* total sampling period */
	u64	sample_runtime;		/* active sampling portion of period */
	u64	stop_tracing;		/* stop trace in the internal operation (loop/irq) */
	u64	stop_tracing_total;	/* stop trace in the final operation (report/thread) */
#ifdef CONFIG_TIMERLAT_TRACER
	u64	timerlat_period;	/* timerlat period */
	u64	print_stack;		/* print IRQ stack if total > */
	int	timerlat_tracer;	/* timerlat tracer */
#endif
	bool	tainted;		/* infor users and developers about a problem */
} osnoise_data = {
	.sample_period			= DEFAULT_SAMPLE_PERIOD,
	.sample_runtime			= DEFAULT_SAMPLE_RUNTIME,
	.stop_tracing			= 0,
	.stop_tracing_total		= 0,
#ifdef CONFIG_TIMERLAT_TRACER
	.print_stack			= 0,
	.timerlat_period		= DEFAULT_TIMERLAT_PERIOD,
	.timerlat_tracer		= 0,
#endif
};


#ifdef CONFIG_TIMERLAT_TRACER
static bool timerlat_enbabled()
{
	return osnoise_data.timerlat_tracer;
}
static void timerlat_softirq_exit(void)
{
	struct timerlat_variables *tlat_var;
	tlat_var = this_cpu_tmr_var();
	if (!tlat_var->tracing_thread) {
		osn_var->softirq.arrival_time = 0;
		osn_var->softirq.delta_start = 0;
	}
}
#else
static inline bool timerlat_enbabled()
{
	return false;
}
static void timerlat_softirq_exit(void) { }
#endif

Then in places like trace_softirq_exit_callback() you can have:

	if (unlikely(timerlat_enabled())
		timerlat_softirq_exit();

And this will help in making mistakes like you did with the compile
failure.

So there should be no #ifdef in any functions (it's fine to wrap
functions).

-- Steve