perf parse-events: Specially handle uncore event alias in small groups

PATCH Superseded [email protected] 2 participants 5 messages Apr 25, 2018 perf events

lore

Share & tools

lore

[email protected]

author

2018-04-25 13:51 UTC #0 lore raw

PATCH v2 1/1 perf parse-events: Specially handle uncore event alias in small groups

From: Kan Liang <[email protected]>

Perf stat doesn't count the uncore event aliases from the same uncore block in a group, for example:

perf stat -e '{unc_m_cas_count.all,unc_m_clockticks}' -a -I 1000
# time counts unit events

       1.000447342      <not counted>      unc_m_cas_count.all
       1.000447342      <not counted>      unc_m_clockticks
       2.000740654      <not counted>      unc_m_cas_count.all
       2.000740654      <not counted>      unc_m_clockticks

The output is very misleading. It gives a wrong impression that the uncore event doesn't work.

An uncore block could be composed by several PMUs. An uncore event alias is a joint name which means the same event runs on all PMUs of a block. Perf doesn't support mixed events from different PMUs in the same group. It is wrong to put uncore event aliases in a big group.

The right way is to split the big group into multiple small groups which only include the events from the same PMU.
Only uncore event aliases from the same uncore block should be specially handled here. It doesn't make sense to mix the uncore events with other uncore events from different blocks or even core events in a group.

With the patch:
# time counts unit events

     1.001557653            140,833      unc_m_cas_count.all
     1.001557653      1,330,231,332      unc_m_clockticks
     2.002709483             85,007      unc_m_cas_count.all
     2.002709483      1,429,494,563      unc_m_clockticks

Reported-by Andi Kleen <[email protected]>

Signed-off-by Kan Liang <[email protected]>

---
 tools/perf/util/evsel.h        |  1 +
 tools/perf/util/parse-events.c | 95 ++++++++++++++++++++++++++++++++++++++++--
 tools/perf/util/parse-events.h |  7 +++-
 tools/perf/util/parse-events.y |  8 ++--
 4 files changed, 102 insertions(+), 9 deletions(-)

tools/perf/util/evsel.h +1 -0

index 92ec009..b13f5f2 100644
@@ -127,6 +127,7 @@ struct perf_evsel {
 	bool			precise_max;
 	bool			ignore_missing_thread;
 	bool			forced_leader;
+	bool			use_uncore_alias;
 	/* parse modifier helper */
 	int			exclude_GH;
 	int			nr_members;

tools/perf/util/parse-events.c +92 -3

142 lines not on this page · raw message

tools/perf/util/parse-events.h +5 -2

index 5015cfd..4473dac 100644
@@ -167,7 +167,9 @@ int parse_events_add_breakpoint(struct list_head *list, int *idx,
 				void *ptr, char *type, u64 len);
 int parse_events_add_pmu(struct parse_events_state *parse_state,
 			 struct list_head *list, char *name,
-			 struct list_head *head_config, bool auto_merge_stats);
+			 struct list_head *head_config,
+			 bool auto_merge_stats,
+			 bool use_alias);
 
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			       char *str,

@@ -178,7 +180,8 @@ int parse_events_copy_term_list(struct list_head *old,
 
 enum perf_pmu_event_symbol_type
 perf_pmu__parse_check(const char *name);
-void parse_events__set_leader(char *name, struct list_head *list);
+void parse_events__set_leader(char *name, struct list_head *list,
+			      struct parse_events_state *parse_state);
 void parse_events_update_lists(struct list_head *list_event,
 			       struct list_head *list_all);
 void parse_events_evlist_error(struct parse_events_state *parse_state,

tools/perf/util/parse-events.y +4 -4

index d14464c..47f6399 100644
@@ -161,7 +161,7 @@ PE_NAME '{' events '}'
 	struct list_head *list = $3;
 
 	inc_group_count(list, _parse_state);
-	parse_events__set_leader($1, list);
+	parse_events__set_leader($1, list, _parse_state);
 	$$ = list;
 }
 |

@@ -170,7 +170,7 @@ PE_NAME '{' events '}'
 	struct list_head *list = $2;
 
 	inc_group_count(list, _parse_state);
-	parse_events__set_leader(NULL, list);
+	parse_events__set_leader(NULL, list, _parse_state);
 	$$ = list;
 }

@@ -232,7 +232,7 @@ PE_NAME '/' event_config '/'
 		YYABORT;
 
 	ALLOC_LIST(list);
-	if (parse_events_add_pmu(_parse_state, list, $1, $3, false)) {
+	if (parse_events_add_pmu(_parse_state, list, $1, $3, false, false)) {
 		struct perf_pmu *pmu = NULL;
 		int ok = 0;
 		char *pattern;

@@ -251,7 +251,7 @@ PE_NAME '/' event_config '/'
 					free(pattern);
 					YYABORT;
 				}
-				if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true))
+				if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true, false))
 					ok++;
 				parse_events_terms__delete(terms);
 			}

Signature · 1 line

2.7.4

Jiri Olsa

2018-04-25 14:23 UTC #1 lore raw

Quoted 26 lines · On Wed, Apr 25, 2018 at 06:50:18AM -0700, [email protected] wrote:

From: Kan Liang <[email protected]>
Perf stat doesn't count the uncore event aliases from the same uncore block in a group, for example:
perf stat -e '{unc_m_cas_count.all,unc_m_clockticks}' -a -I 1000
# time counts unit events
       1.000447342      <not counted>      unc_m_cas_count.all
       1.000447342      <not counted>      unc_m_clockticks
       2.000740654      <not counted>      unc_m_cas_count.all
       2.000740654      <not counted>      unc_m_clockticks
The output is very misleading. It gives a wrong impression that the uncore event doesn't work.
An uncore block could be composed by several PMUs. An uncore event alias is a joint name which means the same event runs on all PMUs of a block. Perf doesn't support mixed events from different PMUs in the same group. It is wrong to put uncore event aliases in a big group.
The right way is to split the big group into multiple small groups which only include the events from the same PMU.
Only uncore event aliases from the same uncore block should be specially handled here. It doesn't make sense to mix the uncore events with other uncore events from different blocks or even core events in a group.

Quoted 6 lines

With the patch:
# time counts unit events

     1.001557653            140,833      unc_m_cas_count.all
     1.001557653      1,330,231,332      unc_m_clockticks
     2.002709483             85,007      unc_m_cas_count.all
     2.002709483      1,429,494,563      unc_m_clockticks

hum, I still can see the original behaviour even with the patch:

[jolsa@krava perf]$ sudo ./perf stat -e '{data_reads,clockticks}' -a -I 1000
# time counts unit events

     1.000184929      <not counted> MiB  data_reads                                                  
     1.000184929    <not supported>      clockticks                                                  
     2.000552765      <not counted> MiB  data_reads                                                  
     2.000552765    <not supported>      clockticks                                                  
     3.000846298      <not counted> MiB  data_reads                                                  
     3.000846298    <not supported>      clockticks                                                  
     4.001044852      <not counted> MiB  data_reads                                                  
     4.001044852    <not supported>      clockticks                                                  
     5.001247131      <not counted> MiB  data_reads                                                  
     5.001247131    <not supported>      clockticks                                                  
^C     5.546917060      <not counted> MiB  data_reads                                                  
     5.546917060    <not supported>      clockticks

jirka

Liang, Kan

author

2018-04-25 14:42 UTC #2 lore raw

Quoted 27 lines · Jiri Olsa

Quoted 26 lines · On Wed, Apr 25, 2018 at 06:50:18AM -0700, [email protected] wrote:
From: Kan Liang <[email protected]>
Perf stat doesn't count the uncore event aliases from the same uncore block in a group, for example:
   perf stat -e '{unc_m_cas_count.all,unc_m_clockticks}' -a -I 1000
   #           time             counts unit events
        1.000447342      <not counted>      unc_m_cas_count.all
        1.000447342      <not counted>      unc_m_clockticks
        2.000740654      <not counted>      unc_m_cas_count.all
        2.000740654      <not counted>      unc_m_clockticks
The output is very misleading. It gives a wrong impression that the uncore event doesn't work.
An uncore block could be composed by several PMUs. An uncore event alias is a joint name which means the same event runs on all PMUs of a block. Perf doesn't support mixed events from different PMUs in the same group. It is wrong to put uncore event aliases in a big group.
The right way is to split the big group into multiple small groups which only include the events from the same PMU.
Only uncore event aliases from the same uncore block should be specially handled here. It doesn't make sense to mix the uncore events with other uncore events from different blocks or even core events in a group.

Quoted 9 lines

Quoted 6 lines

With the patch:

   #           time             counts unit events
      1.001557653            140,833      unc_m_cas_count.all
      1.001557653      1,330,231,332      unc_m_clockticks
      2.002709483             85,007      unc_m_cas_count.all
      2.002709483      1,429,494,563      unc_m_clockticks

hum, I still can see the original behaviour even with the patch:

That's because the data_reads and clockticks are from different uncore blocks. The data_reads is a IMC event. The clockticks is a CBOX event.

The patch only supports the events from the same uncore blocks in group.

Thanks,
Kan

Quoted 18 lines

[jolsa@krava perf]$ sudo ./perf stat -e '{data_reads,clockticks}' -a -I 1000
# time counts unit events

      1.000184929      <not counted> MiB  data_reads
      1.000184929    <not supported>      clockticks
      2.000552765      <not counted> MiB  data_reads
      2.000552765    <not supported>      clockticks
      3.000846298      <not counted> MiB  data_reads
      3.000846298    <not supported>      clockticks
      4.001044852      <not counted> MiB  data_reads
      4.001044852    <not supported>      clockticks
      5.001247131      <not counted> MiB  data_reads
      5.001247131    <not supported>      clockticks
^C     5.546917060      <not counted> MiB  data_reads
      5.546917060    <not supported>      clockticks

jirka

Jiri Olsa

2018-04-25 14:59 UTC #3 lore raw

Quoted 37 lines · Kan

Quoted 34 lines · Jiri Olsa
Quoted 32 lines · On Wed, Apr 25, 2018 at 06:50:18AM -0700, [email protected] wrote:
From: Kan Liang <[email protected]>
Perf stat doesn't count the uncore event aliases from the same uncore block in a group, for example:
   perf stat -e '{unc_m_cas_count.all,unc_m_clockticks}' -a -I 1000
   #           time             counts unit events
        1.000447342      <not counted>      unc_m_cas_count.all
        1.000447342      <not counted>      unc_m_clockticks
        2.000740654      <not counted>      unc_m_cas_count.all
        2.000740654      <not counted>      unc_m_clockticks
The output is very misleading. It gives a wrong impression that the uncore event doesn't work.
An uncore block could be composed by several PMUs. An uncore event alias is a joint name which means the same event runs on all PMUs of a block. Perf doesn't support mixed events from different PMUs in the same group. It is wrong to put uncore event aliases in a big group.
The right way is to split the big group into multiple small groups which only include the events from the same PMU.
Only uncore event aliases from the same uncore block should be specially handled here. It doesn't make sense to mix the uncore events with other uncore events from different blocks or even core events in a group.
With the patch:
   #           time             counts unit events
      1.001557653            140,833      unc_m_cas_count.all
      1.001557653      1,330,231,332      unc_m_clockticks
      2.002709483             85,007      unc_m_cas_count.all
      2.002709483      1,429,494,563      unc_m_clockticks

Quoted 7 lines

Quoted 2 lines
hum, I still can see the original behaviour even with the patch:
That's because the data_reads and clockticks are from different uncore blocks. The data_reads is a IMC event. The clockticks is a CBOX event.
The patch only supports the events from the same uncore blocks in group.

I see, found another example from same uncore blocks and it seems to work:
# perf stat -e '{cas_count_read,cas_count_write}' -a -I 1000

but I have it aborted on:

  # ./perf stat -e '{cas_count_read,cas_count_write,cas_count_read}' -a -I 1000
  perf: util/evsel.c:1483: get_group_fd: Assertion `!(!leader->fd)' failed.
  Aborted (core dumped)

jirka

Liang, Kan

author

2018-04-25 15:28 UTC #4 lore raw

Quoted 46 lines · Jiri Olsa

Quoted 37 lines · Kan
Quoted 34 lines · Jiri Olsa
Quoted 32 lines · On Wed, Apr 25, 2018 at 06:50:18AM -0700, [email protected] wrote:
From: Kan Liang <[email protected]>
Perf stat doesn't count the uncore event aliases from the same uncore block in a group, for example:
    perf stat -e '{unc_m_cas_count.all,unc_m_clockticks}' -a -I 1000
    #           time             counts unit events
         1.000447342      <not counted>      unc_m_cas_count.all
         1.000447342      <not counted>      unc_m_clockticks
         2.000740654      <not counted>      unc_m_cas_count.all
         2.000740654      <not counted>      unc_m_clockticks
The output is very misleading. It gives a wrong impression that the uncore event doesn't work.
An uncore block could be composed by several PMUs. An uncore event alias is a joint name which means the same event runs on all PMUs of a block. Perf doesn't support mixed events from different PMUs in the same group. It is wrong to put uncore event aliases in a big group.
The right way is to split the big group into multiple small groups which only include the events from the same PMU.
Only uncore event aliases from the same uncore block should be specially handled here. It doesn't make sense to mix the uncore events with other uncore events from different blocks or even core events in a group.
With the patch:
    #           time             counts unit events
       1.001557653            140,833      unc_m_cas_count.all
       1.001557653      1,330,231,332      unc_m_clockticks
       2.002709483             85,007      unc_m_cas_count.all
       2.002709483      1,429,494,563      unc_m_clockticks
Quoted 7 lines
Quoted 2 lines
hum, I still can see the original behaviour even with the patch:
That's because the data_reads and clockticks are from different uncore blocks. The data_reads is a IMC event. The clockticks is a CBOX event.
The patch only supports the events from the same uncore blocks in group.

Quoted 8 lines

I see, found another example from same uncore blocks and it seems to work:

   #  perf stat -e '{cas_count_read,cas_count_write}' -a -I 1000

but I have it aborted on:

   # ./perf stat -e '{cas_count_read,cas_count_write,cas_count_read}' -a -I 1000
   perf: util/evsel.c:1483: get_group_fd: Assertion `!(!leader->fd)' failed.
   Aborted (core dumped)

Yes, there is a bug if leader and member have the same event name. I will fix it in V3. Thanks for the test.

Thanks,
Kan