diff mbox series

[v3,1/2] selftests/amd-pstate: Fix broken paths to run workloads in amd-pstate-ut

Message ID 20231003051006.6343-2-swapnil.sapkal@amd.com
State New
Headers show
Series [v3,1/2] selftests/amd-pstate: Fix broken paths to run workloads in amd-pstate-ut | expand

Commit Message

Swapnil Sapkal Oct. 3, 2023, 5:10 a.m. UTC
In selftests/amd-pstate, tbench and gitsource microbenchmarks are
used to compare the performance with different governors. In Current
implementation relative path to run `amd_pstate_tracer.py` are broken.
Fixed this by using absolute paths.

Signed-off-by: Swapnil Sapkal <swapnil.sapkal@amd.com>
---
 .../x86/amd_pstate_tracer/amd_pstate_trace.py      |  2 +-
 tools/testing/selftests/amd-pstate/gitsource.sh    | 14 +++++++++-----
 tools/testing/selftests/amd-pstate/run.sh          |  9 ++++++---
 tools/testing/selftests/amd-pstate/tbench.sh       |  2 +-
 4 files changed, 17 insertions(+), 10 deletions(-)

Comments

Mario Limonciello Oct. 6, 2023, 7:05 p.m. UTC | #1
On 10/3/2023 00:10, Swapnil Sapkal wrote:
> In selftests/amd-pstate, tbench and gitsource microbenchmarks are
> used to compare the performance with different governors. In Current

s/Current/current/

> implementation relative path to run `amd_pstate_tracer.py` are broken.

The plurality of this sentence needs some work.  I suggest:

s,relative,the relative,
s,are broken,is broken,

> Fixed this by using absolute paths.

The tense is wrong.

s,Fixed,Fix/,

> 
> Signed-off-by: Swapnil Sapkal <swapnil.sapkal@amd.com>
> ---
>   .../x86/amd_pstate_tracer/amd_pstate_trace.py      |  2 +-
>   tools/testing/selftests/amd-pstate/gitsource.sh    | 14 +++++++++-----
>   tools/testing/selftests/amd-pstate/run.sh          |  9 ++++++---
>   tools/testing/selftests/amd-pstate/tbench.sh       |  2 +-
>   4 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/power/x86/amd_pstate_tracer/amd_pstate_trace.py b/tools/power/x86/amd_pstate_tracer/amd_pstate_trace.py
> index 904df0ea0a1e..2448bb07973f 100755
> --- a/tools/power/x86/amd_pstate_tracer/amd_pstate_trace.py
> +++ b/tools/power/x86/amd_pstate_tracer/amd_pstate_trace.py
> @@ -30,7 +30,7 @@ import getopt
>   import Gnuplot
>   from numpy import *
>   from decimal import *
> -sys.path.append('../intel_pstate_tracer')
> +sys.path.append(os.path.join(os.path.dirname(__file__), '../intel_pstate_tracer'))

If you're using os.path.join, shouldn't you not be hardcoding a "/" in 
there?

IE it should be:

sys.path.append(os.path.join(os.path.dirname(__file__), "..", 
"intel_pstate_tracer"))

>   #import intel_pstate_tracer

I think another patch should remove this commented line, it conveys zero 
information.

>   import intel_pstate_tracer as ipt
>   
> diff --git a/tools/testing/selftests/amd-pstate/gitsource.sh b/tools/testing/selftests/amd-pstate/gitsource.sh
> index 5f2171f0116d..d0ad2ed5ba9d 100755
> --- a/tools/testing/selftests/amd-pstate/gitsource.sh
> +++ b/tools/testing/selftests/amd-pstate/gitsource.sh
> @@ -66,12 +66,15 @@ post_clear_gitsource()
>   
>   install_gitsource()
>   {
> -	if [ ! -d $git_name ]; then
> +	if [ ! -d $SCRIPTDIR/$git_name ]; then
> +		BACKUP_DIR=$(pwd)
> +		cd $SCRIPTDIR
>   		printf "Download gitsource, please wait a moment ...\n\n"
>   		wget -O $git_tar $gitsource_url > /dev/null 2>&1
>   
>   		printf "Tar gitsource ...\n\n"
>   		tar -xzf $git_tar
> +		cd $BACKUP_DIR

If you change to /bin/bash instead of /bin/sh you could use pushd/popd 
instead.  If your goal is to keep compatibility with things like 
/bin/dash then this makes ense.

>   	fi
>   }
>   
> @@ -79,12 +82,13 @@ install_gitsource()
>   run_gitsource()
>   {
>   	echo "Launching amd pstate tracer for $1 #$2 tracer_interval: $TRACER_INTERVAL"
> -	./amd_pstate_trace.py -n tracer-gitsource-$1-$2 -i $TRACER_INTERVAL > /dev/null 2>&1 &
> +	$TRACER -n tracer-gitsource-$1-$2 -i $TRACER_INTERVAL > /dev/null 2>&1 &
>   
>   	printf "Make and test gitsource for $1 #$2 make_cpus: $MAKE_CPUS\n"
> -	cd $git_name
> -	perf stat -a --per-socket -I 1000 -e power/energy-pkg/ /usr/bin/time -o ../$OUTFILE_GIT.time-gitsource-$1-$2.log make test -j$MAKE_CPUS > ../$OUTFILE_GIT-perf-$1-$2.log 2>&1
> -	cd ..
> +	BACKUP_DIR=$(pwd)
> +	cd $SCRIPTDIR/$git_name
> +	perf stat -a --per-socket -I 1000 -e power/energy-pkg/ /usr/bin/time -o $BACKUP_DIR/$OUTFILE_GIT.time-gitsource-$1-$2.log make test -j$MAKE_CPUS > $BACKUP_DIR/$OUTFILE_GIT-perf-$1-$2.log 2>&1
> +	cd $BACKUP_DIR

Similar pushd/popd comment could apply here.
>   
>   	for job in `jobs -p`
>   	do
> diff --git a/tools/testing/selftests/amd-pstate/run.sh b/tools/testing/selftests/amd-pstate/run.sh
> index de4d8e9c9565..279d073c5728 100755
> --- a/tools/testing/selftests/amd-pstate/run.sh
> +++ b/tools/testing/selftests/amd-pstate/run.sh
> @@ -8,9 +8,12 @@ else
>   	FILE_MAIN=DONE
>   fi
>   
> -source basic.sh
> -source tbench.sh
> -source gitsource.sh
> +SCRIPTDIR=`dirname "$0"`
> +TRACER=$SCRIPTDIR/../../../power/x86/amd_pstate_tracer/amd_pstate_trace.py
> +
> +source $SCRIPTDIR/basic.sh
> +source $SCRIPTDIR/tbench.sh
> +source $SCRIPTDIR/gitsource.sh
>   
>   # amd-pstate-ut only run on x86/x86_64 AMD systems.
>   ARCH=$(uname -m 2>/dev/null | sed -e 's/i.86/x86/' -e 's/x86_64/x86/')
> diff --git a/tools/testing/selftests/amd-pstate/tbench.sh b/tools/testing/selftests/amd-pstate/tbench.sh
> index 49c9850341f6..4d2e8ce2da3b 100755
> --- a/tools/testing/selftests/amd-pstate/tbench.sh
> +++ b/tools/testing/selftests/amd-pstate/tbench.sh
> @@ -64,7 +64,7 @@ post_clear_tbench()
>   run_tbench()
>   {
>   	echo "Launching amd pstate tracer for $1 #$2 tracer_interval: $TRACER_INTERVAL"
> -	./amd_pstate_trace.py -n tracer-tbench-$1-$2 -i $TRACER_INTERVAL > /dev/null 2>&1 &
> +	$TRACER -n tracer-tbench-$1-$2 -i $TRACER_INTERVAL > /dev/null 2>&1 &
>   
>   	printf "Test tbench for $1 #$2 time_limit: $TIME_LIMIT procs_num: $PROCESS_NUM\n"
>   	tbench_srv > /dev/null 2>&1 &
Swapnil Sapkal Oct. 12, 2023, 5 a.m. UTC | #2
Hello Mario,

Thanks for reviewing.

On 10/7/2023 12:35 AM, Mario Limonciello wrote:
> On 10/3/2023 00:10, Swapnil Sapkal wrote:
>> In selftests/amd-pstate, tbench and gitsource microbenchmarks are
>> used to compare the performance with different governors. In Current
> 
> s/Current/current/
> 
I will fix this.

>> implementation relative path to run `amd_pstate_tracer.py` are broken.
> 
> The plurality of this sentence needs some work.  I suggest:
> 
> s,relative,the relative,
> s,are broken,is broken,
> 
I will fix this.

>> Fixed this by using absolute paths.
> 
> The tense is wrong.
> 
> s,Fixed,Fix/,
>
I will fix this.
  
>>
>> Signed-off-by: Swapnil Sapkal <swapnil.sapkal@amd.com>
>> ---
>>   .../x86/amd_pstate_tracer/amd_pstate_trace.py      |  2 +-
>>   tools/testing/selftests/amd-pstate/gitsource.sh    | 14 +++++++++-----
>>   tools/testing/selftests/amd-pstate/run.sh          |  9 ++++++---
>>   tools/testing/selftests/amd-pstate/tbench.sh       |  2 +-
>>   4 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/power/x86/amd_pstate_tracer/amd_pstate_trace.py b/tools/power/x86/amd_pstate_tracer/amd_pstate_trace.py
>> index 904df0ea0a1e..2448bb07973f 100755
>> --- a/tools/power/x86/amd_pstate_tracer/amd_pstate_trace.py
>> +++ b/tools/power/x86/amd_pstate_tracer/amd_pstate_trace.py
>> @@ -30,7 +30,7 @@ import getopt
>>   import Gnuplot
>>   from numpy import *
>>   from decimal import *
>> -sys.path.append('../intel_pstate_tracer')
>> +sys.path.append(os.path.join(os.path.dirname(__file__), '../intel_pstate_tracer'))
> 
> If you're using os.path.join, shouldn't you not be hardcoding a "/" in there?
> 
> IE it should be:
> 
> sys.path.append(os.path.join(os.path.dirname(__file__), "..", "intel_pstate_tracer"))
> 
I will fix this in next version.

>>   #import intel_pstate_tracer
> 
> I think another patch should remove this commented line, it conveys zero information.
> 
I will remove this line.

>>   import intel_pstate_tracer as ipt
>> diff --git a/tools/testing/selftests/amd-pstate/gitsource.sh b/tools/testing/selftests/amd-pstate/gitsource.sh
>> index 5f2171f0116d..d0ad2ed5ba9d 100755
>> --- a/tools/testing/selftests/amd-pstate/gitsource.sh
>> +++ b/tools/testing/selftests/amd-pstate/gitsource.sh
>> @@ -66,12 +66,15 @@ post_clear_gitsource()
>>   install_gitsource()
>>   {
>> -    if [ ! -d $git_name ]; then
>> +    if [ ! -d $SCRIPTDIR/$git_name ]; then
>> +        BACKUP_DIR=$(pwd)
>> +        cd $SCRIPTDIR
>>           printf "Download gitsource, please wait a moment ...\n\n"
>>           wget -O $git_tar $gitsource_url > /dev/null 2>&1
>>           printf "Tar gitsource ...\n\n"
>>           tar -xzf $git_tar
>> +        cd $BACKUP_DIR
> 
> If you change to /bin/bash instead of /bin/sh you could use pushd/popd instead.  If your goal is to keep compatibility with things like /bin/dash then this makes ense.
> I will update this in next version.

>>       fi
>>   }
>> @@ -79,12 +82,13 @@ install_gitsource()
>>   run_gitsource()
>>   {
>>       echo "Launching amd pstate tracer for $1 #$2 tracer_interval: $TRACER_INTERVAL"
>> -    ./amd_pstate_trace.py -n tracer-gitsource-$1-$2 -i $TRACER_INTERVAL > /dev/null 2>&1 &
>> +    $TRACER -n tracer-gitsource-$1-$2 -i $TRACER_INTERVAL > /dev/null 2>&1 &
>>       printf "Make and test gitsource for $1 #$2 make_cpus: $MAKE_CPUS\n"
>> -    cd $git_name
>> -    perf stat -a --per-socket -I 1000 -e power/energy-pkg/ /usr/bin/time -o ../$OUTFILE_GIT.time-gitsource-$1-$2.log make test -j$MAKE_CPUS > ../$OUTFILE_GIT-perf-$1-$2.log 2>&1
>> -    cd ..
>> +    BACKUP_DIR=$(pwd)
>> +    cd $SCRIPTDIR/$git_name
>> +    perf stat -a --per-socket -I 1000 -e power/energy-pkg/ /usr/bin/time -o $BACKUP_DIR/$OUTFILE_GIT.time-gitsource-$1-$2.log make test -j$MAKE_CPUS > $BACKUP_DIR/$OUTFILE_GIT-perf-$1-$2.log 2>&1
>> +    cd $BACKUP_DIR
> 
> Similar pushd/popd comment could apply here.

I will update this in next version.

>>       for job in `jobs -p`
>>       do
>> diff --git a/tools/testing/selftests/amd-pstate/run.sh b/tools/testing/selftests/amd-pstate/run.sh
>> index de4d8e9c9565..279d073c5728 100755
>> --- a/tools/testing/selftests/amd-pstate/run.sh
>> +++ b/tools/testing/selftests/amd-pstate/run.sh
>> @@ -8,9 +8,12 @@ else
>>       FILE_MAIN=DONE
>>   fi
>> -source basic.sh
>> -source tbench.sh
>> -source gitsource.sh
>> +SCRIPTDIR=`dirname "$0"`
>> +TRACER=$SCRIPTDIR/../../../power/x86/amd_pstate_tracer/amd_pstate_trace.py
>> +
>> +source $SCRIPTDIR/basic.sh
>> +source $SCRIPTDIR/tbench.sh
>> +source $SCRIPTDIR/gitsource.sh
>>   # amd-pstate-ut only run on x86/x86_64 AMD systems.
>>   ARCH=$(uname -m 2>/dev/null | sed -e 's/i.86/x86/' -e 's/x86_64/x86/')
>> diff --git a/tools/testing/selftests/amd-pstate/tbench.sh b/tools/testing/selftests/amd-pstate/tbench.sh
>> index 49c9850341f6..4d2e8ce2da3b 100755
>> --- a/tools/testing/selftests/amd-pstate/tbench.sh
>> +++ b/tools/testing/selftests/amd-pstate/tbench.sh
>> @@ -64,7 +64,7 @@ post_clear_tbench()
>>   run_tbench()
>>   {
>>       echo "Launching amd pstate tracer for $1 #$2 tracer_interval: $TRACER_INTERVAL"
>> -    ./amd_pstate_trace.py -n tracer-tbench-$1-$2 -i $TRACER_INTERVAL > /dev/null 2>&1 &
>> +    $TRACER -n tracer-tbench-$1-$2 -i $TRACER_INTERVAL > /dev/null 2>&1 &
>>       printf "Test tbench for $1 #$2 time_limit: $TIME_LIMIT procs_num: $PROCESS_NUM\n"
>>       tbench_srv > /dev/null 2>&1 &
> 
--
Thanks and regards,
Swapnil
diff mbox series

Patch

diff --git a/tools/power/x86/amd_pstate_tracer/amd_pstate_trace.py b/tools/power/x86/amd_pstate_tracer/amd_pstate_trace.py
index 904df0ea0a1e..2448bb07973f 100755
--- a/tools/power/x86/amd_pstate_tracer/amd_pstate_trace.py
+++ b/tools/power/x86/amd_pstate_tracer/amd_pstate_trace.py
@@ -30,7 +30,7 @@  import getopt
 import Gnuplot
 from numpy import *
 from decimal import *
-sys.path.append('../intel_pstate_tracer')
+sys.path.append(os.path.join(os.path.dirname(__file__), '../intel_pstate_tracer'))
 #import intel_pstate_tracer
 import intel_pstate_tracer as ipt
 
diff --git a/tools/testing/selftests/amd-pstate/gitsource.sh b/tools/testing/selftests/amd-pstate/gitsource.sh
index 5f2171f0116d..d0ad2ed5ba9d 100755
--- a/tools/testing/selftests/amd-pstate/gitsource.sh
+++ b/tools/testing/selftests/amd-pstate/gitsource.sh
@@ -66,12 +66,15 @@  post_clear_gitsource()
 
 install_gitsource()
 {
-	if [ ! -d $git_name ]; then
+	if [ ! -d $SCRIPTDIR/$git_name ]; then
+		BACKUP_DIR=$(pwd)
+		cd $SCRIPTDIR
 		printf "Download gitsource, please wait a moment ...\n\n"
 		wget -O $git_tar $gitsource_url > /dev/null 2>&1
 
 		printf "Tar gitsource ...\n\n"
 		tar -xzf $git_tar
+		cd $BACKUP_DIR
 	fi
 }
 
@@ -79,12 +82,13 @@  install_gitsource()
 run_gitsource()
 {
 	echo "Launching amd pstate tracer for $1 #$2 tracer_interval: $TRACER_INTERVAL"
-	./amd_pstate_trace.py -n tracer-gitsource-$1-$2 -i $TRACER_INTERVAL > /dev/null 2>&1 &
+	$TRACER -n tracer-gitsource-$1-$2 -i $TRACER_INTERVAL > /dev/null 2>&1 &
 
 	printf "Make and test gitsource for $1 #$2 make_cpus: $MAKE_CPUS\n"
-	cd $git_name
-	perf stat -a --per-socket -I 1000 -e power/energy-pkg/ /usr/bin/time -o ../$OUTFILE_GIT.time-gitsource-$1-$2.log make test -j$MAKE_CPUS > ../$OUTFILE_GIT-perf-$1-$2.log 2>&1
-	cd ..
+	BACKUP_DIR=$(pwd)
+	cd $SCRIPTDIR/$git_name
+	perf stat -a --per-socket -I 1000 -e power/energy-pkg/ /usr/bin/time -o $BACKUP_DIR/$OUTFILE_GIT.time-gitsource-$1-$2.log make test -j$MAKE_CPUS > $BACKUP_DIR/$OUTFILE_GIT-perf-$1-$2.log 2>&1
+	cd $BACKUP_DIR
 
 	for job in `jobs -p`
 	do
diff --git a/tools/testing/selftests/amd-pstate/run.sh b/tools/testing/selftests/amd-pstate/run.sh
index de4d8e9c9565..279d073c5728 100755
--- a/tools/testing/selftests/amd-pstate/run.sh
+++ b/tools/testing/selftests/amd-pstate/run.sh
@@ -8,9 +8,12 @@  else
 	FILE_MAIN=DONE
 fi
 
-source basic.sh
-source tbench.sh
-source gitsource.sh
+SCRIPTDIR=`dirname "$0"`
+TRACER=$SCRIPTDIR/../../../power/x86/amd_pstate_tracer/amd_pstate_trace.py
+
+source $SCRIPTDIR/basic.sh
+source $SCRIPTDIR/tbench.sh
+source $SCRIPTDIR/gitsource.sh
 
 # amd-pstate-ut only run on x86/x86_64 AMD systems.
 ARCH=$(uname -m 2>/dev/null | sed -e 's/i.86/x86/' -e 's/x86_64/x86/')
diff --git a/tools/testing/selftests/amd-pstate/tbench.sh b/tools/testing/selftests/amd-pstate/tbench.sh
index 49c9850341f6..4d2e8ce2da3b 100755
--- a/tools/testing/selftests/amd-pstate/tbench.sh
+++ b/tools/testing/selftests/amd-pstate/tbench.sh
@@ -64,7 +64,7 @@  post_clear_tbench()
 run_tbench()
 {
 	echo "Launching amd pstate tracer for $1 #$2 tracer_interval: $TRACER_INTERVAL"
-	./amd_pstate_trace.py -n tracer-tbench-$1-$2 -i $TRACER_INTERVAL > /dev/null 2>&1 &
+	$TRACER -n tracer-tbench-$1-$2 -i $TRACER_INTERVAL > /dev/null 2>&1 &
 
 	printf "Test tbench for $1 #$2 time_limit: $TIME_LIMIT procs_num: $PROCESS_NUM\n"
 	tbench_srv > /dev/null 2>&1 &