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

[Namhyung Kim]

On Wed, Sep 23, 2020 at 7:23 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> On Wed, Sep 23, 2020 at 10:59:43AM +0900, Namhyung Kim wrote:
>
> SNIP
>
>
> > diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
> > index 8b6a4fa49082..dcd18ef268a1 100644
> > --- a/tools/perf/util/cgroup.c
> > +++ b/tools/perf/util/cgroup.c
> > @@ -3,6 +3,9 @@
> >  #include "evsel.h"
> >  #include "cgroup.h"
> >  #include "evlist.h"
> > +#include "rblist.h"
> > +#include "metricgroup.h"
> > +#include "stat.h"
> >  #include <linux/zalloc.h>
> >  #include <sys/types.h>
> >  #include <sys/stat.h>
> > @@ -193,10 +196,12 @@ int parse_cgroups(const struct option *opt, const char *str,
> >       return 0;
> >  }
> >
> > -int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> > +int evlist__expand_cgroup(struct evlist *evlist, const char *str,
> > +                       struct rblist *metric_events)
> >  {
> >       struct evlist *orig_list, *tmp_list;
> >       struct evsel *pos, *evsel, *leader;
> > +     struct rblist orig_metric_events;
> >       struct cgroup *cgrp = NULL;
> >       const char *p, *e, *eos = str + strlen(str);
> >       int ret = -1;
> > @@ -216,6 +221,8 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> >       /* save original events and init evlist */
> >       perf_evlist__splice_list_tail(orig_list, &evlist->core.entries);
> >       evlist->core.nr_entries = 0;
> > +     orig_metric_events = *metric_events;
> > +     rblist__init(metric_events);
> >
> >       for (;;) {
> >               p = strchr(str, ',');
> > @@ -255,6 +262,11 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> >               cgroup__put(cgrp);
> >               nr_cgroups++;
> >
> > +             perf_stat__collect_metric_expr(tmp_list);
>
> I know you added the option just for perf stat, not record,
> but the code looks generic apart from using this function

Right.  I've tried to make it generic as much as possible.
But the above code works for an evlist assuming all the
evsels are in the same cgroup as far as I can see.
So I put it here to pass the tmp_list for each cgroup
separately.

>
> I wonder if this would cause any issues if it was called in record
> context.. maybe we could just skip it in that case, but that's for
> future to worry about ;-)

Yeah, I believe we can just skip as it's all for metrics which is
used for perf stat only, I guess?

Thanks
Namhyung

>
> > +             if (metricgroup__copy_metric_events(tmp_list, cgrp, metric_events,
> > +                                                 &orig_metric_events) < 0)
> > +                     break;
> > +
> >               perf_evlist__splice_list_tail(evlist, &tmp_list->core.entries);
> >               tmp_list->core.nr_entries = 0;
> >
> > @@ -268,6 +280,7 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> >  out_err:
> >       evlist__delete(orig_list);
> >       evlist__delete(tmp_list);
> > +     rblist__exit(&orig_metric_events);
> >
> >       return ret;
> >  }
>
> SNIP
>