Re: [RFC PATCH v3 2/6] perf stat: Fork and launch perf record when perf stat needs to get retire latency value for a metric.

[Namhyung Kim]

Hello Weilin,

On Fri, Mar 1, 2024 at 4:11 PM <weilin.wang@xxxxxxxxx> wrote:
>
> From: Weilin Wang <weilin.wang@xxxxxxxxx>
>
> When retire_latency value is used in a metric formula, perf stat would fork a
> perf record process with "-e" and "-W" options. Perf record will collect
> required retire_latency values in parallel while perf stat is collecting
> counting values.
>
> At the point of time that perf stat stops counting, it would send sigterm signal
> to perf record process and receiving sampling data back from perf record from a
> pipe. Perf stat will then process the received data to get retire latency data
> and calculate metric result.
>
> Signed-off-by: Weilin Wang <weilin.wang@xxxxxxxxx>
> ---
>  tools/perf/builtin-stat.c     | 179 +++++++++++++++++++++++++++++++++-
>  tools/perf/util/data.c        |   4 +
>  tools/perf/util/data.h        |   1 +
>  tools/perf/util/metricgroup.h |   7 ++
>  tools/perf/util/stat.h        |   3 +
>  5 files changed, 191 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 5a3093541cff..3890a579349e 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -94,8 +94,13 @@
>  #include <perf/evlist.h>
>  #include <internal/threadmap.h>
>
> +#include "util/sample.h"
> +#include <sys/param.h>
> +#include <subcmd/run-command.h>
> +
>  #define DEFAULT_SEPARATOR      " "
>  #define FREEZE_ON_SMI_PATH     "devices/cpu/freeze_on_smi"
> +#define PERF_DATA              "-"
>
>  static void print_counters(struct timespec *ts, int argc, const char **argv);
>
> @@ -162,7 +167,8 @@ static struct perf_stat_config stat_config = {
>         .ctl_fd                 = -1,
>         .ctl_fd_ack             = -1,
>         .iostat_run             = false,
> -       .tpebs_event_size = 0,
> +       .tpebs_event_size       = 0,
> +       .tpebs_pid              = -1,
>  };
>
>  static bool cpus_map_matched(struct evsel *a, struct evsel *b)
> @@ -687,12 +693,163 @@ static enum counter_recovery stat_handle_error(struct evsel *counter)
>         return COUNTER_FATAL;
>  }
>
> -static int __run_perf_record(void)
> +static int __run_perf_record(const char **record_argv)
>  {
> +       int i = 0;
> +       struct tpebs_event *e;

Please put a blank line after the declaration.


>         pr_debug("Prepare perf record for retire_latency\n");
> +
> +

A duplicate new line.

> +       record_argv[i++] = "perf";
> +       record_argv[i++] = "record";
> +       record_argv[i++] = "-W";
> +
> +       if (stat_config.user_requested_cpu_list) {
> +               record_argv[i++] = "-C";
> +               record_argv[i++] = stat_config.user_requested_cpu_list;
> +       }
> +
> +       if (stat_config.system_wide)
> +               record_argv[i++] = "-a";
> +
> +       list_for_each_entry(e, &stat_config.tpebs_events, nd) {
> +               record_argv[i++] = "-e";
> +               record_argv[i++] = e->name;
> +       }
> +
> +       record_argv[i++] = "-o";
> +       record_argv[i++] = PERF_DATA;

I don't think you need side-band records and synthesizing for this.
I'd like to disable all of them but it'd require changes in perf record.
For now, you need to pass --synth=no at least.

Thanks,
Namhyung


> +
>         return 0;
>  }