| Message ID | 20231003051006.6343-2-swapnil.sapkal@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix issues observed in selftests/amd-pstate | expand |
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 &
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 --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 &
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(-)