Re: [PATCH V2 3/4] perf stat: Support new per thread TopDown metrics

[Liang, Kan]

On 9/10/2020 11:37 PM, Namhyung Kim wrote:
> Hello,
>
> On Thu, Sep 10, 2020 at 10:48 PM <kan.liang@xxxxxxxxxxxxxxx> wrote:
> > From: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> >
> > Icelake has support for reporting per thread TopDown metrics.
> > These are reported differently than the previous TopDown support,
> > each metric is standalone, but scaled to pipeline "slots".
> > We don't need to do anything special for HyperThreading anymore.
> > Teach perf stat --topdown to handle these new metrics and
> > print them in the same way as the previous TopDown metrics.
> > The restrictions of only being able to report information per core is
> > gone.
> >
> > Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Co-developed-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> > Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> > Signed-off-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> > ---
> >   tools/perf/Documentation/perf-stat.txt |  7 +-
> >   tools/perf/builtin-stat.c              | 30 ++++++++-
> >   tools/perf/util/stat-shadow.c          | 89 ++++++++++++++++++++++++++
> >   tools/perf/util/stat.c                 |  4 ++
> >   tools/perf/util/stat.h                 |  8 +++
> >   5 files changed, 134 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
> > index c9bfefc051fb..e803dbdc88a8 100644
> > --- a/tools/perf/Documentation/perf-stat.txt
> > +++ b/tools/perf/Documentation/perf-stat.txt
> > @@ -357,6 +357,11 @@ if the workload is actually bound by the CPU and not by something else.
> >   For best results it is usually a good idea to use it with interval
> >   mode like -I 1000, as the bottleneck of workloads can change often.
> >
> > +This enables --metric-only, unless overridden with --no-metric-only.
> > +
> > +The following restrictions only apply to older Intel CPUs and Atom,
> > +on newer CPUs (IceLake and later) TopDown can be collected for any thread:
> > +
> >   The top down metrics are collected per core instead of per
> >   CPU thread. Per core mode is automatically enabled
> >   and -a (global monitoring) is needed, requiring root rights or
> > @@ -368,8 +373,6 @@ echo 0 > /proc/sys/kernel/nmi_watchdog
> >   for best results. Otherwise the bottlenecks may be inconsistent
> >   on workload with changing phases.
> >
> > -This enables --metric-only, unless overridden with --no-metric-only.
> > -
> >   To interpret the results it is usually needed to know on which
> >   CPUs the workload runs on. If needed the CPUs can be forced using
> >   taskset.
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index 5583e22ca808..6290da5bd142 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -128,6 +128,15 @@ static const char * topdown_attrs[] = {
> >          NULL,
> >   };
> >
> > +static const char *topdown_metric_attrs[] = {
> > +       "slots",
> > +       "topdown-retiring",
> > +       "topdown-bad-spec",
> > +       "topdown-fe-bound",
> > +       "topdown-be-bound",
> > +       NULL,
> > +};
> > +
> >   static const char *smi_cost_attrs = {
> >          "{"
> >          "msr/aperf/,"
> > @@ -1691,6 +1700,24 @@ static int add_default_attributes(void)
> >                  char *str = NULL;
> >                  bool warn = false;
> >
> > +               if (!force_metric_only)
> > +                       stat_config.metric_only = true;
> > +
> > +               if (topdown_filter_events(topdown_metric_attrs, &str, 1) < 0) {
> > +                       pr_err("Out of memory\n");
> > +                       return -1;
> > +               }
> > +               if (topdown_metric_attrs[0] && str) {
> > +                       if (!stat_config.interval && !stat_config.metric_only) {
> > +                               fprintf(stat_config.output,
> > +                                       "Topdown accuracy may decrease when measuring long periods.\n"
> > +                                       "Please print the result regularly, e.g. -I1000\n");
> > +                       }
> > +                       goto setup_metrics;
> > +               }
> > +
> > +               str = NULL;
>
> zfree(&str) ?

Yes, even the topdown events don't exist, str is still allocated.
The str should be free.

I will send V3 to fix it shortly.

Thanks,
Kan

> Thanks
> Namhyung
>
>
> > +
> >                  if (stat_config.aggr_mode != AGGR_GLOBAL &&
> >                      stat_config.aggr_mode != AGGR_CORE) {
> >                          pr_err("top down event configuration requires --per-core mode\n");
> > @@ -1702,8 +1729,6 @@ static int add_default_attributes(void)
> >                          return -1;
> >                  }
> >
> > -               if (!force_metric_only)
> > -                       stat_config.metric_only = true;
> >                  if (topdown_filter_events(topdown_attrs, &str,
> >                                  arch_topdown_check_group(&warn)) < 0) {
> >                          pr_err("Out of memory\n");
> > @@ -1712,6 +1737,7 @@ static int add_default_attributes(void)
> >                  if (topdown_attrs[0] && str) {
> >                          if (warn)
> >                                  arch_topdown_group_warn();
> > +setup_metrics:
> >                          err = parse_events(evsel_list, str, &errinfo);
> >                          if (err) {
> >                                  fprintf(stderr,