[LTP] [PATCH V2 9/9] ftrace_stress: cleanup and use ltp API

Cyril Hrubis
2016-05-04T14:56:30Z

Hi!
> --- a/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_buffer_size_kb.sh
> +++ b/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_buffer_size_kb.sh
> @@ -26,20 +26,20 @@ if [ $step -eq 0 ]; then
> LOOP=50
> fi
>
> -for ((; ;))
> -{
> +while true; do
> new_size=1
> - for ((i = 0; i < $LOOP; i++))
> - {
> + i=0;

The semicolon after i=0 is useless (and in all i=0; lines in this
patch).

...

> diff --git a/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_trace_clock.sh b/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_trace_clock.sh
> index de6bbea..70135c1 100755
> --- a/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_trace_clock.sh
> +++ b/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_trace_clock.sh
> @@ -13,6 +13,9 @@
> # #
> ###############################################################################
>
> +. test.sh

No need to source test.sh for the tst_kvercmp which is a separate
binary.

> +TRACING_PATH=/sys/kernel/debug/tracing/

Shouldn't this be propagated from the main script? What's the point of
redefining it here?

> LOOP=400
>
> # In kernel which is older than 2.6.32, we set global clock
> @@ -24,23 +27,23 @@ else
> old_kernel=0
> fi
>
> -for ((; ;))
> -{
> - if [ $old_kernel -eq 1 ];
> - then
> - for ((i = 0; i < $LOOP; i++))
> - {
> +while true; do
> + i=0;
> + if [ $old_kernel -eq 1 ]; then
> + while [ $i -lt $LOOP ]; do
> echo 1 > "$TRACING_PATH"/options/global-clock
> echo 0 > "$TRACING_PATH"/options/global-clock
> - }
> + i=$((i + 1))
> + done
> else
> - for ((i = 0; i < $LOOP; i++))
> - {
> + while [ $i -lt $LOOP ]; do
> echo local > "$TRACING_PATH"/trace_clock
> echo global > "$TRACING_PATH"/trace_clock
> - }
> + i=$((i + 1))
> + done
> +
> fi
>
> sleep 1
> -}
> +done
>
> diff --git a/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_trace_pipe.sh b/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_trace_pipe.sh
> index 47d42bc..a24008a 100755
> --- a/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_trace_pipe.sh
> +++ b/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_trace_pipe.sh
> @@ -13,10 +13,11 @@
> # #
> ###############################################################################
>
> +. test.sh

You don't have to add test.sh for tst_sleep.

> ftrace_sleep()
> {
> - # usleep is not a standard command?
> - usleep 200000 2> /dev/null
> + tst_sleep 200000us
> if [ $? -ne 0 ]; then
> sleep 1
> fi

You should remove the fallback to sleep 1 if the usleep wasn't found.
Ideally the whole function ftrace_sleep should be removed and you should
just call tst_sleep from the main loop.

> @@ -33,10 +34,9 @@ trap kill_this_pid SIGUSR1
>
> LOOP=20
>
> -for ((; ;))
> -{
> - for ((i = 0; i < $LOOP; i++))
> - {
> +while true; do
> + i=0;
> + while [ $i -lt $LOOP ]; do
> cat "$TRACING_PATH"/trace_pipe > /dev/null &
>
> this_pid=$!
> @@ -45,8 +45,7 @@ for ((; ;))
> wait $this_pid
> this_pid=0
> ftrace_sleep
> - }
> -
> + i=$((i + 1))
> + done
> sleep 2
> -}
> -
> +done

And you should also fix the kill called by the full path in this script.
It should really be just kill not /bin/kill.

> diff --git a/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_trace_stat.sh b/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_trace_stat.sh
> index d7e6fd3..2691991 100755
> --- a/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_trace_stat.sh
> +++ b/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_trace_stat.sh
> @@ -16,6 +16,8 @@
> LOOP=200
>
> should_skip=0
> +rand_gen=tst_random

What's the point of assigning tst_random into a variable instead of
using it directly?

> +nr_cpus=`tst_ncpus`
>
> if [ ! -e "$TRACING_PATH"/function_profile_enabled ]; then
> should_skip=1
> @@ -28,18 +30,17 @@ if [ $? -eq 0 ]; then
> should_skip=1
> fi
>
> -for ((; ;))
> -{
> +while true; do
> if [ $should_skip -eq 1 ]; then
> sleep 2
> continue
> fi
> -
> - for ((i = 0; i < $LOOP; i++))
> - {
> - cat "$TRACING_PATH"/trace_stat/function0 > /dev/null 2>&1
> - }
> + cpu=$($rand_gen 0 $((nr_cpus - 1)))
> + i=0;
> + while [ $i -lt $LOOP ]; do
> + cat "$TRACING_PATH"/trace_stat/function${cpu} > /dev/null 2>&1
> + i=$((i + 1))
> + done
>
> sleep 1
> -}
> -
> +done

...

> diff --git a/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_tracing_max_latency.sh b/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_tracing_max_latency.sh
> index fbaceb8..6090a91 100755
> --- a/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_tracing_max_latency.sh
> +++ b/testcases/kernel/tracing/ftrace_test/ftrace_stress/ftrace_tracing_max_latency.sh
> @@ -15,12 +15,13 @@
>
> MAX_LATENCY=100000
>
> -for ((; ;))
> -{
> - for ((i = 0; i < $MAX_LATENCY; i += 400))
> - {
> +while true; do
> + i=0;
> + while [ $i -lt $MAX_LATENCY ]; do
> echo $i > "$TRACING_PATH"/tracing_max_latency
> - }
> + i=$((i + 400))
> + done
>
> sleep 1
> -}
> +
> +Vdone
^
Typo?

--
Cyril Hrubis
chrubis@suse.cz