* [PATCH v3 0/2] Fix issues observed in selftests/amd-pstate
@ 2023-10-03 5:10 Swapnil Sapkal
2023-10-03 5:10 ` [PATCH v3 1/2] selftests/amd-pstate: Fix broken paths to run workloads in amd-pstate-ut Swapnil Sapkal
2023-10-03 5:10 ` [PATCH v3 2/2] selftests/amd-pstate: Added option to provide perf binary path Swapnil Sapkal
0 siblings, 2 replies; 7+ messages in thread
From: Swapnil Sapkal @ 2023-10-03 5:10 UTC (permalink / raw)
To: ray.huang, shuah
Cc: sukrut.bellary, li.meng, gautham.shenoy, wyes.karny, Perry.Yuan,
Mario.Limonciello, linux-pm, linux-kernel, linux-kselftest,
Swapnil Sapkal
This series fixes issues observed with selftests/amd-pstate while
running performance comparison tests with different governors. First
patch changes relative paths with absolute path and also change it
with correct path wherever it is broken.
The second patch adds an option to provide perf binary path to
handle the case where distro perf does not work.
Changelog v2->v3:
* Split the patch into two patches
Swapnil Sapkal (2):
selftests/amd-pstate: Fix broken paths to run workloads in amd-pstate-ut
selftests/amd-pstate: Added option to provide perf binary path
.../x86/amd_pstate_tracer/amd_pstate_trace.py | 2 +-
.../testing/selftests/amd-pstate/gitsource.sh | 14 +++++++----
tools/testing/selftests/amd-pstate/run.sh | 23 +++++++++++++------
tools/testing/selftests/amd-pstate/tbench.sh | 4 ++--
4 files changed, 28 insertions(+), 15 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 1/2] selftests/amd-pstate: Fix broken paths to run workloads in amd-pstate-ut
2023-10-03 5:10 [PATCH v3 0/2] Fix issues observed in selftests/amd-pstate Swapnil Sapkal
@ 2023-10-03 5:10 ` Swapnil Sapkal
2023-10-06 19:05 ` Mario Limonciello
2023-10-03 5:10 ` [PATCH v3 2/2] selftests/amd-pstate: Added option to provide perf binary path Swapnil Sapkal
1 sibling, 1 reply; 7+ messages in thread
From: Swapnil Sapkal @ 2023-10-03 5:10 UTC (permalink / raw)
To: ray.huang, shuah
Cc: sukrut.bellary, li.meng, gautham.shenoy, wyes.karny, Perry.Yuan,
Mario.Limonciello, linux-pm, linux-kernel, linux-kselftest,
Swapnil Sapkal
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(-)
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 2/2] selftests/amd-pstate: Added option to provide perf binary path
2023-10-03 5:10 [PATCH v3 0/2] Fix issues observed in selftests/amd-pstate Swapnil Sapkal
2023-10-03 5:10 ` [PATCH v3 1/2] selftests/amd-pstate: Fix broken paths to run workloads in amd-pstate-ut Swapnil Sapkal
@ 2023-10-03 5:10 ` Swapnil Sapkal
2023-10-06 18:59 ` Mario Limonciello
1 sibling, 1 reply; 7+ messages in thread
From: Swapnil Sapkal @ 2023-10-03 5:10 UTC (permalink / raw)
To: ray.huang, shuah
Cc: sukrut.bellary, li.meng, gautham.shenoy, wyes.karny, Perry.Yuan,
Mario.Limonciello, linux-pm, linux-kernel, linux-kselftest,
Swapnil Sapkal
In selftests/amd-pstate, distro `perf` is used to capture `perf stat`
while running microbenchmarks. Distro `perf` is not working with
upstream kernel. Fixed this by providing an option to give the perf
binary path.
Signed-off-by: Swapnil Sapkal <swapnil.sapkal@amd.com>
---
tools/testing/selftests/amd-pstate/gitsource.sh | 2 +-
tools/testing/selftests/amd-pstate/run.sh | 14 ++++++++++----
tools/testing/selftests/amd-pstate/tbench.sh | 2 +-
3 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/amd-pstate/gitsource.sh b/tools/testing/selftests/amd-pstate/gitsource.sh
index d0ad2ed5ba9d..5acc065e9e3e 100755
--- a/tools/testing/selftests/amd-pstate/gitsource.sh
+++ b/tools/testing/selftests/amd-pstate/gitsource.sh
@@ -87,7 +87,7 @@ run_gitsource()
printf "Make and test gitsource for $1 #$2 make_cpus: $MAKE_CPUS\n"
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
+ $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`
diff --git a/tools/testing/selftests/amd-pstate/run.sh b/tools/testing/selftests/amd-pstate/run.sh
index 279d073c5728..b87cdc5bfe4a 100755
--- a/tools/testing/selftests/amd-pstate/run.sh
+++ b/tools/testing/selftests/amd-pstate/run.sh
@@ -25,6 +25,7 @@ OUTFILE=selftest
OUTFILE_TBENCH="$OUTFILE.tbench"
OUTFILE_GIT="$OUTFILE.gitsource"
+PERF=/usr/bin/perf
SYSFS=
CPUROOT=
CPUFREQROOT=
@@ -152,8 +153,9 @@ help()
gitsource: Gitsource testing.>]
[-t <tbench time limit>]
[-p <tbench process number>]
- [-l <loop times for tbench>]
+ [-l <loop times for tbench/gitsource>]
[-i <amd tracer interval>]
+ [-b <perf binary>]
[-m <comparative test: acpi-cpufreq>]
\n"
exit 2
@@ -161,7 +163,7 @@ help()
parse_arguments()
{
- while getopts ho:c:t:p:l:i:m: arg
+ while getopts ho:c:t:p:l:i:b:m: arg
do
case $arg in
h) # --help
@@ -192,6 +194,10 @@ parse_arguments()
TRACER_INTERVAL=$OPTARG
;;
+ b) # --perf-binary
+ PERF=`realpath $OPTARG`
+ ;;
+
m) # --comparative-test
COMPARATIVE_TEST=$OPTARG
;;
@@ -205,8 +211,8 @@ parse_arguments()
command_perf()
{
- if ! command -v perf > /dev/null; then
- echo $msg please install perf. >&2
+ if ! $PERF -v; then
+ echo $msg please install perf or provide perf binary path as argument >&2
exit $ksft_skip
fi
}
diff --git a/tools/testing/selftests/amd-pstate/tbench.sh b/tools/testing/selftests/amd-pstate/tbench.sh
index 4d2e8ce2da3b..2a98d9c9202e 100755
--- a/tools/testing/selftests/amd-pstate/tbench.sh
+++ b/tools/testing/selftests/amd-pstate/tbench.sh
@@ -68,7 +68,7 @@ run_tbench()
printf "Test tbench for $1 #$2 time_limit: $TIME_LIMIT procs_num: $PROCESS_NUM\n"
tbench_srv > /dev/null 2>&1 &
- perf stat -a --per-socket -I 1000 -e power/energy-pkg/ tbench -t $TIME_LIMIT $PROCESS_NUM > $OUTFILE_TBENCH-perf-$1-$2.log 2>&1
+ $PERF stat -a --per-socket -I 1000 -e power/energy-pkg/ tbench -t $TIME_LIMIT $PROCESS_NUM > $OUTFILE_TBENCH-perf-$1-$2.log 2>&1
pid=`pidof tbench_srv`
kill $pid
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] selftests/amd-pstate: Added option to provide perf binary path
2023-10-03 5:10 ` [PATCH v3 2/2] selftests/amd-pstate: Added option to provide perf binary path Swapnil Sapkal
@ 2023-10-06 18:59 ` Mario Limonciello
2023-10-12 5:04 ` Swapnil Sapkal
0 siblings, 1 reply; 7+ messages in thread
From: Mario Limonciello @ 2023-10-06 18:59 UTC (permalink / raw)
To: Swapnil Sapkal, ray.huang, shuah
Cc: sukrut.bellary, li.meng, gautham.shenoy, wyes.karny, Perry.Yuan,
linux-pm, linux-kernel, linux-kselftest
On 10/3/2023 00:10, Swapnil Sapkal wrote:
> In selftests/amd-pstate, distro `perf` is used to capture `perf stat`
> while running microbenchmarks. Distro `perf` is not working with
> upstream kernel. Fixed this by providing an option to give the perf
> binary path.
>
> Signed-off-by: Swapnil Sapkal <swapnil.sapkal@amd.com>
> ---
One small nit, otherwise:
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> tools/testing/selftests/amd-pstate/gitsource.sh | 2 +-
> tools/testing/selftests/amd-pstate/run.sh | 14 ++++++++++----
> tools/testing/selftests/amd-pstate/tbench.sh | 2 +-
> 3 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/amd-pstate/gitsource.sh b/tools/testing/selftests/amd-pstate/gitsource.sh
> index d0ad2ed5ba9d..5acc065e9e3e 100755
> --- a/tools/testing/selftests/amd-pstate/gitsource.sh
> +++ b/tools/testing/selftests/amd-pstate/gitsource.sh
> @@ -87,7 +87,7 @@ run_gitsource()
> printf "Make and test gitsource for $1 #$2 make_cpus: $MAKE_CPUS\n"
> 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
> + $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`
> diff --git a/tools/testing/selftests/amd-pstate/run.sh b/tools/testing/selftests/amd-pstate/run.sh
> index 279d073c5728..b87cdc5bfe4a 100755
> --- a/tools/testing/selftests/amd-pstate/run.sh
> +++ b/tools/testing/selftests/amd-pstate/run.sh
> @@ -25,6 +25,7 @@ OUTFILE=selftest
> OUTFILE_TBENCH="$OUTFILE.tbench"
> OUTFILE_GIT="$OUTFILE.gitsource"
>
> +PERF=/usr/bin/perf
> SYSFS=
> CPUROOT=
> CPUFREQROOT=
> @@ -152,8 +153,9 @@ help()
> gitsource: Gitsource testing.>]
> [-t <tbench time limit>]
> [-p <tbench process number>]
> - [-l <loop times for tbench>]
> + [-l <loop times for tbench/gitsource>]
This looks like unrelated change.
> [-i <amd tracer interval>]
> + [-b <perf binary>]
> [-m <comparative test: acpi-cpufreq>]
> \n"
> exit 2
> @@ -161,7 +163,7 @@ help()
>
> parse_arguments()
> {
> - while getopts ho:c:t:p:l:i:m: arg
> + while getopts ho:c:t:p:l:i:b:m: arg
> do
> case $arg in
> h) # --help
> @@ -192,6 +194,10 @@ parse_arguments()
> TRACER_INTERVAL=$OPTARG
> ;;
>
> + b) # --perf-binary
> + PERF=`realpath $OPTARG`
> + ;;
> +
> m) # --comparative-test
> COMPARATIVE_TEST=$OPTARG
> ;;
> @@ -205,8 +211,8 @@ parse_arguments()
>
> command_perf()
> {
> - if ! command -v perf > /dev/null; then
> - echo $msg please install perf. >&2
> + if ! $PERF -v; then
> + echo $msg please install perf or provide perf binary path as argument >&2
> exit $ksft_skip
> fi
> }
> diff --git a/tools/testing/selftests/amd-pstate/tbench.sh b/tools/testing/selftests/amd-pstate/tbench.sh
> index 4d2e8ce2da3b..2a98d9c9202e 100755
> --- a/tools/testing/selftests/amd-pstate/tbench.sh
> +++ b/tools/testing/selftests/amd-pstate/tbench.sh
> @@ -68,7 +68,7 @@ run_tbench()
>
> printf "Test tbench for $1 #$2 time_limit: $TIME_LIMIT procs_num: $PROCESS_NUM\n"
> tbench_srv > /dev/null 2>&1 &
> - perf stat -a --per-socket -I 1000 -e power/energy-pkg/ tbench -t $TIME_LIMIT $PROCESS_NUM > $OUTFILE_TBENCH-perf-$1-$2.log 2>&1
> + $PERF stat -a --per-socket -I 1000 -e power/energy-pkg/ tbench -t $TIME_LIMIT $PROCESS_NUM > $OUTFILE_TBENCH-perf-$1-$2.log 2>&1
>
> pid=`pidof tbench_srv`
> kill $pid
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] selftests/amd-pstate: Fix broken paths to run workloads in amd-pstate-ut
2023-10-03 5:10 ` [PATCH v3 1/2] selftests/amd-pstate: Fix broken paths to run workloads in amd-pstate-ut Swapnil Sapkal
@ 2023-10-06 19:05 ` Mario Limonciello
2023-10-12 5:00 ` Swapnil Sapkal
0 siblings, 1 reply; 7+ messages in thread
From: Mario Limonciello @ 2023-10-06 19:05 UTC (permalink / raw)
To: Swapnil Sapkal, ray.huang, shuah
Cc: sukrut.bellary, li.meng, gautham.shenoy, wyes.karny, Perry.Yuan,
linux-pm, linux-kernel, linux-kselftest
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 &
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] selftests/amd-pstate: Fix broken paths to run workloads in amd-pstate-ut
2023-10-06 19:05 ` Mario Limonciello
@ 2023-10-12 5:00 ` Swapnil Sapkal
0 siblings, 0 replies; 7+ messages in thread
From: Swapnil Sapkal @ 2023-10-12 5:00 UTC (permalink / raw)
To: Mario Limonciello, ray.huang, shuah
Cc: sukrut.bellary, li.meng, gautham.shenoy, wyes.karny, Perry.Yuan,
linux-pm, linux-kernel, linux-kselftest
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] selftests/amd-pstate: Added option to provide perf binary path
2023-10-06 18:59 ` Mario Limonciello
@ 2023-10-12 5:04 ` Swapnil Sapkal
0 siblings, 0 replies; 7+ messages in thread
From: Swapnil Sapkal @ 2023-10-12 5:04 UTC (permalink / raw)
To: Mario Limonciello, ray.huang, shuah
Cc: sukrut.bellary, li.meng, gautham.shenoy, wyes.karny, Perry.Yuan,
linux-pm, linux-kernel, linux-kselftest
Hello Mario,
On 10/7/2023 12:29 AM, Mario Limonciello wrote:
> On 10/3/2023 00:10, Swapnil Sapkal wrote:
>> In selftests/amd-pstate, distro `perf` is used to capture `perf stat`
>> while running microbenchmarks. Distro `perf` is not working with
>> upstream kernel. Fixed this by providing an option to give the perf
>> binary path.
>>
>> Signed-off-by: Swapnil Sapkal <swapnil.sapkal@amd.com>
>> ---
>
> One small nit, otherwise:
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>
I will include the tag in next version.
>> tools/testing/selftests/amd-pstate/gitsource.sh | 2 +-
>> tools/testing/selftests/amd-pstate/run.sh | 14 ++++++++++----
>> tools/testing/selftests/amd-pstate/tbench.sh | 2 +-
>> 3 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/amd-pstate/gitsource.sh b/tools/testing/selftests/amd-pstate/gitsource.sh
>> index d0ad2ed5ba9d..5acc065e9e3e 100755
>> --- a/tools/testing/selftests/amd-pstate/gitsource.sh
>> +++ b/tools/testing/selftests/amd-pstate/gitsource.sh
>> @@ -87,7 +87,7 @@ run_gitsource()
>> printf "Make and test gitsource for $1 #$2 make_cpus: $MAKE_CPUS\n"
>> 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
>> + $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`
>> diff --git a/tools/testing/selftests/amd-pstate/run.sh b/tools/testing/selftests/amd-pstate/run.sh
>> index 279d073c5728..b87cdc5bfe4a 100755
>> --- a/tools/testing/selftests/amd-pstate/run.sh
>> +++ b/tools/testing/selftests/amd-pstate/run.sh
>> @@ -25,6 +25,7 @@ OUTFILE=selftest
>> OUTFILE_TBENCH="$OUTFILE.tbench"
>> OUTFILE_GIT="$OUTFILE.gitsource"
>> +PERF=/usr/bin/perf
>> SYSFS=
>> CPUROOT=
>> CPUFREQROOT=
>> @@ -152,8 +153,9 @@ help()
>> gitsource: Gitsource testing.>]
>> [-t <tbench time limit>]
>> [-p <tbench process number>]
>> - [-l <loop times for tbench>]
>> + [-l <loop times for tbench/gitsource>]
>
> This looks like unrelated change.
I will remove this change.
>
>> [-i <amd tracer interval>]
>> + [-b <perf binary>]
>> [-m <comparative test: acpi-cpufreq>]
>> \n"
>> exit 2
>> @@ -161,7 +163,7 @@ help()
>> parse_arguments()
>> {
>> - while getopts ho:c:t:p:l:i:m: arg
>> + while getopts ho:c:t:p:l:i:b:m: arg
>> do
>> case $arg in
>> h) # --help
>> @@ -192,6 +194,10 @@ parse_arguments()
>> TRACER_INTERVAL=$OPTARG
>> ;;
>> + b) # --perf-binary
>> + PERF=`realpath $OPTARG`
>> + ;;
>> +
>> m) # --comparative-test
>> COMPARATIVE_TEST=$OPTARG
>> ;;
>> @@ -205,8 +211,8 @@ parse_arguments()
>> command_perf()
>> {
>> - if ! command -v perf > /dev/null; then
>> - echo $msg please install perf. >&2
>> + if ! $PERF -v; then
>> + echo $msg please install perf or provide perf binary path as argument >&2
>> exit $ksft_skip
>> fi
>> }
>> diff --git a/tools/testing/selftests/amd-pstate/tbench.sh b/tools/testing/selftests/amd-pstate/tbench.sh
>> index 4d2e8ce2da3b..2a98d9c9202e 100755
>> --- a/tools/testing/selftests/amd-pstate/tbench.sh
>> +++ b/tools/testing/selftests/amd-pstate/tbench.sh
>> @@ -68,7 +68,7 @@ run_tbench()
>> printf "Test tbench for $1 #$2 time_limit: $TIME_LIMIT procs_num: $PROCESS_NUM\n"
>> tbench_srv > /dev/null 2>&1 &
>> - perf stat -a --per-socket -I 1000 -e power/energy-pkg/ tbench -t $TIME_LIMIT $PROCESS_NUM > $OUTFILE_TBENCH-perf-$1-$2.log 2>&1
>> + $PERF stat -a --per-socket -I 1000 -e power/energy-pkg/ tbench -t $TIME_LIMIT $PROCESS_NUM > $OUTFILE_TBENCH-perf-$1-$2.log 2>&1
>> pid=`pidof tbench_srv`
>> kill $pid
>
--
Thanks and Regards,
Swapnil
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-10-12 5:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-03 5:10 [PATCH v3 0/2] Fix issues observed in selftests/amd-pstate Swapnil Sapkal
2023-10-03 5:10 ` [PATCH v3 1/2] selftests/amd-pstate: Fix broken paths to run workloads in amd-pstate-ut Swapnil Sapkal
2023-10-06 19:05 ` Mario Limonciello
2023-10-12 5:00 ` Swapnil Sapkal
2023-10-03 5:10 ` [PATCH v3 2/2] selftests/amd-pstate: Added option to provide perf binary path Swapnil Sapkal
2023-10-06 18:59 ` Mario Limonciello
2023-10-12 5:04 ` Swapnil Sapkal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).