Hi Masami, > -----Original Message----- > From: linux-rt-users-owner@xxxxxxxxxxxxxxx [mailto:linux-rt-users- > owner@xxxxxxxxxxxxxxx] On Behalf Of Masami Hiramatsu > Sent: Tuesday, October 31, 2017 7:29 AM > To: Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx> > Cc: rostedt@xxxxxxxxxxx; tglx@xxxxxxxxxxxxx; mhiramat@xxxxxxxxxx; > namhyung@xxxxxxxxxx; Patel, Vedang <vedang.patel@xxxxxxxxx>; > bigeasy@xxxxxxxxxxxxx; joel.opensrc@xxxxxxxxx; joelaf@xxxxxxxxxx; > mathieu.desnoyers@xxxxxxxxxxxx; Liu, Baohong <baohong.liu@xxxxxxxxx>; > Jingar, Rajvi <rajvi.jingar@xxxxxxxxx>; julia@xxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; linux-rt-users@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v4 37/37] selftests: ftrace: Add inter-event hist triggers > testcases > > On Mon, 30 Oct 2017 15:52:19 -0500 > Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx> wrote: > > > From: Rajvi Jingar <rajvi.jingar@xxxxxxxxx> > > > > This adds inter-event hist triggers testcases which covers following: > > - create/remove synthetic event > > - disable histogram for synthetic event > > - extended error support > > - field variable support > > - histogram variables > > - histogram trigger onmatch action > > - histogram trigger onmax action > > - histogram trigger onmatch-onmax action > > - simple expression support > > - combined histogram > > Thank you for adding testcases :) > That helps me to understand how to use it. > I have some comments, see below; > > > [..] > > diff --git > > a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-ex > > tended-error-support.tc > > b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-ex > > tended-error-support.tc > > new file mode 100644 > > index 0000000..41dbf4c > > --- /dev/null > > +++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigge > > +++ r-extended-error-support.tc > > @@ -0,0 +1,39 @@ > > +#!/bin/sh > > +# description: event trigger - test extended error support > > + > > + > > +do_reset() { > > + reset_trigger > > + echo > set_event > > + clear_trace > > +} > > + > > +fail() { #msg > > + do_reset > > + echo $1 > > + exit $FAIL > > +} > > + > > +if [ ! -f set_event ]; then > > + echo "event tracing is not supported" > > + exit_unsupported > > +fi > > + > > +if [ ! -f synthetic_events ]; then > > + echo "synthetic event is not supported" > > + exit_unsupported > > +fi > > + > > +reset_tracer > > +do_reset > > + > > +echo "Test extended error support" > > +echo 'hist:keys=pid:ts0=$common_timestamp.usecs if comm=="ping"' >> > > +events/sched/sched_wakeup/trigger echo > > +'hist:keys=pid:ts0=$common_timestamp.usecs if comm=="ping"' >> > > +events/sched/sched_wakeup/trigger && > > What is the last "&&"? Would you expect that the second echo is fail? > Yes, The the second echo is expected to fail on trying to add same trigger(mainly same variable name). I'll change last "&&" to redirect error/output to /dev/null instead. > > +if ! grep "ERROR:" events/sched/sched_wakeup/hist; then > > Would you like to show the grep result? or you can use -q option for grep > command. > No, I don't need to show grep results. Will add -q option and will do so for all the places with similar case. > [...] > > diff --git > > a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-sy > > nthetic-event-createremove.tc > > b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-sy > > nthetic-event-createremove.tc > > new file mode 100644 > > index 0000000..c73e491 > > --- /dev/null > > +++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigge > > +++ r-synthetic-event-createremove.tc > > @@ -0,0 +1,46 @@ > > +#!/bin/sh > > +# description: event trigger - test synthetic event create remove > > +do_reset() { > > + reset_trigger > > + echo > set_event > > + clear_trace > > +} > > + > > +fail() { #msg > > + do_reset > > + echo $1 > > + exit $FAIL > > +} > > + > > +if [ ! -f set_event ]; then > > + echo "event tracing is not supported" > > + exit_unsupported > > +fi > > + > > +if [ ! -f synthetic_events ]; then > > + echo "synthetic event is not supported" > > + exit_unsupported > > +fi > > + > > +clear_synthetic_events > > +reset_tracer > > +do_reset > > + > > +echo "Test create synthetic event" > > + > > +echo 'wakeup_latency u64 lat pid_t pid char comm[16]' > > > +synthetic_events if [ ! -d events/synthetic/wakeup_latency ]; then > > + fail "Failed to create wakeup_latency synthetic event" > > +fi > > + > > +reset_trigger > > + > > +echo "Test remove synthetic event" > > +echo '!wakeup_latency u64 lat; pid_t pid; char[16] comm' > > > +synthetic_events > > both char[16] and char comm[16] are acceptable? > But I would like to see same style in creating and removing. > Will change to same style in creating and removing. > > +if [ -d events/synthetic/wakeup_latency ]; then > > + fail "Failed to delete wakeup_latency synthetic event" > > +fi > > There seems no expected failure test cases. Could you add such test? > like as echoing events with invalid format etc. > Sure, will add a case trying to add an event with wrong format. > > <off topic> > BTW, $FAIL is not for expressing failure, it is a value for $SIG_RESULT. But it > seems many test cases use it for return code. > > Note that any failure in this script treated as error and exit soon. > Which means caller can get any return code from this script, because the return > code depends on executed command. So, any NON-ZERO code can be treated as > FAILURE code. > > "exit $FAIL" is OK, but just making myself review easier, I'll introduce > exit_pass()/exit_fail(). > <off topic> > > Thank you, > > -- > Masami Hiramatsu <mhiramat@xxxxxxxxxx> > -- > To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in the > body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at > http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
