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

[Chunyu Hu <chuhu@redhat.com>]

Hi Cyril,

Thank you for reading this. But this patch is depending on previous
patches, not a dependent one, it can't be applied cleanly without 
several patches. we can first work on the tst_random, which is totally
independent. And will fixes the issues you mentioned in this series.

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Chunyu Hu" <chuhu@redhat.com>
> Cc: ltp@lists.linux.it, liwan@redhat.com
> Sent: Wednesday, May 4, 2016 10:56:30 PM
> Subject: Re: [LTP] [PATCH V2 9/9] ftrace_stress: cleanup and use ltp API
> 
> 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).

Thanks, new knowledge to me. will do remove.

> ...
> 
> > 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.

OK. We just remove it in next version, we indeed didn't call it here.
 
> > +TRACING_PATH=/sys/kernel/debug/tracing/
> 
> Shouldn't this be propagated from the main script? What's the point of
> redefining it here?

Thanks, I think you are right. this is defined in ftrace_lib.sh and not 
needed to be redefined here. will remove in next version.

> >  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.

OK. will remove this.

> >  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.

I think using tst_sleep directly is good way. thanks.

> > @@ -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.

OK. Will fix this.

> > 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?

at the moment as this is still not in the test lib,so it is just marking
a pending state. If that generator can be included, we can remove this 
and use 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?

Here it is weird, I checked my repo and my email in ltp list, there is no char 'V',
I will have a deeper check.

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

-- 
Regards,
Chunyu Hu