Re: [PATCH 1/2] perf sched: Prefer sched_waking event when it exists

Arnaldo Carvalho de Melo
2020-08-07T19:43:41Z

Em Fri, Aug 07, 2020 at 10:48:44AM -0600, David Ahern escreveu:
> Commit fbd705a0c618 ("sched: Introduce the 'trace_sched_waking' tracepoint")
> added sched_waking tracepoint which should be preferred over sched_wakeup
> when analyzing scheduling delays.
>
> Update 'perf sched record' to collect sched_waking events if it exists
> and fallback to sched_wakeup if it does not. Similarly, update timehist
> command to skip sched_wakeup events if the session includes
> sched_waking (ie., sched_waking is preferred over sched_wakeup).
>
> Signed-off-by: David Ahern <dsahern@kernel.org>
> ---
> tools/perf/builtin-sched.c | 32 +++++++++++++++++++++++++++++---
> 1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 459e4229945e..0c7d599fa555 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -2398,6 +2398,15 @@ static void timehist_print_wakeup_event(struct perf_sched *sched,
> printf("\n");
> }
>
> +static int timehist_sched_wakeup_ignore(struct perf_tool *tool __maybe_unused,
> + union perf_event *event __maybe_unused,
> + struct evsel *evsel __maybe_unused,
> + struct perf_sample *sample __maybe_unused,
> + struct machine *machine __maybe_unused)
> +{
> + return 0;
> +}
> +
> static int timehist_sched_wakeup_event(struct perf_tool *tool,
> union perf_event *event __maybe_unused,
> struct evsel *evsel,
> @@ -2958,9 +2967,10 @@ static int timehist_check_attr(struct perf_sched *sched,
>
> static int perf_sched__timehist(struct perf_sched *sched)
> {
> - const struct evsel_str_handler handlers[] = {
> + struct evsel_str_handler handlers[] = {
> { "sched:sched_switch", timehist_sched_switch_event, },
> { "sched:sched_wakeup", timehist_sched_wakeup_event, },
> + { "sched:sched_waking", timehist_sched_wakeup_event, },
> { "sched:sched_wakeup_new", timehist_sched_wakeup_event, },
> };
> const struct evsel_str_handler migrate_handlers[] = {
> @@ -3018,6 +3028,11 @@ static int perf_sched__timehist(struct perf_sched *sched)
>
> setup_pager();
>
> + /* prefer sched_waking if it is captured */
> + if (perf_evlist__find_tracepoint_by_name(session->evlist,
> + "sched:sched_waking"))
> + handlers[1].handler = timehist_sched_wakeup_ignore;
> +

ouch, can't we figure out if its present and then don't ask for the
wakeup one to be recorded?

- Arnaldo

> /* setup per-evsel handlers */
> if (perf_session__set_tracepoints_handlers(session, handlers))
> goto out;
> @@ -3330,12 +3345,16 @@ static int __cmd_record(int argc, const char **argv)
> "-e", "sched:sched_stat_iowait",
> "-e", "sched:sched_stat_runtime",
> "-e", "sched:sched_process_fork",
> - "-e", "sched:sched_wakeup",
> "-e", "sched:sched_wakeup_new",
> "-e", "sched:sched_migrate_task",
> };
> + struct tep_event *waking_event;
>
> - rec_argc = ARRAY_SIZE(record_args) + argc - 1;
> + /*
> + * +2 for either "-e", "sched:sched_wakeup" or
> + * "-e", "sched:sched_waking"
> + */
> + rec_argc = ARRAY_SIZE(record_args) + 2 + argc - 1;
> rec_argv = calloc(rec_argc + 1, sizeof(char *));
>
> if (rec_argv == NULL)
> @@ -3344,6 +3363,13 @@ 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]);
>
> + rec_argv[i++] = "-e";
> + waking_event = trace_event__tp_format("sched", "sched_waking");
> + if (!IS_ERR(waking_event))
> + rec_argv[i++] = strdup("sched:sched_waking");
> + else
> + rec_argv[i++] = strdup("sched:sched_wakeup");
> +
> for (j = 1; j < (unsigned int)argc; j++, i++)
> rec_argv[i] = argv[j];
>
> --
> 2.17.1
>

--

- Arnaldo