Re: [PATCH 1/2] perf sched: Prefer sched_waking event when it exists
Em Tue, Aug 11, 2020 at 03:42:45PM +0900, Namhyung Kim escreveu: > Hi David, > > On Sat, Aug 8, 2020 at 1:48 AM David Ahern wrote: > > > > 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 > > Acked-by: Namhyung Kim Thanks, applied. - Arnaldo
Re: [PATCH 1/2] perf sched: Prefer sched_waking event when it exists
Hi David, On Sat, Aug 8, 2020 at 1:48 AM David Ahern wrote: > > 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 Acked-by: Namhyung Kim Thanks Namhyung
Re: [PATCH 1/2] perf sched: Prefer sched_waking event when it exists
Em Fri, Aug 07, 2020 at 01:50:47PM -0600, David Ahern escreveu: > On 8/7/20 1:43 PM, Arnaldo Carvalho de Melo wrote: > >> @@ -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? > > > > This is the analysis side. If someone recorded with sched:* we do not > want to analyze both sched_wakeup and sched_waking. Rather, it should > prefer the latter and ignore the former. Right you are, thans for the explanation, I should've noticed that :) - Arnaldoi
Re: [PATCH 1/2] perf sched: Prefer sched_waking event when it exists
On 8/7/20 1:43 PM, Arnaldo Carvalho de Melo wrote: >> @@ -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? > This is the analysis side. If someone recorded with sched:* we do not want to analyze both sched_wakeup and sched_waking. Rather, it should prefer the latter and ignore the former.
Re: [PATCH 1/2] perf sched: Prefer sched_waking event when it exists
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 > --- > 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
[PATCH 1/2] perf sched: Prefer sched_waking event when it exists
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 --- 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; + /* 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