* [V2 PATCH] perf parse-events: Specially handle uncore event alias in small groups
@ 2018-04-25 13:50 kan.liang
2018-04-25 14:23 ` Jiri Olsa
0 siblings, 1 reply; 5+ messages in thread
From: kan.liang @ 2018-04-25 13:50 UTC (permalink / raw)
To: acme, mingo, peterz, linux-kernel
Cc: jolsa, namhyung, ganapatrao.kulkarni, zhangshaokun, yao.jin,
will.deacon, ak, agustinv, Kan Liang
From: Kan Liang <kan.liang@linux.intel.com>
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 <ak@linux.intel.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
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(-)
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 92ec009..b13f5f2 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -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;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 2fb0272..dd75092 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1219,13 +1219,16 @@ int parse_events_add_numeric(struct parse_events_state *parse_state,
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)
{
struct perf_event_attr attr;
struct perf_pmu_info info;
struct perf_pmu *pmu;
struct perf_evsel *evsel;
struct parse_events_error *err = parse_state->error;
+ bool use_uncore_alias;
LIST_HEAD(config_terms);
pmu = perf_pmu__find(name);
@@ -1244,11 +1247,14 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
memset(&attr, 0, sizeof(attr));
}
+ use_uncore_alias = (pmu->is_uncore && use_alias);
+
if (!head_config) {
attr.type = pmu->type;
evsel = __add_event(list, &parse_state->idx, &attr, NULL, pmu, NULL, auto_merge_stats);
if (evsel) {
evsel->pmu_name = name;
+ evsel->use_uncore_alias = use_uncore_alias;
return 0;
} else {
return -ENOMEM;
@@ -1282,6 +1288,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
evsel->metric_expr = info.metric_expr;
evsel->metric_name = info.metric_name;
evsel->pmu_name = name;
+ evsel->use_uncore_alias = use_uncore_alias;
}
return evsel ? 0 : -ENOMEM;
@@ -1317,7 +1324,8 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
list_add_tail(&term->list, head);
if (!parse_events_add_pmu(parse_state, list,
- pmu->name, head, true)) {
+ pmu->name, head,
+ true, true)) {
pr_debug("%s -> %s/%s/\n", str,
pmu->name, alias->str);
ok++;
@@ -1339,7 +1347,85 @@ int parse_events__modifier_group(struct list_head *list,
return parse_events__modifier_event(list, event_mod, true);
}
-void parse_events__set_leader(char *name, struct list_head *list)
+/*
+ * Check if the two uncore PMUs are from the same uncore block
+ * The format of the uncore PMU name is uncore_#blockname_#pmuidx
+ */
+static bool is_same_uncore_block(const char *pmu_name_a, const char *pmu_name_b)
+{
+ char *end_a, *end_b;
+
+ end_a = strrchr(pmu_name_a, '_');
+ end_b = strrchr(pmu_name_b, '_');
+
+ if (!end_a || !end_b)
+ return false;
+
+ if ((end_a - pmu_name_a) != (end_b - pmu_name_b))
+ return false;
+
+ return (strncmp(pmu_name_a, pmu_name_b, end_a - pmu_name_a) == 0);
+}
+
+static int
+parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
+ struct parse_events_state *parse_state)
+{
+ struct perf_evsel *evsel, *leader;
+ uintptr_t *leaders;
+ int i = 0, nr_pmu = 0, total_members, ret = 0;
+
+ leader = list_entry(list->next, struct perf_evsel, node);
+ evsel = list_entry(list->prev, struct perf_evsel, node);
+ total_members = evsel->idx - leader->idx + 1;
+
+ leaders = calloc(total_members, sizeof(uintptr_t));
+ if (!leaders)
+ return ret;
+
+ __evlist__for_each_entry(list, evsel) {
+
+ /* Only split the uncore group which members use alias */
+ if (!evsel->use_uncore_alias)
+ goto out;
+
+ /* The events must be from the same uncore block */
+ if (!is_same_uncore_block(leader->pmu_name, evsel->pmu_name))
+ goto out;
+
+ if (!strcmp(leader->name, evsel->name))
+ leaders[nr_pmu++] = (uintptr_t) evsel;
+ }
+
+ /* only one event alias */
+ if (nr_pmu == total_members) {
+ parse_state->nr_groups--;
+ goto handled;
+ }
+
+ __evlist__for_each_entry(list, evsel) {
+ if (i >= nr_pmu)
+ i = 0;
+ evsel->leader = (struct perf_evsel *) leaders[i++];
+ }
+
+ for (i = 0; i < nr_pmu; i++) {
+ evsel = (struct perf_evsel *) leaders[i];
+ evsel->nr_members = total_members / nr_pmu;
+ evsel->group_name = name ? strdup(name) : NULL;
+ }
+
+ parse_state->nr_groups += nr_pmu - 1;
+
+handled:
+ ret = 1;
+out:
+ free(leaders);
+ return ret;
+}
+
+void parse_events__set_leader(char *name, struct list_head *list,
+ struct parse_events_state *parse_state)
{
struct perf_evsel *leader;
@@ -1348,6 +1434,9 @@ void parse_events__set_leader(char *name, struct list_head *list)
return;
}
+ if (parse_events__set_leader_for_uncore_aliase(name, list, parse_state))
+ return;
+
__perf_evlist__set_leader(list);
leader = list_entry(list->next, struct perf_evsel, node);
leader->group_name = name ? strdup(name) : NULL;
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 5015cfd..4473dac 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -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,
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index d14464c..47f6399 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -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);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [V2 PATCH] perf parse-events: Specially handle uncore event alias in small groups
2018-04-25 13:50 [V2 PATCH] perf parse-events: Specially handle uncore event alias in small groups kan.liang
@ 2018-04-25 14:23 ` Jiri Olsa
2018-04-25 14:42 ` Liang, Kan
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Olsa @ 2018-04-25 14:23 UTC (permalink / raw)
To: kan.liang
Cc: acme, mingo, peterz, linux-kernel, namhyung, ganapatrao.kulkarni,
zhangshaokun, yao.jin, will.deacon, ak, agustinv
On Wed, Apr 25, 2018 at 06:50:18AM -0700, kan.liang@linux.intel.com wrote:
> From: Kan Liang <kan.liang@linux.intel.com>
>
> 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
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [V2 PATCH] perf parse-events: Specially handle uncore event alias in small groups
2018-04-25 14:23 ` Jiri Olsa
@ 2018-04-25 14:42 ` Liang, Kan
2018-04-25 14:59 ` Jiri Olsa
0 siblings, 1 reply; 5+ messages in thread
From: Liang, Kan @ 2018-04-25 14:42 UTC (permalink / raw)
To: Jiri Olsa
Cc: acme, mingo, peterz, linux-kernel, namhyung, ganapatrao.kulkarni,
zhangshaokun, yao.jin, will.deacon, ak, agustinv
On 4/25/2018 10:23 AM, Jiri Olsa wrote:
> On Wed, Apr 25, 2018 at 06:50:18AM -0700, kan.liang@linux.intel.com wrote:
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> 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
>
> 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
> [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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [V2 PATCH] perf parse-events: Specially handle uncore event alias in small groups
2018-04-25 14:42 ` Liang, Kan
@ 2018-04-25 14:59 ` Jiri Olsa
2018-04-25 15:28 ` Liang, Kan
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Olsa @ 2018-04-25 14:59 UTC (permalink / raw)
To: Liang, Kan
Cc: acme, mingo, peterz, linux-kernel, namhyung, ganapatrao.kulkarni,
zhangshaokun, yao.jin, will.deacon, ak, agustinv
On Wed, Apr 25, 2018 at 10:42:27AM -0400, Liang, Kan wrote:
>
>
> On 4/25/2018 10:23 AM, Jiri Olsa wrote:
> > On Wed, Apr 25, 2018 at 06:50:18AM -0700, kan.liang@linux.intel.com wrote:
> > > From: Kan Liang <kan.liang@linux.intel.com>
> > >
> > > 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
> >
> > 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [V2 PATCH] perf parse-events: Specially handle uncore event alias in small groups
2018-04-25 14:59 ` Jiri Olsa
@ 2018-04-25 15:28 ` Liang, Kan
0 siblings, 0 replies; 5+ messages in thread
From: Liang, Kan @ 2018-04-25 15:28 UTC (permalink / raw)
To: Jiri Olsa
Cc: acme, mingo, peterz, linux-kernel, namhyung, ganapatrao.kulkarni,
zhangshaokun, yao.jin, will.deacon, ak, agustinv
On 4/25/2018 10:59 AM, Jiri Olsa wrote:
> On Wed, Apr 25, 2018 at 10:42:27AM -0400, Liang, Kan wrote:
>>
>>
>> On 4/25/2018 10:23 AM, Jiri Olsa wrote:
>>> On Wed, Apr 25, 2018 at 06:50:18AM -0700, kan.liang@linux.intel.com wrote:
>>>> From: Kan Liang <kan.liang@linux.intel.com>
>>>>
>>>> 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
>>>
>>> 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)
>
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-04-25 15:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-25 13:50 [V2 PATCH] perf parse-events: Specially handle uncore event alias in small groups kan.liang
2018-04-25 14:23 ` Jiri Olsa
2018-04-25 14:42 ` Liang, Kan
2018-04-25 14:59 ` Jiri Olsa
2018-04-25 15:28 ` Liang, Kan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.