diff mbox series

fstests: add basic ftrace support

Message ID 20210601073133.194598-1-wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series fstests: add basic ftrace support | expand

Commit Message

Qu Wenruo June 1, 2021, 7:31 a.m. UTC
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

Comments

Darrick J. Wong June 1, 2021, 4:42 p.m. UTC | #1
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
>
Qu Wenruo June 1, 2021, 11:45 p.m. UTC | #2
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
>>
>
Darrick J. Wong June 3, 2021, 4:26 p.m. UTC | #3
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
> > > 
> > 
>
Qu Wenruo June 3, 2021, 11:22 p.m. UTC | #4
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
>>>>
>>>
>>
>
diff mbox series

Patch

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=