[LTP] [PATCH V2 1/9] tracing: reorganize ftrace-stress tests to general tests

[Chunyu Hu]

Thanks. Some issue has been done in patch 2/9, will fix
other issues you mentioned in next step.

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Chunyu Hu" <chuhu@redhat.com>
> Cc: ltp@lists.linux.it, liwan@redhat.com
> Sent: Thursday, May 5, 2016 12:26:56 AM
> Subject: Re: [LTP] [PATCH V2 1/9] tracing: reorganize ftrace-stress tests to general tests
> 
> Hi!
> > diff --git a/testcases/kernel/tracing/ftrace_test/ftrace_lib.sh
> > b/testcases/kernel/tracing/ftrace_test/ftrace_lib.sh
> > new file mode 100755
> > index 0000000..ea082dc
> > --- /dev/null
> > +++ b/testcases/kernel/tracing/ftrace_test/ftrace_lib.sh
> > @@ -0,0 +1,156 @@
> > +#! /bin/sh
>      ^
>      The space should be removed

will fix this in patch 2/9 "skip unsupported tests and early cleanup". 

> > +###########################################################################
> > +##
> > ##
> > +## Copyright (c) 2010 FUJITSU LIMITED
> > ##
> > +##
> > ##
> > +## This program is free software: you can redistribute it and/or modify
> > ##
> > +## it under the terms of the GNU General Public License as published by
> > ##
> > +## the Free Software Foundation, either version 3 of the License, or
> > ##
> > +## (at your option) any later version.
> > ##
> > +##
> > ##
> > +## This program is distributed in the hope that it will be useful,
> > ##
> > +## but WITHOUT ANY WARRANTY; without even the implied warranty of
> > ##
> > +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > ##
> > +## GNU General Public License for more details.
> > ##
> > +##
> > ##
> > +## You should have received a copy of the GNU General Public License
> > ##
> > +## along with this program. If not, see <http://www.gnu.org/licenses/>.
> > ##
> > +##
> > ##
> > +## Author: Li Zefan <lizf@cn.fujitsu.com>
> > ##
> > +##
> > ##
> > +###########################################################################
> > +
> > +cd $LTPROOT/testcases/bin
> > +
> > +export TPATH="$PWD"
> > +export DEBUGFS_PATH="$PWD/debugfs"
> > +export TRACING_PATH="$PWD/debugfs/tracing"
> 
> The test must not create files and mount things outside of the test
> temporary directory. Expecially not in $LTPROOT. You should call
> tst_tmpdir to create temporary directory instead, then create directory
> there.

Thanks for the suggestion, will take this way in next version, also
add it into patch 2/9.

> Also the ftrace filesystem seems to be mounted on
> /sys/kernel/debug/tracing/ on recent distributions, why don't we just
> set TRACING_PATH accodingly if it's already mounted and fallback to
> mounting it only when debugfs couldn't be found in /proc/mounts?

Thanks. Will take this way in next version, we check whether the mounting is
needed, through /proc/mounts , and then only mount/umount 
if it's not mounted by default.

> > +export FPATH="$TPATH/ftrace_function"
> > +export RPATH="$TPATH/ftrace_regression"
> > +export SPATH="$TPATH/ftrace_stress"
> > +
> > +. test.sh
> > +
> > +test_interval=$1
> > +
> > +save_old_setting()
> > +{
> > +	cd $TRACING_PATH
> > +
> > +	old_trace_options=( `cat trace_options` )
> > +	old_tracing_on=`cat tracing_on`
> > +	old_tracing_enabled=`cat tracing_enabled`
> > +	old_buffer_size=`cat buffer_size_kb`
> > +
> > +	if [ -e stack_max_size ]; then
> > +		old_stack_tracer_enabled=`cat /proc/sys/kernel/stack_tracer_enabled`
> > +	fi
> > +
> > +	if [ -e "/proc/sys/kernel/ftrace_enabled" ]; then
> > +		old_ftrace_enabled=`cat /proc/sys/kernel/ftrace_enabled`
> > +	fi
> > +
> > +	if [ -e "function_profile_enabled" ]; then
> > +		old_profile_enabled=`cat function_profile_enabled`
> > +	fi
> > +
> > +	cd - > /dev/null
> > +}
> > +
> > +
> > +restore_old_setting()
> > +{
> > +	cd $TRACING_PATH
> > +
> > +	echo nop > current_tracer
> > +	echo 0 > events/enable
> > +	echo 0 > tracing_max_latency 2> /dev/null
> > +
> > +	if [ -e trace_clock ]; then
> > +		echo local > trace_clock
> > +	fi
> > +
> > +	if [ -e "function_pofile_enabled" ]; then
> > +		echo $old_profile_enabled > function_profile_enabled
> > +	fi
> > +
> > +	if [ -e "/proc/sys/kernel/ftrace_enabled" ]; then
> > +		echo $old_ftrace_enabled > /proc/sys/kernel/ftrace_enabled
> > +	fi
> > +
> > +	if [ -e stack_max_size ]; then
> > +		echo $old_stack_tracer_enabled > /proc/sys/kernel/stack_tracer_enabled
> > +		echo 0 > stack_max_size
> > +	fi
> > +
> > +	echo $old_buffer_size > buffer_size_kb
> > +	echo $old_tracing_on > tracing_on
> > +	echo $old_tracing_enabled > tracing_enabled
> > +
> > +	for option in $old_trace_options
> > +	do
> > +		echo $option > trace_options 2> /dev/null
> > +	done
> > +
> > +	echo > trace
> > +
> > +	cd - > /dev/null
> > +}
> > +
> > +clean_up()
> > +{
> > +	restore_old_setting
> > +
> > +	umount $DEBUGFS_PATH
> > +	rmdir $DEBUGFS_PATH
> > +}
> > +
> > +clean_up_exit()
> > +{
> > +	clean_up
> > +	exit 1
> > +}
> > +
> > +test_begin()
> > +{
> > +	start_time=`date +%s`
> > +}
> > +
> > +test_wait()
> > +{
> > +	for ((; ;))
> > +	{
> > +		sleep 2
> > +
> > +		cur_time=`date +%s`
> > +		elapsed=$(( $cur_time - $start_time ))
> > +
> > +		# run the test for $test_interval secs
> > +		if [ $elapsed -ge $test_interval ]; then
> > +			break
> > +		fi
> > +	}
> > +}
> 
> Why don't we just do sleep $test_interval instead?

Good idea, will leave only a tst_sleep in the func in 
next step.

> > +trap clean_up_exit INT
> > +
> > +tst_require_root
> > +
> > +# Don't run the test on kernels older than 2.6.34, otherwise
> > +# it can crash the system if the kernel is not latest-stable
> > +tst_kvercmp 2 6 34
> > +if [ $? -eq 0 ]; then
> > +	tst_brkm TCONF ignored "The test should be run in kernels >= 2.6.34. Skip
> > the test..."
> > +	exit 0
> > +fi
> 
> Don't do this. Really the whole point of LTP is to validate kernel, if
> we skip known bugs by default we would give users false impression that
> the kernel was thoroughly tested.

OK, will remove the ignore, and remove this skip step, I agree that
this skip is not needed. If user found panic,  should fix the 
issue asap. and skip steps should be done in local repo if needed.

> Also since you include the test.sh here the tst_brkm format is different
> and it exits the test, so the exit 0 is never reached.

OK. Will remove this. 

> > +mkdir $DEBUGFS_PATH
> > +mount -t debugfs xxx $DEBUGFS_PATH
> > +
> > +# Check to see tracing feature is supported or not
> > +if [ ! -d $TRACING_PATH ]; then
> > +	tst_brkm TCONF ignored "Tracing is not supported. Skip the test..."
> > +	umount $DEBUGFS_PATH
> > +	rmdir $DEBUGFS_PATH
> > +	exit 0
> > +fi
> 
> Here as well, the tst_brkm from test.sh exit the test. Ideally you
> should setup TST_CLEANUP to point to a cleanup function once you do the
> test initalization which would be then executed before the test exits
> automatically.

OK. I think we can add a function for test initiating. including check/mount
debugfs, exporting the env like TRACING_PATH,  and setting up the cleanup
callback TST_CLEANUP=cleanup

> ...
> 
> 
> > diff --git a/testcases/kernel/tracing/ftrace_test/ftrace_stress_test.sh
> > b/testcases/kernel/tracing/ftrace_test/ftrace_stress_test.sh
> > new file mode 100755
> > index 0000000..d9f7f8b
> > --- /dev/null
> > +++ b/testcases/kernel/tracing/ftrace_test/ftrace_stress_test.sh
> > @@ -0,0 +1,123 @@
> > +#! /bin/sh
> > +
> > +###########################################################################
> > +##
> > ##
> > +## Copyright (c) 2010 FUJITSU LIMITED
> > ##
> > +##
> > ##
> > +## This program is free software: you can redistribute it and/or modify
> > ##
> > +## it under the terms of the GNU General Public License as published by
> > ##
> > +## the Free Software Foundation, either version 3 of the License, or
> > ##
> > +## (at your option) any later version.
> > ##
> > +##
> > ##
> > +## This program is distributed in the hope that it will be useful,
> > ##
> > +## but WITHOUT ANY WARRANTY; without even the implied warranty of
> > ##
> > +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > ##
> > +## GNU General Public License for more details.
> > ##
> > +##
> > ##
> > +## You should have received a copy of the GNU General Public License
> > ##
> > +## along with this program. If not, see <http://www.gnu.org/licenses/>.
> > ##
> > +##
> > ##
> > +## Author: Li Zefan <lizf@cn.fujitsu.com>
> > ##
> > +##
> > ##
> > +###########################################################################
> > +
> > +
> > +export TCID="ftrace-stress-test"
> > +export TST_TOTAL=1
> > +export TST_COUNT=1
> > +
> > +. ftrace_lib.sh
> > +
> > +test_success=true
> > +
> > +export_pids()
> > +{
> > +	export pid1 pid2 pid3 pid4 pid5 pid6 pid7 pid8 pid9 pid10 pid11 pid12 \
> > +		pid13 pid14 pid15 pid16
> > +
> > +	export NR_PIDS=16
> > +}
> > +
> > +test_stress()
> > +{
> > +	export_pids
> > +
> > +	$SPATH/ftrace_trace_clock.sh &
> > +	pid1=$!
> > +	$SPATH/ftrace_current_tracer.sh &
> > +	pid2=$!
> > +	$SPATH/ftrace_trace_options.sh &
> > +	pid3=$!
> > +	$SPATH/ftrace_tracing_max_latency.sh &
> > +	pid4=$!
> > +	$SPATH/ftrace_stack_trace.sh &
> > +	pid5=$!
> > +	$SPATH/ftrace_stack_max_size.sh &
> > +	pid6=$!
> > +	$SPATH/ftrace_tracing_on.sh &
> > +	pid7=$!
> > +	$SPATH/ftrace_tracing_enabled.sh &
> > +	pid8=$!
> > +	$SPATH/ftrace_set_event.sh &
> > +	pid9=$!
> > +	$SPATH/ftrace_buffer_size.sh &
> > +	pid10=$!
> > +	$SPATH/ftrace_trace.sh &
> > +	pid11=$!
> > +	$SPATH/ftrace_trace_pipe.sh &
> > +	pid12=$!
> > +	$SPATH/ftrace_ftrace_enabled.sh &
> > +	pid13=$!
> > +	$SPATH/ftrace_set_ftrace_pid.sh &
> > +	pid14=$!
> > +	$SPATH/ftrace_profile_enabled.sh &
> > +	pid15=$!
> > +	$SPATH/ftrace_trace_stat.sh &
> > +	pid16=$!
> > +}
> > +
> > +test_kill()
> > +{
> > +	kill -KILL $pid1 || test_success=false
> > +	kill -KILL $pid2 || test_success=false
> > +	kill -KILL $pid3 || test_success=false
> > +	kill -KILL $pid4 || test_success=false
> > +	kill -KILL $pid5 || test_success=false
> > +	kill -KILL $pid6 || test_success=false
> > +	kill -KILL $pid7 || test_success=false
> > +	kill -KILL $pid8 || test_success=false
> > +	kill -KILL $pid9 || test_success=false
> > +	kill -KILL $pid10 || test_success=false
> > +	kill -KILL $pid11 || test_success=false
> > +	kill -USR1 $pid12 || test_success=false
> > +	kill -KILL $pid13 || test_success=false
> > +	kill -KILL $pid14 || test_success=false
> > +	kill -KILL $pid15 || test_success=false
> > +	kill -KILL $pid16 || test_success=false
> > +
> > +	sleep 2
> 
> I remember telling you to call wait on each pid instead of the sleep 2
> here.

The sleep 2 wil be removed in next step into patch 2/9.

> > +	clean_up
> > +}
> > +
> > +
> > +# ----------------------------
> > +echo "Ftrace Stress Test Begin"
> 
> That should be tst_resm TINFO "..." but that is very minor.

This has been fixed in patch 2/9

> > +save_old_setting
> > +
> > +test_begin
> > +
> > +test_stress
> > +
> > +test_wait
> > +
> > +test_kill
> 
> Hmm, shouldn't you restore old settings here?
> 
> > +echo "Ftrace Stress Test End"
> 
> Here as well.

Fixed in patch 2/9.

> > +if $test_success; then
> > +	tst_resm TPASS "finished running the test. Run dmesg to double-check for
> > bugs"
> > +else
> > +	tst_resm TFAIL "please check log message."
> > +	exit 1
> > +fi
> 
> You should call tst_exit at the end of the test instead of doing exit 1
> after the tst_resm. This is another leftover caused by the
> broken-by-desing tst_resm and tst_brkm binaries.

This has been done in 2/9. thank you for the detailed info.

> --
> Cyril Hrubis
> chrubis@suse.cz
> 

-- 
Regards,
Chunyu Hu