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

[Disha Goel <disgoel@linux.ibm.com>]

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