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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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