Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups

2020-09-28 Thread Jiri Olsa 
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  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 
> 
> I take this is for the entire patchset, right?

yes

jirka

Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups

2020-09-28 Thread Arnaldo Carvalho de Melo 
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 
> Cc: Kajol Jain 
> Cc: Ian Rogers 
> Reported-by: kernel test robot 
> Signed-off-by: Namhyung Kim 
> ---
>  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,
> +   _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 
>  #include 
>  #include 
> @@ -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, >core.entries);
>   evlist->core.nr_entries = 0;
>  
> + if (metric_events) {
> + orig_metric_events = *metric_events;
> + rblist__init(metric_events);
> + } else {
> + rblist__init(_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,
> + 
> _metric_events) < 0)
> + break;
> + }
> +
>   perf_evlist__splice_list_tail(evlist, _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(_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

Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups

2020-09-28 Thread Arnaldo Carvalho de Melo 
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  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 

I take this is for the entire patchset, right?

- Arnaldo

Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups

2020-09-25 Thread Jiri Olsa 
On Fri, Sep 25, 2020 at 10:44:53PM +0900, Namhyung Kim wrote:
> On Fri, Sep 25, 2020 at 10:26 PM Jiri Olsa  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(_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,
> > > + 
> > > _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 

thanks,
jirka

Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups

2020-09-25 Thread Namhyung Kim 
On Fri, Sep 25, 2020 at 10:26 PM Jiri Olsa  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(_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,
> > + 
> > _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

Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups

2020-09-25 Thread Jiri Olsa 
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(_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,
> + 
> _metric_events) < 0)
> + break;
> + }

looks good, do you plan to actualy add support for record?
my ack from last version stays

thanks,
jirka

[PATCH 3/5] perf tools: Copy metric events properly when expand cgroups

2020-09-24 Thread Namhyung Kim 
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 
Cc: Kajol Jain 
Cc: Ian Rogers 
Reported-by: kernel test robot 
Signed-off-by: Namhyung Kim 
---
 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,
+ _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 
 #include 
 #include 
@@ -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, >core.entries);
evlist->core.nr_entries = 0;
 
+   if (metric_events) {
+   orig_metric_events = *metric_events;
+   rblist__init(metric_events);
+   } else {
+   rblist__init(_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,
+   
_metric_events) < 0)
+   break;
+   }
+
perf_evlist__splice_list_tail(evlist, _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(_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;
+
+

Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups

2020-09-23 Thread Namhyung Kim 
On Wed, Sep 23, 2020 at 7:23 PM Jiri Olsa  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 
> >  #include 
> >  #include 
> > @@ -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, >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,
> > + _metric_events) < 0)
> > + break;
> > +
> >   perf_evlist__splice_list_tail(evlist, 
> > _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(_metric_events);
> >
> >   return ret;
> >  }
>
> SNIP
>

Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups

2020-09-23 Thread Jiri Olsa 
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 
>  #include 
>  #include 
> @@ -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, >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,
> + _metric_events) < 0)
> + break;
> +
>   perf_evlist__splice_list_tail(evlist, _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(_metric_events);
>  
>   return ret;
>  }

SNIP

[PATCH 3/5] perf tools: Copy metric events properly when expand cgroups

2020-09-22 Thread Namhyung Kim 
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 
Cc: Kajol Jain 
Cc: Ian Rogers 
Signed-off-by: Namhyung Kim 
---
 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,
+ _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 
 #include 
 #include 
@@ -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, >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,
+   _metric_events) < 0)
+   break;
+
perf_evlist__splice_list_tail(evlist, _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(_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

Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups

2020-09-22 Thread Namhyung Kim 
On Wed, Sep 23, 2020 at 6:29 AM Jiri Olsa  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,
> > + _metric_events) < 0)
> > + break;
> > +
> >   perf_evlist__splice_list_tail(evlist, 
> > _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(_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
>

Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups

2020-09-22 Thread Jiri Olsa 
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,
> + _metric_events) < 0)
> + break;
> +
>   perf_evlist__splice_list_tail(evlist, _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(_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

[PATCH 3/5] perf tools: Copy metric events properly when expand cgroups

2020-09-21 Thread Namhyung Kim 
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 
Cc: Kajol Jain 
Cc: Ian Rogers 
Signed-off-by: Namhyung Kim 
---
 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,
+ _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 
 #include 
 #include 
@@ -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, >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,
+   _metric_events) < 0)
+   break;
+
perf_evlist__splice_list_tail(evlist, _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(_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