| 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
| ||
|
Date: Wed, 13 Jan 2021 14:34:30 +0300
From: Alexander Antonov <alexander.antonov@...ux.intel.com>
To: Namhyung Kim <namhyung@...nel.org>
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
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.
>
>> 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()?
>
>> +
>> /*
>> * 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?
>
>> +
>> 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;
>>
>> @@ -2406,6 +2434,9 @@ int cmd_stat(int argc, const char **argv)
>> perf_stat__exit_aggr_mode();
>> perf_evlist__free_stats(evsel_list);
>> out:
>> + if (stat_config.iiostat_run)
>> + iiostat_delete_root_ports(evsel_list);
>> +
>> zfree(&stat_config.walltime_run);
>>
>> if (smi_cost && smi_reset)
>> diff --git a/tools/perf/util/iiostat.h b/tools/perf/util/iiostat.h
>> new file mode 100644
>> index 000000000000..8d4226df9975
>> --- /dev/null
>> +++ b/tools/perf/util/iiostat.h
>> @@ -0,0 +1,33 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * perf iiostat
>> + *
>> + * Copyright (C) 2020, Intel Corporation
>> + *
>> + * Authors: Alexander Antonov <alexander.antonov@...ux.intel.com>
>> + */
>> +
>> +#ifndef _IIOSTAT_H
>> +#define _IIOSTAT_H
>> +
>> +#include <subcmd/parse-options.h>
>> +#include "util/stat.h"
>> +#include "util/parse-events.h"
>> +#include "util/evlist.h"
>> +
>> +struct option;
>> +struct perf_stat_config;
>> +struct evlist;
>> +struct timespec;
>> +
>> +int iiostat_parse(const struct option *opt, const char *str,
>> + int unset __maybe_unused);
>> +void iiostat_prefix(struct perf_stat_config *config, struct evlist *evlist,
>> + char *prefix, struct timespec *ts);
>> +void iiostat_print_metric(struct perf_stat_config *config, struct evsel *evsel,
>> + struct perf_stat_output_ctx *out);
>> +int iiostat_show_root_ports(struct evlist *evlist,
>> + struct perf_stat_config *config);
>> +void iiostat_delete_root_ports(struct evlist *evlist);
>> +
>> +#endif /* _IIOSTAT_H */
>> 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?
>
> Thanks,
> Namhyung
>
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.
Thanks,
Alexander
>> + 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