Date:   Wed, 1 Jun 2022 23:21:34 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jiri Olsa <jolsa@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-perf-users@...r.kernel.org, Will Deacon <will@...nel.org>,
        Waiman Long <longman@...hat.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Davidlohr Bueso <dave@...olabs.net>
Subject: Re: [PATCH 2/5] perf lock: Add lock contention tracepoints record support

On Tue, May 31, 2022 at 11:58 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> When LOCKDEP and LOCK_STAT events are not available, it falls back to
> record the new lock contention tracepoints.
>
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> ---
>  tools/perf/builtin-lock.c | 70 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 63 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 23a33ac15e68..3e3320b8cede 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -495,6 +495,12 @@ struct trace_lock_handler {
>
>         int (*release_event)(struct evsel *evsel,
>                              struct perf_sample *sample);
> +
> +       int (*contention_begin_event)(struct evsel *evsel,
> +                                     struct perf_sample *sample);
> +
> +       int (*contention_end_event)(struct evsel *evsel,
> +                                   struct perf_sample *sample);

Would it make sense to add a comment here about LOCKDEP and LOCK_STAT?
It could be confusing if the handler isn't called on different
kernels.

Thanks,
Ian

>  };
>
>  static struct lock_seq_stat *get_seq(struct thread_stat *ts, u64 addr)
> @@ -822,6 +828,20 @@ static int evsel__process_lock_release(struct evsel *evsel, struct perf_sample *
>         return 0;
>  }
>
> +static int evsel__process_contention_begin(struct evsel *evsel, struct perf_sample *sample)
> +{
> +       if (trace_handler->contention_begin_event)
> +               return trace_handler->contention_begin_event(evsel, sample);
> +       return 0;
> +}
> +
> +static int evsel__process_contention_end(struct evsel *evsel, struct perf_sample *sample)
> +{
> +       if (trace_handler->contention_end_event)
> +               return trace_handler->contention_end_event(evsel, sample);
> +       return 0;
> +}
> +
>  static void print_bad_events(int bad, int total)
>  {
>         /* Output for debug, this have to be removed */
> @@ -1023,6 +1043,11 @@ static const struct evsel_str_handler lock_tracepoints[] = {
>         { "lock:lock_release",   evsel__process_lock_release,   }, /* CONFIG_LOCKDEP */
>  };
>
> +static const struct evsel_str_handler contention_tracepoints[] = {
> +       { "lock:contention_begin", evsel__process_contention_begin, },
> +       { "lock:contention_end",   evsel__process_contention_end,   },
> +};
> +
>  static bool force;
>
>  static int __cmd_report(bool display_info)
> @@ -1086,20 +1111,41 @@ static int __cmd_record(int argc, const char **argv)
>                 "record", "-R", "-m", "1024", "-c", "1", "--synth", "task",
>         };
>         unsigned int rec_argc, i, j, ret;
> +       unsigned int nr_tracepoints;
>         const char **rec_argv;
> +       bool has_lock_stat = true;
>
>         for (i = 0; i < ARRAY_SIZE(lock_tracepoints); i++) {
>                 if (!is_valid_tracepoint(lock_tracepoints[i].name)) {
> -                               pr_err("tracepoint %s is not enabled. "
> -                                      "Are CONFIG_LOCKDEP and CONFIG_LOCK_STAT enabled?\n",
> -                                      lock_tracepoints[i].name);
> -                               return 1;
> +                       pr_debug("tracepoint %s is not enabled. "
> +                                "Are CONFIG_LOCKDEP and CONFIG_LOCK_STAT enabled?\n",
> +                                lock_tracepoints[i].name);
> +                       has_lock_stat = false;
> +                       break;
> +               }
> +       }
> +
> +       if (has_lock_stat)
> +               goto setup_args;
> +
> +       for (i = 0; i < ARRAY_SIZE(contention_tracepoints); i++) {
> +               if (!is_valid_tracepoint(contention_tracepoints[i].name)) {
> +                       pr_err("tracepoint %s is not enabled.\n",
> +                              contention_tracepoints[i].name);
> +                       return 1;
>                 }
>         }
>
> +setup_args:
>         rec_argc = ARRAY_SIZE(record_args) + argc - 1;
> +
> +       if (has_lock_stat)
> +               nr_tracepoints = ARRAY_SIZE(lock_tracepoints);
> +       else
> +               nr_tracepoints = ARRAY_SIZE(contention_tracepoints);
> +
>         /* factor of 2 is for -e in front of each tracepoint */
> -       rec_argc += 2 * ARRAY_SIZE(lock_tracepoints);
> +       rec_argc += 2 * nr_tracepoints;
>
>         rec_argv = calloc(rec_argc + 1, sizeof(char *));
>         if (!rec_argv)
> @@ -1108,9 +1154,19 @@ static int __cmd_record(int argc, const char **argv)
>         for (i = 0; i < ARRAY_SIZE(record_args); i++)
>                 rec_argv[i] = strdup(record_args[i]);
>
> -       for (j = 0; j < ARRAY_SIZE(lock_tracepoints); j++) {
> +       for (j = 0; j < nr_tracepoints; j++) {
> +               const char *ev_name;
> +
> +               if (has_lock_stat)
> +                       ev_name = strdup(lock_tracepoints[j].name);
> +               else
> +                       ev_name = strdup(contention_tracepoints[j].name);
> +
> +               if (!ev_name)
> +                       return -ENOMEM;
> +
>                 rec_argv[i++] = "-e";
> -               rec_argv[i++] = strdup(lock_tracepoints[j].name);
> +               rec_argv[i++] = ev_name;
>         }
>
>         for (j = 1; j < (unsigned int)argc; j++, i++)
> --
> 2.36.1.255.ge46751e96f-goog
>

Powered by blists - more mailing lists