RE: [PATCH v4 37/37] selftests: ftrace: Add inter-event hist triggers testcases

[Jingar, Rajvi]

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