[PATCH 0/2] perf PMU events test: Add scenario for arch std events

[PATCH 0/2] perf PMU events test: Add scenario for arch std events

[John Garry]

The small series covers the following:
- Tidy error handling in jevents a bit
- Expands on PMU events test to cover jevents arch std events support

John Garry (2):
  perf jevents: Tidy error handling
  perf jevents: Add test for arch std events

 .../pmu-events/arch/test/arch-std-events.json |  8 ++
 .../pmu-events/arch/test/test_cpu/cache.json  |  5 ++
 tools/perf/pmu-events/jevents.c               | 85 ++++++++-----------
 tools/perf/tests/pmu-events.c                 | 14 +++
 4 files changed, 64 insertions(+), 48 deletions(-)
 create mode 100644 tools/perf/pmu-events/arch/test/arch-std-events.json
 create mode 100644 tools/perf/pmu-events/arch/test/test_cpu/cache.json

-- 
2.26.2

[PATCH 1/2] perf jevents: Tidy error handling

[John Garry]

There is much duplication in the error handling for directory traversing
for processing JSONs.

Factor out the common code to tidy a bit.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 tools/perf/pmu-events/jevents.c | 81 ++++++++++++++-------------------
 1 file changed, 33 insertions(+), 48 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index e47644cab3fa..ed2374661665 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -1100,12 +1100,13 @@ static int process_one_file(const char *fpath, const struct stat *sb,
  */
 int main(int argc, char *argv[])
 {
-	int rc, ret = 0;
+	int rc, ret = 0, empty_map = 0;
 	int maxfds;
 	char ldirname[PATH_MAX];
 	const char *arch;
 	const char *output_file;
 	const char *start_dirname;
+	char *err_string_ext = "";
 	struct stat stbuf;
 
 	prog = basename(argv[0]);
@@ -1133,7 +1134,8 @@ int main(int argc, char *argv[])
 	/* If architecture does not have any event lists, bail out */
 	if (stat(ldirname, &stbuf) < 0) {
 		pr_info("%s: Arch %s has no PMU event lists\n", prog, arch);
-		goto empty_map;
+		empty_map = 1;
+		goto err_close_eventsfp;
 	}
 
 	/* Include pmu-events.h first */
@@ -1150,75 +1152,58 @@ int main(int argc, char *argv[])
 	 */
 
 	maxfds = get_maxfds();
-	mapfile = NULL;
 	rc = nftw(ldirname, preprocess_arch_std_files, maxfds, 0);
-	if (rc && verbose) {
-		pr_info("%s: Error preprocessing arch standard files %s\n",
-			prog, ldirname);
-		goto empty_map;
-	} else if (rc < 0) {
-		/* Make build fail */
-		fclose(eventsfp);
-		free_arch_std_events();
-		return 1;
-	} else if (rc) {
-		goto empty_map;
-	}
+	if (rc)
+		goto err_processing_std_arch_event_dir;
 
 	rc = nftw(ldirname, process_one_file, maxfds, 0);
-	if (rc && verbose) {
-		pr_info("%s: Error walking file tree %s\n", prog, ldirname);
-		goto empty_map;
-	} else if (rc < 0) {
-		/* Make build fail */
-		fclose(eventsfp);
-		free_arch_std_events();
-		ret = 1;
-		goto out_free_mapfile;
-	} else if (rc) {
-		goto empty_map;
-	}
+	if (rc)
+		goto err_processing_dir;
 
 	sprintf(ldirname, "%s/test", start_dirname);
 
 	rc = nftw(ldirname, process_one_file, maxfds, 0);
-	if (rc && verbose) {
-		pr_info("%s: Error walking file tree %s rc=%d for test\n",
-			prog, ldirname, rc);
-		goto empty_map;
-	} else if (rc < 0) {
-		/* Make build fail */
-		free_arch_std_events();
-		ret = 1;
-		goto out_free_mapfile;
-	} else if (rc) {
-		goto empty_map;
-	}
+	if (rc)
+		goto err_processing_dir;
 
 	if (close_table)
 		print_events_table_suffix(eventsfp);
 
 	if (!mapfile) {
 		pr_info("%s: No CPU->JSON mapping?\n", prog);
-		goto empty_map;
+		empty_map = 1;
+		goto err_close_eventsfp;
 	}
 
-	if (process_mapfile(eventsfp, mapfile)) {
+	rc = process_mapfile(eventsfp, mapfile);
+	fclose(eventsfp);
+	if (rc) {
 		pr_info("%s: Error processing mapfile %s\n", prog, mapfile);
 		/* Make build fail */
-		fclose(eventsfp);
-		free_arch_std_events();
 		ret = 1;
+		goto err_out;
 	}
 
+	free_arch_std_events();
+	free(mapfile);
+	return 0;
 
-	goto out_free_mapfile;
-
-empty_map:
+err_processing_std_arch_event_dir:
+	err_string_ext = " for std arch event";
+err_processing_dir:
+	if (verbose || rc > 0) {
+		pr_info("%s: Error walking file tree %s%s\n", prog, ldirname,
+			err_string_ext);
+		empty_map = 1;
+	} else {
+		ret = 1;
+	}
+err_close_eventsfp:
 	fclose(eventsfp);
-	create_empty_mapping(output_file);
+	if (empty_map)
+		create_empty_mapping(output_file);
+err_out:
 	free_arch_std_events();
-out_free_mapfile:
 	free(mapfile);
 	return ret;
 }
-- 
2.26.2

Re: [PATCH 1/2] perf jevents: Tidy error handling

[Namhyung Kim]

Hello,

On Tue, Oct 20, 2020 at 12:42 AM John Garry <john.garry@huawei.com> wrote:
>
> There is much duplication in the error handling for directory traversing
> for processing JSONs.
>
> Factor out the common code to tidy a bit.
>
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
[SNIP]
> -empty_map:
> +err_processing_std_arch_event_dir:
> +       err_string_ext = " for std arch event";
> +err_processing_dir:
> +       if (verbose || rc > 0) {
> +               pr_info("%s: Error walking file tree %s%s\n", prog, ldirname,
> +                       err_string_ext);

This was printed only if verbose is set but now changed.

Thanks
Namhyung


> +               empty_map = 1;
> +       } else {
> +               ret = 1;
> +       }
> +err_close_eventsfp:
>         fclose(eventsfp);
> -       create_empty_mapping(output_file);
> +       if (empty_map)
> +               create_empty_mapping(output_file);
> +err_out:
>         free_arch_std_events();
> -out_free_mapfile:
>         free(mapfile);
>         return ret;
>  }
> --
> 2.26.2
>

Re: [PATCH 1/2] perf jevents: Tidy error handling

[kajoljain]

On 10/20/20 11:13 AM, Namhyung Kim wrote:
> Hello,
> 
> On Tue, Oct 20, 2020 at 12:42 AM John Garry <john.garry@huawei.com> wrote:
>>
>> There is much duplication in the error handling for directory traversing
>> for processing JSONs.
>>
>> Factor out the common code to tidy a bit.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
> [SNIP]
>> -empty_map:
>> +err_processing_std_arch_event_dir:
>> +       err_string_ext = " for std arch event";
>> +err_processing_dir:
>> +       if (verbose || rc > 0) {
>> +               pr_info("%s: Error walking file tree %s%s\n", prog, ldirname,
>> +                       err_string_ext);
> 
> This was printed only if verbose is set but now changed.

Hi John,
   Yes I agree with Namhyung.
So, this is our original checks

if (rc && verbose) {
		pr_info("%s: Error walking file tree %s\n", prog, ldirname);
		goto empty_map;
	} else if (rc < 0) {
		/* Make build fail */
		fclose(eventsfp);
		free_arch_std_events();
		ret = 1;
		goto out_free_mapfile;
	} else if (rc) {
		goto empty_map;
	}

May be we can use similar checks:

if( verbose)
  pr_info("%s: Error walking file tree %s%s\n", prog, ldirname,err_string_ext);
if(rc > 0)
   empty_map = 1;
else
  ret = 1;

Thanks,
Kajol Jain
  
> 
> Thanks
> Namhyung
> 
> 
>> +               empty_map = 1;
>> +       } else {
>> +               ret = 1;
>> +       }
>> +err_close_eventsfp:
>>         fclose(eventsfp);
>> -       create_empty_mapping(output_file);
>> +       if (empty_map)
>> +               create_empty_mapping(output_file);
>> +err_out:
>>         free_arch_std_events();
>> -out_free_mapfile:
>>         free(mapfile);
>>         return ret;
>>  }
>> --
>> 2.26.2
>>

Re: [PATCH 1/2] perf jevents: Tidy error handling

[John Garry]

On 20/10/2020 09:14, kajoljain wrote:
> 
> 
> On 10/20/20 11:13 AM, Namhyung Kim wrote:
>> Hello,
>>
>> On Tue, Oct 20, 2020 at 12:42 AM John Garry <john.garry@huawei.com> wrote:
>>>
>>> There is much duplication in the error handling for directory traversing
>>> for processing JSONs.
>>>
>>> Factor out the common code to tidy a bit.
>>>
>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>> ---
>> [SNIP]
>>> -empty_map:
>>> +err_processing_std_arch_event_dir:
>>> +       err_string_ext = " for std arch event";
>>> +err_processing_dir:
>>> +       if (verbose || rc > 0) {
>>> +               pr_info("%s: Error walking file tree %s%s\n", prog, ldirname,
>>> +                       err_string_ext);
>>
>> This was printed only if verbose is set but now changed.
> 

pr_info() is silenced if verbose is unset, so in effect it is the same - 
see pr_info() definition in json.h

> Hi John,
>     Yes I agree with Namhyung.
> So, this is our original checks
> 
> if (rc && verbose) {
> 		pr_info("%s: Error walking file tree %s\n", prog, ldirname);
> 		goto empty_map;
> 	} else if (rc < 0) {
> 		/* Make build fail */
> 		fclose(eventsfp);
> 		free_arch_std_events();
> 		ret = 1;
> 		goto out_free_mapfile;
> 	} else if (rc) {
> 		goto empty_map;
> 	}
> 
> May be we can use similar checks:
> 
> if( verbose)
>    pr_info("%s: Error walking file tree %s%s\n", prog, ldirname,err_string_ext);
> if(rc > 0)
>     empty_map = 1;
> else
>    ret = 1;
> 

Not that it matters much, this logic is slightly different for verbose 
set and rc < 0. I don't mind going with that, so let me know.

Thanks for checking,
John

> Thanks,
> Kajol Jain
>    
>>
>> Thanks
>> Namhyung
>>
>>
>>> +               empty_map = 1;
>>> +       } else {
>>> +               ret = 1;
>>> +       }
>>> +err_close_eventsfp:
>>>          fclose(eventsfp);
>>> -       create_empty_mapping(output_file);
>>> +       if (empty_map)
>>> +               create_empty_mapping(output_file);
>>> +err_out:
>>>          free_arch_std_events();
>>> -out_free_mapfile:
>>>          free(mapfile);
>>>          return ret;
>>>   }
>>> --
>>> 2.26.2
>>>
> .
> 

Re: [PATCH 1/2] perf jevents: Tidy error handling

[kajoljain]

On 10/20/20 2:05 PM, John Garry wrote:
> On 20/10/2020 09:14, kajoljain wrote:
>>
>>
>> On 10/20/20 11:13 AM, Namhyung Kim wrote:
>>> Hello,
>>>
>>> On Tue, Oct 20, 2020 at 12:42 AM John Garry <john.garry@huawei.com> wrote:
>>>>
>>>> There is much duplication in the error handling for directory traversing
>>>> for processing JSONs.
>>>>
>>>> Factor out the common code to tidy a bit.
>>>>
>>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>>> ---
>>> [SNIP]
>>>> -empty_map:
>>>> +err_processing_std_arch_event_dir:
>>>> +       err_string_ext = " for std arch event";
>>>> +err_processing_dir:
>>>> +       if (verbose || rc > 0) {
>>>> +               pr_info("%s: Error walking file tree %s%s\n", prog, ldirname,
>>>> +                       err_string_ext);
>>>
>>> This was printed only if verbose is set but now changed.
>>
> 
> pr_info() is silenced if verbose is unset, so in effect it is the same - see pr_info() definition in json.h
> 
>> Hi John,
>>     Yes I agree with Namhyung.
>> So, this is our original checks
>>
>> if (rc && verbose) {
>>         pr_info("%s: Error walking file tree %s\n", prog, ldirname);
>>         goto empty_map;
>>     } else if (rc < 0) {
>>         /* Make build fail */
>>         fclose(eventsfp);
>>         free_arch_std_events();
>>         ret = 1;
>>         goto out_free_mapfile;
>>     } else if (rc) {
>>         goto empty_map;
>>     }
>>
>> May be we can use similar checks:
>>
>> if( verbose)
>>    pr_info("%s: Error walking file tree %s%s\n", prog, ldirname,err_string_ext);
>> if(rc > 0)
>>     empty_map = 1;
>> else
>>    ret = 1;
>>
> 
> Not that it matters much, this logic is slightly different for verbose set and rc < 0. I don't mind going with that, so let me know.

Yes right. Sure if required we can made changes on these checks and include appropriate condition for verbose set and rc < 0. Seems fine to me.

Thanks,
Kajol Jain
> 
> Thanks for checking,
> John
> 
>> Thanks,
>> Kajol Jain
>>   
>>>
>>> Thanks
>>> Namhyung
>>>
>>>
>>>> +               empty_map = 1;
>>>> +       } else {
>>>> +               ret = 1;
>>>> +       }
>>>> +err_close_eventsfp:
>>>>          fclose(eventsfp);
>>>> -       create_empty_mapping(output_file);
>>>> +       if (empty_map)
>>>> +               create_empty_mapping(output_file);
>>>> +err_out:
>>>>          free_arch_std_events();
>>>> -out_free_mapfile:
>>>>          free(mapfile);
>>>>          return ret;
>>>>   }
>>>> -- 
>>>> 2.26.2
>>>>
>> .
>>
> 

Re: [PATCH 1/2] perf jevents: Tidy error handling

[John Garry]

On 21/10/2020 14:37, kajoljain wrote:
>>> May be we can use similar checks:
>>>
>>> if( verbose)
>>>     pr_info("%s: Error walking file tree %s%s\n", prog, ldirname,err_string_ext);
>>> if(rc > 0)
>>>      empty_map = 1;
>>> else
>>>     ret = 1;
>>>
>> Not that it matters much, this logic is slightly different for verbose set and rc < 0. I don't mind going with that, so let me know.
> Yes right. Sure if required we can made changes on these checks and include appropriate condition for verbose set and rc < 0. Seems fine to me.

I will just revert to the original logic for now. Someone can try to 
change later if they want.

Thanks

[PATCH 2/2] perf jevents: Add test for arch std events

[John Garry]

Recently there was an undetected breakage for std arch event support.

Add support in "PMU events" testcase to detect such breakages.

For this, the "test" arch needs has support added to process std arch
events. And a test event is added for the test, ifself.

Also add a few code comments to help understand the code a bit better.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 .../perf/pmu-events/arch/test/arch-std-events.json |  8 ++++++++
 .../perf/pmu-events/arch/test/test_cpu/cache.json  |  5 +++++
 tools/perf/pmu-events/jevents.c                    |  4 ++++
 tools/perf/tests/pmu-events.c                      | 14 ++++++++++++++
 4 files changed, 31 insertions(+)
 create mode 100644 tools/perf/pmu-events/arch/test/arch-std-events.json
 create mode 100644 tools/perf/pmu-events/arch/test/test_cpu/cache.json

diff --git a/tools/perf/pmu-events/arch/test/arch-std-events.json b/tools/perf/pmu-events/arch/test/arch-std-events.json
new file mode 100644
index 000000000000..43f6f729d6ae
--- /dev/null
+++ b/tools/perf/pmu-events/arch/test/arch-std-events.json
@@ -0,0 +1,8 @@
+[
+    {
+        "PublicDescription": "Attributable Level 3 cache access, read",
+        "EventCode": "0x40",
+        "EventName": "L3_CACHE_RD",
+        "BriefDescription": "L3 cache access, read"
+    }
+]
diff --git a/tools/perf/pmu-events/arch/test/test_cpu/cache.json b/tools/perf/pmu-events/arch/test/test_cpu/cache.json
new file mode 100644
index 000000000000..036d0efdb2bb
--- /dev/null
+++ b/tools/perf/pmu-events/arch/test/test_cpu/cache.json
@@ -0,0 +1,5 @@
+[
+    {
+	 "ArchStdEvent": "L3_CACHE_RD"
+    }
+]
\ No newline at end of file
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index ed2374661665..6d099f645811 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -1162,6 +1162,10 @@ int main(int argc, char *argv[])
 
 	sprintf(ldirname, "%s/test", start_dirname);
 
+	rc = nftw(ldirname, preprocess_arch_std_files, maxfds, 0);
+	if (rc)
+		goto err_processing_std_arch_event_dir;
+
 	rc = nftw(ldirname, process_one_file, maxfds, 0);
 	if (rc)
 		goto err_processing_dir;
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index d3517a74d95e..ad2b21591275 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -14,8 +14,10 @@
 #include "util/parse-events.h"
 
 struct perf_pmu_test_event {
+	/* used for matching against events from generated pmu-events.c */
 	struct pmu_event event;
 
+	/* used for matching against event aliases */
 	/* extra events for aliases */
 	const char *alias_str;
 
@@ -78,6 +80,17 @@ static struct perf_pmu_test_event test_cpu_events[] = {
 		.alias_str = "umask=0,(null)=0x30d40,event=0x3a",
 		.alias_long_desc = "Number of Enhanced Intel SpeedStep(R) Technology (EIST) transitions",
 	},
+	{
+		.event = {
+			.name = "l3_cache_rd",
+			.event = "event=0x40",
+			.desc = "L3 cache access, read",
+			.long_desc = "Attributable Level 3 cache access, read",
+			.topic = "cache",
+		},
+		.alias_str = "event=0x40",
+		.alias_long_desc = "Attributable Level 3 cache access, read",
+	},
 	{ /* sentinel */
 		.event = {
 			.name = NULL,
@@ -357,6 +370,7 @@ static int __test__pmu_event_aliases(char *pmu_name, int *count)
 }
 
 
+/* Test that aliases generated are as expected */
 static int test_aliases(void)
 {
 	struct perf_pmu *pmu = NULL;
-- 
2.26.2