* [PATCH 1/5] perf evsel: Add evsel__clone() function
2020-09-24 12:44 [PATCHSET v5 0/5] perf stat: Expand events for each cgroup Namhyung Kim
@ 2020-09-24 12:44 ` Namhyung Kim
2020-09-24 12:44 ` [PATCH 2/5] perf stat: Add --for-each-cgroup option Namhyung Kim
` (3 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: Namhyung Kim @ 2020-09-24 12:44 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, Mark Rutland, Alexander Shishkin,
LKML, Stephane Eranian, Andi Kleen, Ian Rogers
The evsel__clone() is to create an exactly same evsel from same
attributes. The function assumes the given evsel is not configured
yet so it cares fields set during event parsing. Those fields are now
moved together as Jiri suggested. Note that metric events will be
handled by later patch.
It will be used by perf stat to generate separate events for each
cgroup.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/evsel.c | 104 ++++++++++++++++++++++++++++++++++++++++
tools/perf/util/evsel.h | 93 ++++++++++++++++++++---------------
2 files changed, 158 insertions(+), 39 deletions(-)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 2ac19a2530a2..1cad6051d8b0 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -331,6 +331,110 @@ struct evsel *evsel__new_cycles(bool precise)
goto out;
}
+static int evsel__copy_config_terms(struct evsel *dst, struct evsel *src)
+{
+ struct evsel_config_term *pos, *tmp;
+
+ list_for_each_entry(pos, &src->config_terms, list) {
+ tmp = malloc(sizeof(*tmp));
+ if (tmp == NULL)
+ return -ENOMEM;
+
+ *tmp = *pos;
+ if (tmp->free_str) {
+ tmp->val.str = strdup(pos->val.str);
+ if (tmp->val.str == NULL) {
+ free(tmp);
+ return -ENOMEM;
+ }
+ }
+ list_add_tail(&tmp->list, &dst->config_terms);
+ }
+ return 0;
+}
+
+/**
+ * evsel__clone - create a new evsel copied from @orig
+ * @orig: original evsel
+ *
+ * The assumption is that @orig is not configured nor opened yet.
+ * So we only care about the attributes that can be set while it's parsed.
+ */
+struct evsel *evsel__clone(struct evsel *orig)
+{
+ struct evsel *evsel;
+
+ BUG_ON(orig->core.fd);
+ BUG_ON(orig->counts);
+ BUG_ON(orig->priv);
+ BUG_ON(orig->per_pkg_mask);
+
+ /* cannot handle BPF objects for now */
+ if (orig->bpf_obj)
+ return NULL;
+
+ evsel = evsel__new(&orig->core.attr);
+ if (evsel == NULL)
+ return NULL;
+
+ evsel->core.cpus = perf_cpu_map__get(orig->core.cpus);
+ evsel->core.own_cpus = perf_cpu_map__get(orig->core.own_cpus);
+ evsel->core.threads = perf_thread_map__get(orig->core.threads);
+ evsel->core.nr_members = orig->core.nr_members;
+ evsel->core.system_wide = orig->core.system_wide;
+
+ if (orig->name) {
+ evsel->name = strdup(orig->name);
+ if (evsel->name == NULL)
+ goto out_err;
+ }
+ if (orig->group_name) {
+ evsel->group_name = strdup(orig->group_name);
+ if (evsel->group_name == NULL)
+ goto out_err;
+ }
+ if (orig->pmu_name) {
+ evsel->pmu_name = strdup(orig->pmu_name);
+ if (evsel->pmu_name == NULL)
+ goto out_err;
+ }
+ if (orig->filter) {
+ evsel->filter = strdup(orig->filter);
+ if (evsel->filter == NULL)
+ goto out_err;
+ }
+ evsel->cgrp = cgroup__get(orig->cgrp);
+ evsel->tp_format = orig->tp_format;
+ evsel->handler = orig->handler;
+ evsel->leader = orig->leader;
+
+ evsel->max_events = orig->max_events;
+ evsel->tool_event = orig->tool_event;
+ evsel->unit = orig->unit;
+ evsel->scale = orig->scale;
+ evsel->snapshot = orig->snapshot;
+ evsel->per_pkg = orig->per_pkg;
+ evsel->percore = orig->percore;
+ evsel->precise_max = orig->precise_max;
+ evsel->use_uncore_alias = orig->use_uncore_alias;
+ evsel->is_libpfm_event = orig->is_libpfm_event;
+
+ evsel->exclude_GH = orig->exclude_GH;
+ evsel->sample_read = orig->sample_read;
+ evsel->auto_merge_stats = orig->auto_merge_stats;
+ evsel->collect_stat = orig->collect_stat;
+ evsel->weak_group = orig->weak_group;
+
+ if (evsel__copy_config_terms(evsel, orig) < 0)
+ goto out_err;
+
+ return evsel;
+
+out_err:
+ evsel__delete(evsel);
+ return NULL;
+}
+
/*
* Returns pointer with encoded error via <linux/err.h> interface.
*/
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 35e3f6d66085..79a860d8e3ee 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -42,65 +42,79 @@ enum perf_tool_event {
*/
struct evsel {
struct perf_evsel core;
- struct evlist *evlist;
- char *filter;
+ struct evlist *evlist;
+ off_t id_offset;
+ int idx;
+ int id_pos;
+ int is_pos;
+ unsigned int sample_size;
+
+ /*
+ * These fields can be set in the parse-events code or similar.
+ * Please check evsel__clone() to copy them properly so that
+ * they can be released properly.
+ */
+ struct {
+ char *name;
+ char *group_name;
+ const char *pmu_name;
+ struct tep_event *tp_format;
+ char *filter;
+ unsigned long max_events;
+ double scale;
+ const char *unit;
+ struct cgroup *cgrp;
+ enum perf_tool_event tool_event;
+ /* parse modifier helper */
+ int exclude_GH;
+ int sample_read;
+ bool snapshot;
+ bool per_pkg;
+ bool percore;
+ bool precise_max;
+ bool use_uncore_alias;
+ bool is_libpfm_event;
+ bool auto_merge_stats;
+ bool collect_stat;
+ bool weak_group;
+ int bpf_fd;
+ struct bpf_object *bpf_obj;
+ };
+
+ /*
+ * metric fields are similar, but needs more care as they can have
+ * references to other metric (evsel).
+ */
+ const char * metric_expr;
+ const char * metric_name;
+ struct evsel **metric_events;
+ struct evsel *metric_leader;
+
+ void *handler;
struct perf_counts *counts;
struct perf_counts *prev_raw_counts;
- int idx;
- unsigned long max_events;
unsigned long nr_events_printed;
- char *name;
- double scale;
- const char *unit;
- struct tep_event *tp_format;
- off_t id_offset;
struct perf_stat_evsel *stats;
void *priv;
u64 db_id;
- struct cgroup *cgrp;
- void *handler;
- unsigned int sample_size;
- int id_pos;
- int is_pos;
- enum perf_tool_event tool_event;
bool uniquified_name;
- bool snapshot;
bool supported;
bool needs_swap;
bool disabled;
bool no_aux_samples;
bool immediate;
bool tracking;
- bool per_pkg;
- bool precise_max;
bool ignore_missing_thread;
bool forced_leader;
- bool use_uncore_alias;
- bool is_libpfm_event;
- /* parse modifier helper */
- int exclude_GH;
- int sample_read;
- unsigned long *per_pkg_mask;
- struct evsel *leader;
- char *group_name;
bool cmdline_group_boundary;
- struct list_head config_terms;
- struct bpf_object *bpf_obj;
- int bpf_fd;
- int err;
- bool auto_merge_stats;
bool merged_stat;
- const char * metric_expr;
- const char * metric_name;
- struct evsel **metric_events;
- struct evsel *metric_leader;
- bool collect_stat;
- bool weak_group;
bool reset_group;
bool errored;
- bool percore;
+ unsigned long *per_pkg_mask;
+ struct evsel *leader;
+ struct list_head config_terms;
+ int err;
int cpu_iter;
- const char *pmu_name;
struct {
evsel__sb_cb_t *cb;
void *data;
@@ -169,6 +183,7 @@ static inline struct evsel *evsel__new(struct perf_event_attr *attr)
return evsel__new_idx(attr, 0);
}
+struct evsel *evsel__clone(struct evsel *orig);
struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx);
/*
--
2.28.0.681.g6f77f65b4e-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 2/5] perf stat: Add --for-each-cgroup option
2020-09-24 12:44 [PATCHSET v5 0/5] perf stat: Expand events for each cgroup Namhyung Kim
2020-09-24 12:44 ` [PATCH 1/5] perf evsel: Add evsel__clone() function Namhyung Kim
@ 2020-09-24 12:44 ` Namhyung Kim
2020-09-24 12:44 ` [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups Namhyung Kim
` (2 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: Namhyung Kim @ 2020-09-24 12:44 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, Mark Rutland, Alexander Shishkin,
LKML, Stephane Eranian, Andi Kleen, Ian Rogers
The --for-each-cgroup option is a syntax sugar to monitor large number
of cgroups easily. Current command line requires to list all the
events and cgroups even if users want to monitor same events for each
cgroup. This patch addresses that usage by copying given events for
each cgroup on user's behalf.
For instance, if they want to monitor 6 events for 200 cgroups each
they should write 1200 event names (with -e) AND 1200 cgroup names
(with -G) on the command line. But with this change, they can just
specify 6 events and 200 cgroups with a new option.
A simpler example below: It wants to measure 3 events for 2 cgroups
('A' and 'B'). The result is that total 6 events are counted like
below.
$ ./perf stat -a -e cpu-clock,cycles,instructions --for-each-cgroup A,B sleep 1
Performance counter stats for 'system wide':
988.18 msec cpu-clock A # 0.987 CPUs utilized
3,153,761,702 cycles A # 3.200 GHz (100.00%)
8,067,769,847 instructions A # 2.57 insn per cycle (100.00%)
982.71 msec cpu-clock B # 0.982 CPUs utilized
3,136,093,298 cycles B # 3.182 GHz (99.99%)
8,109,619,327 instructions B # 2.58 insn per cycle (99.99%)
1.001228054 seconds time elapsed
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/Documentation/perf-stat.txt | 5 ++
tools/perf/builtin-stat.c | 27 ++++++++-
tools/perf/util/cgroup.c | 79 ++++++++++++++++++++++++++
tools/perf/util/cgroup.h | 1 +
tools/perf/util/stat.h | 1 +
5 files changed, 112 insertions(+), 1 deletion(-)
diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
index 5bf3d7ae4660..9f9f29025e49 100644
--- a/tools/perf/Documentation/perf-stat.txt
+++ b/tools/perf/Documentation/perf-stat.txt
@@ -166,6 +166,11 @@ use '-e e1 -e e2 -G foo,foo' or just use '-e e1 -e e2 -G foo'.
If wanting to monitor, say, 'cycles' for a cgroup and also for system wide, this
command line can be used: 'perf stat -e cycles -G cgroup_name -a -e cycles'.
+--for-each-cgroup name::
+Expand event list for each cgroup in "name" (allow multiple cgroups separated
+by comma). This has same effect that repeating -e option and -G option for
+each event x name. This option cannot be used with -G/--cgroup option.
+
-o file::
--output file::
Print the output into the designated file.
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index cbb2d977eec7..0c9e21a29795 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1060,6 +1060,17 @@ static int parse_control_option(const struct option *opt,
return evlist__parse_control(str, &config->ctl_fd, &config->ctl_fd_ack, &config->ctl_fd_close);
}
+static int parse_stat_cgroups(const struct option *opt,
+ const char *str, int unset)
+{
+ if (stat_config.cgroup_list) {
+ pr_err("--cgroup and --for-each-cgroup cannot be used together\n");
+ return -1;
+ }
+
+ return parse_cgroups(opt, str, unset);
+}
+
static struct option stat_options[] = {
OPT_BOOLEAN('T', "transaction", &transaction_run,
"hardware transaction statistics"),
@@ -1103,7 +1114,9 @@ static struct option stat_options[] = {
OPT_STRING('x', "field-separator", &stat_config.csv_sep, "separator",
"print counts with custom separator"),
OPT_CALLBACK('G', "cgroup", &evsel_list, "name",
- "monitor event in cgroup name only", parse_cgroups),
+ "monitor event in cgroup name only", parse_stat_cgroups),
+ OPT_STRING(0, "for-each-cgroup", &stat_config.cgroup_list, "name",
+ "expand events for each cgroup"),
OPT_STRING('o', "output", &output_name, "file", "output file name"),
OPT_BOOLEAN(0, "append", &append_file, "append to the output file"),
OPT_INTEGER(0, "log-fd", &output_fd,
@@ -2213,6 +2226,18 @@ int cmd_stat(int argc, const char **argv)
if (add_default_attributes())
goto out;
+ if (stat_config.cgroup_list) {
+ if (nr_cgroups > 0) {
+ pr_err("--cgroup and --for-each-cgroup cannot be used together\n");
+ parse_options_usage(stat_usage, stat_options, "G", 1);
+ parse_options_usage(NULL, stat_options, "for-each-cgroup", 0);
+ goto out;
+ }
+
+ if (evlist__expand_cgroup(evsel_list, stat_config.cgroup_list) < 0)
+ goto out;
+ }
+
target__validate(&target);
if ((stat_config.aggr_mode == AGGR_THREAD) && (target.system_wide))
diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
index 050dea9f1e88..8b6a4fa49082 100644
--- a/tools/perf/util/cgroup.c
+++ b/tools/perf/util/cgroup.c
@@ -193,6 +193,85 @@ int parse_cgroups(const struct option *opt, const char *str,
return 0;
}
+int evlist__expand_cgroup(struct evlist *evlist, const char *str)
+{
+ struct evlist *orig_list, *tmp_list;
+ struct evsel *pos, *evsel, *leader;
+ struct cgroup *cgrp = NULL;
+ const char *p, *e, *eos = str + strlen(str);
+ int ret = -1;
+
+ if (evlist->core.nr_entries == 0) {
+ fprintf(stderr, "must define events before cgroups\n");
+ return -EINVAL;
+ }
+
+ orig_list = evlist__new();
+ tmp_list = evlist__new();
+ if (orig_list == NULL || tmp_list == NULL) {
+ fprintf(stderr, "memory allocation failed\n");
+ return -ENOMEM;
+ }
+
+ /* save original events and init evlist */
+ perf_evlist__splice_list_tail(orig_list, &evlist->core.entries);
+ evlist->core.nr_entries = 0;
+
+ for (;;) {
+ p = strchr(str, ',');
+ e = p ? p : eos;
+
+ /* allow empty cgroups, i.e., skip */
+ if (e - str) {
+ /* termination added */
+ char *name = strndup(str, e - str);
+ if (!name)
+ goto out_err;
+
+ cgrp = cgroup__new(name);
+ free(name);
+ if (cgrp == NULL)
+ goto out_err;
+ } else {
+ cgrp = NULL;
+ }
+
+ leader = NULL;
+ evlist__for_each_entry(orig_list, pos) {
+ evsel = evsel__clone(pos);
+ if (evsel == NULL)
+ goto out_err;
+
+ cgroup__put(evsel->cgrp);
+ evsel->cgrp = cgroup__get(cgrp);
+
+ if (evsel__is_group_leader(pos))
+ leader = evsel;
+ evsel->leader = leader;
+
+ evlist__add(tmp_list, evsel);
+ }
+ /* cgroup__new() has a refcount, release it here */
+ cgroup__put(cgrp);
+ nr_cgroups++;
+
+ perf_evlist__splice_list_tail(evlist, &tmp_list->core.entries);
+ tmp_list->core.nr_entries = 0;
+
+ if (!p) {
+ ret = 0;
+ break;
+ }
+ str = p+1;
+ }
+
+out_err:
+ evlist__delete(orig_list);
+ evlist__delete(tmp_list);
+
+ return ret;
+}
+
static struct cgroup *__cgroup__findnew(struct rb_root *root, uint64_t id,
bool create, const char *path)
{
diff --git a/tools/perf/util/cgroup.h b/tools/perf/util/cgroup.h
index e98d5975fe55..32893018296f 100644
--- a/tools/perf/util/cgroup.h
+++ b/tools/perf/util/cgroup.h
@@ -24,6 +24,7 @@ void cgroup__put(struct cgroup *cgroup);
struct evlist;
struct cgroup *evlist__findnew_cgroup(struct evlist *evlist, const char *name);
+int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups);
void evlist__set_default_cgroup(struct evlist *evlist, struct cgroup *cgroup);
diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
index f36c8c92efa0..487010c624be 100644
--- a/tools/perf/util/stat.h
+++ b/tools/perf/util/stat.h
@@ -145,6 +145,7 @@ struct perf_stat_config {
int ctl_fd;
int ctl_fd_ack;
bool ctl_fd_close;
+ const char *cgroup_list;
};
void perf_stat__set_big_num(int set);
--
2.28.0.681.g6f77f65b4e-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups
2020-09-24 12:44 [PATCHSET v5 0/5] perf stat: Expand events for each cgroup Namhyung Kim
2020-09-24 12:44 ` [PATCH 1/5] perf evsel: Add evsel__clone() function Namhyung Kim
2020-09-24 12:44 ` [PATCH 2/5] perf stat: Add --for-each-cgroup option Namhyung Kim
@ 2020-09-24 12:44 ` Namhyung Kim
2020-09-25 13:26 ` Jiri Olsa
2020-09-28 12:17 ` Arnaldo Carvalho de Melo
2020-09-24 12:44 ` [PATCH 4/5] perf tools: Allow creation of cgroup without open Namhyung Kim
2020-09-24 12:44 ` [PATCH 5/5] perf test: Add expand cgroup event test Namhyung Kim
4 siblings, 2 replies; 18+ messages in thread
From: Namhyung Kim @ 2020-09-24 12:44 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, Mark Rutland, Alexander Shishkin,
LKML, Stephane Eranian, Andi Kleen, Ian Rogers, John Garry,
Kajol Jain, kernel test robot
The metricgroup__copy_metric_events() is to handle metrics events when
expanding event for cgroups. As the metric events keep pointers to
evsel, it should be refreshed when events are cloned during the
operation.
The perf_stat__collect_metric_expr() is also called in case an event
has a metric directly.
During the copy, it references evsel by index as the evlist now has
cloned evsels for the given cgroup.
Also kernel test robot found an issue in the python module import
so add empty implementations of those two functions to fix it.
Cc: John Garry <john.garry@huawei.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Ian Rogers <irogers@google.com>
Reported-by: kernel test robot <rong.a.chen@intel.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/builtin-stat.c | 3 +-
tools/perf/util/cgroup.c | 23 +++++++++-
tools/perf/util/cgroup.h | 4 +-
tools/perf/util/evlist.c | 11 +++++
tools/perf/util/evlist.h | 1 +
tools/perf/util/metricgroup.c | 85 +++++++++++++++++++++++++++++++++++
tools/perf/util/metricgroup.h | 6 +++
tools/perf/util/python.c | 19 ++++++++
8 files changed, 149 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 0c9e21a29795..2751699a8969 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2234,7 +2234,8 @@ int cmd_stat(int argc, const char **argv)
goto out;
}
- if (evlist__expand_cgroup(evsel_list, stat_config.cgroup_list) < 0)
+ if (evlist__expand_cgroup(evsel_list, stat_config.cgroup_list,
+ &stat_config.metric_events) < 0)
goto out;
}
diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
index 8b6a4fa49082..6e64c908fa71 100644
--- a/tools/perf/util/cgroup.c
+++ b/tools/perf/util/cgroup.c
@@ -3,6 +3,9 @@
#include "evsel.h"
#include "cgroup.h"
#include "evlist.h"
+#include "rblist.h"
+#include "metricgroup.h"
+#include "stat.h"
#include <linux/zalloc.h>
#include <sys/types.h>
#include <sys/stat.h>
@@ -193,10 +196,12 @@ int parse_cgroups(const struct option *opt, const char *str,
return 0;
}
-int evlist__expand_cgroup(struct evlist *evlist, const char *str)
+int evlist__expand_cgroup(struct evlist *evlist, const char *str,
+ struct rblist *metric_events)
{
struct evlist *orig_list, *tmp_list;
struct evsel *pos, *evsel, *leader;
+ struct rblist orig_metric_events;
struct cgroup *cgrp = NULL;
const char *p, *e, *eos = str + strlen(str);
int ret = -1;
@@ -217,6 +222,13 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
perf_evlist__splice_list_tail(orig_list, &evlist->core.entries);
evlist->core.nr_entries = 0;
+ if (metric_events) {
+ orig_metric_events = *metric_events;
+ rblist__init(metric_events);
+ } else {
+ rblist__init(&orig_metric_events);
+ }
+
for (;;) {
p = strchr(str, ',');
e = p ? p : eos;
@@ -255,6 +267,14 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
cgroup__put(cgrp);
nr_cgroups++;
+ if (metric_events) {
+ perf_stat__collect_metric_expr(tmp_list);
+ if (metricgroup__copy_metric_events(tmp_list, cgrp,
+ metric_events,
+ &orig_metric_events) < 0)
+ break;
+ }
+
perf_evlist__splice_list_tail(evlist, &tmp_list->core.entries);
tmp_list->core.nr_entries = 0;
@@ -268,6 +288,7 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
out_err:
evlist__delete(orig_list);
evlist__delete(tmp_list);
+ rblist__exit(&orig_metric_events);
return ret;
}
diff --git a/tools/perf/util/cgroup.h b/tools/perf/util/cgroup.h
index 32893018296f..eea6df8ee373 100644
--- a/tools/perf/util/cgroup.h
+++ b/tools/perf/util/cgroup.h
@@ -22,9 +22,11 @@ struct cgroup *cgroup__get(struct cgroup *cgroup);
void cgroup__put(struct cgroup *cgroup);
struct evlist;
+struct rblist;
struct cgroup *evlist__findnew_cgroup(struct evlist *evlist, const char *name);
-int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups);
+int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups,
+ struct rblist *metric_events);
void evlist__set_default_cgroup(struct evlist *evlist, struct cgroup *cgroup);
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index e971daf946d0..8bdf3d2c907c 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1969,3 +1969,14 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
return err;
}
+
+struct evsel *evlist__find_evsel(struct evlist *evlist, int idx)
+{
+ struct evsel *evsel;
+
+ evlist__for_each_entry(evlist, evsel) {
+ if (evsel->idx == idx)
+ return evsel;
+ }
+ return NULL;
+}
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index bc38a53f6a1a..e1a450322bc5 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -386,4 +386,5 @@ int evlist__ctlfd_ack(struct evlist *evlist);
#define EVLIST_ENABLED_MSG "Events enabled\n"
#define EVLIST_DISABLED_MSG "Events disabled\n"
+struct evsel *evlist__find_evsel(struct evlist *evlist, int idx);
#endif /* __PERF_EVLIST_H */
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 941702cb6a79..55ea514ac0ce 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -24,6 +24,7 @@
#include <api/fs/fs.h>
#include "util.h"
#include <asm/bug.h>
+#include "cgroup.h"
struct metric_event *metricgroup__lookup(struct rblist *metric_events,
struct evsel *evsel,
@@ -1120,3 +1121,87 @@ bool metricgroup__has_metric(const char *metric)
}
return false;
}
+
+int metricgroup__copy_metric_events(struct evlist *evlist, struct cgroup *cgrp,
+ struct rblist *new_metric_events,
+ struct rblist *old_metric_events)
+{
+ unsigned i;
+
+ for (i = 0; i < rblist__nr_entries(old_metric_events); i++) {
+ struct rb_node *nd;
+ struct metric_event *old_me, *new_me;
+ struct metric_expr *old_expr, *new_expr;
+ struct evsel *evsel;
+ size_t alloc_size;
+ int idx, nr;
+
+ nd = rblist__entry(old_metric_events, i);
+ old_me = container_of(nd, struct metric_event, nd);
+
+ evsel = evlist__find_evsel(evlist, old_me->evsel->idx);
+ if (!evsel)
+ return -EINVAL;
+ new_me = metricgroup__lookup(new_metric_events, evsel, true);
+ if (!new_me)
+ return -ENOMEM;
+
+ pr_debug("copying metric event for cgroup '%s': %s (idx=%d)\n",
+ cgrp ? cgrp->name : "root", evsel->name, evsel->idx);
+
+ list_for_each_entry(old_expr, &old_me->head, nd) {
+ new_expr = malloc(sizeof(*new_expr));
+ if (!new_expr)
+ return -ENOMEM;
+
+ new_expr->metric_expr = old_expr->metric_expr;
+ new_expr->metric_name = old_expr->metric_name;
+ new_expr->metric_unit = old_expr->metric_unit;
+ new_expr->runtime = old_expr->runtime;
+
+ if (old_expr->metric_refs) {
+ /* calculate number of metric_events */
+ for (nr = 0; old_expr->metric_refs[nr].metric_name; nr++)
+ continue;
+ alloc_size = sizeof(*new_expr->metric_refs);
+ new_expr->metric_refs = calloc(nr + 1, alloc_size);
+ if (!new_expr->metric_refs) {
+ free(new_expr);
+ return -ENOMEM;
+ }
+
+ memcpy(new_expr->metric_refs, old_expr->metric_refs,
+ nr * alloc_size);
+ } else {
+ new_expr->metric_refs = NULL;
+ }
+
+ /* calculate number of metric_events */
+ for (nr = 0; old_expr->metric_events[nr]; nr++)
+ continue;
+ alloc_size = sizeof(*new_expr->metric_events);
+ new_expr->metric_events = calloc(nr + 1, alloc_size);
+ if (!new_expr->metric_events) {
+ free(new_expr->metric_refs);
+ free(new_expr);
+ return -ENOMEM;
+ }
+
+ /* copy evsel in the same position */
+ for (idx = 0; idx < nr; idx++) {
+ evsel = old_expr->metric_events[idx];
+ evsel = evlist__find_evsel(evlist, evsel->idx);
+ if (evsel == NULL) {
+ free(new_expr->metric_events);
+ free(new_expr->metric_refs);
+ free(new_expr);
+ return -EINVAL;
+ }
+ new_expr->metric_events[idx] = evsel;
+ }
+
+ list_add(&new_expr->nd, &new_me->head);
+ }
+ }
+ return 0;
+}
diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
index 491a5d78252d..ed1b9392e624 100644
--- a/tools/perf/util/metricgroup.h
+++ b/tools/perf/util/metricgroup.h
@@ -7,11 +7,13 @@
#include <stdbool.h>
#include "pmu-events/pmu-events.h"
+struct evlist;
struct evsel;
struct evlist;
struct option;
struct rblist;
struct pmu_events_map;
+struct cgroup;
struct metric_event {
struct rb_node nd;
@@ -55,4 +57,8 @@ void metricgroup__print(bool metrics, bool groups, char *filter,
bool metricgroup__has_metric(const char *metric);
int arch_get_runtimeparam(struct pmu_event *pe __maybe_unused);
void metricgroup__rblist_exit(struct rblist *metric_events);
+
+int metricgroup__copy_metric_events(struct evlist *evlist, struct cgroup *cgrp,
+ struct rblist *new_metric_events,
+ struct rblist *old_metric_events);
#endif
diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 74f85948d101..ae8edde7c50e 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -15,6 +15,8 @@
#include "thread_map.h"
#include "trace-event.h"
#include "mmap.h"
+#include "stat.h"
+#include "metricgroup.h"
#include "util/env.h"
#include <internal/lib.h>
#include "util.h"
@@ -60,6 +62,23 @@ int parse_callchain_record(const char *arg __maybe_unused,
*/
struct perf_env perf_env;
+/*
+ * Add this one here not to drag util/stat-shadow.c
+ */
+void perf_stat__collect_metric_expr(struct evlist *evsel_list)
+{
+}
+
+/*
+ * Add this one here not to drag util/metricgroup.c
+ */
+int metricgroup__copy_metric_events(struct evlist *evlist, struct cgroup *cgrp,
+ struct rblist *new_metric_events,
+ struct rblist *old_metric_events)
+{
+ return 0;
+}
+
/*
* Support debug printing even though util/debug.c is not linked. That means
* implementing 'verbose' and 'eprintf'.
--
2.28.0.681.g6f77f65b4e-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups
2020-09-24 12:44 ` [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups Namhyung Kim
@ 2020-09-25 13:26 ` Jiri Olsa
2020-09-25 13:44 ` Namhyung Kim
2020-09-28 12:17 ` Arnaldo Carvalho de Melo
1 sibling, 1 reply; 18+ messages in thread
From: Jiri Olsa @ 2020-09-25 13:26 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
Mark Rutland, Alexander Shishkin, LKML, Stephane Eranian,
Andi Kleen, Ian Rogers, John Garry, Kajol Jain,
kernel test robot
On Thu, Sep 24, 2020 at 09:44:53PM +0900, Namhyung Kim wrote:
SNIP
>
> + if (metric_events) {
> + orig_metric_events = *metric_events;
> + rblist__init(metric_events);
> + } else {
> + rblist__init(&orig_metric_events);
> + }
> +
> for (;;) {
> p = strchr(str, ',');
> e = p ? p : eos;
> @@ -255,6 +267,14 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> cgroup__put(cgrp);
> nr_cgroups++;
>
> + if (metric_events) {
> + perf_stat__collect_metric_expr(tmp_list);
> + if (metricgroup__copy_metric_events(tmp_list, cgrp,
> + metric_events,
> + &orig_metric_events) < 0)
> + break;
> + }
looks good, do you plan to actualy add support for record?
my ack from last version stays
thanks,
jirka
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups
2020-09-25 13:26 ` Jiri Olsa
@ 2020-09-25 13:44 ` Namhyung Kim
2020-09-25 13:51 ` Jiri Olsa
0 siblings, 1 reply; 18+ messages in thread
From: Namhyung Kim @ 2020-09-25 13:44 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
Mark Rutland, Alexander Shishkin, LKML, Stephane Eranian,
Andi Kleen, Ian Rogers, John Garry, Kajol Jain,
kernel test robot
On Fri, Sep 25, 2020 at 10:26 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Sep 24, 2020 at 09:44:53PM +0900, Namhyung Kim wrote:
>
> SNIP
>
> >
> > + if (metric_events) {
> > + orig_metric_events = *metric_events;
> > + rblist__init(metric_events);
> > + } else {
> > + rblist__init(&orig_metric_events);
> > + }
> > +
> > for (;;) {
> > p = strchr(str, ',');
> > e = p ? p : eos;
> > @@ -255,6 +267,14 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> > cgroup__put(cgrp);
> > nr_cgroups++;
> >
> > + if (metric_events) {
> > + perf_stat__collect_metric_expr(tmp_list);
> > + if (metricgroup__copy_metric_events(tmp_list, cgrp,
> > + metric_events,
> > + &orig_metric_events) < 0)
> > + break;
> > + }
>
> looks good, do you plan to actualy add support for record?
No actually, I still think perf record should use --all-cgroups.
> my ack from last version stays
Thanks! But I didn't see your ack for this patch set.
(I've only seen it for the perf inject patchset..)
Thanks
Namhyung
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups
2020-09-25 13:44 ` Namhyung Kim
@ 2020-09-25 13:51 ` Jiri Olsa
2020-09-28 11:54 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 18+ messages in thread
From: Jiri Olsa @ 2020-09-25 13:51 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
Mark Rutland, Alexander Shishkin, LKML, Stephane Eranian,
Andi Kleen, Ian Rogers, John Garry, Kajol Jain,
kernel test robot
On Fri, Sep 25, 2020 at 10:44:53PM +0900, Namhyung Kim wrote:
> On Fri, Sep 25, 2020 at 10:26 PM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Thu, Sep 24, 2020 at 09:44:53PM +0900, Namhyung Kim wrote:
> >
> > SNIP
> >
> > >
> > > + if (metric_events) {
> > > + orig_metric_events = *metric_events;
> > > + rblist__init(metric_events);
> > > + } else {
> > > + rblist__init(&orig_metric_events);
> > > + }
> > > +
> > > for (;;) {
> > > p = strchr(str, ',');
> > > e = p ? p : eos;
> > > @@ -255,6 +267,14 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> > > cgroup__put(cgrp);
> > > nr_cgroups++;
> > >
> > > + if (metric_events) {
> > > + perf_stat__collect_metric_expr(tmp_list);
> > > + if (metricgroup__copy_metric_events(tmp_list, cgrp,
> > > + metric_events,
> > > + &orig_metric_events) < 0)
> > > + break;
> > > + }
> >
> > looks good, do you plan to actualy add support for record?
>
> No actually, I still think perf record should use --all-cgroups.
>
> > my ack from last version stays
>
> Thanks! But I didn't see your ack for this patch set.
> (I've only seen it for the perf inject patchset..)
ah that was for the build id inject speed up.. too many
patchsets flying around ;-)
Acked-by: Jiri Olsa <jolsa@redhat.com>
thanks,
jirka
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups
2020-09-25 13:51 ` Jiri Olsa
@ 2020-09-28 11:54 ` Arnaldo Carvalho de Melo
2020-09-28 12:36 ` Jiri Olsa
0 siblings, 1 reply; 18+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-09-28 11:54 UTC (permalink / raw)
To: Jiri Olsa
Cc: Namhyung Kim, Ingo Molnar, Peter Zijlstra, Mark Rutland,
Alexander Shishkin, LKML, Stephane Eranian, Andi Kleen,
Ian Rogers, John Garry, Kajol Jain, kernel test robot
Em Fri, Sep 25, 2020 at 03:51:33PM +0200, Jiri Olsa escreveu:
> On Fri, Sep 25, 2020 at 10:44:53PM +0900, Namhyung Kim wrote:
> > On Fri, Sep 25, 2020 at 10:26 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > > On Thu, Sep 24, 2020 at 09:44:53PM +0900, Namhyung Kim wrote:
> > No actually, I still think perf record should use --all-cgroups.
> > > my ack from last version stays
> > Thanks! But I didn't see your ack for this patch set.
> > (I've only seen it for the perf inject patchset..)
> ah that was for the build id inject speed up.. too many
> patchsets flying around ;-)
> Acked-by: Jiri Olsa <jolsa@redhat.com>
I take this is for the entire patchset, right?
- Arnaldo
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups
2020-09-28 11:54 ` Arnaldo Carvalho de Melo
@ 2020-09-28 12:36 ` Jiri Olsa
0 siblings, 0 replies; 18+ messages in thread
From: Jiri Olsa @ 2020-09-28 12:36 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Namhyung Kim, Ingo Molnar, Peter Zijlstra, Mark Rutland,
Alexander Shishkin, LKML, Stephane Eranian, Andi Kleen,
Ian Rogers, John Garry, Kajol Jain, kernel test robot
On Mon, Sep 28, 2020 at 08:54:04AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Sep 25, 2020 at 03:51:33PM +0200, Jiri Olsa escreveu:
> > On Fri, Sep 25, 2020 at 10:44:53PM +0900, Namhyung Kim wrote:
> > > On Fri, Sep 25, 2020 at 10:26 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > > > On Thu, Sep 24, 2020 at 09:44:53PM +0900, Namhyung Kim wrote:
> > > No actually, I still think perf record should use --all-cgroups.
>
> > > > my ack from last version stays
>
> > > Thanks! But I didn't see your ack for this patch set.
> > > (I've only seen it for the perf inject patchset..)
>
> > ah that was for the build id inject speed up.. too many
> > patchsets flying around ;-)
>
> > Acked-by: Jiri Olsa <jolsa@redhat.com>
>
> I take this is for the entire patchset, right?
yes
jirka
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups
2020-09-24 12:44 ` [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups Namhyung Kim
2020-09-25 13:26 ` Jiri Olsa
@ 2020-09-28 12:17 ` Arnaldo Carvalho de Melo
1 sibling, 0 replies; 18+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-09-28 12:17 UTC (permalink / raw)
To: Namhyung Kim
Cc: Jiri Olsa, Ingo Molnar, Peter Zijlstra, Mark Rutland,
Alexander Shishkin, LKML, Stephane Eranian, Andi Kleen,
Ian Rogers, John Garry, Kajol Jain, kernel test robot
Em Thu, Sep 24, 2020 at 09:44:53PM +0900, Namhyung Kim escreveu:
> The metricgroup__copy_metric_events() is to handle metrics events when
> expanding event for cgroups. As the metric events keep pointers to
> evsel, it should be refreshed when events are cloned during the
> operation.
>
> The perf_stat__collect_metric_expr() is also called in case an event
> has a metric directly.
>
> During the copy, it references evsel by index as the evlist now has
> cloned evsels for the given cgroup.
>
> Also kernel test robot found an issue in the python module import
> so add empty implementations of those two functions to fix it.
So, this is the alias I have to building perf:
alias m='perf stat -e cycles:u,instructions:u make -k O=/tmp/build/perf -C tools/perf install-bin ; perf test python'
:-)
Thanks, applying the whole series.
- Arnaldo
> Cc: John Garry <john.garry@huawei.com>
> Cc: Kajol Jain <kjain@linux.ibm.com>
> Cc: Ian Rogers <irogers@google.com>
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> tools/perf/builtin-stat.c | 3 +-
> tools/perf/util/cgroup.c | 23 +++++++++-
> tools/perf/util/cgroup.h | 4 +-
> tools/perf/util/evlist.c | 11 +++++
> tools/perf/util/evlist.h | 1 +
> tools/perf/util/metricgroup.c | 85 +++++++++++++++++++++++++++++++++++
> tools/perf/util/metricgroup.h | 6 +++
> tools/perf/util/python.c | 19 ++++++++
> 8 files changed, 149 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 0c9e21a29795..2751699a8969 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -2234,7 +2234,8 @@ int cmd_stat(int argc, const char **argv)
> goto out;
> }
>
> - if (evlist__expand_cgroup(evsel_list, stat_config.cgroup_list) < 0)
> + if (evlist__expand_cgroup(evsel_list, stat_config.cgroup_list,
> + &stat_config.metric_events) < 0)
> goto out;
> }
>
> diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
> index 8b6a4fa49082..6e64c908fa71 100644
> --- a/tools/perf/util/cgroup.c
> +++ b/tools/perf/util/cgroup.c
> @@ -3,6 +3,9 @@
> #include "evsel.h"
> #include "cgroup.h"
> #include "evlist.h"
> +#include "rblist.h"
> +#include "metricgroup.h"
> +#include "stat.h"
> #include <linux/zalloc.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> @@ -193,10 +196,12 @@ int parse_cgroups(const struct option *opt, const char *str,
> return 0;
> }
>
> -int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> +int evlist__expand_cgroup(struct evlist *evlist, const char *str,
> + struct rblist *metric_events)
> {
> struct evlist *orig_list, *tmp_list;
> struct evsel *pos, *evsel, *leader;
> + struct rblist orig_metric_events;
> struct cgroup *cgrp = NULL;
> const char *p, *e, *eos = str + strlen(str);
> int ret = -1;
> @@ -217,6 +222,13 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> perf_evlist__splice_list_tail(orig_list, &evlist->core.entries);
> evlist->core.nr_entries = 0;
>
> + if (metric_events) {
> + orig_metric_events = *metric_events;
> + rblist__init(metric_events);
> + } else {
> + rblist__init(&orig_metric_events);
> + }
> +
> for (;;) {
> p = strchr(str, ',');
> e = p ? p : eos;
> @@ -255,6 +267,14 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> cgroup__put(cgrp);
> nr_cgroups++;
>
> + if (metric_events) {
> + perf_stat__collect_metric_expr(tmp_list);
> + if (metricgroup__copy_metric_events(tmp_list, cgrp,
> + metric_events,
> + &orig_metric_events) < 0)
> + break;
> + }
> +
> perf_evlist__splice_list_tail(evlist, &tmp_list->core.entries);
> tmp_list->core.nr_entries = 0;
>
> @@ -268,6 +288,7 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> out_err:
> evlist__delete(orig_list);
> evlist__delete(tmp_list);
> + rblist__exit(&orig_metric_events);
>
> return ret;
> }
> diff --git a/tools/perf/util/cgroup.h b/tools/perf/util/cgroup.h
> index 32893018296f..eea6df8ee373 100644
> --- a/tools/perf/util/cgroup.h
> +++ b/tools/perf/util/cgroup.h
> @@ -22,9 +22,11 @@ struct cgroup *cgroup__get(struct cgroup *cgroup);
> void cgroup__put(struct cgroup *cgroup);
>
> struct evlist;
> +struct rblist;
>
> struct cgroup *evlist__findnew_cgroup(struct evlist *evlist, const char *name);
> -int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups);
> +int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups,
> + struct rblist *metric_events);
>
> void evlist__set_default_cgroup(struct evlist *evlist, struct cgroup *cgroup);
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index e971daf946d0..8bdf3d2c907c 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1969,3 +1969,14 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
>
> return err;
> }
> +
> +struct evsel *evlist__find_evsel(struct evlist *evlist, int idx)
> +{
> + struct evsel *evsel;
> +
> + evlist__for_each_entry(evlist, evsel) {
> + if (evsel->idx == idx)
> + return evsel;
> + }
> + return NULL;
> +}
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index bc38a53f6a1a..e1a450322bc5 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -386,4 +386,5 @@ int evlist__ctlfd_ack(struct evlist *evlist);
> #define EVLIST_ENABLED_MSG "Events enabled\n"
> #define EVLIST_DISABLED_MSG "Events disabled\n"
>
> +struct evsel *evlist__find_evsel(struct evlist *evlist, int idx);
> #endif /* __PERF_EVLIST_H */
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 941702cb6a79..55ea514ac0ce 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -24,6 +24,7 @@
> #include <api/fs/fs.h>
> #include "util.h"
> #include <asm/bug.h>
> +#include "cgroup.h"
>
> struct metric_event *metricgroup__lookup(struct rblist *metric_events,
> struct evsel *evsel,
> @@ -1120,3 +1121,87 @@ bool metricgroup__has_metric(const char *metric)
> }
> return false;
> }
> +
> +int metricgroup__copy_metric_events(struct evlist *evlist, struct cgroup *cgrp,
> + struct rblist *new_metric_events,
> + struct rblist *old_metric_events)
> +{
> + unsigned i;
> +
> + for (i = 0; i < rblist__nr_entries(old_metric_events); i++) {
> + struct rb_node *nd;
> + struct metric_event *old_me, *new_me;
> + struct metric_expr *old_expr, *new_expr;
> + struct evsel *evsel;
> + size_t alloc_size;
> + int idx, nr;
> +
> + nd = rblist__entry(old_metric_events, i);
> + old_me = container_of(nd, struct metric_event, nd);
> +
> + evsel = evlist__find_evsel(evlist, old_me->evsel->idx);
> + if (!evsel)
> + return -EINVAL;
> + new_me = metricgroup__lookup(new_metric_events, evsel, true);
> + if (!new_me)
> + return -ENOMEM;
> +
> + pr_debug("copying metric event for cgroup '%s': %s (idx=%d)\n",
> + cgrp ? cgrp->name : "root", evsel->name, evsel->idx);
> +
> + list_for_each_entry(old_expr, &old_me->head, nd) {
> + new_expr = malloc(sizeof(*new_expr));
> + if (!new_expr)
> + return -ENOMEM;
> +
> + new_expr->metric_expr = old_expr->metric_expr;
> + new_expr->metric_name = old_expr->metric_name;
> + new_expr->metric_unit = old_expr->metric_unit;
> + new_expr->runtime = old_expr->runtime;
> +
> + if (old_expr->metric_refs) {
> + /* calculate number of metric_events */
> + for (nr = 0; old_expr->metric_refs[nr].metric_name; nr++)
> + continue;
> + alloc_size = sizeof(*new_expr->metric_refs);
> + new_expr->metric_refs = calloc(nr + 1, alloc_size);
> + if (!new_expr->metric_refs) {
> + free(new_expr);
> + return -ENOMEM;
> + }
> +
> + memcpy(new_expr->metric_refs, old_expr->metric_refs,
> + nr * alloc_size);
> + } else {
> + new_expr->metric_refs = NULL;
> + }
> +
> + /* calculate number of metric_events */
> + for (nr = 0; old_expr->metric_events[nr]; nr++)
> + continue;
> + alloc_size = sizeof(*new_expr->metric_events);
> + new_expr->metric_events = calloc(nr + 1, alloc_size);
> + if (!new_expr->metric_events) {
> + free(new_expr->metric_refs);
> + free(new_expr);
> + return -ENOMEM;
> + }
> +
> + /* copy evsel in the same position */
> + for (idx = 0; idx < nr; idx++) {
> + evsel = old_expr->metric_events[idx];
> + evsel = evlist__find_evsel(evlist, evsel->idx);
> + if (evsel == NULL) {
> + free(new_expr->metric_events);
> + free(new_expr->metric_refs);
> + free(new_expr);
> + return -EINVAL;
> + }
> + new_expr->metric_events[idx] = evsel;
> + }
> +
> + list_add(&new_expr->nd, &new_me->head);
> + }
> + }
> + return 0;
> +}
> diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
> index 491a5d78252d..ed1b9392e624 100644
> --- a/tools/perf/util/metricgroup.h
> +++ b/tools/perf/util/metricgroup.h
> @@ -7,11 +7,13 @@
> #include <stdbool.h>
> #include "pmu-events/pmu-events.h"
>
> +struct evlist;
> struct evsel;
> struct evlist;
> struct option;
> struct rblist;
> struct pmu_events_map;
> +struct cgroup;
>
> struct metric_event {
> struct rb_node nd;
> @@ -55,4 +57,8 @@ void metricgroup__print(bool metrics, bool groups, char *filter,
> bool metricgroup__has_metric(const char *metric);
> int arch_get_runtimeparam(struct pmu_event *pe __maybe_unused);
> void metricgroup__rblist_exit(struct rblist *metric_events);
> +
> +int metricgroup__copy_metric_events(struct evlist *evlist, struct cgroup *cgrp,
> + struct rblist *new_metric_events,
> + struct rblist *old_metric_events);
> #endif
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 74f85948d101..ae8edde7c50e 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -15,6 +15,8 @@
> #include "thread_map.h"
> #include "trace-event.h"
> #include "mmap.h"
> +#include "stat.h"
> +#include "metricgroup.h"
> #include "util/env.h"
> #include <internal/lib.h>
> #include "util.h"
> @@ -60,6 +62,23 @@ int parse_callchain_record(const char *arg __maybe_unused,
> */
> struct perf_env perf_env;
>
> +/*
> + * Add this one here not to drag util/stat-shadow.c
> + */
> +void perf_stat__collect_metric_expr(struct evlist *evsel_list)
> +{
> +}
> +
> +/*
> + * Add this one here not to drag util/metricgroup.c
> + */
> +int metricgroup__copy_metric_events(struct evlist *evlist, struct cgroup *cgrp,
> + struct rblist *new_metric_events,
> + struct rblist *old_metric_events)
> +{
> + return 0;
> +}
> +
> /*
> * Support debug printing even though util/debug.c is not linked. That means
> * implementing 'verbose' and 'eprintf'.
> --
> 2.28.0.681.g6f77f65b4e-goog
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/5] perf tools: Allow creation of cgroup without open
2020-09-24 12:44 [PATCHSET v5 0/5] perf stat: Expand events for each cgroup Namhyung Kim
` (2 preceding siblings ...)
2020-09-24 12:44 ` [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups Namhyung Kim
@ 2020-09-24 12:44 ` Namhyung Kim
2020-09-24 12:44 ` [PATCH 5/5] perf test: Add expand cgroup event test Namhyung Kim
4 siblings, 0 replies; 18+ messages in thread
From: Namhyung Kim @ 2020-09-24 12:44 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, Mark Rutland, Alexander Shishkin,
LKML, Stephane Eranian, Andi Kleen, Ian Rogers
This is a preparation for a test case of expanding events for multiple
cgroups. Instead of using real system cgroup, the test will use fake
cgroups so it needs a way to have them without a open file descriptor.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/builtin-stat.c | 2 +-
tools/perf/util/cgroup.c | 19 ++++++++++++-------
tools/perf/util/cgroup.h | 2 +-
3 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 2751699a8969..b01af171d94f 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2235,7 +2235,7 @@ int cmd_stat(int argc, const char **argv)
}
if (evlist__expand_cgroup(evsel_list, stat_config.cgroup_list,
- &stat_config.metric_events) < 0)
+ &stat_config.metric_events, true) < 0)
goto out;
}
diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
index 6e64c908fa71..b81324a13a2b 100644
--- a/tools/perf/util/cgroup.c
+++ b/tools/perf/util/cgroup.c
@@ -51,7 +51,7 @@ static struct cgroup *evlist__find_cgroup(struct evlist *evlist, const char *str
return NULL;
}
-static struct cgroup *cgroup__new(const char *name)
+static struct cgroup *cgroup__new(const char *name, bool do_open)
{
struct cgroup *cgroup = zalloc(sizeof(*cgroup));
@@ -61,9 +61,14 @@ static struct cgroup *cgroup__new(const char *name)
cgroup->name = strdup(name);
if (!cgroup->name)
goto out_err;
- cgroup->fd = open_cgroup(name);
- if (cgroup->fd == -1)
- goto out_free_name;
+
+ if (do_open) {
+ cgroup->fd = open_cgroup(name);
+ if (cgroup->fd == -1)
+ goto out_free_name;
+ } else {
+ cgroup->fd = -1;
+ }
}
return cgroup;
@@ -79,7 +84,7 @@ struct cgroup *evlist__findnew_cgroup(struct evlist *evlist, const char *name)
{
struct cgroup *cgroup = evlist__find_cgroup(evlist, name);
- return cgroup ?: cgroup__new(name);
+ return cgroup ?: cgroup__new(name, true);
}
static int add_cgroup(struct evlist *evlist, const char *str)
@@ -197,7 +202,7 @@ int parse_cgroups(const struct option *opt, const char *str,
}
int evlist__expand_cgroup(struct evlist *evlist, const char *str,
- struct rblist *metric_events)
+ struct rblist *metric_events, bool open_cgroup)
{
struct evlist *orig_list, *tmp_list;
struct evsel *pos, *evsel, *leader;
@@ -240,7 +245,7 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str,
if (!name)
goto out_err;
- cgrp = cgroup__new(name);
+ cgrp = cgroup__new(name, open_cgroup);
free(name);
if (cgrp == NULL)
goto out_err;
diff --git a/tools/perf/util/cgroup.h b/tools/perf/util/cgroup.h
index eea6df8ee373..162906f3412a 100644
--- a/tools/perf/util/cgroup.h
+++ b/tools/perf/util/cgroup.h
@@ -26,7 +26,7 @@ struct rblist;
struct cgroup *evlist__findnew_cgroup(struct evlist *evlist, const char *name);
int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups,
- struct rblist *metric_events);
+ struct rblist *metric_events, bool open_cgroup);
void evlist__set_default_cgroup(struct evlist *evlist, struct cgroup *cgroup);
--
2.28.0.681.g6f77f65b4e-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 5/5] perf test: Add expand cgroup event test
2020-09-24 12:44 [PATCHSET v5 0/5] perf stat: Expand events for each cgroup Namhyung Kim
` (3 preceding siblings ...)
2020-09-24 12:44 ` [PATCH 4/5] perf tools: Allow creation of cgroup without open Namhyung Kim
@ 2020-09-24 12:44 ` Namhyung Kim
4 siblings, 0 replies; 18+ messages in thread
From: Namhyung Kim @ 2020-09-24 12:44 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, Mark Rutland, Alexander Shishkin,
LKML, Stephane Eranian, Andi Kleen, Ian Rogers, John Garry
It'll expand given events for cgroups A, B and C.
$ ./perf test -v expansion
69: Event expansion for cgroups :
--- start ---
test child forked, pid 983140
metric expr 1 / IPC for CPI
metric expr instructions / cycles for IPC
found event instructions
found event cycles
adding {instructions,cycles}:W
copying metric event for cgroup 'A': instructions (idx=0)
copying metric event for cgroup 'B': instructions (idx=0)
copying metric event for cgroup 'C': instructions (idx=0)
test child finished with 0
---- end ----
Event expansion for cgroups: Ok
Cc: John Garry <john.garry@huawei.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/tests/Build | 1 +
tools/perf/tests/builtin-test.c | 4 +
tools/perf/tests/expand-cgroup.c | 241 +++++++++++++++++++++++++++++++
tools/perf/tests/tests.h | 1 +
4 files changed, 247 insertions(+)
create mode 100644 tools/perf/tests/expand-cgroup.c
diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index 69bea7996f18..4d15bf6041fb 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -61,6 +61,7 @@ perf-y += demangle-java-test.o
perf-y += pfm.o
perf-y += parse-metric.o
perf-y += pe-file-parsing.o
+perf-y += expand-cgroup.o
$(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
$(call rule_mkdir)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 651b8ea3354a..132bdb3e6c31 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -345,6 +345,10 @@ static struct test generic_tests[] = {
.desc = "PE file support",
.func = test__pe_file_parsing,
},
+ {
+ .desc = "Event expansion for cgroups",
+ .func = test__expand_cgroup_events,
+ },
{
.func = NULL,
},
diff --git a/tools/perf/tests/expand-cgroup.c b/tools/perf/tests/expand-cgroup.c
new file mode 100644
index 000000000000..d5771e4d094f
--- /dev/null
+++ b/tools/perf/tests/expand-cgroup.c
@@ -0,0 +1,241 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "tests.h"
+#include "debug.h"
+#include "evlist.h"
+#include "cgroup.h"
+#include "rblist.h"
+#include "metricgroup.h"
+#include "parse-events.h"
+#include "pmu-events/pmu-events.h"
+#include "pfm.h"
+#include <subcmd/parse-options.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+static int test_expand_events(struct evlist *evlist,
+ struct rblist *metric_events)
+{
+ int i, ret = TEST_FAIL;
+ int nr_events;
+ bool was_group_event;
+ int nr_members; /* for the first evsel only */
+ const char cgrp_str[] = "A,B,C";
+ const char *cgrp_name[] = { "A", "B", "C" };
+ int nr_cgrps = ARRAY_SIZE(cgrp_name);
+ char **ev_name;
+ struct evsel *evsel;
+
+ TEST_ASSERT_VAL("evlist is empty", !perf_evlist__empty(evlist));
+
+ nr_events = evlist->core.nr_entries;
+ ev_name = calloc(nr_events, sizeof(*ev_name));
+ if (ev_name == NULL) {
+ pr_debug("memory allocation failure\n");
+ return TEST_FAIL;
+ }
+ i = 0;
+ evlist__for_each_entry(evlist, evsel) {
+ ev_name[i] = strdup(evsel->name);
+ if (ev_name[i] == NULL) {
+ pr_debug("memory allocation failure\n");
+ goto out;
+ }
+ i++;
+ }
+ /* remember grouping info */
+ was_group_event = evsel__is_group_event(evlist__first(evlist));
+ nr_members = evlist__first(evlist)->core.nr_members;
+
+ ret = evlist__expand_cgroup(evlist, cgrp_str, metric_events, false);
+ if (ret < 0) {
+ pr_debug("failed to expand events for cgroups\n");
+ goto out;
+ }
+
+ ret = TEST_FAIL;
+ if (evlist->core.nr_entries != nr_events * nr_cgrps) {
+ pr_debug("event count doesn't match\n");
+ goto out;
+ }
+
+ i = 0;
+ evlist__for_each_entry(evlist, evsel) {
+ if (strcmp(evsel->name, ev_name[i % nr_events])) {
+ pr_debug("event name doesn't match:\n");
+ pr_debug(" evsel[%d]: %s\n expected: %s\n",
+ i, evsel->name, ev_name[i % nr_events]);
+ goto out;
+ }
+ if (strcmp(evsel->cgrp->name, cgrp_name[i / nr_events])) {
+ pr_debug("cgroup name doesn't match:\n");
+ pr_debug(" evsel[%d]: %s\n expected: %s\n",
+ i, evsel->cgrp->name, cgrp_name[i / nr_events]);
+ goto out;
+ }
+
+ if ((i % nr_events) == 0) {
+ if (evsel__is_group_event(evsel) != was_group_event) {
+ pr_debug("event group doesn't match: got %s, expect %s\n",
+ evsel__is_group_event(evsel) ? "true" : "false",
+ was_group_event ? "true" : "false");
+ goto out;
+ }
+ if (evsel->core.nr_members != nr_members) {
+ pr_debug("event group member doesn't match: %d vs %d\n",
+ evsel->core.nr_members, nr_members);
+ goto out;
+ }
+ }
+ i++;
+ }
+ ret = TEST_OK;
+
+out: for (i = 0; i < nr_events; i++)
+ free(ev_name[i]);
+ free(ev_name);
+ return ret;
+}
+
+static int expand_default_events(void)
+{
+ int ret;
+ struct evlist *evlist;
+ struct rblist metric_events;
+
+ evlist = perf_evlist__new_default();
+ TEST_ASSERT_VAL("failed to get evlist", evlist);
+
+ rblist__init(&metric_events);
+ ret = test_expand_events(evlist, &metric_events);
+ evlist__delete(evlist);
+ return ret;
+}
+
+static int expand_group_events(void)
+{
+ int ret;
+ struct evlist *evlist;
+ struct rblist metric_events;
+ struct parse_events_error err;
+ const char event_str[] = "{cycles,instructions}";
+
+ symbol_conf.event_group = true;
+
+ evlist = evlist__new();
+ TEST_ASSERT_VAL("failed to get evlist", evlist);
+
+ ret = parse_events(evlist, event_str, &err);
+ if (ret < 0) {
+ pr_debug("failed to parse event '%s', err %d, str '%s'\n",
+ event_str, ret, err.str);
+ parse_events_print_error(&err, event_str);
+ goto out;
+ }
+
+ rblist__init(&metric_events);
+ ret = test_expand_events(evlist, &metric_events);
+out:
+ evlist__delete(evlist);
+ return ret;
+}
+
+static int expand_libpfm_events(void)
+{
+ int ret;
+ struct evlist *evlist;
+ struct rblist metric_events;
+ const char event_str[] = "UNHALTED_CORE_CYCLES";
+ struct option opt = {
+ .value = &evlist,
+ };
+
+ symbol_conf.event_group = true;
+
+ evlist = evlist__new();
+ TEST_ASSERT_VAL("failed to get evlist", evlist);
+
+ ret = parse_libpfm_events_option(&opt, event_str, 0);
+ if (ret < 0) {
+ pr_debug("failed to parse libpfm event '%s', err %d\n",
+ event_str, ret);
+ goto out;
+ }
+ if (perf_evlist__empty(evlist)) {
+ pr_debug("libpfm was not enabled\n");
+ goto out;
+ }
+
+ rblist__init(&metric_events);
+ ret = test_expand_events(evlist, &metric_events);
+out:
+ evlist__delete(evlist);
+ return ret;
+}
+
+static int expand_metric_events(void)
+{
+ int ret;
+ struct evlist *evlist;
+ struct rblist metric_events;
+ const char metric_str[] = "CPI";
+
+ struct pmu_event pme_test[] = {
+ {
+ .metric_expr = "instructions / cycles",
+ .metric_name = "IPC",
+ },
+ {
+ .metric_expr = "1 / IPC",
+ .metric_name = "CPI",
+ },
+ {
+ .metric_expr = NULL,
+ .metric_name = NULL,
+ },
+ };
+ struct pmu_events_map ev_map = {
+ .cpuid = "test",
+ .version = "1",
+ .type = "core",
+ .table = pme_test,
+ };
+
+ evlist = evlist__new();
+ TEST_ASSERT_VAL("failed to get evlist", evlist);
+
+ rblist__init(&metric_events);
+ ret = metricgroup__parse_groups_test(evlist, &ev_map, metric_str,
+ false, false, &metric_events);
+ if (ret < 0) {
+ pr_debug("failed to parse '%s' metric\n", metric_str);
+ goto out;
+ }
+
+ ret = test_expand_events(evlist, &metric_events);
+
+out:
+ metricgroup__rblist_exit(&metric_events);
+ evlist__delete(evlist);
+ return ret;
+}
+
+int test__expand_cgroup_events(struct test *test __maybe_unused,
+ int subtest __maybe_unused)
+{
+ int ret;
+
+ ret = expand_default_events();
+ TEST_ASSERT_EQUAL("failed to expand default events", ret, 0);
+
+ ret = expand_group_events();
+ TEST_ASSERT_EQUAL("failed to expand event group", ret, 0);
+
+ ret = expand_libpfm_events();
+ TEST_ASSERT_EQUAL("failed to expand event group", ret, 0);
+
+ ret = expand_metric_events();
+ TEST_ASSERT_EQUAL("failed to expand metric events", ret, 0);
+
+ return ret;
+}
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index ef0f33c6ba23..c85a2c08e407 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -123,6 +123,7 @@ const char *test__pfm_subtest_get_desc(int subtest);
int test__pfm_subtest_get_nr(void);
int test__parse_metric(struct test *test, int subtest);
int test__pe_file_parsing(struct test *test, int subtest);
+int test__expand_cgroup_events(struct test *test, int subtest);
bool test__bp_signal_is_supported(void);
bool test__bp_account_is_supported(void);
--
2.28.0.681.g6f77f65b4e-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups
2020-09-23 1:59 [PATCHSET v4 0/5] perf stat: Expand events for each cgroup Namhyung Kim
@ 2020-09-23 1:59 ` Namhyung Kim
2020-09-23 10:22 ` Jiri Olsa
0 siblings, 1 reply; 18+ messages in thread
From: Namhyung Kim @ 2020-09-23 1:59 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, Mark Rutland, Alexander Shishkin,
LKML, Stephane Eranian, Andi Kleen, Ian Rogers, John Garry,
Kajol Jain
The metricgroup__copy_metric_events() is to handle metrics events when
expanding event for cgroups. As the metric events keep pointers to
evsel, it should be refreshed when events are cloned during the
operation.
The perf_stat__collect_metric_expr() is also called in case an event
has a metric directly.
During the copy, it references evsel by index as the evlist now has
cloned evsels for the given cgroup.
Cc: John Garry <john.garry@huawei.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/builtin-stat.c | 3 +-
tools/perf/util/cgroup.c | 15 ++++++-
tools/perf/util/cgroup.h | 4 +-
tools/perf/util/evlist.c | 11 +++++
tools/perf/util/evlist.h | 1 +
tools/perf/util/metricgroup.c | 85 +++++++++++++++++++++++++++++++++++
tools/perf/util/metricgroup.h | 6 +++
7 files changed, 122 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 23abf14b6e16..66a33d97192d 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2255,7 +2255,8 @@ int cmd_stat(int argc, const char **argv)
goto out;
}
- if (evlist__expand_cgroup(evsel_list, stat_config.cgroup_list) < 0)
+ if (evlist__expand_cgroup(evsel_list, stat_config.cgroup_list,
+ &stat_config.metric_events) < 0)
goto out;
}
diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
index 8b6a4fa49082..dcd18ef268a1 100644
--- a/tools/perf/util/cgroup.c
+++ b/tools/perf/util/cgroup.c
@@ -3,6 +3,9 @@
#include "evsel.h"
#include "cgroup.h"
#include "evlist.h"
+#include "rblist.h"
+#include "metricgroup.h"
+#include "stat.h"
#include <linux/zalloc.h>
#include <sys/types.h>
#include <sys/stat.h>
@@ -193,10 +196,12 @@ int parse_cgroups(const struct option *opt, const char *str,
return 0;
}
-int evlist__expand_cgroup(struct evlist *evlist, const char *str)
+int evlist__expand_cgroup(struct evlist *evlist, const char *str,
+ struct rblist *metric_events)
{
struct evlist *orig_list, *tmp_list;
struct evsel *pos, *evsel, *leader;
+ struct rblist orig_metric_events;
struct cgroup *cgrp = NULL;
const char *p, *e, *eos = str + strlen(str);
int ret = -1;
@@ -216,6 +221,8 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
/* save original events and init evlist */
perf_evlist__splice_list_tail(orig_list, &evlist->core.entries);
evlist->core.nr_entries = 0;
+ orig_metric_events = *metric_events;
+ rblist__init(metric_events);
for (;;) {
p = strchr(str, ',');
@@ -255,6 +262,11 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
cgroup__put(cgrp);
nr_cgroups++;
+ perf_stat__collect_metric_expr(tmp_list);
+ if (metricgroup__copy_metric_events(tmp_list, cgrp, metric_events,
+ &orig_metric_events) < 0)
+ break;
+
perf_evlist__splice_list_tail(evlist, &tmp_list->core.entries);
tmp_list->core.nr_entries = 0;
@@ -268,6 +280,7 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
out_err:
evlist__delete(orig_list);
evlist__delete(tmp_list);
+ rblist__exit(&orig_metric_events);
return ret;
}
diff --git a/tools/perf/util/cgroup.h b/tools/perf/util/cgroup.h
index 32893018296f..eea6df8ee373 100644
--- a/tools/perf/util/cgroup.h
+++ b/tools/perf/util/cgroup.h
@@ -22,9 +22,11 @@ struct cgroup *cgroup__get(struct cgroup *cgroup);
void cgroup__put(struct cgroup *cgroup);
struct evlist;
+struct rblist;
struct cgroup *evlist__findnew_cgroup(struct evlist *evlist, const char *name);
-int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups);
+int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups,
+ struct rblist *metric_events);
void evlist__set_default_cgroup(struct evlist *evlist, struct cgroup *cgroup);
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index ee7b576d3b12..aae79b2b5041 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1964,3 +1964,14 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
return err;
}
+
+struct evsel *evlist__find_evsel(struct evlist *evlist, int idx)
+{
+ struct evsel *evsel;
+
+ evlist__for_each_entry(evlist, evsel) {
+ if (evsel->idx == idx)
+ return evsel;
+ }
+ return NULL;
+}
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index bc38a53f6a1a..e1a450322bc5 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -386,4 +386,5 @@ int evlist__ctlfd_ack(struct evlist *evlist);
#define EVLIST_ENABLED_MSG "Events enabled\n"
#define EVLIST_DISABLED_MSG "Events disabled\n"
+struct evsel *evlist__find_evsel(struct evlist *evlist, int idx);
#endif /* __PERF_EVLIST_H */
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index d03bac65a3c2..14e8f13e07f7 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -24,6 +24,7 @@
#include <api/fs/fs.h>
#include "util.h"
#include <asm/bug.h>
+#include "cgroup.h"
struct metric_event *metricgroup__lookup(struct rblist *metric_events,
struct evsel *evsel,
@@ -1105,3 +1106,87 @@ bool metricgroup__has_metric(const char *metric)
}
return false;
}
+
+int metricgroup__copy_metric_events(struct evlist *evlist, struct cgroup *cgrp,
+ struct rblist *new_metric_events,
+ struct rblist *old_metric_events)
+{
+ unsigned i;
+
+ for (i = 0; i < rblist__nr_entries(old_metric_events); i++) {
+ struct rb_node *nd;
+ struct metric_event *old_me, *new_me;
+ struct metric_expr *old_expr, *new_expr;
+ struct evsel *evsel;
+ size_t alloc_size;
+ int idx, nr;
+
+ nd = rblist__entry(old_metric_events, i);
+ old_me = container_of(nd, struct metric_event, nd);
+
+ evsel = evlist__find_evsel(evlist, old_me->evsel->idx);
+ if (!evsel)
+ return -EINVAL;
+ new_me = metricgroup__lookup(new_metric_events, evsel, true);
+ if (!new_me)
+ return -ENOMEM;
+
+ pr_debug("copying metric event for cgroup '%s': %s (idx=%d)\n",
+ cgrp ? cgrp->name : "root", evsel->name, evsel->idx);
+
+ list_for_each_entry(old_expr, &old_me->head, nd) {
+ new_expr = malloc(sizeof(*new_expr));
+ if (!new_expr)
+ return -ENOMEM;
+
+ new_expr->metric_expr = old_expr->metric_expr;
+ new_expr->metric_name = old_expr->metric_name;
+ new_expr->metric_unit = old_expr->metric_unit;
+ new_expr->runtime = old_expr->runtime;
+
+ if (old_expr->metric_refs) {
+ /* calculate number of metric_events */
+ for (nr = 0; old_expr->metric_refs[nr].metric_name; nr++)
+ continue;
+ alloc_size = sizeof(*new_expr->metric_refs);
+ new_expr->metric_refs = calloc(nr + 1, alloc_size);
+ if (!new_expr->metric_refs) {
+ free(new_expr);
+ return -ENOMEM;
+ }
+
+ memcpy(new_expr->metric_refs, old_expr->metric_refs,
+ nr * alloc_size);
+ } else {
+ new_expr->metric_refs = NULL;
+ }
+
+ /* calculate number of metric_events */
+ for (nr = 0; old_expr->metric_events[nr]; nr++)
+ continue;
+ alloc_size = sizeof(*new_expr->metric_events);
+ new_expr->metric_events = calloc(nr + 1, alloc_size);
+ if (!new_expr->metric_events) {
+ free(new_expr->metric_refs);
+ free(new_expr);
+ return -ENOMEM;
+ }
+
+ /* copy evsel in the same position */
+ for (idx = 0; idx < nr; idx++) {
+ evsel = old_expr->metric_events[idx];
+ evsel = evlist__find_evsel(evlist, evsel->idx);
+ if (evsel == NULL) {
+ free(new_expr->metric_events);
+ free(new_expr->metric_refs);
+ free(new_expr);
+ return -EINVAL;
+ }
+ new_expr->metric_events[idx] = evsel;
+ }
+
+ list_add(&new_expr->nd, &new_me->head);
+ }
+ }
+ return 0;
+}
diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
index 491a5d78252d..ed1b9392e624 100644
--- a/tools/perf/util/metricgroup.h
+++ b/tools/perf/util/metricgroup.h
@@ -7,11 +7,13 @@
#include <stdbool.h>
#include "pmu-events/pmu-events.h"
+struct evlist;
struct evsel;
struct evlist;
struct option;
struct rblist;
struct pmu_events_map;
+struct cgroup;
struct metric_event {
struct rb_node nd;
@@ -55,4 +57,8 @@ void metricgroup__print(bool metrics, bool groups, char *filter,
bool metricgroup__has_metric(const char *metric);
int arch_get_runtimeparam(struct pmu_event *pe __maybe_unused);
void metricgroup__rblist_exit(struct rblist *metric_events);
+
+int metricgroup__copy_metric_events(struct evlist *evlist, struct cgroup *cgrp,
+ struct rblist *new_metric_events,
+ struct rblist *old_metric_events);
#endif
--
2.28.0.681.g6f77f65b4e-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups
2020-09-23 1:59 ` [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups Namhyung Kim
@ 2020-09-23 10:22 ` Jiri Olsa
2020-09-24 3:12 ` Namhyung Kim
0 siblings, 1 reply; 18+ messages in thread
From: Jiri Olsa @ 2020-09-23 10:22 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
Mark Rutland, Alexander Shishkin, LKML, Stephane Eranian,
Andi Kleen, Ian Rogers, John Garry, Kajol Jain
On Wed, Sep 23, 2020 at 10:59:43AM +0900, Namhyung Kim wrote:
SNIP
> diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
> index 8b6a4fa49082..dcd18ef268a1 100644
> --- a/tools/perf/util/cgroup.c
> +++ b/tools/perf/util/cgroup.c
> @@ -3,6 +3,9 @@
> #include "evsel.h"
> #include "cgroup.h"
> #include "evlist.h"
> +#include "rblist.h"
> +#include "metricgroup.h"
> +#include "stat.h"
> #include <linux/zalloc.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> @@ -193,10 +196,12 @@ int parse_cgroups(const struct option *opt, const char *str,
> return 0;
> }
>
> -int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> +int evlist__expand_cgroup(struct evlist *evlist, const char *str,
> + struct rblist *metric_events)
> {
> struct evlist *orig_list, *tmp_list;
> struct evsel *pos, *evsel, *leader;
> + struct rblist orig_metric_events;
> struct cgroup *cgrp = NULL;
> const char *p, *e, *eos = str + strlen(str);
> int ret = -1;
> @@ -216,6 +221,8 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> /* save original events and init evlist */
> perf_evlist__splice_list_tail(orig_list, &evlist->core.entries);
> evlist->core.nr_entries = 0;
> + orig_metric_events = *metric_events;
> + rblist__init(metric_events);
>
> for (;;) {
> p = strchr(str, ',');
> @@ -255,6 +262,11 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> cgroup__put(cgrp);
> nr_cgroups++;
>
> + perf_stat__collect_metric_expr(tmp_list);
I know you added the option just for perf stat, not record,
but the code looks generic apart from using this function
I wonder if this would cause any issues if it was called in record
context.. maybe we could just skip it in that case, but that's for
future to worry about ;-)
jirka
> + if (metricgroup__copy_metric_events(tmp_list, cgrp, metric_events,
> + &orig_metric_events) < 0)
> + break;
> +
> perf_evlist__splice_list_tail(evlist, &tmp_list->core.entries);
> tmp_list->core.nr_entries = 0;
>
> @@ -268,6 +280,7 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> out_err:
> evlist__delete(orig_list);
> evlist__delete(tmp_list);
> + rblist__exit(&orig_metric_events);
>
> return ret;
> }
SNIP
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups
2020-09-23 10:22 ` Jiri Olsa
@ 2020-09-24 3:12 ` Namhyung Kim
0 siblings, 0 replies; 18+ messages in thread
From: Namhyung Kim @ 2020-09-24 3:12 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
Mark Rutland, Alexander Shishkin, LKML, Stephane Eranian,
Andi Kleen, Ian Rogers, John Garry, Kajol Jain
On Wed, Sep 23, 2020 at 7:23 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Sep 23, 2020 at 10:59:43AM +0900, Namhyung Kim wrote:
>
> SNIP
>
>
> > diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
> > index 8b6a4fa49082..dcd18ef268a1 100644
> > --- a/tools/perf/util/cgroup.c
> > +++ b/tools/perf/util/cgroup.c
> > @@ -3,6 +3,9 @@
> > #include "evsel.h"
> > #include "cgroup.h"
> > #include "evlist.h"
> > +#include "rblist.h"
> > +#include "metricgroup.h"
> > +#include "stat.h"
> > #include <linux/zalloc.h>
> > #include <sys/types.h>
> > #include <sys/stat.h>
> > @@ -193,10 +196,12 @@ int parse_cgroups(const struct option *opt, const char *str,
> > return 0;
> > }
> >
> > -int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> > +int evlist__expand_cgroup(struct evlist *evlist, const char *str,
> > + struct rblist *metric_events)
> > {
> > struct evlist *orig_list, *tmp_list;
> > struct evsel *pos, *evsel, *leader;
> > + struct rblist orig_metric_events;
> > struct cgroup *cgrp = NULL;
> > const char *p, *e, *eos = str + strlen(str);
> > int ret = -1;
> > @@ -216,6 +221,8 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> > /* save original events and init evlist */
> > perf_evlist__splice_list_tail(orig_list, &evlist->core.entries);
> > evlist->core.nr_entries = 0;
> > + orig_metric_events = *metric_events;
> > + rblist__init(metric_events);
> >
> > for (;;) {
> > p = strchr(str, ',');
> > @@ -255,6 +262,11 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> > cgroup__put(cgrp);
> > nr_cgroups++;
> >
> > + perf_stat__collect_metric_expr(tmp_list);
>
> I know you added the option just for perf stat, not record,
> but the code looks generic apart from using this function
Right. I've tried to make it generic as much as possible.
But the above code works for an evlist assuming all the
evsels are in the same cgroup as far as I can see.
So I put it here to pass the tmp_list for each cgroup
separately.
>
> I wonder if this would cause any issues if it was called in record
> context.. maybe we could just skip it in that case, but that's for
> future to worry about ;-)
Yeah, I believe we can just skip as it's all for metrics which is
used for perf stat only, I guess?
Thanks
Namhyung
>
> > + if (metricgroup__copy_metric_events(tmp_list, cgrp, metric_events,
> > + &orig_metric_events) < 0)
> > + break;
> > +
> > perf_evlist__splice_list_tail(evlist, &tmp_list->core.entries);
> > tmp_list->core.nr_entries = 0;
> >
> > @@ -268,6 +280,7 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> > out_err:
> > evlist__delete(orig_list);
> > evlist__delete(tmp_list);
> > + rblist__exit(&orig_metric_events);
> >
> > return ret;
> > }
>
> SNIP
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups
2020-09-21 9:46 [PATCHSET v3 0/5] perf stat: Expand events for each cgroup Namhyung Kim
@ 2020-09-21 9:46 ` Namhyung Kim
2020-09-22 21:29 ` Jiri Olsa
0 siblings, 1 reply; 18+ messages in thread
From: Namhyung Kim @ 2020-09-21 9:46 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa
Cc: Ingo Molnar, Peter Zijlstra, Mark Rutland, Alexander Shishkin,
LKML, Stephane Eranian, Andi Kleen, Ian Rogers, John Garry,
Kajol Jain
The metricgroup__copy_metric_events() is to handle metrics events when
expanding event for cgroups. As the metric events keep pointers to
evsel, it should be refreshed when events are cloned during the
operation.
The perf_stat__collect_metric_expr() is also called in case an event
has a metric directly.
During the copy, it references evsel by index as the evlist now has
cloned evsels for the given cgroup.
Cc: John Garry <john.garry@huawei.com>
Cc: Kajol Jain <kjain@linux.ibm.com>
Cc: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/builtin-stat.c | 3 +-
tools/perf/util/cgroup.c | 15 ++++++-
tools/perf/util/cgroup.h | 4 +-
tools/perf/util/evlist.c | 11 +++++
tools/perf/util/evlist.h | 1 +
tools/perf/util/metricgroup.c | 83 +++++++++++++++++++++++++++++++++++
tools/perf/util/metricgroup.h | 6 +++
7 files changed, 120 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index a43e58e0a088..8b81d62ab18b 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2248,7 +2248,8 @@ int cmd_stat(int argc, const char **argv)
goto out;
if (stat_config.cgroup_list) {
- if (evlist__expand_cgroup(evsel_list, stat_config.cgroup_list) < 0)
+ if (evlist__expand_cgroup(evsel_list, stat_config.cgroup_list,
+ &stat_config.metric_events) < 0)
goto out;
}
diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
index e4916ed740ac..adf60597520b 100644
--- a/tools/perf/util/cgroup.c
+++ b/tools/perf/util/cgroup.c
@@ -3,6 +3,9 @@
#include "evsel.h"
#include "cgroup.h"
#include "evlist.h"
+#include "rblist.h"
+#include "metricgroup.h"
+#include "stat.h"
#include <linux/zalloc.h>
#include <sys/types.h>
#include <sys/stat.h>
@@ -198,10 +201,12 @@ int parse_cgroups(const struct option *opt, const char *str,
return 0;
}
-int evlist__expand_cgroup(struct evlist *evlist, const char *str)
+int evlist__expand_cgroup(struct evlist *evlist, const char *str,
+ struct rblist *metric_events)
{
struct evlist *orig_list, *tmp_list;
struct evsel *pos, *evsel, *leader;
+ struct rblist orig_metric_events;
struct cgroup *cgrp = NULL;
const char *p, *e, *eos = str + strlen(str);
int ret = -1;
@@ -221,6 +226,8 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
/* save original events and init evlist */
perf_evlist__splice_list_tail(orig_list, &evlist->core.entries);
evlist->core.nr_entries = 0;
+ orig_metric_events = *metric_events;
+ rblist__init(metric_events);
for (;;) {
p = strchr(str, ',');
@@ -260,6 +267,11 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
cgroup__put(cgrp);
nr_cgroups++;
+ perf_stat__collect_metric_expr(tmp_list);
+ if (metricgroup__copy_metric_events(tmp_list, cgrp, metric_events,
+ &orig_metric_events) < 0)
+ break;
+
perf_evlist__splice_list_tail(evlist, &tmp_list->core.entries);
tmp_list->core.nr_entries = 0;
@@ -273,6 +285,7 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
out_err:
evlist__delete(orig_list);
evlist__delete(tmp_list);
+ rblist__exit(&orig_metric_events);
return ret;
}
diff --git a/tools/perf/util/cgroup.h b/tools/perf/util/cgroup.h
index 32893018296f..eea6df8ee373 100644
--- a/tools/perf/util/cgroup.h
+++ b/tools/perf/util/cgroup.h
@@ -22,9 +22,11 @@ struct cgroup *cgroup__get(struct cgroup *cgroup);
void cgroup__put(struct cgroup *cgroup);
struct evlist;
+struct rblist;
struct cgroup *evlist__findnew_cgroup(struct evlist *evlist, const char *name);
-int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups);
+int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups,
+ struct rblist *metric_events);
void evlist__set_default_cgroup(struct evlist *evlist, struct cgroup *cgroup);
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index ee7b576d3b12..424209c4bcd2 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1964,3 +1964,14 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
return err;
}
+
+struct evsel *evlist__get_evsel(struct evlist *evlist, int idx)
+{
+ struct evsel *evsel;
+
+ evlist__for_each_entry(evlist, evsel) {
+ if (evsel->idx == idx)
+ return evsel;
+ }
+ return NULL;
+}
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index bc38a53f6a1a..8c5721cb14b2 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -386,4 +386,5 @@ int evlist__ctlfd_ack(struct evlist *evlist);
#define EVLIST_ENABLED_MSG "Events enabled\n"
#define EVLIST_DISABLED_MSG "Events disabled\n"
+struct evsel *evlist__get_evsel(struct evlist *evlist, int idx);
#endif /* __PERF_EVLIST_H */
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index d03bac65a3c2..a88bd125880a 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -24,6 +24,7 @@
#include <api/fs/fs.h>
#include "util.h"
#include <asm/bug.h>
+#include "cgroup.h"
struct metric_event *metricgroup__lookup(struct rblist *metric_events,
struct evsel *evsel,
@@ -1105,3 +1106,85 @@ bool metricgroup__has_metric(const char *metric)
}
return false;
}
+
+int metricgroup__copy_metric_events(struct evlist *evlist, struct cgroup *cgrp,
+ struct rblist *new_metric_events,
+ struct rblist *old_metric_events)
+{
+ unsigned i;
+
+ for (i = 0; i < rblist__nr_entries(old_metric_events); i++) {
+ struct rb_node *nd;
+ struct metric_event *old_me, *new_me;
+ struct metric_expr *old_expr, *new_expr;
+ struct evsel *evsel;
+ size_t alloc_size;
+ int idx, nr;
+
+ nd = rblist__entry(old_metric_events, i);
+ old_me = container_of(nd, struct metric_event, nd);
+
+ evsel = evlist__get_evsel(evlist, old_me->evsel->idx);
+ new_me = metricgroup__lookup(new_metric_events, evsel, true);
+ if (!new_me)
+ return -ENOMEM;
+
+ pr_debug("copying metric event for cgroup '%s': %s (idx=%d)\n",
+ cgrp ? cgrp->name : "root", evsel->name, evsel->idx);
+
+ list_for_each_entry(old_expr, &old_me->head, nd) {
+ new_expr = malloc(sizeof(*new_expr));
+ if (!new_expr)
+ return -ENOMEM;
+
+ new_expr->metric_expr = old_expr->metric_expr;
+ new_expr->metric_name = old_expr->metric_name;
+ new_expr->metric_unit = old_expr->metric_unit;
+ new_expr->runtime = old_expr->runtime;
+
+ if (old_expr->metric_refs) {
+ /* calculate number of metric_events */
+ for (nr = 0; old_expr->metric_refs[nr].metric_name; nr++)
+ continue;
+ alloc_size = sizeof(*new_expr->metric_refs);
+ new_expr->metric_refs = calloc(nr + 1, alloc_size);
+ if (!new_expr->metric_refs) {
+ free(new_expr);
+ return -ENOMEM;
+ }
+
+ memcpy(new_expr->metric_refs, old_expr->metric_refs,
+ nr * alloc_size);
+ } else {
+ new_expr->metric_refs = NULL;
+ }
+
+ /* calculate number of metric_events */
+ for (nr = 0; old_expr->metric_events[nr]; nr++)
+ continue;
+ alloc_size = sizeof(*new_expr->metric_events);
+ new_expr->metric_events = calloc(nr + 1, alloc_size);
+ if (!new_expr->metric_events) {
+ free(new_expr->metric_refs);
+ free(new_expr);
+ return -ENOMEM;
+ }
+
+ /* copy evsel in the same position */
+ for (idx = 0; idx < nr; idx++) {
+ evsel = old_expr->metric_events[idx];
+ evsel = evlist__get_evsel(evlist, evsel->idx);
+ if (evsel == NULL) {
+ free(new_expr->metric_events);
+ free(new_expr->metric_refs);
+ free(new_expr);
+ return -EINVAL;
+ }
+ new_expr->metric_events[idx] = evsel;
+ }
+
+ list_add(&new_expr->nd, &new_me->head);
+ }
+ }
+ return 0;
+}
diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
index 491a5d78252d..ed1b9392e624 100644
--- a/tools/perf/util/metricgroup.h
+++ b/tools/perf/util/metricgroup.h
@@ -7,11 +7,13 @@
#include <stdbool.h>
#include "pmu-events/pmu-events.h"
+struct evlist;
struct evsel;
struct evlist;
struct option;
struct rblist;
struct pmu_events_map;
+struct cgroup;
struct metric_event {
struct rb_node nd;
@@ -55,4 +57,8 @@ void metricgroup__print(bool metrics, bool groups, char *filter,
bool metricgroup__has_metric(const char *metric);
int arch_get_runtimeparam(struct pmu_event *pe __maybe_unused);
void metricgroup__rblist_exit(struct rblist *metric_events);
+
+int metricgroup__copy_metric_events(struct evlist *evlist, struct cgroup *cgrp,
+ struct rblist *new_metric_events,
+ struct rblist *old_metric_events);
#endif
--
2.28.0.681.g6f77f65b4e-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups
2020-09-21 9:46 ` [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups Namhyung Kim
@ 2020-09-22 21:29 ` Jiri Olsa
2020-09-22 22:46 ` Namhyung Kim
0 siblings, 1 reply; 18+ messages in thread
From: Jiri Olsa @ 2020-09-22 21:29 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
Mark Rutland, Alexander Shishkin, LKML, Stephane Eranian,
Andi Kleen, Ian Rogers, John Garry, Kajol Jain
On Mon, Sep 21, 2020 at 06:46:08PM +0900, Namhyung Kim wrote:
SNIP
> @@ -260,6 +267,11 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> cgroup__put(cgrp);
> nr_cgroups++;
>
> + perf_stat__collect_metric_expr(tmp_list);
> + if (metricgroup__copy_metric_events(tmp_list, cgrp, metric_events,
> + &orig_metric_events) < 0)
> + break;
> +
> perf_evlist__splice_list_tail(evlist, &tmp_list->core.entries);
> tmp_list->core.nr_entries = 0;
>
> @@ -273,6 +285,7 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> out_err:
> evlist__delete(orig_list);
> evlist__delete(tmp_list);
> + rblist__exit(&orig_metric_events);
>
> return ret;
> }
> diff --git a/tools/perf/util/cgroup.h b/tools/perf/util/cgroup.h
> index 32893018296f..eea6df8ee373 100644
> --- a/tools/perf/util/cgroup.h
> +++ b/tools/perf/util/cgroup.h
> @@ -22,9 +22,11 @@ struct cgroup *cgroup__get(struct cgroup *cgroup);
> void cgroup__put(struct cgroup *cgroup);
>
> struct evlist;
> +struct rblist;
>
> struct cgroup *evlist__findnew_cgroup(struct evlist *evlist, const char *name);
> -int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups);
> +int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups,
> + struct rblist *metric_events);
>
> void evlist__set_default_cgroup(struct evlist *evlist, struct cgroup *cgroup);
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index ee7b576d3b12..424209c4bcd2 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1964,3 +1964,14 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
>
> return err;
> }
> +
> +struct evsel *evlist__get_evsel(struct evlist *evlist, int idx)
perhaps evlist__find_evsel is better name?
we have get/put names for functions changing refcount
jirka
> +{
> + struct evsel *evsel;
> +
> + evlist__for_each_entry(evlist, evsel) {
> + if (evsel->idx == idx)
> + return evsel;
> + }
> + return NULL;
> +}
SNIP
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups
2020-09-22 21:29 ` Jiri Olsa
@ 2020-09-22 22:46 ` Namhyung Kim
0 siblings, 0 replies; 18+ messages in thread
From: Namhyung Kim @ 2020-09-22 22:46 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
Mark Rutland, Alexander Shishkin, LKML, Stephane Eranian,
Andi Kleen, Ian Rogers, John Garry, Kajol Jain
On Wed, Sep 23, 2020 at 6:29 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Sep 21, 2020 at 06:46:08PM +0900, Namhyung Kim wrote:
>
> SNIP
>
> > @@ -260,6 +267,11 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> > cgroup__put(cgrp);
> > nr_cgroups++;
> >
> > + perf_stat__collect_metric_expr(tmp_list);
> > + if (metricgroup__copy_metric_events(tmp_list, cgrp, metric_events,
> > + &orig_metric_events) < 0)
> > + break;
> > +
> > perf_evlist__splice_list_tail(evlist, &tmp_list->core.entries);
> > tmp_list->core.nr_entries = 0;
> >
> > @@ -273,6 +285,7 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> > out_err:
> > evlist__delete(orig_list);
> > evlist__delete(tmp_list);
> > + rblist__exit(&orig_metric_events);
> >
> > return ret;
> > }
> > diff --git a/tools/perf/util/cgroup.h b/tools/perf/util/cgroup.h
> > index 32893018296f..eea6df8ee373 100644
> > --- a/tools/perf/util/cgroup.h
> > +++ b/tools/perf/util/cgroup.h
> > @@ -22,9 +22,11 @@ struct cgroup *cgroup__get(struct cgroup *cgroup);
> > void cgroup__put(struct cgroup *cgroup);
> >
> > struct evlist;
> > +struct rblist;
> >
> > struct cgroup *evlist__findnew_cgroup(struct evlist *evlist, const char *name);
> > -int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups);
> > +int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups,
> > + struct rblist *metric_events);
> >
> > void evlist__set_default_cgroup(struct evlist *evlist, struct cgroup *cgroup);
> >
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index ee7b576d3b12..424209c4bcd2 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -1964,3 +1964,14 @@ int evlist__ctlfd_process(struct evlist *evlist, enum evlist_ctl_cmd *cmd)
> >
> > return err;
> > }
> > +
> > +struct evsel *evlist__get_evsel(struct evlist *evlist, int idx)
>
> perhaps evlist__find_evsel is better name?
> we have get/put names for functions changing refcount
Looks better, will change.
Thanks
Namhyung
>
> > +{
> > + struct evsel *evsel;
> > +
> > + evlist__for_each_entry(evlist, evsel) {
> > + if (evsel->idx == idx)
> > + return evsel;
> > + }
> > + return NULL;
> > +}
>
> SNIP
>
^ permalink raw reply [flat|nested] 18+ messages in thread