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: <20201223130320.3930-1-alexander.antonov@linux.intel.com>
 <20201223130320.3930-4-alexander.antonov@linux.intel.com> <CAM9d7ciA3MzvzobN=_NEChKwet+RzHUu3gf+KTzdXcvTmiChLw@mail.gmail.com>
 <64c262e4-fc97-c200-6983-81d966e922e0@linux.intel.com>
In-Reply-To: <64c262e4-fc97-c200-6983-81d966e922e0@linux.intel.com>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Thu, 14 Jan 2021 12:34:08 +0900
Message-ID: <CAM9d7ci5qSnm1V4VCpdZn+b5uPajs27uAV+J-+W2QHPCbCohTQ@mail.gmail.com>
Subject: Re: [PATCH v2 3/6] perf stat: Basic support for iiostat in perf
To:     Alexander Antonov <alexander.antonov@linux.intel.com>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Jiri Olsa <jolsa@redhat.com>, Andi Kleen <ak@linux.intel.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Ian Rogers <irogers@google.com>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hello,

On Wed, Jan 13, 2021 at 8:34 PM Alexander Antonov
<alexander.antonov@linux.intel.com> wrote:
>
>
> On 1/6/2021 11:56 AM, Namhyung Kim wrote:
> > On Wed, Dec 23, 2020 at 10:03 PM Alexander Antonov
> > <alexander.antonov@linux.intel.com> wrote:
> >> Add basic flow for a new iiostat mode in perf. Mode is intended to
> >> provide four I/O performance metrics per each IIO stack: Inbound Read,
> >> Inbound Write, Outbound Read, Outbound Write.
> > It seems like a generic analysis and other archs can extend it later..
> > Then we can make it a bit more general.. at least, names? :)
> I'm not sure that I fully understand you. Do you mean to rename metrics?
> The mode is intended to provide PCIe metrics which are appliable for
> other archs
> as well.
> Actually, I suppose we can rename 'iiostat' to 'pciestat' or something
> like this
> to make it a bit more general because the name 'IIO' (Integrated I/O
> stack) is
> Intel specific and it can be named in different way on other platforms.
> In this
> case the code has to be updated in the same way as well.

Maybe just 'iostat' ?

> >
> >> The actual code to compute the metrics and attribute it to
> >> evsel::perf_device is in follow-on patches.
> >>
> >> Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
> >> ---
> >>   tools/perf/builtin-stat.c      | 33 ++++++++++++++++++++++++++++-
> >>   tools/perf/util/iiostat.h      | 33 +++++++++++++++++++++++++++++
> >>   tools/perf/util/stat-display.c | 38 +++++++++++++++++++++++++++++++++-
> >>   tools/perf/util/stat-shadow.c  | 11 +++++++++-
> >>   tools/perf/util/stat.h         |  1 +
> >>   5 files changed, 113 insertions(+), 3 deletions(-)
> >>   create mode 100644 tools/perf/util/iiostat.h
> >>
> >> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> >> index 72f9d0aa3f96..14c3da136927 100644
> >> --- a/tools/perf/builtin-stat.c
> >> +++ b/tools/perf/builtin-stat.c
> >> @@ -67,6 +67,7 @@
> >>   #include "util/top.h"
> >>   #include "util/affinity.h"
> >>   #include "util/pfm.h"
> >> +#include "util/iiostat.h"
> >>   #include "asm/bug.h"
> >>
> >>   #include <linux/time64.h>
> >> @@ -198,7 +199,8 @@ static struct perf_stat_config stat_config = {
> >>          .walltime_nsecs_stats   = &walltime_nsecs_stats,
> >>          .big_num                = true,
> >>          .ctl_fd                 = -1,
> >> -       .ctl_fd_ack             = -1
> >> +       .ctl_fd_ack             = -1,
> >> +       .iiostat_run            = false,
> >>   };
> >>
> >>   static bool cpus_map_matched(struct evsel *a, struct evsel *b)
> >> @@ -1073,6 +1075,14 @@ static int parse_stat_cgroups(const struct option *opt,
> >>          return parse_cgroups(opt, str, unset);
> >>   }
> >>
> >> +__weak int iiostat_parse(const struct option *opt __maybe_unused,
> >> +                        const char *str __maybe_unused,
> >> +                        int unset __maybe_unused)
> >> +{
> >> +       pr_err("iiostat mode is not supported\n");
> >> +       return -1;
> >> +}
> >> +
> >>   static struct option stat_options[] = {
> >>          OPT_BOOLEAN('T', "transaction", &transaction_run,
> >>                      "hardware transaction statistics"),
> >> @@ -1185,6 +1195,8 @@ static struct option stat_options[] = {
> >>                       "\t\t\t  Optionally send control command completion ('ack\\n') to ack-fd descriptor.\n"
> >>                       "\t\t\t  Alternatively, ctl-fifo / ack-fifo will be opened and used as ctl-fd / ack-fd.",
> >>                        parse_control_option),
> >> +       OPT_CALLBACK_OPTARG(0, "iiostat", &evsel_list, &stat_config, "root port",
> >> +                           "measure PCIe metrics per IIO stack", iiostat_parse),
> >>          OPT_END()
> >>   };
> >>
> >> @@ -1509,6 +1521,12 @@ static int perf_stat_init_aggr_mode_file(struct perf_stat *st)
> >>          return 0;
> >>   }
> >>
> >> +__weak int iiostat_show_root_ports(struct evlist *evlist __maybe_unused,
> >> +                                  struct perf_stat_config *config __maybe_unused)
> >> +{
> >> +       return 0;
> >> +}
> > I think it's too specific, maybe iiostat_prepare() ?
> What do you think about iiostat_show_root_ports() -> iiostat_show()?

I'm ok with it, I thought it needs some initialization work there.

> >
> >> +
> >>   /*
> >>    * Add default attributes, if there were no attributes specified or
> >>    * if -d/--detailed, -d -d or -d -d -d is used:
> >> @@ -2054,6 +2072,10 @@ static void setup_system_wide(int forks)
> >>          }
> >>   }
> >>
> >> +__weak void iiostat_delete_root_ports(struct evlist *evlist __maybe_unused)
> >> +{
> >> +}
> > Same here..
> I suggest to rename iiostat_delete_root_ports() -> iiostat_release().
> What do you think?

Looks good.

> >
> >> +
> >>   int cmd_stat(int argc, const char **argv)
> >>   {
> >>          const char * const stat_usage[] = {
> >> @@ -2230,6 +2252,12 @@ int cmd_stat(int argc, const char **argv)
> >>                  goto out;
> >>          }
> >>
> >> +       if (stat_config.iiostat_run) {
> >> +               status = iiostat_show_root_ports(evsel_list, &stat_config);
> >> +               if (status || !stat_config.iiostat_run)
> >> +                       goto out;
> >> +       }
> >> +
> >>          if (add_default_attributes())
> >>                  goto out;
> >>

[SNIP]
> >> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> >> index 3bfcdb80443a..9eb8484e8b90 100644
> >> --- a/tools/perf/util/stat-display.c
> >> +++ b/tools/perf/util/stat-display.c
> >> @@ -17,6 +17,7 @@
> >>   #include "cgroup.h"
> >>   #include <api/fs/fs.h>
> >>   #include "util.h"
> >> +#include "iiostat.h"
> >>
> >>   #define CNTR_NOT_SUPPORTED     "<not supported>"
> >>   #define CNTR_NOT_COUNTED       "<not counted>"
> >> @@ -310,6 +311,12 @@ static void print_metric_header(struct perf_stat_config *config,
> >>          struct outstate *os = ctx;
> >>          char tbuf[1024];
> >>
> >> +       /* In case of iiostat, print metric header for first perf_device only */
> >> +       if (os->evsel->perf_device && os->evsel->evlist->selected->perf_device &&
> >> +           config->iiostat_run &&
> > When is the perf_device set?  Is it possible to be NULL in the iiostat mode?
> >
> The perf_device field is initialized inside iiostat.c::iiostat_event_group()
> and it cannot be NULL.
> The idea is to attribute events to PCIe ports through perf_device field.
>

If it's guaranteed non-NULL, we can check config->iiostat_run only and make
the condition simpler.

Thanks,
Namhyung


> >> +           os->evsel->perf_device != os->evsel->evlist->selected->perf_device)
> >> +               return;
> >> +
> >>          if (!valid_only_metric(unit))
> >>                  return;
> >>          unit = fixunit(tbuf, os->evsel, unit);