Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200921094610.83736-1-namhyung@kernel.org> <20200921094610.83736-3-namhyung@kernel.org>
In-Reply-To: <20200921094610.83736-3-namhyung@kernel.org>
From:   Stephane Eranian <eranian@google.com>
Date:   Tue, 22 Sep 2020 14:52:09 -0700
Message-ID: <CABPqkBRnzGbfk0_K_eK47XDU9Fu4CTJC=4RXTXYc=cFLE-xtkg@mail.gmail.com>
Subject: Re: [PATCH 2/5] perf stat: Add --for-each-cgroup option
To:     Namhyung Kim <namhyung@kernel.org>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Jiri Olsa <jolsa@redhat.com>, Ingo Molnar <mingo@kernel.org>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Andi Kleen <ak@linux.intel.com>,
        Ian Rogers <irogers@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Mon, Sep 21, 2020 at 2:46 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> 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
>
You could also do it by keeping the -G option and providing
--for-each-cgroup as a modifier
of the behavior of -G:

$ ./perf stat -a -e cpu-clock,cycles,instructions --for-each-cgroup -G
 A,B sleep 1

That way, you do not have to handle the case where both are used.
And it makes transitioning to the new style simpler, i.e., the -G
option remains, just need
to trim the number of cgroups to 200 in your example.

Just a suggestion.

>    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  | 84 +++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/cgroup.h  |  1 +
>  tools/perf/util/stat.h    |  1 +
>  4 files changed, 105 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..e4916ed740ac 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,85 @@ int parse_cgroups(const struct option *opt, const char *str,
>         return 0;
>  }
>
> +int evlist__expand_cgroup(struct evlist *evlist, const char *str)
> +{
> +       struct evlist *orig_list, *tmp_list;
> +       struct evsel *pos, *evsel, *leader;
> +       struct cgroup *cgrp = NULL;
> +       const char *p, *e, *eos = str + strlen(str);
> +       int ret = -1;
> +
> +       if (evlist->core.nr_entries == 0) {
> +               fprintf(stderr, "must define events before cgroups\n");
> +               return -EINVAL;
> +       }
> +
> +       orig_list = evlist__new();
> +       tmp_list = evlist__new();
> +       if (orig_list == NULL || tmp_list == NULL) {
> +               fprintf(stderr, "memory allocation failed\n");
> +               return -ENOMEM;
> +       }
> +
> +       /* save original events and init evlist */
> +       perf_evlist__splice_list_tail(orig_list, &evlist->core.entries);
> +       evlist->core.nr_entries = 0;
> +
> +       for (;;) {
> +               p = strchr(str, ',');
> +               e = p ? p : eos;
> +
> +               /* allow empty cgroups, i.e., skip */
> +               if (e - str) {
> +                       /* termination added */
> +                       char *name = strndup(str, e - str);
> +                       if (!name)
> +                               goto out_err;
> +
> +                       cgrp = cgroup__new(name);
> +                       free(name);
> +                       if (cgrp == NULL)
> +                               goto out_err;
> +               } else {
> +                       cgrp = NULL;
> +               }
> +
> +               leader = NULL;
> +               evlist__for_each_entry(orig_list, pos) {
> +                       evsel = evsel__clone(pos);
> +                       if (evsel == NULL)
> +                               goto out_err;
> +
> +                       cgroup__put(evsel->cgrp);
> +                       evsel->cgrp = cgroup__get(cgrp);
> +
> +                       if (evsel__is_group_leader(pos))
> +                               leader = evsel;
> +                       evsel->leader = leader;
> +
> +                       evlist__add(tmp_list, evsel);
> +               }
> +               /* cgroup__new() has a refcount, release it here */
> +               cgroup__put(cgrp);
> +               nr_cgroups++;
> +
> +               perf_evlist__splice_list_tail(evlist, &tmp_list->core.entries);
> +               tmp_list->core.nr_entries = 0;
> +
> +               if (!p) {
> +                       ret = 0;
> +                       break;
> +               }
> +               str = p+1;
> +       }
> +
> +out_err:
> +       evlist__delete(orig_list);
> +       evlist__delete(tmp_list);
> +
> +       return ret;
> +}
> +
>  static struct cgroup *__cgroup__findnew(struct rb_root *root, uint64_t id,
>                                         bool create, const char *path)
>  {
> diff --git a/tools/perf/util/cgroup.h b/tools/perf/util/cgroup.h
> index e98d5975fe55..32893018296f 100644
> --- a/tools/perf/util/cgroup.h
> +++ b/tools/perf/util/cgroup.h
> @@ -24,6 +24,7 @@ void cgroup__put(struct cgroup *cgroup);
>  struct evlist;
>
>  struct cgroup *evlist__findnew_cgroup(struct evlist *evlist, const char *name);
> +int evlist__expand_cgroup(struct evlist *evlist, const char *cgroups);
>
>  void evlist__set_default_cgroup(struct evlist *evlist, struct cgroup *cgroup);
>
> diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
> index 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.681.g6f77f65b4e-goog
>