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

[Swapnil Sapkal]

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