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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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.
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
Hi David,
On Sat, Aug 8, 2020 at 1:48 AM David Ahern <[email protected]> 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 <[email protected]>
Acked-by: Namhyung Kim <[email protected]>
Thanks
Namhyung
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 <[email protected]> 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 <[email protected]>
>
> Acked-by: Namhyung Kim <[email protected]>
Thanks, applied.
- Arnaldo