Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 22 Jan 2019 16:53:11 -0500
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Viktor Rosendahl <Viktor.Rosendahl@bmw.de>
Cc:     Ingo Molnar <mingo@redhat.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/4] preemptirq_delay_test: Add the burst feature and a
 sysfs trigger
Message-ID: <20190122215311.GA200493@google.com>
References: <1548074113-16599-1-git-send-email-Viktor.Rosendahl@bmw.de>
 <1548074113-16599-3-git-send-email-Viktor.Rosendahl@bmw.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1548074113-16599-3-git-send-email-Viktor.Rosendahl@bmw.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Viktor,

Could you CC me on the other patches as well, next time? I am quite
interested and recently have worked on the latency tracer.

Some comments below:

On Mon, Jan 21, 2019 at 02:35:11PM +0200, Viktor Rosendahl wrote:
> This burst feature enables the user to generate a burst of
> preempt/irqsoff latencies. This makes it possible to test whether we
> are able to detect latencies that systematically occur very close to
> each other.
> 
> In addition, there is a sysfs trigger, so that it's not necessary to
> reload the module to repeat the test. The trigger will appear as
> /sys/kernel/preemptirq_delay_test/trigger in sysfs.
> 
> Signed-off-by: Viktor Rosendahl <Viktor.Rosendahl@bmw.de>
> ---
>  kernel/trace/preemptirq_delay_test.c | 139 +++++++++++++++++++++++----
>  1 file changed, 120 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/trace/preemptirq_delay_test.c b/kernel/trace/preemptirq_delay_test.c
> index d8765c952fab..1fc3cdbdd293 100644
> --- a/kernel/trace/preemptirq_delay_test.c
> +++ b/kernel/trace/preemptirq_delay_test.c
> @@ -3,6 +3,7 @@
>   * Preempt / IRQ disable delay thread to test latency tracers
>   *
>   * Copyright (C) 2018 Joel Fernandes (Google) <joel@joelfernandes.org>
> + * Copyright (C) 2018 BMW Car IT GmbH
>   */
>  
>  #include <linux/trace_clock.h>
> @@ -10,18 +11,23 @@
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
> +#include <linux/kobject.h>
>  #include <linux/kthread.h>
>  #include <linux/module.h>
>  #include <linux/printk.h>
>  #include <linux/string.h>
> +#include <linux/sysfs.h>
>  
>  static ulong delay = 100;
> -static char test_mode[10] = "irq";
> +static char test_mode[12] = "irq";
> +static uint burst_size = 1;
>  
> -module_param_named(delay, delay, ulong, S_IRUGO);
> -module_param_string(test_mode, test_mode, 10, S_IRUGO);
> -MODULE_PARM_DESC(delay, "Period in microseconds (100 uS default)");
> -MODULE_PARM_DESC(test_mode, "Mode of the test such as preempt or irq (default irq)");
> +module_param_named(delay, delay, ulong, 0444);
> +module_param_string(test_mode, test_mode, 12, 0444);
> +module_param_named(burst_size, burst_size, uint, 0444);
> +MODULE_PARM_DESC(delay, "Period in microseconds (100 us default)");
> +MODULE_PARM_DESC(test_mode, "Mode of the test such as preempt, irq, or alternate (default irq)");
> +MODULE_PARM_DESC(burst_size, "The size of a burst (default 1)");

Where are we bounds checking the burst_size here? It seems like a high
burst_size can overflow your array of functions.

>  
>  static void busy_wait(ulong time)
>  {
> @@ -34,37 +40,132 @@ static void busy_wait(ulong time)
>  	} while ((end - start) < (time * 1000));
>  }
>  
> -static int preemptirq_delay_run(void *data)
> +static __always_inline void irqoff_test(void)
>  {
>  	unsigned long flags;
> +	local_irq_save(flags);
> +	busy_wait(delay);
> +	local_irq_restore(flags);
> +}
> +
> +static __always_inline void preemptoff_test(void)
> +{
> +	preempt_disable();
> +	busy_wait(delay);
> +	preempt_enable();
> +}
>  
> -	if (!strcmp(test_mode, "irq")) {
> -		local_irq_save(flags);
> -		busy_wait(delay);
> -		local_irq_restore(flags);
> -	} else if (!strcmp(test_mode, "preempt")) {
> -		preempt_disable();
> -		busy_wait(delay);
> -		preempt_enable();
> +static void execute_preemptirqtest(int idx)
> +{
> +	if (!strcmp(test_mode, "irq"))
> +		irqoff_test();
> +	else if (!strcmp(test_mode, "preempt"))
> +		preemptoff_test();
> +	else if (!strcmp(test_mode, "alternate")) {
> +		if (idx % 2 == 0)
> +			irqoff_test();
> +		else
> +			preemptoff_test();
>  	}
> +}
> +
> +#define DECLARE_TESTFN(POSTFIX)				\
> +	static void preemptirqtest_##POSTFIX(int idx)	\
> +	{						\
> +		execute_preemptirqtest(idx);		\
> +	}						\
> +
> +DECLARE_TESTFN(0)
> +DECLARE_TESTFN(1)
> +DECLARE_TESTFN(2)
> +DECLARE_TESTFN(3)
> +DECLARE_TESTFN(4)
> +DECLARE_TESTFN(5)
> +DECLARE_TESTFN(6)
> +DECLARE_TESTFN(7)
> +DECLARE_TESTFN(8)
> +DECLARE_TESTFN(9)

You really only need 2 functions here, since the odd and even suffixed
functions are identical.

> +static void (*testfuncs[])(int)  = {
> +	preemptirqtest_0,
> +	preemptirqtest_1,
> +	preemptirqtest_2,
> +	preemptirqtest_3,
> +	preemptirqtest_4,
> +	preemptirqtest_5,
> +	preemptirqtest_6,
> +	preemptirqtest_7,
> +	preemptirqtest_8,
> +	preemptirqtest_9,
> +};

And then just have an array of 2 functions. Or nuke the array.

> +#define NR_TEST_FUNCS ARRAY_SIZE(testfuncs)
> +
> +static int preemptirq_delay_run(void *data)
> +{
> +
> +	int i;
> +
> +	for (i = 0; i < burst_size; i++)
> +		(testfuncs[i % NR_TEST_FUNCS])(i);
>  	return 0;
>  }
>  
> -static int __init preemptirq_delay_init(void)
> +static struct task_struct *preemptirq_start_test(void)
>  {
>  	char task_name[50];
> -	struct task_struct *test_task;
>  
>  	snprintf(task_name, sizeof(task_name), "%s_test", test_mode);
> +	return kthread_run(preemptirq_delay_run, NULL, task_name);
> +}
> +
> +
> +static ssize_t trigger_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	preemptirq_start_test();
> +	return count;
> +}

I honestly feel a sysfs trigger file is pointless. Why can't the module be
reloaded? Note also that module parameters can be changed after the module
has been loaded. Perhaps that can be used as a trigger? So if the test_mode
is changed, then the test is re-run.

However, if Steve prefers the sysfs trigger file, then I am Ok with that.

thanks,

 - Joel