[3/3] selftests/ftrace: Use /bin/echo instead of built-in echo
diff mbox series

Message ID 158834028054.28357.398159034694277189.stgit@devnote2
State Changes Requested
Headers show
Series
  • selftests/ftrace: Fix ftracetest testcases for dash, etc.
Related show

Commit Message

Masami Hiramatsu May 1, 2020, 1:38 p.m. UTC
Since the built-in echo has different behavior in POSIX shell
(dash) and bash, we forcibly use /bin/echo -E (not interpret
backslash escapes) by default.

This also fixes some test cases which expects built-in
echo command.

Reported-by: Liu Yiding <yidingx.liu@intel.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/testing/selftests/ftrace/test.d/functions    |    3 +++
 .../test.d/trigger/trigger-trace-marker-hist.tc    |    2 +-
 .../trigger-trace-marker-synthetic-kernel.tc       |    4 ++++
 .../trigger/trigger-trace-marker-synthetic.tc      |    4 ++--
 4 files changed, 10 insertions(+), 3 deletions(-)

Comments

Steven Rostedt May 1, 2020, 2:19 p.m. UTC | #1
On Fri,  1 May 2020 22:38:00 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Since the built-in echo has different behavior in POSIX shell
> (dash) and bash, we forcibly use /bin/echo -E (not interpret
> backslash escapes) by default.
> 
> This also fixes some test cases which expects built-in
> echo command.
> 
> Reported-by: Liu Yiding <yidingx.liu@intel.com>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  tools/testing/selftests/ftrace/test.d/functions    |    3 +++
>  .../test.d/trigger/trigger-trace-marker-hist.tc    |    2 +-
>  .../trigger-trace-marker-synthetic-kernel.tc       |    4 ++++
>  .../trigger/trigger-trace-marker-synthetic.tc      |    4 ++--
>  4 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> index 5d4550591ff9..ea59b6ea2c3e 100644
> --- a/tools/testing/selftests/ftrace/test.d/functions
> +++ b/tools/testing/selftests/ftrace/test.d/functions
> @@ -1,3 +1,6 @@
> +# Since the built-in echo has different behavior in POSIX shell (dash) and
> +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes).
> +alias echo="/bin/echo -E"
>  
>  clear_trace() { # reset trace output
>      echo > trace
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> index ab6bedb25736..b3f70f53ee69 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> @@ -30,7 +30,7 @@ fi
>  
>  echo "Test histogram trace_marker tigger"
>  
> -echo 'hist:keys=common_pid' > events/ftrace/print/trigger
> +echo 'hist:keys=ip' > events/ftrace/print/trigger

This is doing more than just changing the echo being used. It's changing
the test being done.

>  for i in `seq 1 10` ; do echo "hello" > trace_marker; done
>  grep 'hitcount: *10$' events/ftrace/print/hist > /dev/null || \
>      fail "hist trigger did not trigger correct times on trace_marker"
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> index 18b4d1c2807e..c1625d945f4d 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> @@ -44,6 +44,10 @@ echo 'latency u64 lat' > synthetic_events
>  echo 'hist:keys=pid:ts0=common_timestamp.usecs' > events/sched/sched_waking/trigger
>  echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)' > events/ftrace/print/trigger
>  echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger
> +
> +# We have to use the built-in echo here because waking up pid must be same
> +# as echoing pid.
> +alias echo=echo
>  sleep 1
>  echo "hello" > trace_marker
>  
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> index dd262d6d0db6..23e52c8d71de 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> @@ -36,8 +36,8 @@ fi
>  echo "Test histogram trace_marker to trace_marker latency histogram trigger"
>  
>  echo 'latency u64 lat' > synthetic_events
> -echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger
> -echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger
> +echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger
> +echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger

This too. And it's not explained in the change log why. In fact, these
changes look like they belong in a separate patch.

-- Steve

>  echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger
>  echo -n "start" > trace_marker
>  echo -n "end" > trace_marker
Masami Hiramatsu May 2, 2020, 3:08 a.m. UTC | #2
On Fri, 1 May 2020 10:19:42 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri,  1 May 2020 22:38:00 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Since the built-in echo has different behavior in POSIX shell
> > (dash) and bash, we forcibly use /bin/echo -E (not interpret
> > backslash escapes) by default.
> > 
> > This also fixes some test cases which expects built-in
> > echo command.
> > 
> > Reported-by: Liu Yiding <yidingx.liu@intel.com>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  tools/testing/selftests/ftrace/test.d/functions    |    3 +++
> >  .../test.d/trigger/trigger-trace-marker-hist.tc    |    2 +-
> >  .../trigger-trace-marker-synthetic-kernel.tc       |    4 ++++
> >  .../trigger/trigger-trace-marker-synthetic.tc      |    4 ++--
> >  4 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> > index 5d4550591ff9..ea59b6ea2c3e 100644
> > --- a/tools/testing/selftests/ftrace/test.d/functions
> > +++ b/tools/testing/selftests/ftrace/test.d/functions
> > @@ -1,3 +1,6 @@
> > +# Since the built-in echo has different behavior in POSIX shell (dash) and
> > +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes).
> > +alias echo="/bin/echo -E"
> >  
> >  clear_trace() { # reset trace output
> >      echo > trace
> > diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > index ab6bedb25736..b3f70f53ee69 100644
> > --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > @@ -30,7 +30,7 @@ fi
> >  
> >  echo "Test histogram trace_marker tigger"
> >  
> > -echo 'hist:keys=common_pid' > events/ftrace/print/trigger
> > +echo 'hist:keys=ip' > events/ftrace/print/trigger
> 
> This is doing more than just changing the echo being used. It's changing
> the test being done.

Yes, I need Tom's review for this change. As far as I can test, this
fixes the test failure. If this isn't acceptable, we can use "alias echo=echo"
for this test case.

Thank you,

> 
> >  for i in `seq 1 10` ; do echo "hello" > trace_marker; done
> >  grep 'hitcount: *10$' events/ftrace/print/hist > /dev/null || \
> >      fail "hist trigger did not trigger correct times on trace_marker"
> > diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> > index 18b4d1c2807e..c1625d945f4d 100644
> > --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> > @@ -44,6 +44,10 @@ echo 'latency u64 lat' > synthetic_events
> >  echo 'hist:keys=pid:ts0=common_timestamp.usecs' > events/sched/sched_waking/trigger
> >  echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)' > events/ftrace/print/trigger
> >  echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger
> > +
> > +# We have to use the built-in echo here because waking up pid must be same
> > +# as echoing pid.
> > +alias echo=echo
> >  sleep 1
> >  echo "hello" > trace_marker
> >  
> > diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> > index dd262d6d0db6..23e52c8d71de 100644
> > --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> > @@ -36,8 +36,8 @@ fi
> >  echo "Test histogram trace_marker to trace_marker latency histogram trigger"
> >  
> >  echo 'latency u64 lat' > synthetic_events
> > -echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger
> > -echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger
> > +echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger
> > +echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger
> 
> This too. And it's not explained in the change log why. In fact, these
> changes look like they belong in a separate patch.
> 
> -- Steve
> 
> >  echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger
> >  echo -n "start" > trace_marker
> >  echo -n "end" > trace_marker
>
Xiao Yang May 7, 2020, 6:45 a.m. UTC | #3
On 2020/5/1 21:38, Masami Hiramatsu wrote:
> Since the built-in echo has different behavior in POSIX shell
> (dash) and bash, we forcibly use /bin/echo -E (not interpret
> backslash escapes) by default.
>
> This also fixes some test cases which expects built-in
> echo command.
>
> Reported-by: Liu Yiding<yidingx.liu@intel.com>
> Signed-off-by: Masami Hiramatsu<mhiramat@kernel.org>
> ---
>   tools/testing/selftests/ftrace/test.d/functions    |    3 +++
>   .../test.d/trigger/trigger-trace-marker-hist.tc    |    2 +-
>   .../trigger-trace-marker-synthetic-kernel.tc       |    4 ++++
>   .../trigger/trigger-trace-marker-synthetic.tc      |    4 ++--
>   4 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> index 5d4550591ff9..ea59b6ea2c3e 100644
> --- a/tools/testing/selftests/ftrace/test.d/functions
> +++ b/tools/testing/selftests/ftrace/test.d/functions
> @@ -1,3 +1,6 @@
> +# Since the built-in echo has different behavior in POSIX shell (dash) and
> +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes).
> +alias echo="/bin/echo -E"
Hi Masami, Steven

It seems that only kprobe_syntax_errors.tc is impacted by the issue 
currently.  Is it necessary for all tests to use /bin/echo and could we 
just make kprobe_syntax_errors.tc use /bin/echo?

Best Regards,
Xiao Yang

>
>   clear_trace() { # reset trace output
>       echo>  trace
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> index ab6bedb25736..b3f70f53ee69 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> @@ -30,7 +30,7 @@ fi
>
>   echo "Test histogram trace_marker tigger"
>
> -echo 'hist:keys=common_pid'>  events/ftrace/print/trigger
> +echo 'hist:keys=ip'>  events/ftrace/print/trigger
>   for i in `seq 1 10` ; do echo "hello">  trace_marker; done
>   grep 'hitcount: *10$' events/ftrace/print/hist>  /dev/null || \
>       fail "hist trigger did not trigger correct times on trace_marker"
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> index 18b4d1c2807e..c1625d945f4d 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> @@ -44,6 +44,10 @@ echo 'latency u64 lat'>  synthetic_events
>   echo 'hist:keys=pid:ts0=common_timestamp.usecs'>  events/sched/sched_waking/trigger
>   echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)'>  events/ftrace/print/trigger
>   echo 'hist:keys=common_pid,lat:sort=lat'>  events/synthetic/latency/trigger
> +
> +# We have to use the built-in echo here because waking up pid must be same
> +# as echoing pid.
> +alias echo=echo
>   sleep 1
>   echo "hello">  trace_marker
>
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> index dd262d6d0db6..23e52c8d71de 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> @@ -36,8 +36,8 @@ fi
>   echo "Test histogram trace_marker to trace_marker latency histogram trigger"
>
>   echo 'latency u64 lat'>  synthetic_events
> -echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"'>  events/ftrace/print/trigger
> -echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>>  events/ftrace/print/trigger
> +echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"'>  events/ftrace/print/trigger
> +echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>>  events/ftrace/print/trigger
>   echo 'hist:keys=common_pid,lat:sort=lat'>  events/synthetic/latency/trigger
>   echo -n "start">  trace_marker
>   echo -n "end">  trace_marker
>
>
>
> .
>
Masami Hiramatsu May 7, 2020, 9:15 a.m. UTC | #4
On Thu, 7 May 2020 14:45:16 +0800
Xiao Yang <yangx.jy@cn.fujitsu.com> wrote:

> On 2020/5/1 21:38, Masami Hiramatsu wrote:
> > Since the built-in echo has different behavior in POSIX shell
> > (dash) and bash, we forcibly use /bin/echo -E (not interpret
> > backslash escapes) by default.
> >
> > This also fixes some test cases which expects built-in
> > echo command.
> >
> > Reported-by: Liu Yiding<yidingx.liu@intel.com>
> > Signed-off-by: Masami Hiramatsu<mhiramat@kernel.org>
> > ---
> >   tools/testing/selftests/ftrace/test.d/functions    |    3 +++
> >   .../test.d/trigger/trigger-trace-marker-hist.tc    |    2 +-
> >   .../trigger-trace-marker-synthetic-kernel.tc       |    4 ++++
> >   .../trigger/trigger-trace-marker-synthetic.tc      |    4 ++--
> >   4 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> > index 5d4550591ff9..ea59b6ea2c3e 100644
> > --- a/tools/testing/selftests/ftrace/test.d/functions
> > +++ b/tools/testing/selftests/ftrace/test.d/functions
> > @@ -1,3 +1,6 @@
> > +# Since the built-in echo has different behavior in POSIX shell (dash) and
> > +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes).
> > +alias echo="/bin/echo -E"
> Hi Masami, Steven
> 
> It seems that only kprobe_syntax_errors.tc is impacted by the issue 
> currently.  Is it necessary for all tests to use /bin/echo and could we 
> just make kprobe_syntax_errors.tc use /bin/echo?

Yes, I would like to unify the "echo"'s behavior among the testcases
instead of patching each failure in the future.
Or would you have any concern on it?

Thank you,

> 
> Best Regards,
> Xiao Yang
> 
> >
> >   clear_trace() { # reset trace output
> >       echo>  trace
> > diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > index ab6bedb25736..b3f70f53ee69 100644
> > --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > @@ -30,7 +30,7 @@ fi
> >
> >   echo "Test histogram trace_marker tigger"
> >
> > -echo 'hist:keys=common_pid'>  events/ftrace/print/trigger
> > +echo 'hist:keys=ip'>  events/ftrace/print/trigger
> >   for i in `seq 1 10` ; do echo "hello">  trace_marker; done
> >   grep 'hitcount: *10$' events/ftrace/print/hist>  /dev/null || \
> >       fail "hist trigger did not trigger correct times on trace_marker"
> > diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> > index 18b4d1c2807e..c1625d945f4d 100644
> > --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> > @@ -44,6 +44,10 @@ echo 'latency u64 lat'>  synthetic_events
> >   echo 'hist:keys=pid:ts0=common_timestamp.usecs'>  events/sched/sched_waking/trigger
> >   echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)'>  events/ftrace/print/trigger
> >   echo 'hist:keys=common_pid,lat:sort=lat'>  events/synthetic/latency/trigger
> > +
> > +# We have to use the built-in echo here because waking up pid must be same
> > +# as echoing pid.
> > +alias echo=echo
> >   sleep 1
> >   echo "hello">  trace_marker
> >
> > diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> > index dd262d6d0db6..23e52c8d71de 100644
> > --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> > @@ -36,8 +36,8 @@ fi
> >   echo "Test histogram trace_marker to trace_marker latency histogram trigger"
> >
> >   echo 'latency u64 lat'>  synthetic_events
> > -echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"'>  events/ftrace/print/trigger
> > -echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>>  events/ftrace/print/trigger
> > +echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"'>  events/ftrace/print/trigger
> > +echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>>  events/ftrace/print/trigger
> >   echo 'hist:keys=common_pid,lat:sort=lat'>  events/synthetic/latency/trigger
> >   echo -n "start">  trace_marker
> >   echo -n "end">  trace_marker
> >
> >
> >
> > .
> >
> 
> 
>
Andreas Schwab May 7, 2020, 9:22 a.m. UTC | #5
On Mai 01 2020, Masami Hiramatsu wrote:

> Since the built-in echo has different behavior in POSIX shell
> (dash) and bash, we forcibly use /bin/echo -E (not interpret
> backslash escapes) by default.

How about using printf instead (at least where it matters)?

Andreas.
Steven Rostedt May 7, 2020, 1:12 p.m. UTC | #6
On Sat, 2 May 2020 12:08:42 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > > diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > > index ab6bedb25736..b3f70f53ee69 100644
> > > --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > > +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > > @@ -30,7 +30,7 @@ fi
> > >  
> > >  echo "Test histogram trace_marker tigger"
> > >  
> > > -echo 'hist:keys=common_pid' > events/ftrace/print/trigger
> > > +echo 'hist:keys=ip' > events/ftrace/print/trigger  
> > 
> > This is doing more than just changing the echo being used. It's changing
> > the test being done.  
> 
> Yes, I need Tom's review for this change. As far as I can test, this
> fixes the test failure. If this isn't acceptable, we can use "alias echo=echo"
> for this test case.
> 

I still don't see how changing "keys=common_pid" to "keys=ip" has anything
to do with the echo patch. If that is a problem, it should be a different
patch with explanation to why "keys=common_pid" is broken.

-- Steve
Masami Hiramatsu May 7, 2020, 3:50 p.m. UTC | #7
On Thu, 7 May 2020 09:12:07 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat, 2 May 2020 12:08:42 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > > > diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > > > index ab6bedb25736..b3f70f53ee69 100644
> > > > --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > > > +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > > > @@ -30,7 +30,7 @@ fi
> > > >  
> > > >  echo "Test histogram trace_marker tigger"
> > > >  
> > > > -echo 'hist:keys=common_pid' > events/ftrace/print/trigger
> > > > +echo 'hist:keys=ip' > events/ftrace/print/trigger  
> > > 
> > > This is doing more than just changing the echo being used. It's changing
> > > the test being done.  
> > 
> > Yes, I need Tom's review for this change. As far as I can test, this
> > fixes the test failure. If this isn't acceptable, we can use "alias echo=echo"
> > for this test case.
> > 
> 
> I still don't see how changing "keys=common_pid" to "keys=ip" has anything
> to do with the echo patch. If that is a problem, it should be a different
> patch with explanation to why "keys=common_pid" is broken.

This test case uses a trace_marker event to make a histogram with
the common_pid key, and it expects the "echo" command is built-in command
so that the pid is same while writing several events to trace_marker.
I changed it to "ip" which is always same if trace_marker interface is
used.

Thank you,

> 
> -- Steve
>
Masami Hiramatsu May 7, 2020, 3:52 p.m. UTC | #8
On Thu, 07 May 2020 11:22:28 +0200
Andreas Schwab <schwab@linux-m68k.org> wrote:

> On Mai 01 2020, Masami Hiramatsu wrote:
> 
> > Since the built-in echo has different behavior in POSIX shell
> > (dash) and bash, we forcibly use /bin/echo -E (not interpret
> > backslash escapes) by default.
> 
> How about using printf instead (at least where it matters)?

Hmm, I think replacing all echos with printf in the testcase might be
much harder than this...

Thank you,
Steven Rostedt May 7, 2020, 5:25 p.m. UTC | #9
On Fri, 8 May 2020 00:50:28 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > > Yes, I need Tom's review for this change. As far as I can test, this
> > > fixes the test failure. If this isn't acceptable, we can use "alias echo=echo"
> > > for this test case.
> > >   
> > 
> > I still don't see how changing "keys=common_pid" to "keys=ip" has anything
> > to do with the echo patch. If that is a problem, it should be a different
> > patch with explanation to why "keys=common_pid" is broken.  
> 
> This test case uses a trace_marker event to make a histogram with
> the common_pid key, and it expects the "echo" command is built-in command
> so that the pid is same while writing several events to trace_marker.
> I changed it to "ip" which is always same if trace_marker interface is
> used.

Can you explicitly state that in your change log? It wasn't obvious from
what you meant with:

"This also fixes some test cases which expects built-in echo command."

Thanks!

-- Steve
Zanussi, Tom May 7, 2020, 8:32 p.m. UTC | #10
Hi,

On 5/7/2020 12:25 PM, Steven Rostedt wrote:
> On Fri, 8 May 2020 00:50:28 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
>>>> Yes, I need Tom's review for this change. As far as I can test, this
>>>> fixes the test failure. If this isn't acceptable, we can use "alias echo=echo"
>>>> for this test case.
>>>>    
>>>
>>> I still don't see how changing "keys=common_pid" to "keys=ip" has anything
>>> to do with the echo patch. If that is a problem, it should be a different
>>> patch with explanation to why "keys=common_pid" is broken.
>>
>> This test case uses a trace_marker event to make a histogram with
>> the common_pid key, and it expects the "echo" command is built-in command
>> so that the pid is same while writing several events to trace_marker.
>> I changed it to "ip" which is always same if trace_marker interface is
>> used.
> 
> Can you explicitly state that in your change log? It wasn't obvious from
> what you meant with:
> 
> "This also fixes some test cases which expects built-in echo command."
> 

With that change,

Reviewed-by: Tom Zanussi <tom.zanussi@linux.intel.com>

Thanks,

Tom

> Thanks!
> 
> -- Steve
>
Masami Hiramatsu May 8, 2020, 7:11 a.m. UTC | #11
On Thu, 7 May 2020 15:32:46 -0500
"Zanussi, Tom" <tom.zanussi@linux.intel.com> wrote:

> Hi,
> 
> On 5/7/2020 12:25 PM, Steven Rostedt wrote:
> > On Fri, 8 May 2020 00:50:28 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> >>>> Yes, I need Tom's review for this change. As far as I can test, this
> >>>> fixes the test failure. If this isn't acceptable, we can use "alias echo=echo"
> >>>> for this test case.
> >>>>    
> >>>
> >>> I still don't see how changing "keys=common_pid" to "keys=ip" has anything
> >>> to do with the echo patch. If that is a problem, it should be a different
> >>> patch with explanation to why "keys=common_pid" is broken.
> >>
> >> This test case uses a trace_marker event to make a histogram with
> >> the common_pid key, and it expects the "echo" command is built-in command
> >> so that the pid is same while writing several events to trace_marker.
> >> I changed it to "ip" which is always same if trace_marker interface is
> >> used.
> > 
> > Can you explicitly state that in your change log? It wasn't obvious from
> > what you meant with:
> > 
> > "This also fixes some test cases which expects built-in echo command."

OK, will add the description.

> > 
> 
> With that change,
> 
> Reviewed-by: Tom Zanussi <tom.zanussi@linux.intel.com>

Thanks Tom!

> 
> Thanks,
> 
> Tom
> 
> > Thanks!
> > 
> > -- Steve
> >
Xiao Yang May 11, 2020, 7:22 a.m. UTC | #12
On 2020/5/7 17:15, Masami Hiramatsu wrote:
> On Thu, 7 May 2020 14:45:16 +0800
> Xiao Yang<yangx.jy@cn.fujitsu.com>  wrote:
>
>> On 2020/5/1 21:38, Masami Hiramatsu wrote:
>>> Since the built-in echo has different behavior in POSIX shell
>>> (dash) and bash, we forcibly use /bin/echo -E (not interpret
>>> backslash escapes) by default.
>>>
>>> This also fixes some test cases which expects built-in
>>> echo command.
>>>
>>> Reported-by: Liu Yiding<yidingx.liu@intel.com>
>>> Signed-off-by: Masami Hiramatsu<mhiramat@kernel.org>
>>> ---
>>>    tools/testing/selftests/ftrace/test.d/functions    |    3 +++
>>>    .../test.d/trigger/trigger-trace-marker-hist.tc    |    2 +-
>>>    .../trigger-trace-marker-synthetic-kernel.tc       |    4 ++++
>>>    .../trigger/trigger-trace-marker-synthetic.tc      |    4 ++--
>>>    4 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
>>> index 5d4550591ff9..ea59b6ea2c3e 100644
>>> --- a/tools/testing/selftests/ftrace/test.d/functions
>>> +++ b/tools/testing/selftests/ftrace/test.d/functions
>>> @@ -1,3 +1,6 @@
>>> +# Since the built-in echo has different behavior in POSIX shell (dash) and
>>> +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes).
>>> +alias echo="/bin/echo -E"
>> Hi Masami, Steven
>>
>> It seems that only kprobe_syntax_errors.tc is impacted by the issue
>> currently.  Is it necessary for all tests to use /bin/echo and could we
>> just make kprobe_syntax_errors.tc use /bin/echo?
>
> Yes, I would like to unify the "echo"'s behavior among the testcases
> instead of patching each failure in the future.
> Or would you have any concern on it?
Hi Masami,

Very sorry for the late reply.

We may not avoid fixing related failures after your change:
1) We have to reuse built-in echo (do alias echo=echo) if we want to 
test common_pid for histogram.
2) We have to reuse built-in echo if some new tests want to interpret 
backslash escapes in future.

Is it simple to provide two implementations of echo?(built-in echo and 
echo command?) and then just apply echo command for kprobe_syntax_errors.tc?

BTW: My suggestion may not be correct.

Best Regards,
Xiao Yang
>
> Thank you,
>
>>
>> Best Regards,
>> Xiao Yang
>>
>>>
>>>    clear_trace() { # reset trace output
>>>        echo>   trace
>>> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
>>> index ab6bedb25736..b3f70f53ee69 100644
>>> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
>>> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
>>> @@ -30,7 +30,7 @@ fi
>>>
>>>    echo "Test histogram trace_marker tigger"
>>>
>>> -echo 'hist:keys=common_pid'>   events/ftrace/print/trigger
>>> +echo 'hist:keys=ip'>   events/ftrace/print/trigger
>>>    for i in `seq 1 10` ; do echo "hello">   trace_marker; done
>>>    grep 'hitcount: *10$' events/ftrace/print/hist>   /dev/null || \
>>>        fail "hist trigger did not trigger correct times on trace_marker"
>>> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
>>> index 18b4d1c2807e..c1625d945f4d 100644
>>> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
>>> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
>>> @@ -44,6 +44,10 @@ echo 'latency u64 lat'>   synthetic_events
>>>    echo 'hist:keys=pid:ts0=common_timestamp.usecs'>   events/sched/sched_waking/trigger
>>>    echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)'>   events/ftrace/print/trigger
>>>    echo 'hist:keys=common_pid,lat:sort=lat'>   events/synthetic/latency/trigger
>>> +
>>> +# We have to use the built-in echo here because waking up pid must be same
>>> +# as echoing pid.
>>> +alias echo=echo
>>>    sleep 1
>>>    echo "hello">   trace_marker
>>>
>>> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
>>> index dd262d6d0db6..23e52c8d71de 100644
>>> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
>>> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
>>> @@ -36,8 +36,8 @@ fi
>>>    echo "Test histogram trace_marker to trace_marker latency histogram trigger"
>>>
>>>    echo 'latency u64 lat'>   synthetic_events
>>> -echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"'>   events/ftrace/print/trigger
>>> -echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>>   events/ftrace/print/trigger
>>> +echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"'>   events/ftrace/print/trigger
>>> +echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>>   events/ftrace/print/trigger
>>>    echo 'hist:keys=common_pid,lat:sort=lat'>   events/synthetic/latency/trigger
>>>    echo -n "start">   trace_marker
>>>    echo -n "end">   trace_marker
>>>
>>>
>>>
>>> .
>>>
>>
>>
>>
>
>
Masami Hiramatsu May 11, 2020, 9:27 a.m. UTC | #13
On Mon, 11 May 2020 15:22:25 +0800
Xiao Yang <yangx.jy@cn.fujitsu.com> wrote:

> On 2020/5/7 17:15, Masami Hiramatsu wrote:
> > On Thu, 7 May 2020 14:45:16 +0800
> > Xiao Yang<yangx.jy@cn.fujitsu.com>  wrote:
> >
> >> On 2020/5/1 21:38, Masami Hiramatsu wrote:
> >>> Since the built-in echo has different behavior in POSIX shell
> >>> (dash) and bash, we forcibly use /bin/echo -E (not interpret
> >>> backslash escapes) by default.
> >>>
> >>> This also fixes some test cases which expects built-in
> >>> echo command.
> >>>
> >>> Reported-by: Liu Yiding<yidingx.liu@intel.com>
> >>> Signed-off-by: Masami Hiramatsu<mhiramat@kernel.org>
> >>> ---
> >>>    tools/testing/selftests/ftrace/test.d/functions    |    3 +++
> >>>    .../test.d/trigger/trigger-trace-marker-hist.tc    |    2 +-
> >>>    .../trigger-trace-marker-synthetic-kernel.tc       |    4 ++++
> >>>    .../trigger/trigger-trace-marker-synthetic.tc      |    4 ++--
> >>>    4 files changed, 10 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> >>> index 5d4550591ff9..ea59b6ea2c3e 100644
> >>> --- a/tools/testing/selftests/ftrace/test.d/functions
> >>> +++ b/tools/testing/selftests/ftrace/test.d/functions
> >>> @@ -1,3 +1,6 @@
> >>> +# Since the built-in echo has different behavior in POSIX shell (dash) and
> >>> +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes).
> >>> +alias echo="/bin/echo -E"
> >> Hi Masami, Steven
> >>
> >> It seems that only kprobe_syntax_errors.tc is impacted by the issue
> >> currently.  Is it necessary for all tests to use /bin/echo and could we
> >> just make kprobe_syntax_errors.tc use /bin/echo?
> >
> > Yes, I would like to unify the "echo"'s behavior among the testcases
> > instead of patching each failure in the future.
> > Or would you have any concern on it?
> Hi Masami,
> 
> Very sorry for the late reply.
> 
> We may not avoid fixing related failures after your change:
> 1) We have to reuse built-in echo (do alias echo=echo) if we want to 
> test common_pid for histogram.
> 2) We have to reuse built-in echo if some new tests want to interpret 
> backslash escapes in future.

1) yes, that's what I need to do for avoiding "pid" key histogram
  (but I think we should have better way to test it)
2) No, in that case you should use "/bin/echo -e" explicitly.
   dash's built-in echo doesn't support it.

> Is it simple to provide two implementations of echo?(built-in echo and 
> echo command?) and then just apply echo command for kprobe_syntax_errors.tc?

Hmm, OK, there might be another reason we reconsider this patch.

- Alisasing echo (this patch) can avoid dash related issues but
  this also makes "echo" running in another process implicitly.

- Using /bin/echo for backslash explicitly will be missed unless
  user runs it on dash, but it will keep "echo" in same process.

So both have pros/cons, but your idea will be locally effected.
OK, I'll retry it.

Thank you,
David Laight May 11, 2020, 10:34 a.m. UTC | #14
From: Masami Hiramatsu
> Sent: 11 May 2020 10:28
...
> > We may not avoid fixing related failures after your change:
> > 1) We have to reuse built-in echo (do alias echo=echo) if we want to
> > test common_pid for histogram.
> > 2) We have to reuse built-in echo if some new tests want to interpret
> > backslash escapes in future.
> 
> 1) yes, that's what I need to do for avoiding "pid" key histogram
>   (but I think we should have better way to test it)
> 2) No, in that case you should use "/bin/echo -e" explicitly.
>    dash's built-in echo doesn't support it.
> 
> > Is it simple to provide two implementations of echo?(built-in echo and
> > echo command?) and then just apply echo command for kprobe_syntax_errors.tc?
> 
> Hmm, OK, there might be another reason we reconsider this patch.
> 
> - Alisasing echo (this patch) can avoid dash related issues but
>   this also makes "echo" running in another process implicitly.
> 
> - Using /bin/echo for backslash explicitly will be missed unless
>   user runs it on dash, but it will keep "echo" in same process.

Why not change to use printf - probably a builtin in both shells?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Patch
diff mbox series

diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
index 5d4550591ff9..ea59b6ea2c3e 100644
--- a/tools/testing/selftests/ftrace/test.d/functions
+++ b/tools/testing/selftests/ftrace/test.d/functions
@@ -1,3 +1,6 @@ 
+# Since the built-in echo has different behavior in POSIX shell (dash) and
+# bash, we forcibly use /bin/echo -E (not interpret backslash escapes).
+alias echo="/bin/echo -E"
 
 clear_trace() { # reset trace output
     echo > trace
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
index ab6bedb25736..b3f70f53ee69 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
@@ -30,7 +30,7 @@  fi
 
 echo "Test histogram trace_marker tigger"
 
-echo 'hist:keys=common_pid' > events/ftrace/print/trigger
+echo 'hist:keys=ip' > events/ftrace/print/trigger
 for i in `seq 1 10` ; do echo "hello" > trace_marker; done
 grep 'hitcount: *10$' events/ftrace/print/hist > /dev/null || \
     fail "hist trigger did not trigger correct times on trace_marker"
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
index 18b4d1c2807e..c1625d945f4d 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
@@ -44,6 +44,10 @@  echo 'latency u64 lat' > synthetic_events
 echo 'hist:keys=pid:ts0=common_timestamp.usecs' > events/sched/sched_waking/trigger
 echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)' > events/ftrace/print/trigger
 echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger
+
+# We have to use the built-in echo here because waking up pid must be same
+# as echoing pid.
+alias echo=echo
 sleep 1
 echo "hello" > trace_marker
 
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
index dd262d6d0db6..23e52c8d71de 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
@@ -36,8 +36,8 @@  fi
 echo "Test histogram trace_marker to trace_marker latency histogram trigger"
 
 echo 'latency u64 lat' > synthetic_events
-echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger
-echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger
+echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger
+echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger
 echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger
 echo -n "start" > trace_marker
 echo -n "end" > trace_marker