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

[PATCH V2 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

Changelog:
 V1 -> v2:
 Addressed review comments from Kajol.
 Use "strim" to remove space from string. Also
 use "feof" to check for EOF instead of using new
 variable "ret".

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

-- 
2.35.1

[PATCH V2 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 | 53 ++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/header.h |  1 +
 2 files changed, 54 insertions(+)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index a27132e5a5ef..f08857f96606 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -983,6 +983,59 @@ 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;
+
+	if (!search)
+		return NULL;
+
+	file = fopen("/proc/cpuinfo", "r");
+	if (!file)
+		return NULL;
+
+	while (getline(&buf, &len, file) > 0) {
+		if (!strncmp(buf, search, strlen(search)))
+			break;
+	}
+
+	if (feof(file))
+		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, ':');
+
+	/* Go to string after ":" */
+	copy_buf = p + 1;
+	p = strchr(copy_buf, '\n');
+	if (p)
+		*p = '\0';
+
+	/* Copy the filtered string after removing space to buf */
+	strcpy(buf, strim(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 V2 1/2] tools/perf: Add utility function to read /proc/cpuinfo for any field

[Arnaldo Carvalho de Melo]

Em Thu, May 05, 2022 at 03:09:59PM +0530, Athira Rajeev escreveu:
> /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 | 53 ++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/header.h |  1 +
>  2 files changed, 54 insertions(+)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index a27132e5a5ef..f08857f96606 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -983,6 +983,59 @@ 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;
> +
> +	if (!search)
> +		return NULL;
> +
> +	file = fopen("/proc/cpuinfo", "r");
> +	if (!file)
> +		return NULL;
> +
> +	while (getline(&buf, &len, file) > 0) {
> +		if (!strncmp(buf, search, strlen(search)))

Can you save the search string lenght in a variable and use it instead
of calling strlen() for the same buffer for each line in /proc/cpuinfo?

> +			break;
> +	}
> +
> +	if (feof(file))
> +		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, ':');

So you assume that this will always be there, right? Shouldn't we not
assume that and check if p is NULL and bail out instead?

> +
> +	/* Go to string after ":" */
> +	copy_buf = p + 1;
> +	p = strchr(copy_buf, '\n');

Ditto.

> +	if (p)
> +		*p = '\0';
> +
> +	/* Copy the filtered string after removing space to buf */
> +	strcpy(buf, strim(copy_buf));
> +
> +	fclose(file);
> +	return buf;
> +
> +done:

Please rename this goto label to "not_found", "done" isn't intention
revealing.

> +	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

-- 

- Arnaldo

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

[Athira Rajeev]

> On 05-May-2022, at 10:54 PM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> Em Thu, May 05, 2022 at 03:09:59PM +0530, Athira Rajeev escreveu:
>> /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 | 53 ++++++++++++++++++++++++++++++++++++++++
>> tools/perf/util/header.h |  1 +
>> 2 files changed, 54 insertions(+)
>> 
>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>> index a27132e5a5ef..f08857f96606 100644
>> --- a/tools/perf/util/header.c
>> +++ b/tools/perf/util/header.c
>> @@ -983,6 +983,59 @@ 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;
>> +
>> +	if (!search)
>> +		return NULL;
>> +
>> +	file = fopen("/proc/cpuinfo", "r");
>> +	if (!file)
>> +		return NULL;
>> +
>> +	while (getline(&buf, &len, file) > 0) {
>> +		if (!strncmp(buf, search, strlen(search)))
> 
> Can you save the search string lenght in a variable and use it instead
> of calling strlen() for the same buffer for each line in /proc/cpuinfo?


Hi Arnaldo, Michael

Thanks for review comments. Based on suggestion from Michael, I am reworking on patch 2 to SKIP the test
if physical_id is set to -1 irrespective of value from cpuinfo.

In this patch, I had written "cpuinfo_field " function as generic function for retrieving any entry from /proc/cpuinfo.
But it won't be used in patch 2 now. Do you think this function is useful to keep ? Otherwise, I will drop patch 1

Thanks
Athira Rajeev

> 
>> +			break;
>> +	}
>> +
>> +	if (feof(file))
>> +		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, ':');
> 
> So you assume that this will always be there, right? Shouldn't we not
> assume that and check if p is NULL and bail out instead?
> 
>> +
>> +	/* Go to string after ":" */
>> +	copy_buf = p + 1;
>> +	p = strchr(copy_buf, '\n');
> 
> Ditto.
> 
>> +	if (p)
>> +		*p = '\0';
>> +
>> +	/* Copy the filtered string after removing space to buf */
>> +	strcpy(buf, strim(copy_buf));
>> +
>> +	fclose(file);
>> +	return buf;
>> +
>> +done:
> 
> Please rename this goto label to "not_found", "done" isn't intention
> revealing.
> 
>> +	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
> 
> -- 
> 
> - Arnaldo

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

[Athira Rajeev]

> On 06-May-2022, at 3:03 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> 
> 
> 
>> On 05-May-2022, at 10:54 PM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>> 
>> Em Thu, May 05, 2022 at 03:09:59PM +0530, Athira Rajeev escreveu:
>>> /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 | 53 ++++++++++++++++++++++++++++++++++++++++
>>> tools/perf/util/header.h |  1 +
>>> 2 files changed, 54 insertions(+)
>>> 
>>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
>>> index a27132e5a5ef..f08857f96606 100644
>>> --- a/tools/perf/util/header.c
>>> +++ b/tools/perf/util/header.c
>>> @@ -983,6 +983,59 @@ 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;
>>> +
>>> +	if (!search)
>>> +		return NULL;
>>> +
>>> +	file = fopen("/proc/cpuinfo", "r");
>>> +	if (!file)
>>> +		return NULL;
>>> +
>>> +	while (getline(&buf, &len, file) > 0) {
>>> +		if (!strncmp(buf, search, strlen(search)))
>> 
>> Can you save the search string lenght in a variable and use it instead
>> of calling strlen() for the same buffer for each line in /proc/cpuinfo?
> 
> 
> Hi Arnaldo, Michael
> 
> Thanks for review comments. Based on suggestion from Michael, I am reworking on patch 2 to SKIP the test
> if physical_id is set to -1 irrespective of value from cpuinfo.
> 
> In this patch, I had written "cpuinfo_field " function as generic function for retrieving any entry from /proc/cpuinfo.
> But it won't be used in patch 2 now. Do you think this function is useful to keep ? Otherwise, I will drop patch 1

Hi,

Requesting for suggestions on this change

Thanks
Athira
> 
> Thanks
> Athira Rajeev
> 
>> 
>>> +			break;
>>> +	}
>>> +
>>> +	if (feof(file))
>>> +		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, ':');
>> 
>> So you assume that this will always be there, right? Shouldn't we not
>> assume that and check if p is NULL and bail out instead?
>> 
>>> +
>>> +	/* Go to string after ":" */
>>> +	copy_buf = p + 1;
>>> +	p = strchr(copy_buf, '\n');
>> 
>> Ditto.
>> 
>>> +	if (p)
>>> +		*p = '\0';
>>> +
>>> +	/* Copy the filtered string after removing space to buf */
>>> +	strcpy(buf, strim(copy_buf));
>>> +
>>> +	fclose(file);
>>> +	return buf;
>>> +
>>> +done:
>> 
>> Please rename this goto label to "not_found", "done" isn't intention
>> revealing.
>> 
>>> +	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
>> 
>> -- 
>> 
>> - Arnaldo

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

[Arnaldo Carvalho de Melo]

Em Tue, May 10, 2022 at 07:08:47PM +0530, Athira Rajeev escreveu:
> 
> 
> > On 06-May-2022, at 3:03 PM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> > 
> > 
> > 
> >> On 05-May-2022, at 10:54 PM, Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> >> 
> >> Em Thu, May 05, 2022 at 03:09:59PM +0530, Athira Rajeev escreveu:
> >>> /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 | 53 ++++++++++++++++++++++++++++++++++++++++
> >>> tools/perf/util/header.h |  1 +
> >>> 2 files changed, 54 insertions(+)
> >>> 
> >>> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> >>> index a27132e5a5ef..f08857f96606 100644
> >>> --- a/tools/perf/util/header.c
> >>> +++ b/tools/perf/util/header.c
> >>> @@ -983,6 +983,59 @@ 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;
> >>> +
> >>> +	if (!search)
> >>> +		return NULL;
> >>> +
> >>> +	file = fopen("/proc/cpuinfo", "r");
> >>> +	if (!file)
> >>> +		return NULL;
> >>> +
> >>> +	while (getline(&buf, &len, file) > 0) {
> >>> +		if (!strncmp(buf, search, strlen(search)))
> >> 
> >> Can you save the search string lenght in a variable and use it instead
> >> of calling strlen() for the same buffer for each line in /proc/cpuinfo?
> > 
> > 
> > Hi Arnaldo, Michael
> > 
> > Thanks for review comments. Based on suggestion from Michael, I am reworking on patch 2 to SKIP the test
> > if physical_id is set to -1 irrespective of value from cpuinfo.
> > 
> > In this patch, I had written "cpuinfo_field " function as generic function for retrieving any entry from /proc/cpuinfo.
> > But it won't be used in patch 2 now. Do you think this function is useful to keep ? Otherwise, I will drop patch 1

Lets add it when the need arises.

- Arnaldo
 
> Hi,
> 
> Requesting for suggestions on this change
> 
> Thanks
> Athira
> > 
> > Thanks
> > Athira Rajeev
> > 
> >> 
> >>> +			break;
> >>> +	}
> >>> +
> >>> +	if (feof(file))
> >>> +		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, ':');
> >> 
> >> So you assume that this will always be there, right? Shouldn't we not
> >> assume that and check if p is NULL and bail out instead?
> >> 
> >>> +
> >>> +	/* Go to string after ":" */
> >>> +	copy_buf = p + 1;
> >>> +	p = strchr(copy_buf, '\n');
> >> 
> >> Ditto.
> >> 
> >>> +	if (p)
> >>> +		*p = '\0';
> >>> +
> >>> +	/* Copy the filtered string after removing space to buf */
> >>> +	strcpy(buf, strim(copy_buf));
> >>> +
> >>> +	fclose(file);
> >>> +	return buf;
> >>> +
> >>> +done:
> >> 
> >> Please rename this goto label to "not_found", "done" isn't intention
> >> revealing.
> >> 
> >>> +	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
> >> 
> >> -- 
> >> 
> >> - Arnaldo

-- 

- Arnaldo

[PATCH V2 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.

Results:

 After the patch:
 ----------------
 # ./perf test 41
 41: Session topology                  : Skip

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..036143928023 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 (cpuinfo_platform && !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

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

[Michael Ellerman]

Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
> 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.

I don't think that's the right way to fix it.

If we ever had a "pseries" machine that did expose physical_package_id
then this test would continue to skip, even when it could succeed.

So if physical_package_id being -1 is the problem then you should skip
the test based on that.

cheers

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

[kajoljain]

On 5/5/22 15:09, Athira Rajeev wrote:
> 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

Hi Athira,
   Patchset looks good to me.

Reviewed-by: Kajol Jain <kjain@linux.ibm.com>

Thanks,
Kajol Jain

> 
> Changelog:
>  V1 -> v2:
>  Addressed review comments from Kajol.
>  Use "strim" to remove space from string. Also
>  use "feof" to check for EOF instead of using new
>  variable "ret".
> 
>  tools/perf/tests/topology.c | 17 ++++++++++++
>  tools/perf/util/header.c    | 53 +++++++++++++++++++++++++++++++++++++
>  tools/perf/util/header.h    |  1 +
>  3 files changed, 71 insertions(+)
>