Message-Id: <20160619114616.fc166b6aad3b512c553c7f41@kernel.org>
Date:	Sun, 19 Jun 2016 11:46:16 +0900
From:	Masami Hiramatsu <mhiramat@...nel.org>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Namhyung Kim <namhyung@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Shuah Khan <shuahkhan@...il.com>,
	<linux-kselftest@...r.kernel.org>
Subject: Re: [RFC][PATCH] ftracetest: Fix hist unsupported result in hist
 selftests

On Fri, 17 Jun 2016 17:28:47 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:

> On Tue, 24 May 2016 12:02:35 +0900
> Namhyung Kim <namhyung@...nel.org> wrote:
> 
> > On Mon, May 23, 2016 at 10:32:43PM -0400, Steven Rostedt wrote:
> > > On Tue, 24 May 2016 11:16:31 +0900
> > > Namhyung Kim <namhyung@...nel.org> wrote:
> > > 
> > >   
> > > > Why not checking "hist" file then?  
> > > 
> > > I guess that could be done too, but is there anything wrong with my
> > > current solution? Or is it just too hacky? How would one check if
> > > something exists in a file or not? Say, I want to detect if
> > > preemptirqsoff tracer exists or not, and that only happens if I do a
> > > grep of current_tracer (I have tests coming that will need to do that)?  
> > 
> > There's nothing wrong with your approach IMHO.  But I think checking
> > existence of a file is clearer and consistent to other tests.
> > 
> > For the preemptirqsoff tracer case, it seems there's no other way to
> > check it simply.  It's not hacky to me grep-ing contents to check
> > availability of some option.
> > 
> 
> Ah, due to traveling I never got around to finishing this. What about
> this patch?
> 
> From 7bf19b58ba02e66014efce6c051acba2c6cbd861 Mon Sep 17 00:00:00 2001
> From: "Steven Rostedt (Red Hat)" <rostedt@...dmis.org>
> Date: Mon, 23 May 2016 15:06:30 -0400
> Subject: [PATCH] ftracetest: Fix hist unsupported result in hist selftests
> 
> When histograms are not configured in the kernel, the ftracetest histogram
> selftests should return "unsupported" and not "Failed". To detect this, the
> test scripts have:
> 
>  FEATURE=`grep hist events/sched/sched_process_fork/trigger`
>  if [ -z "$FEATURE" ]; then
>      echo "hist trigger is not supported"
>      exit_unsupported
>  fi
> 
> The problem is that '-e' is in effect and any error will cause the program
> to terminate. The grep for 'hist' fails, because it is not compiled it (thus
> unsupported), but because grep has an error code for failing to find the
> string, it causes the program to terminate, and is marked as a failed test.
> 
> Namhyung Kim recommended to test for the "hist" file located in
> events/sched/sched_process_fork/hist instead, as it is more inline with the
> other checks. As the hist file is only created if the histogram feature is
> enabled, that is a valid check.

Looks good to me :)
Thanks Namhyung and Steve for fixing that!

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


> 
> Link: http://lkml.kernel.org/r/20160523151538.4ea9ce0c@gandalf.local.home
> 
> Suggested-by: Namhyung Kim <namhyung@...nel.org>
> Fixes: 76929ab51f0ee ("kselftests/ftrace: Add hist trigger testcases")
> Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
> ---
>  .../testing/selftests/ftrace/test.d/trigger/trigger-hist-mod.tc  | 9 ++++-----
>  tools/testing/selftests/ftrace/test.d/trigger/trigger-hist.tc    | 9 ++++-----
>  .../testing/selftests/ftrace/test.d/trigger/trigger-multihist.tc | 9 ++++-----
>  3 files changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist-mod.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist-mod.tc
> index c2b61c4fda11..0bf5085281f3 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist-mod.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist-mod.tc
> @@ -23,15 +23,14 @@ if [ ! -f events/sched/sched_process_fork/trigger ]; then
>      exit_unsupported
>  fi
>  
> -reset_tracer
> -do_reset
> -
> -FEATURE=`grep hist events/sched/sched_process_fork/trigger`
> -if [ -z "$FEATURE" ]; then
> +if [ ! -f events/sched/sched_process_fork/hist ]; then
>      echo "hist trigger is not supported"
>      exit_unsupported
>  fi
>  
> +reset_tracer
> +do_reset
> +
>  echo "Test histogram with execname modifier"
>  
>  echo 'hist:keys=common_pid.execname' > events/sched/sched_process_fork/trigger
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist.tc
> index b2902d42a537..a00184cd9c95 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-hist.tc
> @@ -23,15 +23,14 @@ if [ ! -f events/sched/sched_process_fork/trigger ]; then
>      exit_unsupported
>  fi
>  
> -reset_tracer
> -do_reset
> -
> -FEATURE=`grep hist events/sched/sched_process_fork/trigger`
> -if [ -z "$FEATURE" ]; then
> +if [ ! -f events/sched/sched_process_fork/hist ]; then
>      echo "hist trigger is not supported"
>      exit_unsupported
>  fi
>  
> +reset_tracer
> +do_reset
> +
>  echo "Test histogram basic tigger"
>  
>  echo 'hist:keys=parent_pid:vals=child_pid' > events/sched/sched_process_fork/trigger
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-multihist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-multihist.tc
> index 03c4a46561fc..3478b00ead57 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-multihist.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-multihist.tc
> @@ -23,15 +23,14 @@ if [ ! -f events/sched/sched_process_fork/trigger ]; then
>      exit_unsupported
>  fi
>  
> -reset_tracer
> -do_reset
> -
> -FEATURE=`grep hist events/sched/sched_process_fork/trigger`
> -if [ -z "$FEATURE" ]; then
> +if [ ! -f events/sched/sched_process_fork/hist ]; then
>      echo "hist trigger is not supported"
>      exit_unsupported
>  fi
>  
> +reset_tracer
> +do_reset
> +
>  reset_trigger
>  
>  echo "Test histogram multiple tiggers"
> -- 
> 1.9.3
> 


-- 
Masami Hiramatsu <mhiramat@...nel.org>

Powered by blists - more mailing lists