linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHSET v2 0/4] perf stat: Expand events for each cgroup
@ 2020-09-16  6:31 Namhyung Kim
  2020-09-16  6:31 ` [PATCH 1/4] perf evsel: Add evsel__clone() function Namhyung Kim
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Namhyung Kim @ 2020-09-16  6:31 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, Mark Rutland, Alexander Shishkin,
	Stephane Eranian, LKML, Andi Kleen, Ian Rogers

Hello,

When we profile cgroup events with perf stat, it's very annoying to
specify events and cgroups on the command line as it requires the
mapping between events and cgroups.  (Note that perf record can use
cgroup sampling but it's not usable for perf stat).

I guess most cases we just want to use a same set of events (N) for
all cgroups (M), but we need to specify NxM events and NxM cgroups.
This is not good especially when profiling large number of cgroups:
say M=200.

So I added --for-each-cgroup option to make it easy for that case.  It
will create NxM events from N events and M cgroups.  One more upside
is that it can handle metrics too.

For example, the following example measures IPC metric for 3 cgroups

  $ cat perf-expand-cgrp.sh
  #!/bin/sh
  
  METRIC=${1:-IPC}
  CGROUP_DIR=/sys/fs/cgroup/perf_event
  
  sudo mkdir $CGROUP_DIR/A $CGROUP_DIR/B $CGROUP_DIR/C
  
  # add backgroupd workload for each cgroup
  echo $$ | sudo tee $CGROUP_DIR/A/cgroup.procs > /dev/null
  yes > /dev/null &
  echo $$ | sudo tee $CGROUP_DIR/B/cgroup.procs > /dev/null
  yes > /dev/null &
  echo $$ | sudo tee $CGROUP_DIR/C/cgroup.procs > /dev/null
  yes > /dev/null &

  # run 'perf stat' in the root cgroup
  echo $$ | sudo tee $CGROUP_DIR/cgroup.procs > /dev/null
  perf stat -a -M $METRIC --for-each-cgroup A,B,C sleep 1
  
  kill %1 %2 %3
  sudo rmdir $CGROUP_DIR/A $CGROUP_DIR/B $CGROUP_DIR/C

  
  $ ./perf-expand-cgrp.sh IPC
  
   Performance counter stats for 'system wide':
  
      11,284,850,010      inst_retired.any          A #     2.71 IPC                    
       4,157,915,982      cpu_clk_unhalted.thread   A                                   
      11,342,188,640      inst_retired.any          B #     2.72 IPC                    
       4,173,014,732      cpu_clk_unhalted.thread   B                                   
      11,135,863,604      inst_retired.any          C #     2.67 IPC                    
       4,171,375,184      cpu_clk_unhalted.thread   C                                   
  
         1.011948803 seconds time elapsed


* Changes from v1:
 - rename the option to --for-each-cgroup  (Jiri)
 - copy evsel fields explicitly  (Jiri)
 - add libpfm4 test  (Ian)


The code is available at 'perf/cgroup-multiply-v2' branch on

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks
Namhyung


Namhyung Kim (4):
  perf evsel: Add evsel__clone() function
  perf stat: Add --for-each-cgroup option
  perf tools: Copy metric events properly when expand cgroups
  perf test: Add expand cgroup event test

 tools/perf/builtin-stat.c        |  21 ++-
 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 +
 tools/perf/util/cgroup.c         | 107 +++++++++++++-
 tools/perf/util/cgroup.h         |   3 +
 tools/perf/util/evlist.c         |  11 ++
 tools/perf/util/evlist.h         |   1 +
 tools/perf/util/evsel.c          |  85 +++++++++++
 tools/perf/util/evsel.h          |   1 +
 tools/perf/util/metricgroup.c    |  77 ++++++++++
 tools/perf/util/metricgroup.h    |   6 +
 tools/perf/util/stat.h           |   1 +
 14 files changed, 554 insertions(+), 6 deletions(-)
 create mode 100644 tools/perf/tests/expand-cgroup.c

-- 
2.28.0.618.gf4bc123cb7-goog


^ permalink raw reply	[flat|nested] 15+ messages in thread
* [PATCH 1/4] perf evsel: Add evsel__clone() function
  2020-09-16  6:31 [PATCHSET v2 0/4] perf stat: Expand events for each cgroup Namhyung Kim
@ 2020-09-16  6:31 ` Namhyung Kim
  2020-09-18 13:31   ` Jiri Olsa
  2020-09-16  6:31 ` [PATCH 2/4] perf stat: Add --for-each-cgroup option Namhyung Kim
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: Namhyung Kim @ 2020-09-16  6:31 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, Mark Rutland, Alexander Shishkin,
	Stephane Eranian, LKML, Andi Kleen, Ian Rogers

The evsel__clone() is to create an exactly same evsel from same
attributes.  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 | 85 +++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/evsel.h |  1 +
 2 files changed, 86 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index fd865002cbbd..29edef353036 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -331,6 +331,91 @@ struct evsel *evsel__new_cycles(bool precise)
 	goto out;
 }
 
+/**
+ * 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;
+	struct evsel_config_term *pos, *tmp;
+
+	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 (orig->group_name)
+		evsel->group_name = strdup(orig->group_name);
+	if (orig->pmu_name)
+		evsel->pmu_name = strdup(orig->pmu_name);
+	if (orig->filter)
+		evsel->filter = strdup(orig->filter);
+	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;
+
+	list_for_each_entry(pos, &orig->config_terms, list) {
+		tmp = malloc(sizeof(*tmp));
+		if (tmp == NULL) {
+			evsel__delete(evsel);
+			evsel = NULL;
+			break;
+		}
+
+		*tmp = *pos;
+		if (tmp->free_str) {
+			tmp->val.str = strdup(pos->val.str);
+			if (tmp->val.str == NULL) {
+				evsel__delete(evsel);
+				evsel = NULL;
+				free(tmp);
+				break;
+			}
+		}
+		list_add_tail(&tmp->list, &evsel->config_terms);
+	}
+
+	return evsel;
+}
+
 /*
  * 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..507c31d6a389 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -169,6 +169,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.618.gf4bc123cb7-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [PATCH 2/4] perf stat: Add --for-each-cgroup option
  2020-09-16  6:31 [PATCHSET v2 0/4] perf stat: Expand events for each cgroup Namhyung Kim
  2020-09-16  6:31 ` [PATCH 1/4] perf evsel: Add evsel__clone() function Namhyung Kim
@ 2020-09-16  6:31 ` Namhyung Kim
  2020-09-16 13:51   ` Arnaldo Carvalho de Melo
  2020-09-18 13:34   ` Jiri Olsa
  2020-09-16  6:31 ` [PATCH 3/4] perf tools: Copy metric events properly when expand cgroups Namhyung Kim
  2020-09-16  6:31 ` [PATCH 4/4] perf test: Add expand cgroup event test Namhyung Kim
  3 siblings, 2 replies; 15+ messages in thread
From: Namhyung Kim @ 2020-09-16  6:31 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, Mark Rutland, Alexander Shishkin,
	Stephane Eranian, LKML, 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/builtin-stat.c | 20 +++++++++-
 tools/perf/util/cgroup.c  | 79 +++++++++++++++++++++++++++++++++++++++
 tools/perf/util/cgroup.h  |  1 +
 tools/perf/util/stat.h    |  1 +
 4 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 7f8d756d9408..a43e58e0a088 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1051,6 +1051,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"),
@@ -1094,7 +1105,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,
@@ -2234,6 +2247,11 @@ int cmd_stat(int argc, const char **argv)
 	if (add_default_attributes())
 		goto out;
 
+	if (stat_config.cgroup_list) {
+		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..01e0a6207207 100644
--- a/tools/perf/util/cgroup.c
+++ b/tools/perf/util/cgroup.c
@@ -12,6 +12,7 @@
 #include <api/fs/fs.h>
 
 int nr_cgroups;
+bool multiply_cgroup;
 
 static int open_cgroup(const char *name)
 {
@@ -156,6 +157,10 @@ int parse_cgroups(const struct option *opt, const char *str,
 		return -1;
 	}
 
+	/* delay processing cgroups after it sees all events */
+	if (multiply_cgroup)
+		return 0;
+
 	for (;;) {
 		p = strchr(str, ',');
 		e = p ? p : eos;
@@ -193,6 +198,80 @@ 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)
+				break;
+
+			cgrp = cgroup__new(name);
+			free(name);
+			if (cgrp == NULL)
+				break;
+		} else {
+			cgrp = NULL;
+		}
+
+		leader = NULL;
+		evlist__for_each_entry(orig_list, pos) {
+			evsel = evsel__clone(pos);
+			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;
+	}
+	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 9911fc6adbfd..7325de5bf2a6 100644
--- a/tools/perf/util/stat.h
+++ b/tools/perf/util/stat.h
@@ -137,6 +137,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.618.gf4bc123cb7-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [PATCH 3/4] perf tools: Copy metric events properly when expand cgroups
  2020-09-16  6:31 [PATCHSET v2 0/4] perf stat: Expand events for each cgroup Namhyung Kim
  2020-09-16  6:31 ` [PATCH 1/4] perf evsel: Add evsel__clone() function Namhyung Kim
  2020-09-16  6:31 ` [PATCH 2/4] perf stat: Add --for-each-cgroup option Namhyung Kim
@ 2020-09-16  6:31 ` Namhyung Kim
  2020-09-18 13:45   ` Jiri Olsa
  2020-09-16  6:31 ` [PATCH 4/4] perf test: Add expand cgroup event test Namhyung Kim
  3 siblings, 1 reply; 15+ messages in thread
From: Namhyung Kim @ 2020-09-16  6:31 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, Mark Rutland, Alexander Shishkin,
	Stephane Eranian, LKML, 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 | 77 +++++++++++++++++++++++++++++++++++
 tools/perf/util/metricgroup.h |  6 +++
 7 files changed, 114 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 01e0a6207207..a1bf345a770b 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, ',');
@@ -257,6 +264,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)
 	}
 	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..869605285022 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,79 @@ 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);
+				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.618.gf4bc123cb7-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [PATCH 4/4] perf test: Add expand cgroup event test
  2020-09-16  6:31 [PATCHSET v2 0/4] perf stat: Expand events for each cgroup Namhyung Kim
                   ` (2 preceding siblings ...)
  2020-09-16  6:31 ` [PATCH 3/4] perf tools: Copy metric events properly when expand cgroups Namhyung Kim
@ 2020-09-16  6:31 ` Namhyung Kim
  2020-09-18 13:51   ` Jiri Olsa
  3 siblings, 1 reply; 15+ messages in thread
From: Namhyung Kim @ 2020-09-16  6:31 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, Mark Rutland, Alexander Shishkin,
	Stephane Eranian, LKML, 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/builtin-stat.c        |   2 +-
 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 +
 tools/perf/util/cgroup.c         |  19 ++-
 tools/perf/util/cgroup.h         |   2 +-
 7 files changed, 261 insertions(+), 9 deletions(-)
 create mode 100644 tools/perf/tests/expand-cgroup.c

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 8b81d62ab18b..f00600d9903e 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2249,7 +2249,7 @@ int cmd_stat(int argc, const char **argv)
 
 	if (stat_config.cgroup_list) {
 		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/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);
diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
index a1bf345a770b..eeffa08251b5 100644
--- a/tools/perf/util/cgroup.c
+++ b/tools/perf/util/cgroup.c
@@ -52,7 +52,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));
 
@@ -62,9 +62,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;
@@ -80,7 +85,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)
@@ -202,7 +207,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)
 				break;
 
-			cgrp = cgroup__new(name);
+			cgrp = cgroup__new(name, open_cgroup);
 			free(name);
 			if (cgrp == NULL)
 				break;
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.618.gf4bc123cb7-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] perf stat: Add --for-each-cgroup option
  2020-09-16  6:31 ` [PATCH 2/4] perf stat: Add --for-each-cgroup option Namhyung Kim
@ 2020-09-16 13:51   ` Arnaldo Carvalho de Melo
  2020-09-17  1:33     ` Namhyung Kim
  2020-09-18 13:34   ` Jiri Olsa
  1 sibling, 1 reply; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-09-16 13:51 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Jiri Olsa, Ingo Molnar, Peter Zijlstra, Mark Rutland,
	Alexander Shishkin, Stephane Eranian, LKML, Andi Kleen,
	Ian Rogers

Em Wed, Sep 16, 2020 at 03:31:27PM +0900, Namhyung Kim escreveu:
> The --for-each-cgroup option is a syntax sugar to monitor large number

You forgot to add the man page entry for this new option.

- Arnaldo

> 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/builtin-stat.c | 20 +++++++++-
>  tools/perf/util/cgroup.c  | 79 +++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/cgroup.h  |  1 +
>  tools/perf/util/stat.h    |  1 +
>  4 files changed, 100 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 7f8d756d9408..a43e58e0a088 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1051,6 +1051,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"),
> @@ -1094,7 +1105,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,
> @@ -2234,6 +2247,11 @@ int cmd_stat(int argc, const char **argv)
>  	if (add_default_attributes())
>  		goto out;
>  
> +	if (stat_config.cgroup_list) {
> +		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..01e0a6207207 100644
> --- a/tools/perf/util/cgroup.c
> +++ b/tools/perf/util/cgroup.c
> @@ -12,6 +12,7 @@
>  #include <api/fs/fs.h>
>  
>  int nr_cgroups;
> +bool multiply_cgroup;
>  
>  static int open_cgroup(const char *name)
>  {
> @@ -156,6 +157,10 @@ int parse_cgroups(const struct option *opt, const char *str,
>  		return -1;
>  	}
>  
> +	/* delay processing cgroups after it sees all events */
> +	if (multiply_cgroup)
> +		return 0;
> +
>  	for (;;) {
>  		p = strchr(str, ',');
>  		e = p ? p : eos;
> @@ -193,6 +198,80 @@ 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)
> +				break;
> +
> +			cgrp = cgroup__new(name);
> +			free(name);
> +			if (cgrp == NULL)
> +				break;
> +		} else {
> +			cgrp = NULL;
> +		}
> +
> +		leader = NULL;
> +		evlist__for_each_entry(orig_list, pos) {
> +			evsel = evsel__clone(pos);
> +			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;
> +	}
> +	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 9911fc6adbfd..7325de5bf2a6 100644
> --- a/tools/perf/util/stat.h
> +++ b/tools/perf/util/stat.h
> @@ -137,6 +137,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.618.gf4bc123cb7-goog
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] perf stat: Add --for-each-cgroup option
  2020-09-16 13:51   ` Arnaldo Carvalho de Melo
@ 2020-09-17  1:33     ` Namhyung Kim
  0 siblings, 0 replies; 15+ messages in thread
From: Namhyung Kim @ 2020-09-17  1:33 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Jiri Olsa, Ingo Molnar, Peter Zijlstra, Mark Rutland,
	Alexander Shishkin, Stephane Eranian, LKML, Andi Kleen,
	Ian Rogers

On Wed, Sep 16, 2020 at 10:51 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Wed, Sep 16, 2020 at 03:31:27PM +0900, Namhyung Kim escreveu:
> > The --for-each-cgroup option is a syntax sugar to monitor large number
>
> You forgot to add the man page entry for this new option.

OK, will add.. any more comments?

Thanks
Namhyung

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

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] perf evsel: Add evsel__clone() function
  2020-09-16  6:31 ` [PATCH 1/4] perf evsel: Add evsel__clone() function Namhyung Kim
@ 2020-09-18 13:31   ` Jiri Olsa
  2020-09-21  5:44     ` Namhyung Kim
  0 siblings, 1 reply; 15+ messages in thread
From: Jiri Olsa @ 2020-09-18 13:31 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
	Mark Rutland, Alexander Shishkin, Stephane Eranian, LKML,
	Andi Kleen, Ian Rogers

On Wed, Sep 16, 2020 at 03:31:26PM +0900, Namhyung Kim wrote:

SNIP

> +struct evsel *evsel__clone(struct evsel *orig)
> +{
> +	struct evsel *evsel;
> +	struct evsel_config_term *pos, *tmp;
> +
> +	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 (orig->group_name)
> +		evsel->group_name = strdup(orig->group_name);
> +	if (orig->pmu_name)
> +		evsel->pmu_name = strdup(orig->pmu_name);
> +	if (orig->filter)
> +		evsel->filter = strdup(orig->filter);

we should check those strdup results

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

so all those evsel's members are possibly defined in parse time right?
perhaps we should separate them in the struct? and make some note about
evsel__clone function that new members should be considered for copy
in evsel__close.. or something like that

> +
> +	list_for_each_entry(pos, &orig->config_terms, list) {
> +		tmp = malloc(sizeof(*tmp));
> +		if (tmp == NULL) {
> +			evsel__delete(evsel);
> +			evsel = NULL;
> +			break;
> +		}
> +
> +		*tmp = *pos;
> +		if (tmp->free_str) {
> +			tmp->val.str = strdup(pos->val.str);
> +			if (tmp->val.str == NULL) {
> +				evsel__delete(evsel);
> +				evsel = NULL;
> +				free(tmp);
> +				break;
> +			}
> +		}
> +		list_add_tail(&tmp->list, &evsel->config_terms);
> +	}

could this go in separate function? copy_terms

thanks,
jirka

> +
> +	return evsel;
> +}
> +
>  /*
>   * 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..507c31d6a389 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -169,6 +169,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.618.gf4bc123cb7-goog
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] perf stat: Add --for-each-cgroup option
  2020-09-16  6:31 ` [PATCH 2/4] perf stat: Add --for-each-cgroup option Namhyung Kim
  2020-09-16 13:51   ` Arnaldo Carvalho de Melo
@ 2020-09-18 13:34   ` Jiri Olsa
  2020-09-21  5:45     ` Namhyung Kim
  1 sibling, 1 reply; 15+ messages in thread
From: Jiri Olsa @ 2020-09-18 13:34 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
	Mark Rutland, Alexander Shishkin, Stephane Eranian, LKML,
	Andi Kleen, Ian Rogers

On Wed, Sep 16, 2020 at 03:31:27PM +0900, Namhyung Kim wrote:

SNIP

> +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)
> +				break;
> +
> +			cgrp = cgroup__new(name);
> +			free(name);
> +			if (cgrp == NULL)
> +				break;
> +		} else {
> +			cgrp = NULL;
> +		}
> +
> +		leader = NULL;
> +		evlist__for_each_entry(orig_list, pos) {
> +			evsel = evsel__clone(pos);

missing check on evsel == NULL

jirka

> +			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;
> +	}
> +	evlist__delete(orig_list);
> +	evlist__delete(tmp_list);
> +
> +	return ret;
> +}

SNIP


^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] perf tools: Copy metric events properly when expand cgroups
  2020-09-16  6:31 ` [PATCH 3/4] perf tools: Copy metric events properly when expand cgroups Namhyung Kim
@ 2020-09-18 13:45   ` Jiri Olsa
  2020-09-21  5:48     ` Namhyung Kim
  0 siblings, 1 reply; 15+ messages in thread
From: Jiri Olsa @ 2020-09-18 13:45 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
	Mark Rutland, Alexander Shishkin, Stephane Eranian, LKML,
	Andi Kleen, Ian Rogers, John Garry, Kajol Jain

On Wed, Sep 16, 2020 at 03:31:28PM +0900, Namhyung Kim wrote:

SNIP

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

this probably won't happen, but evlist__get_evsel can return NULL

jirka

> +				new_expr->metric_events[idx] = evsel;
> +			}
> +
> +			list_add(&new_expr->nd, &new_me->head);
> +		}
> +	}
> +	return 0;
> +}

SNIP


^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] perf test: Add expand cgroup event test
  2020-09-16  6:31 ` [PATCH 4/4] perf test: Add expand cgroup event test Namhyung Kim
@ 2020-09-18 13:51   ` Jiri Olsa
  2020-09-21  5:49     ` Namhyung Kim
  0 siblings, 1 reply; 15+ messages in thread
From: Jiri Olsa @ 2020-09-18 13:51 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
	Mark Rutland, Alexander Shishkin, Stephane Eranian, LKML,
	Andi Kleen, Ian Rogers, John Garry

On Wed, Sep 16, 2020 at 03:31:29PM +0900, Namhyung Kim wrote:

SNIP

> +++ 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);
> diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
> index a1bf345a770b..eeffa08251b5 100644
> --- a/tools/perf/util/cgroup.c
> +++ b/tools/perf/util/cgroup.c
> @@ -52,7 +52,7 @@ static struct cgroup *evlist__find_cgroup(struct evlist *evlist, const char *str
>  	return NULL;
>  }


could you please put the do_open change below into separate patch?

thanks,
jirka

>  
> -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));
>  
> @@ -62,9 +62,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;
> @@ -80,7 +85,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)
> @@ -202,7 +207,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)
>  				break;
>  
> -			cgrp = cgroup__new(name);
> +			cgrp = cgroup__new(name, open_cgroup);
>  			free(name);
>  			if (cgrp == NULL)
>  				break;
> 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.618.gf4bc123cb7-goog
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] perf evsel: Add evsel__clone() function
  2020-09-18 13:31   ` Jiri Olsa
@ 2020-09-21  5:44     ` Namhyung Kim
  0 siblings, 0 replies; 15+ messages in thread
From: Namhyung Kim @ 2020-09-21  5:44 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
	Mark Rutland, Alexander Shishkin, Stephane Eranian, LKML,
	Andi Kleen, Ian Rogers

Hi Jiri,

On Fri, Sep 18, 2020 at 10:31 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Sep 16, 2020 at 03:31:26PM +0900, Namhyung Kim wrote:
>
> SNIP
>
> > +struct evsel *evsel__clone(struct evsel *orig)
> > +{
> > +     struct evsel *evsel;
> > +     struct evsel_config_term *pos, *tmp;
> > +
> > +     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 (orig->group_name)
> > +             evsel->group_name = strdup(orig->group_name);
> > +     if (orig->pmu_name)
> > +             evsel->pmu_name = strdup(orig->pmu_name);
> > +     if (orig->filter)
> > +             evsel->filter = strdup(orig->filter);
>
> we should check those strdup results

ok.

>
> > +     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;
>
> so all those evsel's members are possibly defined in parse time right?
> perhaps we should separate them in the struct? and make some note about
> evsel__clone function that new members should be considered for copy
> in evsel__close.. or something like that

Sounds good.

>
> > +
> > +     list_for_each_entry(pos, &orig->config_terms, list) {
> > +             tmp = malloc(sizeof(*tmp));
> > +             if (tmp == NULL) {
> > +                     evsel__delete(evsel);
> > +                     evsel = NULL;
> > +                     break;
> > +             }
> > +
> > +             *tmp = *pos;
> > +             if (tmp->free_str) {
> > +                     tmp->val.str = strdup(pos->val.str);
> > +                     if (tmp->val.str == NULL) {
> > +                             evsel__delete(evsel);
> > +                             evsel = NULL;
> > +                             free(tmp);
> > +                             break;
> > +                     }
> > +             }
> > +             list_add_tail(&tmp->list, &evsel->config_terms);
> > +     }
>
> could this go in separate function? copy_terms

Will do.

Thanks
Namhyung

>
> > +
> > +     return evsel;
> > +}
> > +
> >  /*
> >   * 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..507c31d6a389 100644
> > --- a/tools/perf/util/evsel.h
> > +++ b/tools/perf/util/evsel.h
> > @@ -169,6 +169,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.618.gf4bc123cb7-goog
> >
>

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] perf stat: Add --for-each-cgroup option
  2020-09-18 13:34   ` Jiri Olsa
@ 2020-09-21  5:45     ` Namhyung Kim
  0 siblings, 0 replies; 15+ messages in thread
From: Namhyung Kim @ 2020-09-21  5:45 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
	Mark Rutland, Alexander Shishkin, Stephane Eranian, LKML,
	Andi Kleen, Ian Rogers

On Fri, Sep 18, 2020 at 10:34 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Sep 16, 2020 at 03:31:27PM +0900, Namhyung Kim wrote:
>
> SNIP
>
> > +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)
> > +                             break;
> > +
> > +                     cgrp = cgroup__new(name);
> > +                     free(name);
> > +                     if (cgrp == NULL)
> > +                             break;
> > +             } else {
> > +                     cgrp = NULL;
> > +             }
> > +
> > +             leader = NULL;
> > +             evlist__for_each_entry(orig_list, pos) {
> > +                     evsel = evsel__clone(pos);
>
> missing check on evsel == NULL

Will check.

Thanks
Namhyung


> > +                     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;
> > +     }
> > +     evlist__delete(orig_list);
> > +     evlist__delete(tmp_list);
> > +
> > +     return ret;
> > +}
>
> SNIP
>

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] perf tools: Copy metric events properly when expand cgroups
  2020-09-18 13:45   ` Jiri Olsa
@ 2020-09-21  5:48     ` Namhyung Kim
  0 siblings, 0 replies; 15+ messages in thread
From: Namhyung Kim @ 2020-09-21  5:48 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
	Mark Rutland, Alexander Shishkin, Stephane Eranian, LKML,
	Andi Kleen, Ian Rogers, John Garry, Kajol Jain

On Fri, Sep 18, 2020 at 10:45 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Sep 16, 2020 at 03:31:28PM +0900, Namhyung Kim wrote:
>
> SNIP
>
> > +                                     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);
>
> this probably won't happen, but evlist__get_evsel can return NULL

Hmm.. yeah, it should not happen.  Anyway I'll check the result.

Thanks
Namhyung

>
> > +                             new_expr->metric_events[idx] = evsel;
> > +                     }
> > +
> > +                     list_add(&new_expr->nd, &new_me->head);
> > +             }
> > +     }
> > +     return 0;
> > +}
>
> SNIP
>

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] perf test: Add expand cgroup event test
  2020-09-18 13:51   ` Jiri Olsa
@ 2020-09-21  5:49     ` Namhyung Kim
  0 siblings, 0 replies; 15+ messages in thread
From: Namhyung Kim @ 2020-09-21  5:49 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
	Mark Rutland, Alexander Shishkin, Stephane Eranian, LKML,
	Andi Kleen, Ian Rogers, John Garry

On Fri, Sep 18, 2020 at 10:52 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Sep 16, 2020 at 03:31:29PM +0900, Namhyung Kim wrote:
>
> SNIP
>
> > +++ 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);
> > diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
> > index a1bf345a770b..eeffa08251b5 100644
> > --- a/tools/perf/util/cgroup.c
> > +++ b/tools/perf/util/cgroup.c
> > @@ -52,7 +52,7 @@ static struct cgroup *evlist__find_cgroup(struct evlist *evlist, const char *str
> >       return NULL;
> >  }
>
>
> could you please put the do_open change below into separate patch?

Ok, will do.

Thanks
Namhyung

> >
> > -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));
> >
> > @@ -62,9 +62,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;
> > @@ -80,7 +85,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)
> > @@ -202,7 +207,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)
> >                               break;
> >
> > -                     cgrp = cgroup__new(name);
> > +                     cgrp = cgroup__new(name, open_cgroup);
> >                       free(name);
> >                       if (cgrp == NULL)
> >                               break;
> > 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.618.gf4bc123cb7-goog
> >
>

^ permalink raw reply	[flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-09-21  5:49 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-16  6:31 [PATCHSET v2 0/4] perf stat: Expand events for each cgroup Namhyung Kim
2020-09-16  6:31 ` [PATCH 1/4] perf evsel: Add evsel__clone() function Namhyung Kim
2020-09-18 13:31   ` Jiri Olsa
2020-09-21  5:44     ` Namhyung Kim
2020-09-16  6:31 ` [PATCH 2/4] perf stat: Add --for-each-cgroup option Namhyung Kim
2020-09-16 13:51   ` Arnaldo Carvalho de Melo
2020-09-17  1:33     ` Namhyung Kim
2020-09-18 13:34   ` Jiri Olsa
2020-09-21  5:45     ` Namhyung Kim
2020-09-16  6:31 ` [PATCH 3/4] perf tools: Copy metric events properly when expand cgroups Namhyung Kim
2020-09-18 13:45   ` Jiri Olsa
2020-09-21  5:48     ` Namhyung Kim
2020-09-16  6:31 ` [PATCH 4/4] perf test: Add expand cgroup event test Namhyung Kim
2020-09-18 13:51   ` Jiri Olsa
2020-09-21  5:49     ` Namhyung Kim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).