[PATCH] selftests/ftrace: remove _do_fork() leftovers

[PATCH] selftests/ftrace: remove _do_fork() leftovers

[Alexander Gordeev]

The _do_fork() is not completely removed from selftests
in favor of the kernel_clone().

Fixes: eea11285dab3 ("tracing: switch to kernel_clone()")
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
---
 tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc | 2 +-
 tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
index acb17ce..0ddb948 100644
--- a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
+++ b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
@@ -39,7 +39,7 @@ do_test() {
     disable_tracing
 
     echo do_execve* > set_ftrace_filter
-    echo *do_fork >> set_ftrace_filter
+    echo kernel_clone >> set_ftrace_filter
 
     echo $PID > set_ftrace_notrace_pid
     echo function > current_tracer
diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
index 9f0a968..71319b3 100644
--- a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
+++ b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
@@ -39,7 +39,7 @@ do_test() {
     disable_tracing
 
     echo do_execve* > set_ftrace_filter
-    echo *do_fork >> set_ftrace_filter
+    echo kernel_clone >> set_ftrace_filter
 
     echo $PID > set_ftrace_pid
     echo function > current_tracer
-- 
1.8.3.1

Re: [PATCH] selftests/ftrace: remove _do_fork() leftovers

[Masami Hiramatsu]

On Fri, 23 Oct 2020 10:52:03 +0200
Alexander Gordeev <agordeev@linux.ibm.com> wrote:

> The _do_fork() is not completely removed from selftests
> in favor of the kernel_clone().
> 

Looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you!

> Fixes: eea11285dab3 ("tracing: switch to kernel_clone()")
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Christian Brauner <christian.brauner@ubuntu.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> ---
>  tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc | 2 +-
>  tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc         | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
> index acb17ce..0ddb948 100644
> --- a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
> +++ b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
> @@ -39,7 +39,7 @@ do_test() {
>      disable_tracing
>  
>      echo do_execve* > set_ftrace_filter
> -    echo *do_fork >> set_ftrace_filter
> +    echo kernel_clone >> set_ftrace_filter
>  
>      echo $PID > set_ftrace_notrace_pid
>      echo function > current_tracer
> diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
> index 9f0a968..71319b3 100644
> --- a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
> +++ b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
> @@ -39,7 +39,7 @@ do_test() {
>      disable_tracing
>  
>      echo do_execve* > set_ftrace_filter
> -    echo *do_fork >> set_ftrace_filter
> +    echo kernel_clone >> set_ftrace_filter
>  
>      echo $PID > set_ftrace_pid
>      echo function > current_tracer
> -- 
> 1.8.3.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] selftests/ftrace: remove _do_fork() leftovers

[Steven Rostedt]

On Fri, 23 Oct 2020 10:52:03 +0200
Alexander Gordeev <agordeev@linux.ibm.com> wrote:

> diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
> index acb17ce..0ddb948 100644
> --- a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
> +++ b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
> @@ -39,7 +39,7 @@ do_test() {
>      disable_tracing
>  
>      echo do_execve* > set_ftrace_filter
> -    echo *do_fork >> set_ftrace_filter
> +    echo kernel_clone >> set_ftrace_filter
>  
>      echo $PID > set_ftrace_notrace_pid
>      echo function > current_tracer
> diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
> index 9f0a968..71319b3 100644
> --- a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
> +++ b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
> @@ -39,7 +39,7 @@ do_test() {
>      disable_tracing
>  
>      echo do_execve* > set_ftrace_filter
> -    echo *do_fork >> set_ftrace_filter
> +    echo kernel_clone >> set_ftrace_filter
>  
>      echo $PID > set_ftrace_pid
>      echo function > current_tracer

The issue I have with this, is that I run these tests on older kernels too,
and tests that use to work on older kernels should still work. In fact,
this fails on the kernel I'm currently adding new changes to!

Perhaps we should have:

	# older kernels have do_fork, but newer kernels have kernel_clone
	echo kernel_clone >> set_ftrace_filter || echo *do_fork >> set_ftrace_filter

The above still seems to work for me.

-- Steve

Re: [PATCH] selftests/ftrace: remove _do_fork() leftovers

[Alexander Gordeev]

On Fri, Oct 23, 2020 at 09:35:23AM -0400, Steven Rostedt wrote:
> On Fri, 23 Oct 2020 10:52:03 +0200
> Alexander Gordeev <agordeev@linux.ibm.com> wrote:
> 
> > diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
> > index acb17ce..0ddb948 100644
> > --- a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
> > @@ -39,7 +39,7 @@ do_test() {
> >      disable_tracing
> >  
> >      echo do_execve* > set_ftrace_filter
> > -    echo *do_fork >> set_ftrace_filter
> > +    echo kernel_clone >> set_ftrace_filter
> >  
> >      echo $PID > set_ftrace_notrace_pid
> >      echo function > current_tracer
> > diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
> > index 9f0a968..71319b3 100644
> > --- a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
> > @@ -39,7 +39,7 @@ do_test() {
> >      disable_tracing
> >  
> >      echo do_execve* > set_ftrace_filter
> > -    echo *do_fork >> set_ftrace_filter
> > +    echo kernel_clone >> set_ftrace_filter
> >  
> >      echo $PID > set_ftrace_pid
> >      echo function > current_tracer
> 
> The issue I have with this, is that I run these tests on older kernels too,
> and tests that use to work on older kernels should still work. In fact,
> this fails on the kernel I'm currently adding new changes to!
> 
> Perhaps we should have:
> 
> 	# older kernels have do_fork, but newer kernels have kernel_clone
> 	echo kernel_clone >> set_ftrace_filter || echo *do_fork >> set_ftrace_filter

Would you suggest to do the same with all occurences in
eea11285dab3 ("tracing: switch to kernel_clone()")?
Otherwise it does not really make sense to just fix couple
of tests out of dozens.

> The above still seems to work for me.
> 
> -- Steve

Re: [PATCH] selftests/ftrace: remove _do_fork() leftovers

[Steven Rostedt]

On Fri, 23 Oct 2020 17:12:44 +0200
Alexander Gordeev <agordeev@linux.ibm.com> wrote:

> > Perhaps we should have:
> > 
> > 	# older kernels have do_fork, but newer kernels have kernel_clone
> > 	echo kernel_clone >> set_ftrace_filter || echo *do_fork >> set_ftrace_filter  
> 
> Would you suggest to do the same with all occurences in
> eea11285dab3 ("tracing: switch to kernel_clone()")?
> Otherwise it does not really make sense to just fix couple
> of tests out of dozens.

Yes. I haven't pulled in the updated tests, so I haven't hit the errors yet
(nor have I merged my work with the switch to the new name yet). So those
will most definitely break my tests.

But because it's a more generic issue, we should have a way to find what to
use. Perhaps add to the test.d/functions, something like:

FUNCTION_FORK=`(if grep '\bkernel_clone\b' /proc/kallsyms > /dev/null; then
                echo kernel_clone; else echo '_do_fork'; fi)`

and use $FUNCTION_FORK everywhere that references it.

-- Steve

Re: [PATCH] selftests/ftrace: remove _do_fork() leftovers

[Steven Rostedt]

On Fri, 23 Oct 2020 11:49:48 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 23 Oct 2020 17:12:44 +0200
> Alexander Gordeev <agordeev@linux.ibm.com> wrote:
> 
> > > Perhaps we should have:
> > > 
> > > 	# older kernels have do_fork, but newer kernels have kernel_clone
> > > 	echo kernel_clone >> set_ftrace_filter || echo *do_fork >> set_ftrace_filter    
> > 
> > Would you suggest to do the same with all occurences in
> > eea11285dab3 ("tracing: switch to kernel_clone()")?
> > Otherwise it does not really make sense to just fix couple
> > of tests out of dozens.  
> 
> Yes. I haven't pulled in the updated tests, so I haven't hit the errors yet
> (nor have I merged my work with the switch to the new name yet). So those
> will most definitely break my tests.
> 
> But because it's a more generic issue, we should have a way to find what to
> use. Perhaps add to the test.d/functions, something like:
> 
> FUNCTION_FORK=`(if grep '\bkernel_clone\b' /proc/kallsyms > /dev/null; then
>                 echo kernel_clone; else echo '_do_fork'; fi)`
> 
> and use $FUNCTION_FORK everywhere that references it.
> 
> 

Let me pull in the latest changes, and whip up a patch that works on both
the older kernels as well as the newer ones.

-- Steve

Re: [PATCH] selftests/ftrace: remove _do_fork() leftovers

[Shuah Khan]

On 10/23/20 9:51 AM, Steven Rostedt wrote:
> On Fri, 23 Oct 2020 11:49:48 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
>> On Fri, 23 Oct 2020 17:12:44 +0200
>> Alexander Gordeev <agordeev@linux.ibm.com> wrote:
>>
>>>> Perhaps we should have:
>>>>
>>>> 	# older kernels have do_fork, but newer kernels have kernel_clone
>>>> 	echo kernel_clone >> set_ftrace_filter || echo *do_fork >> set_ftrace_filter
>>>
>>> Would you suggest to do the same with all occurences in
>>> eea11285dab3 ("tracing: switch to kernel_clone()")?
>>> Otherwise it does not really make sense to just fix couple
>>> of tests out of dozens.
>>
>> Yes. I haven't pulled in the updated tests, so I haven't hit the errors yet
>> (nor have I merged my work with the switch to the new name yet). So those
>> will most definitely break my tests.
>>
>> But because it's a more generic issue, we should have a way to find what to
>> use. Perhaps add to the test.d/functions, something like:
>>
>> FUNCTION_FORK=`(if grep '\bkernel_clone\b' /proc/kallsyms > /dev/null; then
>>                  echo kernel_clone; else echo '_do_fork'; fi)`
>>
>> and use $FUNCTION_FORK everywhere that references it.
>>
>>
> 
> Let me pull in the latest changes, and whip up a patch that works on both
> the older kernels as well as the newer ones.
> 
> -- Steve
> 

Assume this is handled by

selftests/ftrace: Use $FUNCTION_FORK to reference kernel fork function
https://patchwork.kernel.org/project/linux-kselftest/patch/20201026162032.124c728d@gandalf.local.home/

Just making sure.

thanks,
-- Shuah

Re: [PATCH] selftests/ftrace: remove _do_fork() leftovers

[Steven Rostedt]

On Tue, 27 Oct 2020 15:55:32 -0600
Shuah Khan <skhan@linuxfoundation.org> wrote:


> > Let me pull in the latest changes, and whip up a patch that works on both
> > the older kernels as well as the newer ones.
> > 
> > -- Steve
> >   
> 
> Assume this is handled by
> 
> selftests/ftrace: Use $FUNCTION_FORK to reference kernel fork function
> https://patchwork.kernel.org/project/linux-kselftest/patch/20201026162032.124c728d@gandalf.local.home/
> 
> Just making sure.

Yep, that was the result of this thread.

Thanks Shuah,

-- Steve

Re: [PATCH] selftests/ftrace: remove _do_fork() leftovers

[Steven Rostedt]

On Tue, 27 Oct 2020 22:46:57 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Yep, that was the result of this thread.

And if it's not clear. That thread supersedes this one.

-- Steve

Re: [PATCH] selftests/ftrace: remove _do_fork() leftovers

[Masami Hiramatsu]

On Fri, 23 Oct 2020 09:35:23 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 23 Oct 2020 10:52:03 +0200
> Alexander Gordeev <agordeev@linux.ibm.com> wrote:
> 
> > diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
> > index acb17ce..0ddb948 100644
> > --- a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-notrace-pid.tc
> > @@ -39,7 +39,7 @@ do_test() {
> >      disable_tracing
> >  
> >      echo do_execve* > set_ftrace_filter
> > -    echo *do_fork >> set_ftrace_filter
> > +    echo kernel_clone >> set_ftrace_filter
> >  
> >      echo $PID > set_ftrace_notrace_pid
> >      echo function > current_tracer
> > diff --git a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
> > index 9f0a968..71319b3 100644
> > --- a/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/ftrace/func-filter-pid.tc
> > @@ -39,7 +39,7 @@ do_test() {
> >      disable_tracing
> >  
> >      echo do_execve* > set_ftrace_filter
> > -    echo *do_fork >> set_ftrace_filter
> > +    echo kernel_clone >> set_ftrace_filter
> >  
> >      echo $PID > set_ftrace_pid
> >      echo function > current_tracer
> 
> The issue I have with this, is that I run these tests on older kernels too,
> and tests that use to work on older kernels should still work. In fact,
> this fails on the kernel I'm currently adding new changes to!
> 
> Perhaps we should have:
> 
> 	# older kernels have do_fork, but newer kernels have kernel_clone
> 	echo kernel_clone >> set_ftrace_filter || echo *do_fork >> set_ftrace_filter

Good catch. BTW, can we check the filter-bility by grep the pattern from set_ftrace_filter?

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] selftests/ftrace: remove _do_fork() leftovers

[Steven Rostedt]

On Sat, 24 Oct 2020 10:31:12 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > Perhaps we should have:
> > 
> > 	# older kernels have do_fork, but newer kernels have kernel_clone
> > 	echo kernel_clone >> set_ftrace_filter || echo *do_fork >> set_ftrace_filter  
> 
> Good catch. BTW, can we check the filter-bility by grep the pattern from set_ftrace_filter?

I think we need to use /proc/kallsyms, as the kprobe code should still work
if function tracing is disabled, and the function filter files will not
exist.

-- Steve