[PATCH 1/2] tools/perf/tests/shell: Update stat+csv_output.sh to include sanity check for topology

[PATCH 1/2] tools/perf/tests/shell: Update stat+csv_output.sh to include sanity check for topology

[Athira Rajeev]

Testcase stat+csv_output.sh fails in powerpc:

	84: perf stat CSV output linter: FAILED!

The testcase "stat+csv_output.sh" verifies perf stat
CSV output. The test covers aggregation modes like
per-socket, per-core, per-die, -A (no_aggr mode) along
with few other tests. It counts expected fields for
various commands. For example say -A (i.e, AGGR_NONE
mode), expects 7 fields in the output having "CPU" as
first field. Same way, for per-socket, it expects the
first field in result to point to socket id. The testcases
compares the result with expected count.

The values for socket, die, core and cpu are fetched
from topology directory:
/sys/devices/system/cpu/cpu*/topology.
For example, socket value is fetched from
"physical_package_id" file of topology directory.
(cpu__get_topology_int() in util/cpumap.c)

If a platform fails to fetch the topology information,
values will be set to -1. For example, incase of pSeries
platform of powerpc, value for  "physical_package_id" is
restricted and not exposed. So, -1 will be assigned.

Perf code has a checks for valid cpu id in "aggr_printout"
(stat-display.c), which displays the fields. So, in cases
where topology values not exposed, first field of the
output displaying will be empty. This cause the testcase
to fail, as it counts  number of fields in the output.

Incase of -A (AGGR_NONE mode,), testcase expects 7 fields
in the output, becos of -1 value obtained from topology
files for some, only 6 fields are printed. Hence a testcase
failure reported due to mismatch in number of fields in
the output.

Patch here adds a sanity check in the testcase for topology.
Check will help to skip the test if -1 value found.

Fixes: 7473ee56dbc9 ("perf test: Add checking for perf stat CSV output.")
Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Suggested-by: James Clark <james.clark@arm.com>
Suggested-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/stat+csv_output.sh | 43 ++++++++++++++++++++---
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/shell/stat+csv_output.sh b/tools/perf/tests/shell/stat+csv_output.sh
index eb5196f58190..b7f050aa6210 100755
--- a/tools/perf/tests/shell/stat+csv_output.sh
+++ b/tools/perf/tests/shell/stat+csv_output.sh
@@ -6,6 +6,8 @@
 
 set -e
 
+skip_test=0
+
 function commachecker()
 {
 	local -i cnt=0
@@ -156,14 +158,47 @@ check_per_socket()
 	echo "[Success]"
 }
 
+# The perf stat options for per-socket, per-core, per-die
+# and -A ( no_aggr mode ) uses the info fetched from this
+# directory: "/sys/devices/system/cpu/cpu*/topology". For
+# example, socket value is fetched from "physical_package_id"
+# file in topology directory.
+# Reference: cpu__get_topology_int in util/cpumap.c
+# If the platform doesn't expose topology information, values
+# will be set to -1. For example, incase of pSeries platform
+# of powerpc, value for  "physical_package_id" is restricted
+# and set to -1. Check here validates the socket-id read from
+# topology file before proceeding further
+
+FILE_LOC="/sys/devices/system/cpu/cpu*/topology/"
+FILE_NAME="physical_package_id"
+
+check_for_topology()
+{
+	if ! ParanoidAndNotRoot 0
+	then
+		socket_file=`ls $FILE_LOC/$FILE_NAME | head -n 1`
+		[ -z $socket_file ] && return 0
+		socket_id=`cat $socket_file`
+		[ $socket_id == -1 ] && skip_test=1
+		return 0
+	fi
+}
+
+check_for_topology
 check_no_args
 check_system_wide
-check_system_wide_no_aggr
 check_interval
 check_event
-check_per_core
 check_per_thread
-check_per_die
 check_per_node
-check_per_socket
+if [ $skip_test -ne 1 ]
+then
+	check_system_wide_no_aggr
+	check_per_core
+	check_per_die
+	check_per_socket
+else
+	echo "[Skip] Skipping tests for system_wide_no_aggr, per_core, per_die and per_socket since socket id exposed via topology is invalid"
+fi
 exit 0
-- 
2.31.1

[PATCH 2/2] tools/perf/tests/shell: Update stat+json_output.sh to include sanity check for topology

[Athira Rajeev]

Testcase stat+json_output.sh fails in powerpc:

	86: perf stat JSON output linter : FAILED!

The testcase "stat+json_output.sh" verifies perf stat
JSON output. The test covers aggregation modes like
per-socket, per-core, per-die, -A (no_aggr mode) along
with few other tests. It counts expected fields for
various commands. For example say -A (i.e, AGGR_NONE
mode), expects 7 fields in the output having "CPU" as
first field. Same way, for per-socket, it expects the
first field in result to point to socket id. The testcases
compares the result with expected count.

The values for socket, die, core and cpu are fetched
from topology directory:
/sys/devices/system/cpu/cpu*/topology.
For example, socket value is fetched from
"physical_package_id" file of topology directory.
(cpu__get_topology_int() in util/cpumap.c)

If a platform fails to fetch the topology information,
values will be set to -1. For example, incase of pSeries
platform of powerpc, value for  "physical_package_id" is
restricted and not exposed. So, -1 will be assigned.

Perf code has a checks for valid cpu id in "aggr_printout"
(stat-display.c), which displays the fields. So, in cases
where topology values not exposed, first field of the
output displaying will be empty. This cause the testcase
to fail, as it counts  number of fields in the output.

Incase of -A (AGGR_NONE mode,), testcase expects 7 fields
in the output, becos of -1 value obtained from topology
files for some, only 6 fields are printed. Hence a testcase
failure reported due to mismatch in number of fields in
the output.

Patch here adds a sanity check in the testcase for topology.
Check will help to skip the test if -1 value found.

Fixes: 0c343af2a2f8 ("perf test: JSON format checking")
Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Suggested-by: James Clark <james.clark@arm.com>
Suggested-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/stat+json_output.sh | 43 ++++++++++++++++++++--
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/shell/stat+json_output.sh b/tools/perf/tests/shell/stat+json_output.sh
index ea8714a36051..2c4212c641ed 100755
--- a/tools/perf/tests/shell/stat+json_output.sh
+++ b/tools/perf/tests/shell/stat+json_output.sh
@@ -6,6 +6,8 @@
 
 set -e
 
+skip_test=0
+
 pythonchecker=$(dirname $0)/lib/perf_json_output_lint.py
 if [ "x$PYTHON" == "x" ]
 then
@@ -134,14 +136,47 @@ check_per_socket()
 	echo "[Success]"
 }
 
+# The perf stat options for per-socket, per-core, per-die
+# and -A ( no_aggr mode ) uses the info fetched from this
+# directory: "/sys/devices/system/cpu/cpu*/topology". For
+# example, socket value is fetched from "physical_package_id"
+# file in topology directory.
+# Reference: cpu__get_topology_int in util/cpumap.c
+# If the platform doesn't expose topology information, values
+# will be set to -1. For example, incase of pSeries platform
+# of powerpc, value for  "physical_package_id" is restricted
+# and set to -1. Check here validates the socket-id read from
+# topology file before proceeding further
+
+FILE_LOC="/sys/devices/system/cpu/cpu*/topology/"
+FILE_NAME="physical_package_id"
+
+check_for_topology()
+{
+	if ! ParanoidAndNotRoot 0
+	then
+		socket_file=`ls $FILE_LOC/$FILE_NAME | head -n 1`
+		[ -z $socket_file ] && return 0
+		socket_id=`cat $socket_file`
+		[ $socket_id == -1 ] && skip_test=1
+		return 0
+	fi
+}
+
+check_for_topology
 check_no_args
 check_system_wide
-check_system_wide_no_aggr
 check_interval
 check_event
-check_per_core
 check_per_thread
-check_per_die
 check_per_node
-check_per_socket
+if [ $skip_test -ne 1 ]
+then
+	check_system_wide_no_aggr
+	check_per_core
+	check_per_die
+	check_per_socket
+else
+	echo "[Skip] Skipping tests for system_wide_no_aggr, per_core, per_die and per_socket since socket id exposed via topology is invalid"
+fi
 exit 0
-- 
2.31.1

Re: [PATCH 1/2] tools/perf/tests/shell: Update stat+csv_output.sh to include sanity check for topology

[Arnaldo Carvalho de Melo]

Em Thu, Oct 06, 2022 at 09:21:48PM +0530, Athira Rajeev escreveu:
> Testcase stat+csv_output.sh fails in powerpc:
> 
> 	84: perf stat CSV output linter: FAILED!

Thanks, applied both patches.

- Arnaldo

Re: [PATCH 1/2] tools/perf/tests/shell: Update stat+csv_output.sh to include sanity check for topology

[Disha Goel]

On 10/6/22 9:21 PM, Athira Rajeev wrote:
> Testcase stat+csv_output.sh fails in powerpc:
>
> 	84: perf stat CSV output linter: FAILED!
>
> The testcase "stat+csv_output.sh" verifies perf stat
> CSV output. The test covers aggregation modes like
> per-socket, per-core, per-die, -A (no_aggr mode) along
> with few other tests. It counts expected fields for
> various commands. For example say -A (i.e, AGGR_NONE
> mode), expects 7 fields in the output having "CPU" as
> first field. Same way, for per-socket, it expects the
> first field in result to point to socket id. The testcases
> compares the result with expected count.
>
> The values for socket, die, core and cpu are fetched
> from topology directory:
> /sys/devices/system/cpu/cpu*/topology.
> For example, socket value is fetched from
> "physical_package_id" file of topology directory.
> (cpu__get_topology_int() in util/cpumap.c)
>
> If a platform fails to fetch the topology information,
> values will be set to -1. For example, incase of pSeries
> platform of powerpc, value for  "physical_package_id" is
> restricted and not exposed. So, -1 will be assigned.
>
> Perf code has a checks for valid cpu id in "aggr_printout"
> (stat-display.c), which displays the fields. So, in cases
> where topology values not exposed, first field of the
> output displaying will be empty. This cause the testcase
> to fail, as it counts  number of fields in the output.
>
> Incase of -A (AGGR_NONE mode,), testcase expects 7 fields
> in the output, becos of -1 value obtained from topology
> files for some, only 6 fields are printed. Hence a testcase
> failure reported due to mismatch in number of fields in
> the output.
>
> Patch here adds a sanity check in the testcase for topology.
> Check will help to skip the test if -1 value found.
>
> Fixes: 7473ee56dbc9 ("perf test: Add checking for perf stat CSV output.")
> Reported-by: Disha Goel <disgoel@linux.vnet.ibm.com>
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> Suggested-by: James Clark <james.clark@arm.com>
> Suggested-by: Ian Rogers <irogers@google.com>

For the patchset,

Tested-by: Disha Goel <disgoel@linux.vnet.ibm.com>

> ---
>   tools/perf/tests/shell/stat+csv_output.sh | 43 ++++++++++++++++++++---
>   1 file changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/tests/shell/stat+csv_output.sh b/tools/perf/tests/shell/stat+csv_output.sh
> index eb5196f58190..b7f050aa6210 100755
> --- a/tools/perf/tests/shell/stat+csv_output.sh
> +++ b/tools/perf/tests/shell/stat+csv_output.sh
> @@ -6,6 +6,8 @@
>
>   set -e
>
> +skip_test=0
> +
>   function commachecker()
>   {
>   	local -i cnt=0
> @@ -156,14 +158,47 @@ check_per_socket()
>   	echo "[Success]"
>   }
>
> +# The perf stat options for per-socket, per-core, per-die
> +# and -A ( no_aggr mode ) uses the info fetched from this
> +# directory: "/sys/devices/system/cpu/cpu*/topology". For
> +# example, socket value is fetched from "physical_package_id"
> +# file in topology directory.
> +# Reference: cpu__get_topology_int in util/cpumap.c
> +# If the platform doesn't expose topology information, values
> +# will be set to -1. For example, incase of pSeries platform
> +# of powerpc, value for  "physical_package_id" is restricted
> +# and set to -1. Check here validates the socket-id read from
> +# topology file before proceeding further
> +
> +FILE_LOC="/sys/devices/system/cpu/cpu*/topology/"
> +FILE_NAME="physical_package_id"
> +
> +check_for_topology()
> +{
> +	if ! ParanoidAndNotRoot 0
> +	then
> +		socket_file=`ls $FILE_LOC/$FILE_NAME | head -n 1`
> +		[ -z $socket_file ] && return 0
> +		socket_id=`cat $socket_file`
> +		[ $socket_id == -1 ] && skip_test=1
> +		return 0
> +	fi
> +}
> +
> +check_for_topology
>   check_no_args
>   check_system_wide
> -check_system_wide_no_aggr
>   check_interval
>   check_event
> -check_per_core
>   check_per_thread
> -check_per_die
>   check_per_node
> -check_per_socket
> +if [ $skip_test -ne 1 ]
> +then
> +	check_system_wide_no_aggr
> +	check_per_core
> +	check_per_die
> +	check_per_socket
> +else
> +	echo "[Skip] Skipping tests for system_wide_no_aggr, per_core, per_die and per_socket since socket id exposed via topology is invalid"
> +fi
>   exit 0