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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM9d7ci2Arf8vNVRruF9dqOtxH4km2b1MKCVHzj8x+QazzY2mA@mail.gmail.com>
Date:   Thu, 24 Sep 2020 12:12:23 +0900
From:   Namhyung Kim <namhyung@...nel.org>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Stephane Eranian <eranian@...gle.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Ian Rogers <irogers@...gle.com>,
        John Garry <john.garry@...wei.com>,
        Kajol Jain <kjain@...ux.ibm.com>
Subject: Re: [PATCH 3/5] perf tools: Copy metric events properly when expand cgroups

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

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

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

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

Thanks
Namhyung

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