* [PATCH] fstests: add basic ftrace support @ 2021-06-01 7:31 Qu Wenruo 2021-06-01 16:42 ` Darrick J. Wong 0 siblings, 1 reply; 5+ messages in thread From: Qu Wenruo @ 2021-06-01 7:31 UTC (permalink / raw) To: fstests Sometimes developers want trace dump for certain test cases. Normally I just add "trace-cmd" calls in "check", but it would be much better to let fstests to support ftrace dumping. This patchset will add basic ftrace dumping support by: - Clear all buffers before running each test - Start tracing before running each test - End tracing after test finished - Copy the trace to "$seqres.trace" if needed The condition is either: * $KEEP_TRACE environment is set to "yes" * The test case failed Currently we only support the main ftrace buffer, but all supporting functions have support for ftrace instances, for later expansion. Signed-off-by: Qu Wenruo <wqu@suse.com> --- check | 12 +++++++- common/ftrace | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++ common/rc | 1 + 3 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 common/ftrace diff --git a/check b/check index ba192042..0a09dcf9 100755 --- a/check +++ b/check @@ -801,7 +801,7 @@ function run_section() fi # really going to try and run this one - rm -f $seqres.out.bad + rm -f $seqres.out.bad $seqres.trace # check if we really should run it _expunge_test $seqnum @@ -839,6 +839,10 @@ function run_section() # to be reported for each test (echo 1 > $DEBUGFS_MNT/clear_warn_once) > /dev/null 2>&1 + # Clear previous trace and start new trace + _clear_trace_buffers + _start_trace + if [ "$DUMP_OUTPUT" = true ]; then _run_seq 2>&1 | tee $tmp.out # Because $? would get tee's return code @@ -848,6 +852,11 @@ function run_section() sts=$? fi + _end_trace + if [ "$KEEP_TRACE" == "yes" ]; then + _copy_trace "$seqres.trace" + fi + if [ -f core ]; then _dump_err_cont "[dumped core]" mv core $RESULT_BASE/$seqnum.core @@ -932,6 +941,7 @@ function run_section() # make sure we record the status of the last test we ran. if $err ; then + _copy_trace "$seqres.trace" bad="$bad $seqnum" n_bad=`expr $n_bad + 1` tc_status="fail" diff --git a/common/ftrace b/common/ftrace new file mode 100644 index 00000000..36886484 --- /dev/null +++ b/common/ftrace @@ -0,0 +1,82 @@ +# +# Common ftrace related functions +# + +TRACE_DIR="/sys/kernel/debug/tracing" + +_clear_trace_buffers() +{ + if [ ! -d "${TRACE_DIR}" ]; then + return + fi + + # Clear the main buffer + echo 0 > "${TRACE_DIR}/trace" + + # Clear each instance buffer + for i in $(ls "${TRACE_DIR}/instances"); do + echo 0 > "${i}/trace" + done +} + +_start_trace() +{ + instance=$1 + + if [ ! -d "${TRACE_DIR}" ]; then + return + fi + + if [ -z "${instance}" ]; then + echo 1 > "${TRACE_DIR}/tracing_on" + else + mkdir -p "${TRACE_DIR}/instances/${instance}" + echo 1 > "${TRACE_DIR}/instances/${instance}/tracing_on" + fi +} + +_end_trace() +{ + instance=$1 + + if [ ! -d "${TRACE_DIR}" ]; then + return + fi + + if [ -z "${instance}" ]; then + echo 0 > "${TRACE_DIR}/tracing_on" + else + mkdir -p "${TRACE_DIR}/instances/${instance}" + echo 0 > "${TRACE_DIR}/instances/${instance}/tracing_on" + fi +} + +_remove_empty_trace() +{ + file="$1" + if [ ! -f "$file" ]; then + return + fi + + if [ -z "$(head -n 15 $file | sed '/^#/d')" ]; then + rm $file + fi +} + +_copy_trace() +{ + dest="$1" + instance="$2" + + if [ ! -d "${TRACE_DIR}" ]; then + return + fi + + if [ -z "${instance}" ]; then + cp "${TRACE_DIR}/trace" "$dest" + elif [ -d "${TRACE_DIR}/instances/${instance}" ]; then + cp "${TRACE_DIR}/instances/${instance}/trace" "$dest" + fi + + _remove_empty_trace "$dest" +} diff --git a/common/rc b/common/rc index 919028ef..f9de1517 100644 --- a/common/rc +++ b/common/rc @@ -3,6 +3,7 @@ # Copyright (c) 2000-2006 Silicon Graphics, Inc. All Rights Reserved. . common/config +. common/ftrace BC=$(which bc 2> /dev/null) || BC= -- 2.31.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] fstests: add basic ftrace support 2021-06-01 7:31 [PATCH] fstests: add basic ftrace support Qu Wenruo @ 2021-06-01 16:42 ` Darrick J. Wong 2021-06-01 23:45 ` Qu Wenruo 0 siblings, 1 reply; 5+ messages in thread From: Darrick J. Wong @ 2021-06-01 16:42 UTC (permalink / raw) To: Qu Wenruo; +Cc: fstests On Tue, Jun 01, 2021 at 03:31:33PM +0800, Qu Wenruo wrote: > Sometimes developers want trace dump for certain test cases. > > Normally I just add "trace-cmd" calls in "check", but it would be much Heh, so do I! > better to let fstests to support ftrace dumping. > > This patchset will add basic ftrace dumping support by: > > - Clear all buffers before running each test > - Start tracing before running each test > - End tracing after test finished > - Copy the trace to "$seqres.trace" if needed > The condition is either: > * $KEEP_TRACE environment is set to "yes" > * The test case failed > > Currently we only support the main ftrace buffer, but all supporting > functions have support for ftrace instances, for later expansion. I... did not know one could /have/ separate instances. > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > check | 12 +++++++- > common/ftrace | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++ > common/rc | 1 + > 3 files changed, 94 insertions(+), 1 deletion(-) > create mode 100644 common/ftrace > > diff --git a/check b/check > index ba192042..0a09dcf9 100755 > --- a/check > +++ b/check > @@ -801,7 +801,7 @@ function run_section() > fi > > # really going to try and run this one > - rm -f $seqres.out.bad > + rm -f $seqres.out.bad $seqres.trace > > # check if we really should run it > _expunge_test $seqnum > @@ -839,6 +839,10 @@ function run_section() > # to be reported for each test > (echo 1 > $DEBUGFS_MNT/clear_warn_once) > /dev/null 2>&1 > > + # Clear previous trace and start new trace > + _clear_trace_buffers > + _start_trace How do you actually turn on specific tracepoints? Or is the idea here to capture trace data for each test in a separate file, and it's up to the ./check caller to set that up? > + > if [ "$DUMP_OUTPUT" = true ]; then > _run_seq 2>&1 | tee $tmp.out > # Because $? would get tee's return code > @@ -848,6 +852,11 @@ function run_section() > sts=$? > fi > > + _end_trace > + if [ "$KEEP_TRACE" == "yes" ]; then > + _copy_trace "$seqres.trace" > + fi > + > if [ -f core ]; then > _dump_err_cont "[dumped core]" > mv core $RESULT_BASE/$seqnum.core > @@ -932,6 +941,7 @@ function run_section() > > # make sure we record the status of the last test we ran. > if $err ; then > + _copy_trace "$seqres.trace" > bad="$bad $seqnum" > n_bad=`expr $n_bad + 1` > tc_status="fail" > diff --git a/common/ftrace b/common/ftrace > new file mode 100644 > index 00000000..36886484 > --- /dev/null > +++ b/common/ftrace > @@ -0,0 +1,82 @@ > +# > +# Common ftrace related functions > +# New file needs a SPDX header. > + > +TRACE_DIR="/sys/kernel/debug/tracing" > + > +_clear_trace_buffers() > +{ > + if [ ! -d "${TRACE_DIR}" ]; then > + return > + fi > + > + # Clear the main buffer > + echo 0 > "${TRACE_DIR}/trace" If one were already running trace-cmd record, will this mess up its ability to collect trace data? > + > + # Clear each instance buffer > + for i in $(ls "${TRACE_DIR}/instances"); do > + echo 0 > "${i}/trace" > + done > +} > + > +_start_trace() > +{ > + instance=$1 local instance="$1", please don't pollute the caller's environment. > + > + if [ ! -d "${TRACE_DIR}" ]; then > + return > + fi > + > + if [ -z "${instance}" ]; then > + echo 1 > "${TRACE_DIR}/tracing_on" > + else > + mkdir -p "${TRACE_DIR}/instances/${instance}" > + echo 1 > "${TRACE_DIR}/instances/${instance}/tracing_on" > + fi > +} > + > +_end_trace() > +{ > + instance=$1 > + > + if [ ! -d "${TRACE_DIR}" ]; then > + return > + fi > + > + if [ -z "${instance}" ]; then > + echo 0 > "${TRACE_DIR}/tracing_on" > + else > + mkdir -p "${TRACE_DIR}/instances/${instance}" Er, what's the logic here? Ensure that the instance exist so that we can disable it? --D > + echo 0 > "${TRACE_DIR}/instances/${instance}/tracing_on" > + fi > +} > + > +_remove_empty_trace() > +{ > + file="$1" > + if [ ! -f "$file" ]; then > + return > + fi > + > + if [ -z "$(head -n 15 $file | sed '/^#/d')" ]; then > + rm $file > + fi > +} > + > +_copy_trace() > +{ > + dest="$1" > + instance="$2" > + > + if [ ! -d "${TRACE_DIR}" ]; then > + return > + fi > + > + if [ -z "${instance}" ]; then > + cp "${TRACE_DIR}/trace" "$dest" > + elif [ -d "${TRACE_DIR}/instances/${instance}" ]; then > + cp "${TRACE_DIR}/instances/${instance}/trace" "$dest" > + fi > + > + _remove_empty_trace "$dest" > +} > diff --git a/common/rc b/common/rc > index 919028ef..f9de1517 100644 > --- a/common/rc > +++ b/common/rc > @@ -3,6 +3,7 @@ > # Copyright (c) 2000-2006 Silicon Graphics, Inc. All Rights Reserved. > > . common/config > +. common/ftrace > > BC=$(which bc 2> /dev/null) || BC= > > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fstests: add basic ftrace support 2021-06-01 16:42 ` Darrick J. Wong @ 2021-06-01 23:45 ` Qu Wenruo 2021-06-03 16:26 ` Darrick J. Wong 0 siblings, 1 reply; 5+ messages in thread From: Qu Wenruo @ 2021-06-01 23:45 UTC (permalink / raw) To: Darrick J. Wong; +Cc: fstests On 2021/6/2 上午12:42, Darrick J. Wong wrote: > On Tue, Jun 01, 2021 at 03:31:33PM +0800, Qu Wenruo wrote: >> Sometimes developers want trace dump for certain test cases. >> >> Normally I just add "trace-cmd" calls in "check", but it would be much > > Heh, so do I! > >> better to let fstests to support ftrace dumping. >> >> This patchset will add basic ftrace dumping support by: >> >> - Clear all buffers before running each test >> - Start tracing before running each test >> - End tracing after test finished >> - Copy the trace to "$seqres.trace" if needed >> The condition is either: >> * $KEEP_TRACE environment is set to "yes" >> * The test case failed >> >> Currently we only support the main ftrace buffer, but all supporting >> functions have support for ftrace instances, for later expansion. > > I... did not know one could /have/ separate instances. So did I, until I checked the code of "trace-cmd clear". > >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> check | 12 +++++++- >> common/ftrace | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> common/rc | 1 + >> 3 files changed, 94 insertions(+), 1 deletion(-) >> create mode 100644 common/ftrace >> >> diff --git a/check b/check >> index ba192042..0a09dcf9 100755 >> --- a/check >> +++ b/check >> @@ -801,7 +801,7 @@ function run_section() >> fi >> >> # really going to try and run this one >> - rm -f $seqres.out.bad >> + rm -f $seqres.out.bad $seqres.trace >> >> # check if we really should run it >> _expunge_test $seqnum >> @@ -839,6 +839,10 @@ function run_section() >> # to be reported for each test >> (echo 1 > $DEBUGFS_MNT/clear_warn_once) > /dev/null 2>&1 >> >> + # Clear previous trace and start new trace >> + _clear_trace_buffers >> + _start_trace > > How do you actually turn on specific tracepoints? Normally I use trace_printk(), thus no need to enable it manually. > > Or is the idea here to capture trace data for each test in a separate > file, and it's up to the ./check caller to set that up? My idea is, it should be the QA guy to set it up. Either using my stupid trace_printk() way, or enable those events before running the test. > >> + >> if [ "$DUMP_OUTPUT" = true ]; then >> _run_seq 2>&1 | tee $tmp.out >> # Because $? would get tee's return code >> @@ -848,6 +852,11 @@ function run_section() >> sts=$? >> fi >> >> + _end_trace >> + if [ "$KEEP_TRACE" == "yes" ]; then >> + _copy_trace "$seqres.trace" >> + fi >> + >> if [ -f core ]; then >> _dump_err_cont "[dumped core]" >> mv core $RESULT_BASE/$seqnum.core >> @@ -932,6 +941,7 @@ function run_section() >> >> # make sure we record the status of the last test we ran. >> if $err ; then >> + _copy_trace "$seqres.trace" >> bad="$bad $seqnum" >> n_bad=`expr $n_bad + 1` >> tc_status="fail" >> diff --git a/common/ftrace b/common/ftrace >> new file mode 100644 >> index 00000000..36886484 >> --- /dev/null >> +++ b/common/ftrace >> @@ -0,0 +1,82 @@ >> +# >> +# Common ftrace related functions >> +# > > New file needs a SPDX header. Oh, forgot that. > >> + >> +TRACE_DIR="/sys/kernel/debug/tracing" >> + >> +_clear_trace_buffers() >> +{ >> + if [ ! -d "${TRACE_DIR}" ]; then >> + return >> + fi >> + >> + # Clear the main buffer >> + echo 0 > "${TRACE_DIR}/trace" > > If one were already running trace-cmd record, will this mess up its > ability to collect trace data? I guess it may. But that shouldn't be a problem, all existing buffers are before running the tests, thus should be considered as noise. > >> + >> + # Clear each instance buffer >> + for i in $(ls "${TRACE_DIR}/instances"); do >> + echo 0 > "${i}/trace" >> + done >> +} >> + >> +_start_trace() >> +{ >> + instance=$1 > > local instance="$1", please don't pollute the caller's environment. Oh, forgot it again... > >> + >> + if [ ! -d "${TRACE_DIR}" ]; then >> + return >> + fi >> + >> + if [ -z "${instance}" ]; then >> + echo 1 > "${TRACE_DIR}/tracing_on" >> + else >> + mkdir -p "${TRACE_DIR}/instances/${instance}" >> + echo 1 > "${TRACE_DIR}/instances/${instance}/tracing_on" >> + fi >> +} >> + >> +_end_trace() >> +{ >> + instance=$1 >> + >> + if [ ! -d "${TRACE_DIR}" ]; then >> + return >> + fi >> + >> + if [ -z "${instance}" ]; then >> + echo 0 > "${TRACE_DIR}/tracing_on" >> + else >> + mkdir -p "${TRACE_DIR}/instances/${instance}" > > Er, what's the logic here? Ensure that the instance exist so that we > can disable it? Oh, right, we should exit directly... This part is copies from _start_trace() obviously... Thanks, Qu > > --D > >> + echo 0 > "${TRACE_DIR}/instances/${instance}/tracing_on" >> + fi >> +} >> + >> +_remove_empty_trace() >> +{ >> + file="$1" >> + if [ ! -f "$file" ]; then >> + return >> + fi >> + >> + if [ -z "$(head -n 15 $file | sed '/^#/d')" ]; then >> + rm $file >> + fi >> +} >> + >> +_copy_trace() >> +{ >> + dest="$1" >> + instance="$2" >> + >> + if [ ! -d "${TRACE_DIR}" ]; then >> + return >> + fi >> + >> + if [ -z "${instance}" ]; then >> + cp "${TRACE_DIR}/trace" "$dest" >> + elif [ -d "${TRACE_DIR}/instances/${instance}" ]; then >> + cp "${TRACE_DIR}/instances/${instance}/trace" "$dest" >> + fi >> + >> + _remove_empty_trace "$dest" >> +} >> diff --git a/common/rc b/common/rc >> index 919028ef..f9de1517 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -3,6 +3,7 @@ >> # Copyright (c) 2000-2006 Silicon Graphics, Inc. All Rights Reserved. >> >> . common/config >> +. common/ftrace >> >> BC=$(which bc 2> /dev/null) || BC= >> >> -- >> 2.31.1 >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fstests: add basic ftrace support 2021-06-01 23:45 ` Qu Wenruo @ 2021-06-03 16:26 ` Darrick J. Wong 2021-06-03 23:22 ` Qu Wenruo 0 siblings, 1 reply; 5+ messages in thread From: Darrick J. Wong @ 2021-06-03 16:26 UTC (permalink / raw) To: Qu Wenruo; +Cc: fstests On Wed, Jun 02, 2021 at 07:45:22AM +0800, Qu Wenruo wrote: > > > On 2021/6/2 上午12:42, Darrick J. Wong wrote: > > On Tue, Jun 01, 2021 at 03:31:33PM +0800, Qu Wenruo wrote: > > > Sometimes developers want trace dump for certain test cases. > > > > > > Normally I just add "trace-cmd" calls in "check", but it would be much > > > > Heh, so do I! > > > > > better to let fstests to support ftrace dumping. > > > > > > This patchset will add basic ftrace dumping support by: > > > > > > - Clear all buffers before running each test > > > - Start tracing before running each test > > > - End tracing after test finished > > > - Copy the trace to "$seqres.trace" if needed > > > The condition is either: > > > * $KEEP_TRACE environment is set to "yes" > > > * The test case failed > > > > > > Currently we only support the main ftrace buffer, but all supporting > > > functions have support for ftrace instances, for later expansion. > > > > I... did not know one could /have/ separate instances. > > So did I, until I checked the code of "trace-cmd clear". > > > > > > Signed-off-by: Qu Wenruo <wqu@suse.com> > > > --- > > > check | 12 +++++++- > > > common/ftrace | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > > common/rc | 1 + > > > 3 files changed, 94 insertions(+), 1 deletion(-) > > > create mode 100644 common/ftrace > > > > > > diff --git a/check b/check > > > index ba192042..0a09dcf9 100755 > > > --- a/check > > > +++ b/check > > > @@ -801,7 +801,7 @@ function run_section() > > > fi > > > # really going to try and run this one > > > - rm -f $seqres.out.bad > > > + rm -f $seqres.out.bad $seqres.trace > > > # check if we really should run it > > > _expunge_test $seqnum > > > @@ -839,6 +839,10 @@ function run_section() > > > # to be reported for each test > > > (echo 1 > $DEBUGFS_MNT/clear_warn_once) > /dev/null 2>&1 > > > + # Clear previous trace and start new trace > > > + _clear_trace_buffers > > > + _start_trace > > > > How do you actually turn on specific tracepoints? > > Normally I use trace_printk(), thus no need to enable it manually. > > > > > Or is the idea here to capture trace data for each test in a separate > > file, and it's up to the ./check caller to set that up? > > My idea is, it should be the QA guy to set it up. > > Either using my stupid trace_printk() way, or enable those events before > running the test. > > > > > > + > > > if [ "$DUMP_OUTPUT" = true ]; then > > > _run_seq 2>&1 | tee $tmp.out > > > # Because $? would get tee's return code > > > @@ -848,6 +852,11 @@ function run_section() > > > sts=$? > > > fi > > > + _end_trace > > > + if [ "$KEEP_TRACE" == "yes" ]; then > > > + _copy_trace "$seqres.trace" > > > + fi > > > + > > > if [ -f core ]; then > > > _dump_err_cont "[dumped core]" > > > mv core $RESULT_BASE/$seqnum.core > > > @@ -932,6 +941,7 @@ function run_section() > > > # make sure we record the status of the last test we ran. > > > if $err ; then > > > + _copy_trace "$seqres.trace" > > > bad="$bad $seqnum" > > > n_bad=`expr $n_bad + 1` > > > tc_status="fail" > > > diff --git a/common/ftrace b/common/ftrace > > > new file mode 100644 > > > index 00000000..36886484 > > > --- /dev/null > > > +++ b/common/ftrace > > > @@ -0,0 +1,82 @@ > > > +# > > > +# Common ftrace related functions > > > +# > > > > New file needs a SPDX header. > > Oh, forgot that. > > > > > > + > > > +TRACE_DIR="/sys/kernel/debug/tracing" > > > + > > > +_clear_trace_buffers() > > > +{ > > > + if [ ! -d "${TRACE_DIR}" ]; then > > > + return > > > + fi > > > + > > > + # Clear the main buffer > > > + echo 0 > "${TRACE_DIR}/trace" > > > > If one were already running trace-cmd record, will this mess up its > > ability to collect trace data? > > I guess it may. > > But that shouldn't be a problem, all existing buffers are before running the > tests, thus should be considered as noise. Yes, that's true for the first test run, but this clears the trace buffers between every test. If you start trace-cmd and run two tests, isn't it possible that trace buffer content will be lost if trace-cmd hasn't emptied the trace buffer when _clear_trace_buffers runs? Alternate suggestion: Now that we know about the trace instance functionality, what if the test runner passed a list of tracepoints to ./check? Then ./check can set up a new tracing instance for itself with the specified tracepoints, and we can clear the buffer before each test and arrange to dump the contents to the trace file while the test runs. That will have the added benefit of not messing with any other tracing tools that might be running. --D > > > > > > + > > > + # Clear each instance buffer > > > + for i in $(ls "${TRACE_DIR}/instances"); do > > > + echo 0 > "${i}/trace" > > > + done > > > +} > > > + > > > +_start_trace() > > > +{ > > > + instance=$1 > > > > local instance="$1", please don't pollute the caller's environment. > > Oh, forgot it again... > > > > > > + > > > + if [ ! -d "${TRACE_DIR}" ]; then > > > + return > > > + fi > > > + > > > + if [ -z "${instance}" ]; then > > > + echo 1 > "${TRACE_DIR}/tracing_on" > > > + else > > > + mkdir -p "${TRACE_DIR}/instances/${instance}" > > > + echo 1 > "${TRACE_DIR}/instances/${instance}/tracing_on" > > > + fi > > > +} > > > + > > > +_end_trace() > > > +{ > > > + instance=$1 > > > + > > > + if [ ! -d "${TRACE_DIR}" ]; then > > > + return > > > + fi > > > + > > > + if [ -z "${instance}" ]; then > > > + echo 0 > "${TRACE_DIR}/tracing_on" > > > + else > > > + mkdir -p "${TRACE_DIR}/instances/${instance}" > > > > Er, what's the logic here? Ensure that the instance exist so that we > > can disable it? > > Oh, right, we should exit directly... > > This part is copies from _start_trace() obviously... > > Thanks, > Qu > > > > --D > > > > > + echo 0 > "${TRACE_DIR}/instances/${instance}/tracing_on" > > > + fi > > > +} > > > + > > > +_remove_empty_trace() > > > +{ > > > + file="$1" > > > + if [ ! -f "$file" ]; then > > > + return > > > + fi > > > + > > > + if [ -z "$(head -n 15 $file | sed '/^#/d')" ]; then > > > + rm $file > > > + fi > > > +} > > > + > > > +_copy_trace() > > > +{ > > > + dest="$1" > > > + instance="$2" > > > + > > > + if [ ! -d "${TRACE_DIR}" ]; then > > > + return > > > + fi > > > + > > > + if [ -z "${instance}" ]; then > > > + cp "${TRACE_DIR}/trace" "$dest" > > > + elif [ -d "${TRACE_DIR}/instances/${instance}" ]; then > > > + cp "${TRACE_DIR}/instances/${instance}/trace" "$dest" > > > + fi > > > + > > > + _remove_empty_trace "$dest" > > > +} > > > diff --git a/common/rc b/common/rc > > > index 919028ef..f9de1517 100644 > > > --- a/common/rc > > > +++ b/common/rc > > > @@ -3,6 +3,7 @@ > > > # Copyright (c) 2000-2006 Silicon Graphics, Inc. All Rights Reserved. > > > . common/config > > > +. common/ftrace > > > BC=$(which bc 2> /dev/null) || BC= > > > -- > > > 2.31.1 > > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fstests: add basic ftrace support 2021-06-03 16:26 ` Darrick J. Wong @ 2021-06-03 23:22 ` Qu Wenruo 0 siblings, 0 replies; 5+ messages in thread From: Qu Wenruo @ 2021-06-03 23:22 UTC (permalink / raw) To: Darrick J. Wong; +Cc: fstests On 2021/6/4 上午12:26, Darrick J. Wong wrote: > On Wed, Jun 02, 2021 at 07:45:22AM +0800, Qu Wenruo wrote: >> >> >> On 2021/6/2 上午12:42, Darrick J. Wong wrote: >>> On Tue, Jun 01, 2021 at 03:31:33PM +0800, Qu Wenruo wrote: >>>> Sometimes developers want trace dump for certain test cases. >>>> >>>> Normally I just add "trace-cmd" calls in "check", but it would be much >>> >>> Heh, so do I! >>> >>>> better to let fstests to support ftrace dumping. >>>> >>>> This patchset will add basic ftrace dumping support by: >>>> >>>> - Clear all buffers before running each test >>>> - Start tracing before running each test >>>> - End tracing after test finished >>>> - Copy the trace to "$seqres.trace" if needed >>>> The condition is either: >>>> * $KEEP_TRACE environment is set to "yes" >>>> * The test case failed >>>> >>>> Currently we only support the main ftrace buffer, but all supporting >>>> functions have support for ftrace instances, for later expansion. >>> >>> I... did not know one could /have/ separate instances. >> >> So did I, until I checked the code of "trace-cmd clear". >> >>> >>>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>>> --- >>>> check | 12 +++++++- >>>> common/ftrace | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> common/rc | 1 + >>>> 3 files changed, 94 insertions(+), 1 deletion(-) >>>> create mode 100644 common/ftrace >>>> >>>> diff --git a/check b/check >>>> index ba192042..0a09dcf9 100755 >>>> --- a/check >>>> +++ b/check >>>> @@ -801,7 +801,7 @@ function run_section() >>>> fi >>>> # really going to try and run this one >>>> - rm -f $seqres.out.bad >>>> + rm -f $seqres.out.bad $seqres.trace >>>> # check if we really should run it >>>> _expunge_test $seqnum >>>> @@ -839,6 +839,10 @@ function run_section() >>>> # to be reported for each test >>>> (echo 1 > $DEBUGFS_MNT/clear_warn_once) > /dev/null 2>&1 >>>> + # Clear previous trace and start new trace >>>> + _clear_trace_buffers >>>> + _start_trace >>> >>> How do you actually turn on specific tracepoints? >> >> Normally I use trace_printk(), thus no need to enable it manually. >> >>> >>> Or is the idea here to capture trace data for each test in a separate >>> file, and it's up to the ./check caller to set that up? >> >> My idea is, it should be the QA guy to set it up. >> >> Either using my stupid trace_printk() way, or enable those events before >> running the test. >> >>> >>>> + >>>> if [ "$DUMP_OUTPUT" = true ]; then >>>> _run_seq 2>&1 | tee $tmp.out >>>> # Because $? would get tee's return code >>>> @@ -848,6 +852,11 @@ function run_section() >>>> sts=$? >>>> fi >>>> + _end_trace >>>> + if [ "$KEEP_TRACE" == "yes" ]; then >>>> + _copy_trace "$seqres.trace" >>>> + fi >>>> + >>>> if [ -f core ]; then >>>> _dump_err_cont "[dumped core]" >>>> mv core $RESULT_BASE/$seqnum.core >>>> @@ -932,6 +941,7 @@ function run_section() >>>> # make sure we record the status of the last test we ran. >>>> if $err ; then >>>> + _copy_trace "$seqres.trace" >>>> bad="$bad $seqnum" >>>> n_bad=`expr $n_bad + 1` >>>> tc_status="fail" >>>> diff --git a/common/ftrace b/common/ftrace >>>> new file mode 100644 >>>> index 00000000..36886484 >>>> --- /dev/null >>>> +++ b/common/ftrace >>>> @@ -0,0 +1,82 @@ >>>> +# >>>> +# Common ftrace related functions >>>> +# >>> >>> New file needs a SPDX header. >> >> Oh, forgot that. >> >>> >>>> + >>>> +TRACE_DIR="/sys/kernel/debug/tracing" >>>> + >>>> +_clear_trace_buffers() >>>> +{ >>>> + if [ ! -d "${TRACE_DIR}" ]; then >>>> + return >>>> + fi >>>> + >>>> + # Clear the main buffer >>>> + echo 0 > "${TRACE_DIR}/trace" >>> >>> If one were already running trace-cmd record, will this mess up its >>> ability to collect trace data? >> >> I guess it may. >> >> But that shouldn't be a problem, all existing buffers are before running the >> tests, thus should be considered as noise. > > Yes, that's true for the first test run, but this clears the trace > buffers between every test. If you start trace-cmd and run two tests, > isn't it possible that trace buffer content will be lost if trace-cmd > hasn't emptied the trace buffer when _clear_trace_buffers runs? But in that case, we should have buffer saved for each test, no need for the manual trace-cmd to record. > > Alternate suggestion: Now that we know about the trace instance > functionality, what if the test runner passed a list of tracepoints to > ./check? Then ./check can set up a new tracing instance for itself with > the specified tracepoints, and we can clear the buffer before each test > and arrange to dump the contents to the trace file while the test runs. > That will have the added benefit of not messing with any other tracing > tools that might be running. Yeah, that's the planned feature for the future. But my current objective is just to remove the need for manual trace-cmd calls, so that with my trace_printk() added, I can test certain known reproducer without messing the fstests code. So far it works well for my use case. In the future, we may utilize certain instance, like 'fstests'. Thanks, Qu > > --D > >> >>> >>>> + >>>> + # Clear each instance buffer >>>> + for i in $(ls "${TRACE_DIR}/instances"); do >>>> + echo 0 > "${i}/trace" >>>> + done >>>> +} >>>> + >>>> +_start_trace() >>>> +{ >>>> + instance=$1 >>> >>> local instance="$1", please don't pollute the caller's environment. >> >> Oh, forgot it again... >> >>> >>>> + >>>> + if [ ! -d "${TRACE_DIR}" ]; then >>>> + return >>>> + fi >>>> + >>>> + if [ -z "${instance}" ]; then >>>> + echo 1 > "${TRACE_DIR}/tracing_on" >>>> + else >>>> + mkdir -p "${TRACE_DIR}/instances/${instance}" >>>> + echo 1 > "${TRACE_DIR}/instances/${instance}/tracing_on" >>>> + fi >>>> +} >>>> + >>>> +_end_trace() >>>> +{ >>>> + instance=$1 >>>> + >>>> + if [ ! -d "${TRACE_DIR}" ]; then >>>> + return >>>> + fi >>>> + >>>> + if [ -z "${instance}" ]; then >>>> + echo 0 > "${TRACE_DIR}/tracing_on" >>>> + else >>>> + mkdir -p "${TRACE_DIR}/instances/${instance}" >>> >>> Er, what's the logic here? Ensure that the instance exist so that we >>> can disable it? >> >> Oh, right, we should exit directly... >> >> This part is copies from _start_trace() obviously... >> >> Thanks, >> Qu >>> >>> --D >>> >>>> + echo 0 > "${TRACE_DIR}/instances/${instance}/tracing_on" >>>> + fi >>>> +} >>>> + >>>> +_remove_empty_trace() >>>> +{ >>>> + file="$1" >>>> + if [ ! -f "$file" ]; then >>>> + return >>>> + fi >>>> + >>>> + if [ -z "$(head -n 15 $file | sed '/^#/d')" ]; then >>>> + rm $file >>>> + fi >>>> +} >>>> + >>>> +_copy_trace() >>>> +{ >>>> + dest="$1" >>>> + instance="$2" >>>> + >>>> + if [ ! -d "${TRACE_DIR}" ]; then >>>> + return >>>> + fi >>>> + >>>> + if [ -z "${instance}" ]; then >>>> + cp "${TRACE_DIR}/trace" "$dest" >>>> + elif [ -d "${TRACE_DIR}/instances/${instance}" ]; then >>>> + cp "${TRACE_DIR}/instances/${instance}/trace" "$dest" >>>> + fi >>>> + >>>> + _remove_empty_trace "$dest" >>>> +} >>>> diff --git a/common/rc b/common/rc >>>> index 919028ef..f9de1517 100644 >>>> --- a/common/rc >>>> +++ b/common/rc >>>> @@ -3,6 +3,7 @@ >>>> # Copyright (c) 2000-2006 Silicon Graphics, Inc. All Rights Reserved. >>>> . common/config >>>> +. common/ftrace >>>> BC=$(which bc 2> /dev/null) || BC= >>>> -- >>>> 2.31.1 >>>> >>> >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-03 23:22 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-06-01 7:31 [PATCH] fstests: add basic ftrace support Qu Wenruo 2021-06-01 16:42 ` Darrick J. Wong 2021-06-01 23:45 ` Qu Wenruo 2021-06-03 16:26 ` Darrick J. Wong 2021-06-03 23:22 ` Qu Wenruo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox