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;
Date:   Thu, 07 May 2020 08:29:10 -0400
User-Agent: K-9 Mail for Android
In-Reply-To: <5EB3DD4E.7060000@cn.fujitsu.com>
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> <5EB3DD4E.7060000@cn.fujitsu.com>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PATCH] tracing: Wait for preempt irq delay thread to finish
To:     Xiao Yang <yangx.jy@cn.fujitsu.com>,
        Steven Rostedt <rostedt@goodmis.org>
CC:     linux-kernel@vger.kernel.org, Ingo Molnar <mingo@redhat.com>
From:   joel@joelfernandes.org
Message-ID: <8C3E6BAF-255D-41C7-B652-7C68877BDBED@joelfernandes.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On May 7, 2020 6:05:02 AM EDT, Xiao Yang <yangx=2Ejy@cn=2Efujitsu=2Ecom> w=
rote:
>Hi Steven,
>
>Thanks for your further investigation=2E
>
>I used the following ways to test your fix patch on my slow vm and=20
>didn't see any issue:
>1) Insert and remove preemptirq_delay_test in loops=2E
>2) Insert preemptirq_delay_test, write to=20
>/sys/kernel/preemptirq_delay_test/trigger and remove=20
>preemptirq_delay_test in loops=2E
>3) Ran irqsoff_tracer=2Etc in loops=2E
>
>BTW: For irqsoff_tracer=2Etc, should we extend code to test the burst=20
>feature and the sysfs trigger?
>
>Reviewed-by: Xiao Yang <yangx=2Ejy@cn=2Efujitsu=2Ecom>
>

Thanks!

Reviewed-by: Joel Fernandes <joel@joelfernandes=2Eorg>

- Joel


>Thanks,
>Xiao Yang
>On 2020/5/6 22:30, Steven Rostedt wrote:
>> From: "Steven Rostedt (VMware)"<rostedt@goodmis=2Eorg>
>>
>> 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=2E
>>
>> 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=2E
>>
>> Link: http://lore=2Ekernel=2Eorg/r/5EA2B0C8=2E2080706@cn=2Efujitsu=2Eco=
m
>>
>> Cc: stable@vger=2Ekernel=2Eorg
>> Fixes: 793937236d1ee ("lib: Add module for testing preemptoff/irqsoff
>latency tracers")
>> Reported-by: Xiao Yang<yangx=2Ejy@cn=2Efujitsu=2Ecom>
>> Signed-off-by: Steven Rostedt (VMware)<rostedt@goodmis=2Eorg>
>> ---
>>   kernel/trace/preemptirq_delay_test=2Ec | 30
>++++++++++++++++++++++------
>>   1 file changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/kernel/trace/preemptirq_delay_test=2Ec
>b/kernel/trace/preemptirq_delay_test=2Ec
>> index 31c0fad4cb9e=2E=2Ec4c86de63cf9 100644
>> --- a/kernel/trace/preemptirq_delay_test=2Ec
>> +++ b/kernel/trace/preemptirq_delay_test=2Ec
>> @@ -113,22 +113,42 @@ static int preemptirq_delay_run(void *data)
>>
>>   	for (i =3D 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 =3D  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 =3D 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 =3D preemptirq_start_test();
>> -	retval =3D PTR_ERR_OR_ZERO(test_task);
>> +	retval =3D preemptirq_run_test();
>>   	if (retval !=3D 0)
>>   		return retval;
>>

--=20
Sent from my Android device with K-9 Mail=2E Please excuse my brevity=2E