[PATCH 0/2] Fix session topology test for powerpc and add utility function to get cpuinfo entries

[PATCH 0/2] Fix session topology test for powerpc and add utility function to get cpuinfo entries

[Athira Rajeev]

The session topology test fails in powerpc pSeries platform.
Test logs:
<<>>
Session topology : FAILED!
<<>>

This test uses cpu topology information and in powerpc,
some of the topology info is restricted in environment
like virtualized platform. Hence this test needs to be
skipped in pSeries platform for powerpc. The information
about platform is available in /proc/cpuinfo.

Patch 1 adds generic utility function in "util/header.c"
to read /proc/cpuinfo for any entry. Though the testcase
fix needs value from "platform" entry, making this as a
generic function to return value for any entry from the
/proc/cpuinfo file which can be used commonly in future
usecases.

Patch 2 uses the newly added utility function to look for
platform and skip the test in pSeries platform for powerpc.

Athira Rajeev (2):
  tools/perf: Add utility function to read /proc/cpuinfo for any field
  tools/perf/tests: Fix session topology test to skip the test in guest
    environment

 tools/perf/tests/topology.c | 17 ++++++++++++
 tools/perf/util/header.c    | 54 +++++++++++++++++++++++++++++++++++++
 tools/perf/util/header.h    |  1 +
 3 files changed, 72 insertions(+)

-- 
2.35.1

[PATCH 1/2] tools/perf: Add utility function to read /proc/cpuinfo for any field

[Athira Rajeev]

/proc/cpuinfo provides information about type of processor, number
of CPU's etc. Reading /proc/cpuinfo file outputs useful information
by field name like cpu, platform, model (depending on architecture)
and its value separated by colon.

Add new utility function "cpuinfo_field" in "util/header.c" which
accepts field name as input string to search in /proc/cpuinfo content.
This returns the first matching value as resulting string. Example,
calling the function "cpuinfo_field(platform)" in powerpc returns
the platform value. This can be used to fetch processor information
from "cpuinfo" by other utilities/testcases.

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/util/header.c | 54 ++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/header.h |  1 +
 2 files changed, 55 insertions(+)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index a27132e5a5ef..0c8dfd0c1e78 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -983,6 +983,60 @@ static int write_dir_format(struct feat_fd *ff,
 	return do_write(ff, &data->dir.version, sizeof(data->dir.version));
 }
 
+/*
+ * Return entry from /proc/cpuinfo
+ * indicated by "search" parameter.
+ */
+char *cpuinfo_field(const char *search)
+{
+	FILE *file;
+	char *buf = NULL;
+	char *copy_buf = NULL, *p;
+	size_t len = 0;
+	int ret = -1;
+
+	if (!search)
+		return NULL;
+
+	file = fopen("/proc/cpuinfo", "r");
+	if (!file)
+		return NULL;
+
+	while (getline(&buf, &len, file) > 0) {
+		ret = strncmp(buf, search, strlen(search));
+		if (!ret)
+			break;
+	}
+
+	if (ret)
+		goto done;
+
+	/*
+	 * Trim the new line and separate
+	 * value for search field from ":"
+	 * in cpuinfo line output.
+	 * Example output line:
+	 * platform : <value>
+	 */
+	copy_buf = buf;
+	p = strchr(copy_buf, ':');
+	if (p && *(p+1) == ' ' && *(p+2))
+		copy_buf = p + 2;
+	p = strchr(copy_buf, '\n');
+	if (p)
+		*p = '\0';
+
+	/* Copy the filtered string to buf */
+	strcpy(buf, copy_buf);
+
+	fclose(file);
+	return buf;
+
+done:
+	free(buf);
+	fclose(file);
+	return NULL;
+}
 /*
  * Check whether a CPU is online
  *
diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
index 0eb4bc29a5a4..b0f754364bd4 100644
--- a/tools/perf/util/header.h
+++ b/tools/perf/util/header.h
@@ -166,4 +166,5 @@ int get_cpuid(char *buffer, size_t sz);
 
 char *get_cpuid_str(struct perf_pmu *pmu __maybe_unused);
 int strcmp_cpuid_str(const char *s1, const char *s2);
+char *cpuinfo_field(const char *search);
 #endif /* __PERF_HEADER_H */
-- 
2.35.1

Re: [PATCH 1/2] tools/perf: Add utility function to read /proc/cpuinfo for any field

[kajoljain]

On 4/28/22 20:38, Athira Rajeev wrote:
> /proc/cpuinfo provides information about type of processor, number
> of CPU's etc. Reading /proc/cpuinfo file outputs useful information
> by field name like cpu, platform, model (depending on architecture)
> and its value separated by colon.
> 
> Add new utility function "cpuinfo_field" in "util/header.c" which
> accepts field name as input string to search in /proc/cpuinfo content.
> This returns the first matching value as resulting string. Example,
> calling the function "cpuinfo_field(platform)" in powerpc returns
> the platform value. This can be used to fetch processor information
> from "cpuinfo" by other utilities/testcases.
> 
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
>  tools/perf/util/header.c | 54 ++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/header.h |  1 +
>  2 files changed, 55 insertions(+)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index a27132e5a5ef..0c8dfd0c1e78 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -983,6 +983,60 @@ static int write_dir_format(struct feat_fd *ff,
>  	return do_write(ff, &data->dir.version, sizeof(data->dir.version));
>  }
>  
> +/*
> + * Return entry from /proc/cpuinfo
> + * indicated by "search" parameter.
> + */
> +char *cpuinfo_field(const char *search)
> +{
> +	FILE *file;
> +	char *buf = NULL;
> +	char *copy_buf = NULL, *p;
> +	size_t len = 0;
> +	int ret = -1;
> +
> +	if (!search)
> +		return NULL;
> +
> +	file = fopen("/proc/cpuinfo", "r");
> +	if (!file)
> +		return NULL;
> +
> +	while (getline(&buf, &len, file) > 0) {
> +		ret = strncmp(buf, search, strlen(search));
> +		if (!ret)
> +			break;
Hi Athira,
	Do we need ret variable. Since we will come out of the loop only when
we reach EOF.

> +	}
> +
> +	if (ret)
> +		goto done;
> +
> +	/*
> +	 * Trim the new line and separate
> +	 * value for search field from ":"
> +	 * in cpuinfo line output.
> +	 * Example output line:
> +	 * platform : <value>
> +	 */
> +	copy_buf = buf;
> +	p = strchr(copy_buf, ':');
> +	if (p && *(p+1) == ' ' && *(p+2))


Can you try using strim instead to remove whitespaces. This function
will remove leading and trailing whitespaces from the string.

> +		copy_buf = p + 2;
> +	p = strchr(copy_buf, '\n');

do we need to replace `\n` here ?


> +	if (p)
> +		*p = '\0';
> +
> +	/* Copy the filtered string to buf */
> +	strcpy(buf, copy_buf)

You are initializing buf to NULL. So do we need to do fclose and return
buf separately here? Can you move free(buf) in above condition and reuse
`done` code.
> +
> +	fclose(file);
> +	return buf;> +
> +done:
> +	free(buf);
> +	fclose(file);
> +	return NULL;
> +}
>  /*
>   * Check whether a CPU is online
>   *
> diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
> index 0eb4bc29a5a4..b0f754364bd4 100644
> --- a/tools/perf/util/header.h
> +++ b/tools/perf/util/header.h
> @@ -166,4 +166,5 @@ int get_cpuid(char *buffer, size_t sz);
>  
>  char *get_cpuid_str(struct perf_pmu *pmu __maybe_unused);
>  int strcmp_cpuid_str(const char *s1, const char *s2);
> +char *cpuinfo_field(const char *search);
>  #endif /* __PERF_HEADER_H */

Re: [PATCH 1/2] tools/perf: Add utility function to read /proc/cpuinfo for any field

[Athira Rajeev]

> On 04-May-2022, at 7:16 PM, kajoljain <kjain@linux.ibm.com> wrote:
> 
> 
> 
> On 4/28/22 20:38, Athira Rajeev wrote:
>> /proc/cpuinfo provides information about type of processor, number
>> of CPU's etc. Reading /proc/cpuinfo file outputs useful information
>> by field name like cpu, platform, model (depending on architecture)
>> and its value separated by colon.
>> 
>> Add new utility function "cpuinfo_field" in "util/header.c" which
>> accepts field name as input string to search in /proc/cpuinfo content.
>> This returns the first matching value as resulting string. Example,
>> calling the function "cpuinfo_field(platform)" in powerpc returns
>> the platform value. This can be used to fetch processor information
>> from "cpuinfo" by other utilities/testcases.
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> tools/perf/util/header.c | 54 ++++++++++++++++++++++++++++++++++++++++
>> tools/perf/util/header.h |  1 +
>> 2 files changed, 55 insertions(+)
>> 
>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>> index a27132e5a5ef..0c8dfd0c1e78 100644
>> --- a/tools/perf/util/header.c
>> +++ b/tools/perf/util/header.c
>> @@ -983,6 +983,60 @@ static int write_dir_format(struct feat_fd *ff,
>> 	return do_write(ff, &data->dir.version, sizeof(data->dir.version));
>> }
>> 
>> +/*
>> + * Return entry from /proc/cpuinfo
>> + * indicated by "search" parameter.
>> + */
>> +char *cpuinfo_field(const char *search)
>> +{
>> +	FILE *file;
>> +	char *buf = NULL;
>> +	char *copy_buf = NULL, *p;
>> +	size_t len = 0;
>> +	int ret = -1;
>> +
>> +	if (!search)
>> +		return NULL;
>> +
>> +	file = fopen("/proc/cpuinfo", "r");
>> +	if (!file)
>> +		return NULL;
>> +
>> +	while (getline(&buf, &len, file) > 0) {
>> +		ret = strncmp(buf, search, strlen(search));
>> +		if (!ret)
>> +			break;
> Hi Athira,
> 	Do we need ret variable. Since we will come out of the loop only when
> we reach EOF.
> 
>> +	}
>> +
>> +	if (ret)
>> +		goto done;
>> +
>> +	/*
>> +	 * Trim the new line and separate
>> +	 * value for search field from ":"
>> +	 * in cpuinfo line output.
>> +	 * Example output line:
>> +	 * platform : <value>
>> +	 */
>> +	copy_buf = buf;
>> +	p = strchr(copy_buf, ':');
>> +	if (p && *(p+1) == ' ' && *(p+2))
> 
> 
> Can you try using strim instead to remove whitespaces. This function
> will remove leading and trailing whitespaces from the string.

Hi Kajol,

Thanks for review comments.
I will send V2 addressing these changes

Athira
> 
>> +		copy_buf = p + 2;
>> +	p = strchr(copy_buf, '\n');
> 
> do we need to replace `\n` here ?
> 
> 
>> +	if (p)
>> +		*p = '\0';
>> +
>> +	/* Copy the filtered string to buf */
>> +	strcpy(buf, copy_buf)
> 
> You are initializing buf to NULL. So do we need to do fclose and return
> buf separately here? Can you move free(buf) in above condition and reuse
> `done` code.
>> +
>> +	fclose(file);
>> +	return buf;> +
>> +done:
>> +	free(buf);
>> +	fclose(file);
>> +	return NULL;
>> +}
>> /*
>>  * Check whether a CPU is online
>>  *
>> diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
>> index 0eb4bc29a5a4..b0f754364bd4 100644
>> --- a/tools/perf/util/header.h
>> +++ b/tools/perf/util/header.h
>> @@ -166,4 +166,5 @@ int get_cpuid(char *buffer, size_t sz);
>> 
>> char *get_cpuid_str(struct perf_pmu *pmu __maybe_unused);
>> int strcmp_cpuid_str(const char *s1, const char *s2);
>> +char *cpuinfo_field(const char *search);
>> #endif /* __PERF_HEADER_H */

[PATCH 2/2] tools/perf/tests: Fix session topology test to skip the test in guest environment

[Athira Rajeev]

The session topology test fails in powerpc pSeries platform.
Test logs:
<<>>
Session topology : FAILED!
<<>>

This testcases tests cpu topology by checking the core_id and
socket_id stored in perf_env from perf session. The data from
perf session is compared with the cpu topology information
from "/sys/devices/system/cpu/cpuX/topology" like core_id,
physical_package_id. In case of virtual environment, detail
like physical_package_id is restricted to be exposed. Hence
physical_package_id is set to -1. The testcase fails on such
platforms since socket_id can't be fetched from topology info.

Skip the testcase in powerpc for pSeries. Use the utility
function "cpuinfo_field" to check platform from /proc/cpuinfo.

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/tests/topology.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c
index ee1e3dcbc0bd..0ddcafa158db 100644
--- a/tools/perf/tests/topology.c
+++ b/tools/perf/tests/topology.c
@@ -109,6 +109,23 @@ static int check_cpu_topology(char *path, struct perf_cpu_map *map)
 			&& strncmp(session->header.env.arch, "aarch64", 7))
 		return TEST_SKIP;
 
+	/*
+	 * In powerpc pSeries platform, not all the topology information
+	 * are exposed via sysfs. Due to restriction, detail like
+	 * physical_package_id will be set to -1. Hence skip this
+	 * test for pSeries.
+	 */
+	if (strncmp(session->header.env.arch, "powerpc", 7)) {
+		char *cpuinfo_platform = NULL;
+
+		cpuinfo_platform = cpuinfo_field("platform");
+		if (!strcmp(cpuinfo_platform, "pSeries")) {
+			free(cpuinfo_platform);
+			return TEST_SKIP;
+		}
+		free(cpuinfo_platform);
+	}
+
 	TEST_ASSERT_VAL("Session header CPU map not set", session->header.env.cpu);
 
 	for (i = 0; i < session->header.env.nr_cpus_avail; i++) {
-- 
2.35.1