[PATCH 0/2] ftrace/selftest: Fix ftracetest for non existant config and files

[PATCH 0/2] ftrace/selftest: Fix ftracetest for non existant config and files

[Steven Rostedt]

Hi Masami,

I worked a bit more on the one issue that Po-Hsu found, and made it use
tracefs a bit more aggressively. To test this I ran the test on a 3.5
kernel and found another issue, where it failed because set_event_pid
and set_ftrace_pid did not exist.

Can you review these patches. I can take them in my tree, or if Shuah
would like, she can take it in hers (after you give a review).

Thanks!

-- Steve


Steven Rostedt (VMware) (2):
      ftrace/selftests: Return the skip code when tracing directory not configured in kernel
      ftrace/selftest: Test if set_event/ftrace_pid exists before writing

----
 tools/testing/selftests/ftrace/ftracetest       | 38 +++++++++++++++++++++----
 tools/testing/selftests/ftrace/test.d/functions |  4 +--
 2 files changed, 34 insertions(+), 8 deletions(-)

[PATCH 1/2] ftrace/selftests: Return the skip code when tracing directory not configured in kernel

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

If the kernel is not configured with ftrace enabled, the ftracetest
selftests should return the error code of "4" as that is the kselftests
"skip" code, and not "1" which means an error.

To determine if ftrace is enabled, first the newer "tracefs" is searched for
in /proc/mounts. If it is not found, then "debugfs" is searched for (as old
kernels do not have tracefs). If that is not found, an attempt to mount the
tracefs or debugfs is performed. This is done by seeing first if the
/sys/kernel/tracing directory exists. If it does than tracefs is configured
in the kernel and an attempt to mount it is performed.

If /sys/kernel/tracing does not exist, then /sys/kernel/debug is tested to
see if that directory exists. If it does, then an attempt to mount debugfs
on that directory is performed. If it does not exist, then debugfs is not
configured in the running kernel and the test exits with the skip code.

If either mount fails, then a normal error is returned as they do exist in
the kernel but something went wrong to mount them.

This changes the test to always try the tracefs file system first as it has
been in the kernel for some time now and it is better to test it if it is
available instead of always testing debugfs.

Link: http://lkml.kernel.org/r/20190702062358.7330-1-po-hsu.lin@canonical.com

Reported-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 tools/testing/selftests/ftrace/ftracetest | 38 +++++++++++++++++++----
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/ftrace/ftracetest b/tools/testing/selftests/ftrace/ftracetest
index 136387422b00..edf5ea35d2a7 100755
--- a/tools/testing/selftests/ftrace/ftracetest
+++ b/tools/testing/selftests/ftrace/ftracetest
@@ -23,9 +23,15 @@ echo "		            If <dir> is -, all logs output in console only"
 exit $1
 }
 
+# default error
+err_ret=1
+
+# kselftest skip code is 4
+err_skip=4
+
 errexit() { # message
   echo "Error: $1" 1>&2
-  exit 1
+  exit $err_ret
 }
 
 # Ensuring user privilege
@@ -116,11 +122,31 @@ parse_opts() { # opts
 }
 
 # Parameters
-DEBUGFS_DIR=`grep debugfs /proc/mounts | cut -f2 -d' ' | head -1`
-if [ -z "$DEBUGFS_DIR" ]; then
-    TRACING_DIR=`grep tracefs /proc/mounts | cut -f2 -d' ' | head -1`
-else
-    TRACING_DIR=$DEBUGFS_DIR/tracing
+TRACING_DIR=`grep tracefs /proc/mounts | cut -f2 -d' ' | head -1`
+if [ -z "$TRACING_DIR" ]; then
+    DEBUGFS_DIR=`grep debugfs /proc/mounts | cut -f2 -d' ' | head -1`
+    if [ -z "$DEBUGFS_DIR" ]; then
+	# If tracefs exists, then so does /sys/kernel/tracing
+	if [ -d "/sys/kernel/tracing" ]; then
+	    mount -t tracefs nodev /sys/kernel/tracing ||
+	      errexit "Failed to mount /sys/kernel/tracing"
+	    TRACING_DIR="/sys/kernel/tracing"
+	# If debugfs exists, then so does /sys/kernel/debug
+	elif [ -d "/sys/kernel/debug" ]; then
+	    mount -t debugfs nodev /sys/kernel/debug ||
+	      errexit "Failed to mount /sys/kernel/debug"
+	    TRACING_DIR="/sys/kernel/debug/tracing"
+	else
+	    err_ret=$err_skip
+	    errexit "debugfs and tracefs are not configured in this kernel"
+	fi
+    else
+	TRACING_DIR="$DEBUGFS_DIR/tracing"
+    fi
+fi
+if [ ! -d "$TRACING_DIR" ]; then
+    err_ret=$err_skip
+    errexit "ftrace is not configured in this kernel"
 fi
 
 TOP_DIR=`absdir $0`
-- 
2.20.1

Re: [PATCH 1/2] ftrace/selftests: Return the skip code when tracing directory not configured in kernel

[Masami Hiramatsu]

On Wed, 03 Jul 2019 15:50:00 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> If the kernel is not configured with ftrace enabled, the ftracetest
> selftests should return the error code of "4" as that is the kselftests
> "skip" code, and not "1" which means an error.
> 
> To determine if ftrace is enabled, first the newer "tracefs" is searched for
> in /proc/mounts. If it is not found, then "debugfs" is searched for (as old
> kernels do not have tracefs). If that is not found, an attempt to mount the
> tracefs or debugfs is performed. This is done by seeing first if the
> /sys/kernel/tracing directory exists. If it does than tracefs is configured
> in the kernel and an attempt to mount it is performed.
> 
> If /sys/kernel/tracing does not exist, then /sys/kernel/debug is tested to
> see if that directory exists. If it does, then an attempt to mount debugfs
> on that directory is performed. If it does not exist, then debugfs is not
> configured in the running kernel and the test exits with the skip code.
> 
> If either mount fails, then a normal error is returned as they do exist in
> the kernel but something went wrong to mount them.
> 
> This changes the test to always try the tracefs file system first as it has
> been in the kernel for some time now and it is better to test it if it is
> available instead of always testing debugfs.
> 
> Link: http://lkml.kernel.org/r/20190702062358.7330-1-po-hsu.lin@canonical.com
> 
> Reported-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  tools/testing/selftests/ftrace/ftracetest | 38 +++++++++++++++++++----
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/ftrace/ftracetest b/tools/testing/selftests/ftrace/ftracetest
> index 136387422b00..edf5ea35d2a7 100755
> --- a/tools/testing/selftests/ftrace/ftracetest
> +++ b/tools/testing/selftests/ftrace/ftracetest
> @@ -23,9 +23,15 @@ echo "		            If <dir> is -, all logs output in console only"
>  exit $1
>  }
>  
> +# default error
> +err_ret=1
> +
> +# kselftest skip code is 4
> +err_skip=4
> +
>  errexit() { # message
>    echo "Error: $1" 1>&2
> -  exit 1
> +  exit $err_ret
>  }
>  
>  # Ensuring user privilege
> @@ -116,11 +122,31 @@ parse_opts() { # opts
>  }
>  
>  # Parameters
> -DEBUGFS_DIR=`grep debugfs /proc/mounts | cut -f2 -d' ' | head -1`
> -if [ -z "$DEBUGFS_DIR" ]; then
> -    TRACING_DIR=`grep tracefs /proc/mounts | cut -f2 -d' ' | head -1`
> -else
> -    TRACING_DIR=$DEBUGFS_DIR/tracing
> +TRACING_DIR=`grep tracefs /proc/mounts | cut -f2 -d' ' | head -1`
> +if [ -z "$TRACING_DIR" ]; then
> +    DEBUGFS_DIR=`grep debugfs /proc/mounts | cut -f2 -d' ' | head -1`
> +    if [ -z "$DEBUGFS_DIR" ]; then
> +	# If tracefs exists, then so does /sys/kernel/tracing

Ah, indeed. the mount point also may not exist on older kernel.

OK, this looks good to me.

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

Thank you!

> +	if [ -d "/sys/kernel/tracing" ]; then
> +	    mount -t tracefs nodev /sys/kernel/tracing ||
> +	      errexit "Failed to mount /sys/kernel/tracing"
> +	    TRACING_DIR="/sys/kernel/tracing"
> +	# If debugfs exists, then so does /sys/kernel/debug
> +	elif [ -d "/sys/kernel/debug" ]; then
> +	    mount -t debugfs nodev /sys/kernel/debug ||
> +	      errexit "Failed to mount /sys/kernel/debug"
> +	    TRACING_DIR="/sys/kernel/debug/tracing"
> +	else
> +	    err_ret=$err_skip
> +	    errexit "debugfs and tracefs are not configured in this kernel"
> +	fi
> +    else
> +	TRACING_DIR="$DEBUGFS_DIR/tracing"
> +    fi
> +fi
> +if [ ! -d "$TRACING_DIR" ]; then
> +    err_ret=$err_skip
> +    errexit "ftrace is not configured in this kernel"
>  fi
>  
>  TOP_DIR=`absdir $0`
> -- 
> 2.20.1
> 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH 2/2] ftrace/selftest: Test if set_event/ftrace_pid exists before writing

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

While testing on a very old kernel (3.5), the tests failed because the write
to set_event_pid in the setup code, did not exist. The tests themselves
could pass, but the setup failed causing an error.

Other files test for existance before writing to them. Do the same for
set_event_pid and set_ftrace_pid.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 tools/testing/selftests/ftrace/test.d/functions | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
index 779ec11f61bd..a7b06291e32c 100644
--- a/tools/testing/selftests/ftrace/test.d/functions
+++ b/tools/testing/selftests/ftrace/test.d/functions
@@ -91,8 +91,8 @@ initialize_ftrace() { # Reset ftrace to initial-state
     reset_events_filter
     reset_ftrace_filter
     disable_events
-    echo > set_event_pid	# event tracer is always on
-    echo > set_ftrace_pid
+    [ -f set_event_pid ] && echo > set_event_pid  # event tracer is always on
+    [ -f set_ftrace_pid ] && echo > set_ftrace_pid
     [ -f set_ftrace_filter ] && echo | tee set_ftrace_*
     [ -f set_graph_function ] && echo | tee set_graph_*
     [ -f stack_trace_filter ] && echo > stack_trace_filter
-- 
2.20.1

Re: [PATCH 2/2] ftrace/selftest: Test if set_event/ftrace_pid exists before writing

[Steven Rostedt]

On Wed, 03 Jul 2019 15:50:01 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> While testing on a very old kernel (3.5), the tests failed because the write
> to set_event_pid in the setup code, did not exist. The tests themselves
> could pass, but the setup failed causing an error.
> 
> Other files test for existance before writing to them. Do the same for
> set_event_pid and set_ftrace_pid.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  tools/testing/selftests/ftrace/test.d/functions | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> index 779ec11f61bd..a7b06291e32c 100644
> --- a/tools/testing/selftests/ftrace/test.d/functions
> +++ b/tools/testing/selftests/ftrace/test.d/functions
> @@ -91,8 +91,8 @@ initialize_ftrace() { # Reset ftrace to initial-state
>      reset_events_filter
>      reset_ftrace_filter
>      disable_events
> -    echo > set_event_pid	# event tracer is always on
> -    echo > set_ftrace_pid
> +    [ -f set_event_pid ] && echo > set_event_pid  # event tracer is always on

I probably should remove that comment, because I believe that was why
it wasn't tested :-/

-- Steve


> +    [ -f set_ftrace_pid ] && echo > set_ftrace_pid
>      [ -f set_ftrace_filter ] && echo | tee set_ftrace_*
>      [ -f set_graph_function ] && echo | tee set_graph_*
>      [ -f stack_trace_filter ] && echo > stack_trace_filter

Re: [PATCH 2/2] ftrace/selftest: Test if set_event/ftrace_pid exists before writing

[Steven Rostedt]

On Wed, 3 Jul 2019 16:00:09 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> > index 779ec11f61bd..a7b06291e32c 100644
> > --- a/tools/testing/selftests/ftrace/test.d/functions
> > +++ b/tools/testing/selftests/ftrace/test.d/functions
> > @@ -91,8 +91,8 @@ initialize_ftrace() { # Reset ftrace to initial-state
> >      reset_events_filter
> >      reset_ftrace_filter
> >      disable_events
> > -    echo > set_event_pid	# event tracer is always on
> > -    echo > set_ftrace_pid
> > +    [ -f set_event_pid ] && echo > set_event_pid  # event tracer is always on  
> 
> I probably should remove that comment, because I believe that was why
> it wasn't tested :-/

Masami,

I'll wait for your review before posting a v2 without this comment.

-- Steve

Re: [PATCH 2/2] ftrace/selftest: Test if set_event/ftrace_pid exists before writing

[Masami Hiramatsu]

On Wed, 3 Jul 2019 16:00:09 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 03 Jul 2019 15:50:01 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > 
> > While testing on a very old kernel (3.5), the tests failed because the write
> > to set_event_pid in the setup code, did not exist. The tests themselves
> > could pass, but the setup failed causing an error.
> > 
> > Other files test for existance before writing to them. Do the same for
> > set_event_pid and set_ftrace_pid.
> > 
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---
> >  tools/testing/selftests/ftrace/test.d/functions | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> > index 779ec11f61bd..a7b06291e32c 100644
> > --- a/tools/testing/selftests/ftrace/test.d/functions
> > +++ b/tools/testing/selftests/ftrace/test.d/functions
> > @@ -91,8 +91,8 @@ initialize_ftrace() { # Reset ftrace to initial-state
> >      reset_events_filter
> >      reset_ftrace_filter
> >      disable_events
> > -    echo > set_event_pid	# event tracer is always on
> > -    echo > set_ftrace_pid
> > +    [ -f set_event_pid ] && echo > set_event_pid  # event tracer is always on
> 
> I probably should remove that comment, because I believe that was why
> it wasn't tested :-/

Hmm, OK. I think this comment means "the event tracer is always on if clearing
set_event_pid filter". Would this need to be removed?

Thank you,

> 
> -- Steve
> 
> 
> > +    [ -f set_ftrace_pid ] && echo > set_ftrace_pid
> >      [ -f set_ftrace_filter ] && echo | tee set_ftrace_*
> >      [ -f set_graph_function ] && echo | tee set_graph_*
> >      [ -f stack_trace_filter ] && echo > stack_trace_filter
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 2/2] ftrace/selftest: Test if set_event/ftrace_pid exists before writing

[Steven Rostedt]

On Thu, 4 Jul 2019 10:51:26 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > > diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> > > index 779ec11f61bd..a7b06291e32c 100644
> > > --- a/tools/testing/selftests/ftrace/test.d/functions
> > > +++ b/tools/testing/selftests/ftrace/test.d/functions
> > > @@ -91,8 +91,8 @@ initialize_ftrace() { # Reset ftrace to initial-state
> > >      reset_events_filter
> > >      reset_ftrace_filter
> > >      disable_events
> > > -    echo > set_event_pid	# event tracer is always on
> > > -    echo > set_ftrace_pid
> > > +    [ -f set_event_pid ] && echo > set_event_pid  # event tracer is always on  
> > 
> > I probably should remove that comment, because I believe that was why
> > it wasn't tested :-/  
> 
> Hmm, OK. I think this comment means "the event tracer is always on if clearing
> set_event_pid filter". Would this need to be removed?

When this was added in commit 131f840d5b7 ("selftests: ftrace:
Initialize ftrace before each test"), we had this:

+    echo > set_event_pid       # event tracer is always on
+    [ -f set_ftrace_filter ] && echo | tee set_ftrace_*
+    [ -f set_graph_function ] && echo | tee set_graph_*
+    [ -f stack_trace_filter ] && echo > stack_trace_filter
+    [ -f kprobe_events ] && echo > kprobe_events
+    [ -f uprobe_events ] && echo > uprobe_events

Where set_event_pid is the only file not tested for existence. I
figured that comment was the reason for not testing it. If that was the
case, then adding a test, I would think we should remove the comment.

Do you agree?

-- Steve


> 
> Thank you,
> 
> > 
> > -- Steve
> > 
> >   
> > > +    [ -f set_ftrace_pid ] && echo > set_ftrace_pid
> > >      [ -f set_ftrace_filter ] && echo | tee set_ftrace_*
> > >      [ -f set_graph_function ] && echo | tee set_graph_*
> > >      [ -f stack_trace_filter ] && echo > stack_trace_filter  
> >   
> 
> 

Re: [PATCH 2/2] ftrace/selftest: Test if set_event/ftrace_pid exists before writing

[Masami Hiramatsu]

On Wed, 3 Jul 2019 22:01:05 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 4 Jul 2019 10:51:26 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > > > diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> > > > index 779ec11f61bd..a7b06291e32c 100644
> > > > --- a/tools/testing/selftests/ftrace/test.d/functions
> > > > +++ b/tools/testing/selftests/ftrace/test.d/functions
> > > > @@ -91,8 +91,8 @@ initialize_ftrace() { # Reset ftrace to initial-state
> > > >      reset_events_filter
> > > >      reset_ftrace_filter
> > > >      disable_events
> > > > -    echo > set_event_pid	# event tracer is always on
> > > > -    echo > set_ftrace_pid
> > > > +    [ -f set_event_pid ] && echo > set_event_pid  # event tracer is always on  
> > > 
> > > I probably should remove that comment, because I believe that was why
> > > it wasn't tested :-/  
> > 
> > Hmm, OK. I think this comment means "the event tracer is always on if clearing
> > set_event_pid filter". Would this need to be removed?
> 
> When this was added in commit 131f840d5b7 ("selftests: ftrace:
> Initialize ftrace before each test"), we had this:
> 
> +    echo > set_event_pid       # event tracer is always on
> +    [ -f set_ftrace_filter ] && echo | tee set_ftrace_*
> +    [ -f set_graph_function ] && echo | tee set_graph_*
> +    [ -f stack_trace_filter ] && echo > stack_trace_filter
> +    [ -f kprobe_events ] && echo > kprobe_events
> +    [ -f uprobe_events ] && echo > uprobe_events
> 
> Where set_event_pid is the only file not tested for existence. I
> figured that comment was the reason for not testing it. If that was the
> case, then adding a test, I would think we should remove the comment.

Ah, I see.

> 
> Do you agree?

Yes, I agree with removing it.

Thank you!

> 
> -- Steve
> 
> 
> > 
> > Thank you,
> > 
> > > 
> > > -- Steve
> > > 
> > >   
> > > > +    [ -f set_ftrace_pid ] && echo > set_ftrace_pid
> > > >      [ -f set_ftrace_filter ] && echo | tee set_ftrace_*
> > > >      [ -f set_graph_function ] && echo | tee set_graph_*
> > > >      [ -f stack_trace_filter ] && echo > stack_trace_filter  
> > >   
> > 
> > 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 2/2] ftrace/selftest: Test if set_event/ftrace_pid exists before writing

[Masami Hiramatsu]

On Wed, 03 Jul 2019 15:50:01 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> While testing on a very old kernel (3.5), the tests failed because the write
> to set_event_pid in the setup code, did not exist. The tests themselves
> could pass, but the setup failed causing an error.
> 

Oops, I've missed it.

> Other files test for existance before writing to them. Do the same for
> set_event_pid and set_ftrace_pid.

Looks good to me,

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

Thank you!

> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  tools/testing/selftests/ftrace/test.d/functions | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> index 779ec11f61bd..a7b06291e32c 100644
> --- a/tools/testing/selftests/ftrace/test.d/functions
> +++ b/tools/testing/selftests/ftrace/test.d/functions
> @@ -91,8 +91,8 @@ initialize_ftrace() { # Reset ftrace to initial-state
>      reset_events_filter
>      reset_ftrace_filter
>      disable_events
> -    echo > set_event_pid	# event tracer is always on
> -    echo > set_ftrace_pid
> +    [ -f set_event_pid ] && echo > set_event_pid  # event tracer is always on
> +    [ -f set_ftrace_pid ] && echo > set_ftrace_pid
>      [ -f set_ftrace_filter ] && echo | tee set_ftrace_*
>      [ -f set_graph_function ] && echo | tee set_graph_*
>      [ -f stack_trace_filter ] && echo > stack_trace_filter
> -- 
> 2.20.1
> 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>