Re: [PATCH v4 2/5] perf stat: Basic support for iostat in perf

From: Namhyung Kim
Date: Wed Feb 10 2021 - 05:22:44 EST

Next message: Michal Hocko: "Re: [PATCH 3/3] mm: Fix missing mem cgroup soft limit tree updates"
Previous message: tip-bot2 for Arnd Bergmann: "[tip: timers/core] clocksource/drivers/tango: Remove tango driver"
In reply to: Alexander Antonov: "Re: [PATCH v4 2/5] perf stat: Basic support for iostat in perf"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Feb 8, 2021 at 8:55 PM Alexander Antonov
<alexander.antonov@xxxxxxxxxxxxxxx> wrote:
>
>
> On 2/4/2021 3:22 PM, Namhyung Kim wrote:
> > On Wed, Feb 3, 2021 at 10:58 PM Alexander Antonov
> > <alexander.antonov@xxxxxxxxxxxxxxx> wrote:
> >> Add basic flow for a new iostat mode in perf. Mode is intended to
> >> provide four I/O performance metrics per each PCIe root port: Inbound Read,
> >> Inbound Write, Outbound Read, Outbound Write.
> >>
> >> The actual code to compute the metrics and attribute it to
> >> root port is in follow-on patches.
> >>
> >> Signed-off-by: Alexander Antonov <alexander.antonov@xxxxxxxxxxxxxxx>
> >> ---
> >> tools/perf/builtin-stat.c | 31 ++++++++++++++++++++++++++
> >> tools/perf/util/iostat.h | 32 +++++++++++++++++++++++++++
> >> tools/perf/util/stat-display.c | 40 +++++++++++++++++++++++++++++++++-
> >> tools/perf/util/stat-shadow.c | 11 +++++++++-
> >> tools/perf/util/stat.h | 1 +
> >> 5 files changed, 113 insertions(+), 2 deletions(-)
> >> create mode 100644 tools/perf/util/iostat.h
> >>
> >> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> >> index 60fdb6a0805f..66c913692120 100644
> >> --- a/tools/perf/builtin-stat.c
> >> +++ b/tools/perf/builtin-stat.c
> >> @@ -65,6 +65,7 @@
> >> #include "util/target.h"
> >> #include "util/time-utils.h"
> >> #include "util/top.h"
> >> +#include "util/iostat.h"
> >> #include "asm/bug.h"
> >>
> >> #include <linux/time64.h>
> >> @@ -186,6 +187,7 @@ static struct perf_stat_config stat_config = {
> >> .metric_only_len = METRIC_ONLY_LEN,
> >> .walltime_nsecs_stats = &walltime_nsecs_stats,
> >> .big_num = true,
> >> + .iostat_run = false,
> >> };
> >>
> >> static inline void diff_timespec(struct timespec *r, struct timespec *a,
> >> @@ -723,6 +725,14 @@ static int parse_metric_groups(const struct option *opt,
> >> return metricgroup__parse_groups(opt, str, &stat_config.metric_events);
> >> }
> >>
> >> +__weak int iostat_parse(const struct option *opt __maybe_unused,
> >> + const char *str __maybe_unused,
> >> + int unset __maybe_unused)
> >> +{
> >> + pr_err("iostat mode is not supported\n");
> >> + return -1;
> >> +}
> >> +
> >> static struct option stat_options[] = {
> >> OPT_BOOLEAN('T', "transaction", &transaction_run,
> >> "hardware transaction statistics"),
> >> @@ -803,6 +813,8 @@ static struct option stat_options[] = {
> >> OPT_CALLBACK('M', "metrics", &evsel_list, "metric/metric group list",
> >> "monitor specified metrics or metric groups (separated by ,)",
> >> parse_metric_groups),
> >> + OPT_CALLBACK_OPTARG(0, "iostat", &evsel_list, &stat_config, "root port",
> >> + "measure PCIe metrics per root port", iostat_parse),
> > Can we make the help string and default argument more generic?
> > Something like "measure IO metrics provided by arch/platform"
> > and the default value being "default". :)
> >
> Do you mean using "default" instead of "root port"?
> What about the faceless "I/O unit"? :)

Being a generic command, I cannot expect how it can be used later.
So I'd suggest a more general name.

> >> OPT_END()
> >> };
> >>
> >> @@ -1131,6 +1143,12 @@ __weak void arch_topdown_group_warn(void)
> >> {
> >> }
> >>
> >> +__weak int iostat_list(struct evlist *evlist __maybe_unused,
> >> + struct perf_stat_config *config __maybe_unused)
> >> +{
> >> + return 0;
> >> +}
> >> +
> >> /*
> >> * Add default attributes, if there were no attributes specified or
> >> * if -d/--detailed, -d -d or -d -d -d is used:
> >> @@ -1682,6 +1700,10 @@ static void setup_system_wide(int forks)
> >> }
> >> }
> >>
> >> +__weak void iostat_release(struct evlist *evlist __maybe_unused)
> >> +{
> >> +}
> >> +
> >> int cmd_stat(int argc, const char **argv)
> >> {
> >> const char * const stat_usage[] = {
> >> @@ -1858,6 +1880,12 @@ int cmd_stat(int argc, const char **argv)
> >> goto out;
> >> }
> >>
> >> + if (stat_config.iostat_run) {
> >> + status = iostat_list(evsel_list, &stat_config);
> > I think it's unnatural to call iostat_list() unconditionally here.
> > How about this?
> >
> > status = iostat_prepare(...);
> > if (status < 0)
> > goto out;
> >
> > if (status == IOSTAT_LIST)
> > iostat_list(...);
> > else
> > ...
> I think it's applicable.
> In case of 'list' option we will just print list of root ports and exit.
> Also listing of root ports is available in verbose mode. In this case we
> will
> print list and start the collection.

ok.

> >
> >> + if (status || !stat_config.iostat_run)
> >> + goto out;
> >> + }
> >> +
> >> if (add_default_attributes())
> >> goto out;
> >>
> >> @@ -2008,6 +2036,9 @@ int cmd_stat(int argc, const char **argv)
> >> perf_stat__exit_aggr_mode();
> >> perf_evlist__free_stats(evsel_list);
> >> out:
> >> + if (stat_config.iostat_run)
> >> + iostat_release(evsel_list);
> >> +
> >> zfree(&stat_config.walltime_run);
> >>
> >> if (smi_cost && smi_reset)
> >> diff --git a/tools/perf/util/iostat.h b/tools/perf/util/iostat.h
> >> new file mode 100644
> >> index 000000000000..b34ebedfd5e6
> >> --- /dev/null
> >> +++ b/tools/perf/util/iostat.h
> >> @@ -0,0 +1,32 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * perf iostat
> >> + *
> >> + * Copyright (C) 2020, Intel Corporation
> >> + *
> >> + * Authors: Alexander Antonov <alexander.antonov@xxxxxxxxxxxxxxx>
> >> + */
> >> +
> >> +#ifndef _IOSTAT_H
> >> +#define _IOSTAT_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 iostat_parse(const struct option *opt, const char *str,
> >> + int unset __maybe_unused);
> >> +void iostat_prefix(struct perf_stat_config *config, struct evlist *evlist,
> >> + char *prefix, struct timespec *ts);
> >> +void iostat_print_metric(struct perf_stat_config *config, struct evsel *evsel,
> >> + struct perf_stat_output_ctx *out);
> >> +int iostat_list(struct evlist *evlist, struct perf_stat_config *config);
> >> +void iostat_release(struct evlist *evlist);
> >> +
> >> +#endif /* _IOSTAT_H */
> >> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> >> index db1bec115d0b..de78cf6962b9 100644
> >> --- a/tools/perf/util/stat-display.c
> >> +++ b/tools/perf/util/stat-display.c
> >> @@ -16,6 +16,7 @@
> >> #include <linux/ctype.h>
> >> #include "cgroup.h"
> >> #include <api/fs/fs.h>
> >> +#include "iostat.h"
> >>
> >> #define CNTR_NOT_SUPPORTED "<not supported>"
> >> #define CNTR_NOT_COUNTED "<not counted>"
> >> @@ -302,6 +303,11 @@ static void print_metric_header(struct perf_stat_config *config,
> >> struct outstate *os = ctx;
> >> char tbuf[1024];
> >>
> >> + /* In case of iostat, print metric header for first root port only */
> >> + if (config->iostat_run &&
> >> + os->evsel->priv != os->evsel->evlist->selected->priv)
> >> + return;
> >> +
> >> if (!valid_only_metric(unit))
> >> return;
> >> unit = fixunit(tbuf, os->evsel, unit);
> >> @@ -936,6 +942,8 @@ static void print_metric_headers(struct perf_stat_config *config,
> >> fputs("time,", config->output);
> >> fputs(aggr_header_csv[config->aggr_mode], config->output);
> >> }
> >> + if (config->iostat_run && !config->interval && !config->csv_output)
> >> + fprintf(config->output, " port ");
> > It's too specific to the current implementation.
> > Let's make it a callback or a weak function.
> Okay,
> This and other similar blocks will be updated.

ok

> >
> >> /* Print metrics headers only */
> >> evlist__for_each_entry(evlist, counter) {
> >> @@ -954,6 +962,13 @@ static void print_metric_headers(struct perf_stat_config *config,
> >> fputc('\n', config->output);
> >> }
> >>
> >> +__weak void iostat_prefix(struct perf_stat_config *config __maybe_unused,
> >> + struct evlist *evlist __maybe_unused,
> >> + char *prefix __maybe_unused,
> >> + struct timespec *ts __maybe_unused)
> >> +{
> >> +}
> >> +
> >> static void print_interval(struct perf_stat_config *config,
> >> struct evlist *evlist,
> >> char *prefix, struct timespec *ts)
> >> @@ -966,7 +981,10 @@ static void print_interval(struct perf_stat_config *config,
> >> if (config->interval_clear)
> >> puts(CONSOLE_CLEAR);
> >>
> >> - sprintf(prefix, "%6lu.%09lu%s", ts->tv_sec, ts->tv_nsec, config->csv_sep);
> >> + if (!config->iostat_run)
> >> + sprintf(prefix, "%6lu.%09lu%s", ts->tv_sec,
> >> + ts->tv_nsec,
> >> + config->csv_sep);
> >>
> >> if ((num_print_interval == 0 && !config->csv_output) || config->interval_clear) {
> >> switch (config->aggr_mode) {
> >> @@ -996,6 +1014,7 @@ static void print_interval(struct perf_stat_config *config,
> >> fprintf(output, " counts %*s events\n", unit_width, "unit");
> >> break;
> >> case AGGR_PCIE_PORT:
> >> + fprintf(output, "# time port ");
> > Ditto.
> >
> >> break;
> >> case AGGR_GLOBAL:
> >> default:
> >> @@ -1174,6 +1193,10 @@ perf_evlist__print_counters(struct evlist *evlist,
> >> int interval = config->interval;
> >> struct evsel *counter;
> >> char buf[64], *prefix = NULL;
> >> + void *perf_device = NULL;
> >> +
> >> + if (config->iostat_run)
> >> + evlist->selected = evlist__first(evlist);
> >>
> >> if (interval)
> >> print_interval(config, evlist, prefix = buf, ts);
> >> @@ -1222,6 +1245,21 @@ perf_evlist__print_counters(struct evlist *evlist,
> >> }
> >> break;
> >> case AGGR_PCIE_PORT:
> > Ditto. Something like iostat_print_counters().
> >
> >> + counter = evlist__first(evlist);
> >> + perf_evlist__set_selected(evlist, counter);
> >> + iostat_prefix(config, evlist, prefix = buf, ts);
> >> + fprintf(config->output, "%s", prefix);
> >> + evlist__for_each_entry(evlist, counter) {
> >> + perf_device = evlist->selected->priv;
> >> + if (perf_device && perf_device != counter->priv) {
> >> + perf_evlist__set_selected(evlist, counter);
> >> + iostat_prefix(config, evlist, prefix, ts);
> >> + fprintf(config->output, "\n%s", prefix);
> >> + }
> >> + print_counter_aggr(config, counter, prefix);
> > I'm not sure but do you assume each counter has different priv?
> > I don't know if the output is correct (like call iostat_prefix() once
> > and call print_counter_aggr() twice) when they have same one.
> There are 4 counters which are related to single 'priv' field (it's used
> for root
> port object in iostat mode). This means if platform has, for example, 5 root
> ports we will have 20 counters in summary. And print_counter_aggr() will be
> called for each counter.
> I call iostat_prefix() before the loop to print first root port and then
> iostat_prefix() will be called when next counter is related to other
> root port.

Thanks for the explanation.

> >
> >> + if ((counter->idx + 1) == evlist->core.nr_entries)
> >> + fputc('\n', config->output);
> > Can we just move this out of the loop?
> >
> Yes, you are right, we can. I will update it.

Thanks,
Namhyung

> >
> >
> >> + }
> >> break;
> >> case AGGR_UNSET:
> >> default:
> >> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> >> index 2c41d47f6f83..083a450c6dc7 100644
> >> --- a/tools/perf/util/stat-shadow.c
> >> +++ b/tools/perf/util/stat-shadow.c
> >> @@ -9,6 +9,7 @@
> >> #include "expr.h"
> >> #include "metricgroup.h"
> >> #include <linux/zalloc.h>
> >> +#include "iostat.h"
> >>
> >> /*
> >> * AGGR_GLOBAL: Use CPU 0
> >> @@ -814,6 +815,12 @@ static void generic_metric(struct perf_stat_config *config,
> >> zfree(&pctx.ids[i].name);
> >> }
> >>
> >> +__weak void iostat_print_metric(struct perf_stat_config *config __maybe_unused,
> >> + struct evsel *evsel __maybe_unused,
> >> + struct perf_stat_output_ctx *out __maybe_unused)
> >> +{
> >> +}
> >> +
> >> void perf_stat__print_shadow_stats(struct perf_stat_config *config,
> >> struct evsel *evsel,
> >> double avg, int cpu,
> >> @@ -829,7 +836,9 @@ void perf_stat__print_shadow_stats(struct perf_stat_config *config,
> >> struct metric_event *me;
> >> int num = 1;
> >>
> >> - if (perf_evsel__match(evsel, HARDWARE, HW_INSTRUCTIONS)) {
> >> + if (config->iostat_run) {
> >> + iostat_print_metric(config, evsel, out);
> >> + } else if (perf_evsel__match(evsel, HARDWARE, HW_INSTRUCTIONS)) {
> >> total = runtime_stat_avg(st, STAT_CYCLES, ctx, cpu);
> >>
> >> if (total) {
> >> diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
> >> index c7544c28c02a..c2a2b28effd6 100644
> >> --- a/tools/perf/util/stat.h
> >> +++ b/tools/perf/util/stat.h
> >> @@ -107,6 +107,7 @@ struct perf_stat_config {
> >> bool big_num;
> >> bool no_merge;
> >> bool walltime_run_table;
> >> + bool iostat_run;
> >> FILE *output;
> >> unsigned int interval;
> >> unsigned int timeout;
> >> --
> >> 2.19.1
> >>

Next message: Michal Hocko: "Re: [PATCH 3/3] mm: Fix missing mem cgroup soft limit tree updates"
Previous message: tip-bot2 for Arnd Bergmann: "[tip: timers/core] clocksource/drivers/tango: Remove tango driver"
In reply to: Alexander Antonov: "Re: [PATCH v4 2/5] perf stat: Basic support for iostat in perf"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]