Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <5EB3DD4E.7060000@cn.fujitsu.com>
Date:   Thu, 7 May 2020 18:05:02 +0800
From:   Xiao Yang <yangx.jy@cn.fujitsu.com>
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.2; zh-CN; rv:1.9.2.18) Gecko/20110616 Thunderbird/3.1.11
MIME-Version: 1.0
To:     Steven Rostedt <rostedt@goodmis.org>
CC:     Joel Fernandes <joel@joelfernandes.org>,
        <linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCH] tracing: Wait for preempt irq delay thread to finish
References: <20200424223630.224895-1-joel@joelfernandes.org>    <5EA80319.7080005@cn.fujitsu.com>       <20200428104409.0995ceb0@gandalf.local.home>    <20200428104543.3926eaaf@gandalf.local.home>    <5EA96AE8.6000707@cn.fujitsu.com>       <20200429123141.580f89ce@gandalf.local.home>    <20200429191224.GA75562@google.com>     <20200506093805.1f86f3f0@gandalf.local.home> <20200506103017.72abd2cd@gandalf.local.home>
In-Reply-To: <20200506103017.72abd2cd@gandalf.local.home>
Content-Type: text/plain; charset="ISO-8859-1"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Steven,

Thanks for your further investigation.

I used the following ways to test your fix patch on my slow vm and 
didn't see any issue:
1) Insert and remove preemptirq_delay_test in loops.
2) Insert preemptirq_delay_test, write to 
/sys/kernel/preemptirq_delay_test/trigger and remove 
preemptirq_delay_test in loops.
3) Ran irqsoff_tracer.tc in loops.

BTW: For irqsoff_tracer.tc, should we extend code to test the burst 
feature and the sysfs trigger?

Reviewed-by: Xiao Yang <yangx.jy@cn.fujitsu.com>

Thanks,
Xiao Yang
On 2020/5/6 22:30, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)"<rostedt@goodmis.org>
>
> Running on a slower machine, it is possible that the preempt delay kernel
> thread may still be executing if the module was immediately removed after
> added, and this can cause the kernel to crash as the kernel thread might be
> executing after its code has been removed.
>
> There's no reason that the caller of the code shouldn't just wait for the
> delay thread to finish, as the thread can also be created by a trigger in
> the sysfs code, which also has the same issues.
>
> Link: http://lore.kernel.org/r/5EA2B0C8.2080706@cn.fujitsu.com
>
> Cc: stable@vger.kernel.org
> Fixes: 793937236d1ee ("lib: Add module for testing preemptoff/irqsoff latency tracers")
> Reported-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
> Signed-off-by: Steven Rostedt (VMware)<rostedt@goodmis.org>
> ---
>   kernel/trace/preemptirq_delay_test.c | 30 ++++++++++++++++++++++------
>   1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/trace/preemptirq_delay_test.c b/kernel/trace/preemptirq_delay_test.c
> index 31c0fad4cb9e..c4c86de63cf9 100644
> --- a/kernel/trace/preemptirq_delay_test.c
> +++ b/kernel/trace/preemptirq_delay_test.c
> @@ -113,22 +113,42 @@ static int preemptirq_delay_run(void *data)
>
>   	for (i = 0; i<  s; i++)
>   		(testfuncs[i])(i);
> +
> +	set_current_state(TASK_INTERRUPTIBLE);
> +	while (!kthread_should_stop()) {
> +		schedule();
> +		set_current_state(TASK_INTERRUPTIBLE);
> +	}
> +
> +	__set_current_state(TASK_RUNNING);
> +
>   	return 0;
>   }
>
> -static struct task_struct *preemptirq_start_test(void)
> +static int preemptirq_run_test(void)
>   {
> +	struct task_struct *task;
> +
>   	char task_name[50];
>
>   	snprintf(task_name, sizeof(task_name), "%s_test", test_mode);
> -	return kthread_run(preemptirq_delay_run, NULL, task_name);
> +	task =  kthread_run(preemptirq_delay_run, NULL, task_name);
> +	if (IS_ERR(task))
> +		return PTR_ERR(task);
> +	if (task)
> +		kthread_stop(task);
> +	return 0;
>   }
>
>
>   static ssize_t trigger_store(struct kobject *kobj, struct kobj_attribute *attr,
>   			 const char *buf, size_t count)
>   {
> -	preemptirq_start_test();
> +	ssize_t ret;
> +
> +	ret = preemptirq_run_test();
> +	if (ret)
> +		return ret;
>   	return count;
>   }
>
> @@ -148,11 +168,9 @@ static struct kobject *preemptirq_delay_kobj;
>
>   static int __init preemptirq_delay_init(void)
>   {
> -	struct task_struct *test_task;
>   	int retval;
>
> -	test_task = preemptirq_start_test();
> -	retval = PTR_ERR_OR_ZERO(test_task);
> +	retval = preemptirq_run_test();
>   	if (retval != 0)
>   		return retval;
>