| 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
| ||
|
Message-ID: <CAM9d7ci5qSnm1V4VCpdZn+b5uPajs27uAV+J-+W2QHPCbCohTQ@mail.gmail.com>
Date: Thu, 14 Jan 2021 12:34:08 +0900
From: Namhyung Kim <namhyung@...nel.org>
To: Alexander Antonov <alexander.antonov@...ux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
Jiri Olsa <jolsa@...hat.com>, Andi Kleen <ak@...ux.intel.com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Mark Rutland <mark.rutland@....com>,
Ian Rogers <irogers@...gle.com>,
Ingo Molnar <mingo@...hat.com>,
Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v2 3/6] perf stat: Basic support for iiostat in perf
Hello,
On Wed, Jan 13, 2021 at 8:34 PM Alexander Antonov
<alexander.antonov@...ux.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@...ux.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@...ux.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);
Powered by blists - more mailing lists