| Message ID | 158834028054.28357.398159034694277189.stgit@devnote2 (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | selftests/ftrace: Fix ftracetest testcases for dash, etc. | expand |
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
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 >
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 > > > > . >
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 > > > > > > > > . > > > > >
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.
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
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 >
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,
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
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 >
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 > >
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 >>> >>> >>> >>> . >>> >> >> >> > >
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,
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)
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
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(-)