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 <[email protected]>
---
.../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'))
#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 &
--
2.34.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 <[email protected]>
> ---
> .../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 &
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 <[email protected]>
>> ---
>> .../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