Re: [PATCH] tracing: Wait for preempt irq delay thread to execute

From: Joel Fernandes <joel@joelfernandes.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Xiao Yang <yangx.jy@cn.fujitsu.com>
Subject: Re: [PATCH] tracing: Wait for preempt irq delay thread to execute
Date: Mon, 11 May 2020 13:27:53 -0400	[thread overview]
Message-ID: <20200511172753.GA173916@google.com> (raw)
In-Reply-To: <20200510114210.15d9e4af@oasis.local.home>

On Sun, May 10, 2020 at 11:42:10AM -0400, Steven Rostedt wrote:
> \From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> A bug report was posted that running the preempt irq delay module on a slow
> machine, and removing it quickly could lead to the thread created by the
> modlue to execute after the module is removed, and this could cause the
> kernel to crash. The fix for this was to call kthread_stop() after creating
> the thread to make sure it finishes before allowing the module to be
> removed.
> 
> Now this caused the opposite problem on fast machines. What now happens is
> the kthread_stop() can cause the kthread never to execute and the test never
> to run. To fix this, add a completion and wait for the kthread to execute,
> then wait for it to end.
> 
> This issue caused the ftracetest selftests to fail on the preemptirq tests.
> 
> Cc: stable@vger.kernel.org
> Fixes: d16a8c31077e ("tracing: Wait for preempt irq delay thread to finish")

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel


> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  kernel/trace/preemptirq_delay_test.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/preemptirq_delay_test.c b/kernel/trace/preemptirq_delay_test.c
> index c4c86de63cf9..312d1a0ca3b6 100644
> --- a/kernel/trace/preemptirq_delay_test.c
> +++ b/kernel/trace/preemptirq_delay_test.c
> @@ -16,6 +16,7 @@
>  #include <linux/printk.h>
>  #include <linux/string.h>
>  #include <linux/sysfs.h>
> +#include <linux/completion.h>
>  
>  static ulong delay = 100;
>  static char test_mode[12] = "irq";
> @@ -28,6 +29,8 @@ 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)");
>  
> +static struct completion done;
> +
>  #define MIN(x, y) ((x) < (y) ? (x) : (y))
>  
>  static void busy_wait(ulong time)
> @@ -114,6 +117,8 @@ static int preemptirq_delay_run(void *data)
>  	for (i = 0; i < s; i++)
>  		(testfuncs[i])(i);
>  
> +	complete(&done);
> +
>  	set_current_state(TASK_INTERRUPTIBLE);
>  	while (!kthread_should_stop()) {
>  		schedule();
> @@ -128,15 +133,18 @@ static int preemptirq_delay_run(void *data)
>  static int preemptirq_run_test(void)
>  {
>  	struct task_struct *task;
> -
>  	char task_name[50];
>  
> +	init_completion(&done);
> +
>  	snprintf(task_name, sizeof(task_name), "%s_test", test_mode);
>  	task =  kthread_run(preemptirq_delay_run, NULL, task_name);
>  	if (IS_ERR(task))
>  		return PTR_ERR(task);
> -	if (task)
> +	if (task) {
> +		wait_for_completion(&done);
>  		kthread_stop(task);
> +	}
>  	return 0;
>  }
>  
> -- 
> 2.20.1
>