When perf-sched enables the call-graph recording, sample_type of dummy
event does not have PERF_SAMPLE_CALLCHAIN, timehist_check_attr() checks
that the evsel does not have a callchain, and set show_callchain to 0.
Currently perf sched timehist only saves callchain when processing
sched:sched_switch event, timehist_check_attr() only needs to determine
whether the event has PERF_SAMPLE_CALLCHAIN.
Before:
# perf sched record -g true
[ perf record: Woken up 0 times to write data ]
[ perf record: Captured and wrote 4.153 MB perf.data (7536 samples) ]
# perf sched timehist
Samples do not have callchains.
time cpu task name wait time sch delay run time
[tid/pid] (msec) (msec) (msec)
--------------- ------ ------------------------------ --------- --------- ---------
147851.826019 [0000] perf[285035] 0.000 0.000 0.000
147851.826029 [0000] migration/0[15] 0.000 0.003 0.009
147851.826063 [0001] perf[285035] 0.000 0.000 0.000
147851.826069 [0001] migration/1[21] 0.000 0.003 0.006
<SNIP>
After:
# perf sched record -g true
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 2.572 MB perf.data (822 samples) ]
# perf sched timehist
time cpu task name wait time sch delay run time
[tid/pid] (msec) (msec) (msec)
--------------- ------ ------------------------------ --------- --------- ---------
144193.035164 [0000] perf[277062] 0.000 0.000 0.000 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
144193.035174 [0000] migration/0[15] 0.000 0.003 0.009 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
144193.035207 [0001] perf[277062] 0.000 0.000 0.000 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
144193.035214 [0001] migration/1[21] 0.000 0.003 0.007 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
<SNIP>
Signed-off-by: Yang Jihong <[email protected]>
---
tools/perf/builtin-sched.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index b248c433529a..1bfb22347371 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -2963,8 +2963,11 @@ static int timehist_check_attr(struct perf_sched *sched,
return -1;
}
- if (sched->show_callchain && !evsel__has_callchain(evsel)) {
- pr_info("Samples do not have callchains.\n");
+ /* only need to save callchain related to sched_switch event */
+ if (sched->show_callchain &&
+ evsel__name_is(evsel, "sched:sched_switch") &&
+ !evsel__has_callchain(evsel)) {
+ pr_info("Samples of sched_switch event do not have callchains.\n");
sched->show_callchain = 0;
symbol_conf.use_callchain = 0;
}
--
2.25.1
Code cleanup, replace strcmp(evsel__name(evsel, {NAME})) with
evsel__name_is() helper.
No functional change.
Signed-off-by: Yang Jihong <[email protected]>
---
tools/perf/builtin-kmem.c | 2 +-
tools/perf/builtin-sched.c | 4 +--
tools/perf/builtin-script.c | 2 +-
tools/perf/builtin-trace.c | 4 +--
tools/perf/tests/evsel-roundtrip-name.c | 4 +--
tools/perf/tests/parse-events.c | 39 +++++++++----------------
6 files changed, 22 insertions(+), 33 deletions(-)
diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index 9714327fd0ea..6fd95be5032b 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -1408,7 +1408,7 @@ static int __cmd_kmem(struct perf_session *session)
}
evlist__for_each_entry(session->evlist, evsel) {
- if (!strcmp(evsel__name(evsel), "kmem:mm_page_alloc") &&
+ if (evsel__name_is(evsel, "kmem:mm_page_alloc") &&
evsel__field(evsel, "pfn")) {
use_pfn = true;
break;
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 1bfb22347371..0fce7d8986c0 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -2148,7 +2148,7 @@ static bool is_idle_sample(struct perf_sample *sample,
struct evsel *evsel)
{
/* pid 0 == swapper == idle task */
- if (strcmp(evsel__name(evsel), "sched:sched_switch") == 0)
+ if (evsel__name_is(evsel, "sched:sched_switch"))
return evsel__intval(evsel, sample, "prev_pid") == 0;
return sample->pid == 0;
@@ -2375,7 +2375,7 @@ static bool timehist_skip_sample(struct perf_sched *sched,
}
if (sched->idle_hist) {
- if (strcmp(evsel__name(evsel), "sched:sched_switch"))
+ if (!evsel__name_is(evsel, "sched:sched_switch"))
rc = true;
else if (evsel__intval(evsel, sample, "prev_pid") != 0 &&
evsel__intval(evsel, sample, "next_pid") != 0)
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 37088cc0ff1b..cc981531ec00 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -3471,7 +3471,7 @@ static int check_ev_match(char *dir_name, char *scriptname,
match = 0;
evlist__for_each_entry(session->evlist, pos) {
- if (!strcmp(evsel__name(pos), evname)) {
+ if (evsel__name_is(pos, evname)) {
match = 1;
break;
}
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 90eaff8c0f6e..9b93807a1906 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -4902,7 +4902,7 @@ int cmd_trace(int argc, const char **argv)
goto out;
}
trace.syscalls.events.bpf_output = evlist__last(trace.evlist);
- assert(!strcmp(evsel__name(trace.syscalls.events.bpf_output), "__augmented_syscalls__"));
+ assert(evsel__name_is(trace.syscalls.events.bpf_output), "__augmented_syscalls__");
skip_augmentation:
#endif
err = -1;
@@ -4959,7 +4959,7 @@ int cmd_trace(int argc, const char **argv)
*/
if (trace.syscalls.events.bpf_output) {
evlist__for_each_entry(trace.evlist, evsel) {
- bool raw_syscalls_sys_exit = strcmp(evsel__name(evsel), "raw_syscalls:sys_exit") == 0;
+ bool raw_syscalls_sys_exit = evsel__name_is(evsel, "raw_syscalls:sys_exit");
if (raw_syscalls_sys_exit) {
trace.raw_augmented_syscalls = true;
diff --git a/tools/perf/tests/evsel-roundtrip-name.c b/tools/perf/tests/evsel-roundtrip-name.c
index 15ff86f9da0b..1922cac13a24 100644
--- a/tools/perf/tests/evsel-roundtrip-name.c
+++ b/tools/perf/tests/evsel-roundtrip-name.c
@@ -37,7 +37,7 @@ static int perf_evsel__roundtrip_cache_name_test(void)
continue;
}
evlist__for_each_entry(evlist, evsel) {
- if (strcmp(evsel__name(evsel), name)) {
+ if (!evsel__name_is(evsel, name)) {
pr_debug("%s != %s\n", evsel__name(evsel), name);
ret = TEST_FAIL;
}
@@ -71,7 +71,7 @@ static int perf_evsel__name_array_test(const char *const names[], int nr_names)
continue;
}
evlist__for_each_entry(evlist, evsel) {
- if (strcmp(evsel__name(evsel), names[i])) {
+ if (!evsel__name_is(evsel, names[i])) {
pr_debug("%s != %s\n", evsel__name(evsel), names[i]);
ret = TEST_FAIL;
}
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index feb5727584d1..0b70451451b3 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -470,8 +470,7 @@ static int test__checkevent_breakpoint_modifier(struct evlist *evlist)
TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
- TEST_ASSERT_VAL("wrong name",
- !strcmp(evsel__name(evsel), "mem:0:u"));
+ TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "mem:0:u"));
return test__checkevent_breakpoint(evlist);
}
@@ -484,8 +483,7 @@ static int test__checkevent_breakpoint_x_modifier(struct evlist *evlist)
TEST_ASSERT_VAL("wrong exclude_kernel", !evsel->core.attr.exclude_kernel);
TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
- TEST_ASSERT_VAL("wrong name",
- !strcmp(evsel__name(evsel), "mem:0:x:k"));
+ TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "mem:0:x:k"));
return test__checkevent_breakpoint_x(evlist);
}
@@ -498,8 +496,7 @@ static int test__checkevent_breakpoint_r_modifier(struct evlist *evlist)
TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
TEST_ASSERT_VAL("wrong exclude_hv", !evsel->core.attr.exclude_hv);
TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
- TEST_ASSERT_VAL("wrong name",
- !strcmp(evsel__name(evsel), "mem:0:r:hp"));
+ TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "mem:0:r:hp"));
return test__checkevent_breakpoint_r(evlist);
}
@@ -512,8 +509,7 @@ static int test__checkevent_breakpoint_w_modifier(struct evlist *evlist)
TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
- TEST_ASSERT_VAL("wrong name",
- !strcmp(evsel__name(evsel), "mem:0:w:up"));
+ TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "mem:0:w:up"));
return test__checkevent_breakpoint_w(evlist);
}
@@ -526,8 +522,7 @@ static int test__checkevent_breakpoint_rw_modifier(struct evlist *evlist)
TEST_ASSERT_VAL("wrong exclude_kernel", !evsel->core.attr.exclude_kernel);
TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
- TEST_ASSERT_VAL("wrong name",
- !strcmp(evsel__name(evsel), "mem:0:rw:kp"));
+ TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "mem:0:rw:kp"));
return test__checkevent_breakpoint_rw(evlist);
}
@@ -540,8 +535,7 @@ static int test__checkevent_breakpoint_modifier_name(struct evlist *evlist)
TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
- TEST_ASSERT_VAL("wrong name",
- !strcmp(evsel__name(evsel), "breakpoint"));
+ TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "breakpoint"));
return test__checkevent_breakpoint(evlist);
}
@@ -554,8 +548,7 @@ static int test__checkevent_breakpoint_x_modifier_name(struct evlist *evlist)
TEST_ASSERT_VAL("wrong exclude_kernel", !evsel->core.attr.exclude_kernel);
TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
- TEST_ASSERT_VAL("wrong name",
- !strcmp(evsel__name(evsel), "breakpoint"));
+ TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "breakpoint"));
return test__checkevent_breakpoint_x(evlist);
}
@@ -568,8 +561,7 @@ static int test__checkevent_breakpoint_r_modifier_name(struct evlist *evlist)
TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
TEST_ASSERT_VAL("wrong exclude_hv", !evsel->core.attr.exclude_hv);
TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
- TEST_ASSERT_VAL("wrong name",
- !strcmp(evsel__name(evsel), "breakpoint"));
+ TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "breakpoint"));
return test__checkevent_breakpoint_r(evlist);
}
@@ -582,8 +574,7 @@ static int test__checkevent_breakpoint_w_modifier_name(struct evlist *evlist)
TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
- TEST_ASSERT_VAL("wrong name",
- !strcmp(evsel__name(evsel), "breakpoint"));
+ TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "breakpoint"));
return test__checkevent_breakpoint_w(evlist);
}
@@ -596,8 +587,7 @@ static int test__checkevent_breakpoint_rw_modifier_name(struct evlist *evlist)
TEST_ASSERT_VAL("wrong exclude_kernel", !evsel->core.attr.exclude_kernel);
TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
- TEST_ASSERT_VAL("wrong name",
- !strcmp(evsel__name(evsel), "breakpoint"));
+ TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "breakpoint"));
return test__checkevent_breakpoint_rw(evlist);
}
@@ -609,12 +599,12 @@ static int test__checkevent_breakpoint_2_events(struct evlist *evlist)
TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->core.nr_entries);
TEST_ASSERT_VAL("wrong type", PERF_TYPE_BREAKPOINT == evsel->core.attr.type);
- TEST_ASSERT_VAL("wrong name", !strcmp(evsel__name(evsel), "breakpoint1"));
+ TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "breakpoint1"));
evsel = evsel__next(evsel);
TEST_ASSERT_VAL("wrong type", PERF_TYPE_BREAKPOINT == evsel->core.attr.type);
- TEST_ASSERT_VAL("wrong name", !strcmp(evsel__name(evsel), "breakpoint2"));
+ TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "breakpoint2"));
return TEST_OK;
}
@@ -691,15 +681,14 @@ static int test__checkevent_pmu_name(struct evlist *evlist)
TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->core.nr_entries);
TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type);
TEST_ASSERT_VAL("wrong config", test_config(evsel, 1));
- TEST_ASSERT_VAL("wrong name", !strcmp(evsel__name(evsel), "krava"));
+ TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "krava"));
/* cpu/config=2/u" */
evsel = evsel__next(evsel);
TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->core.nr_entries);
TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type);
TEST_ASSERT_VAL("wrong config", test_config(evsel, 2));
- TEST_ASSERT_VAL("wrong name",
- !strcmp(evsel__name(evsel), "cpu/config=2/u"));
+ TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "cpu/config=2/u"));
return TEST_OK;
}
--
2.25.1
On Wed, Mar 27, 2024 at 10:59 PM Yang Jihong <[email protected]> wrote:
>
> When perf-sched enables the call-graph recording, sample_type of dummy
> event does not have PERF_SAMPLE_CALLCHAIN, timehist_check_attr() checks
> that the evsel does not have a callchain, and set show_callchain to 0.
>
> Currently perf sched timehist only saves callchain when processing
> sched:sched_switch event, timehist_check_attr() only needs to determine
> whether the event has PERF_SAMPLE_CALLCHAIN.
>
> Before:
> # perf sched record -g true
> [ perf record: Woken up 0 times to write data ]
> [ perf record: Captured and wrote 4.153 MB perf.data (7536 samples) ]
> # perf sched timehist
> Samples do not have callchains.
> time cpu task name wait time sch delay run time
> [tid/pid] (msec) (msec) (msec)
> --------------- ------ ------------------------------ --------- --------- ---------
> 147851.826019 [0000] perf[285035] 0.000 0.000 0.000
> 147851.826029 [0000] migration/0[15] 0.000 0.003 0.009
> 147851.826063 [0001] perf[285035] 0.000 0.000 0.000
> 147851.826069 [0001] migration/1[21] 0.000 0.003 0.006
> <SNIP>
>
> After:
> # perf sched record -g true
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 2.572 MB perf.data (822 samples) ]
> # perf sched timehist
> time cpu task name wait time sch delay run time
> [tid/pid] (msec) (msec) (msec)
> --------------- ------ ------------------------------ --------- --------- ---------
> 144193.035164 [0000] perf[277062] 0.000 0.000 0.000 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
> 144193.035174 [0000] migration/0[15] 0.000 0.003 0.009 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
> 144193.035207 [0001] perf[277062] 0.000 0.000 0.000 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
> 144193.035214 [0001] migration/1[21] 0.000 0.003 0.007 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
> <SNIP>
This looks good, should there be a Fixes tag for the sake of backports?
Thanks,
Ian
> Signed-off-by: Yang Jihong <[email protected]>
> ---
> tools/perf/builtin-sched.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index b248c433529a..1bfb22347371 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -2963,8 +2963,11 @@ static int timehist_check_attr(struct perf_sched *sched,
> return -1;
> }
>
> - if (sched->show_callchain && !evsel__has_callchain(evsel)) {
> - pr_info("Samples do not have callchains.\n");
> + /* only need to save callchain related to sched_switch event */
> + if (sched->show_callchain &&
> + evsel__name_is(evsel, "sched:sched_switch") &&
> + !evsel__has_callchain(evsel)) {
> + pr_info("Samples of sched_switch event do not have callchains.\n");
> sched->show_callchain = 0;
> symbol_conf.use_callchain = 0;
> }
> --
> 2.25.1
>
On Wed, Mar 27, 2024 at 10:59 PM Yang Jihong <[email protected]> wrote:
>
> Code cleanup, replace strcmp(evsel__name(evsel, {NAME})) with
> evsel__name_is() helper.
>
> No functional change.
>
> Signed-off-by: Yang Jihong <[email protected]>
Reviewed-by: Ian Rogers <[email protected]>
Thanks,
Ian
> ---
> tools/perf/builtin-kmem.c | 2 +-
> tools/perf/builtin-sched.c | 4 +--
> tools/perf/builtin-script.c | 2 +-
> tools/perf/builtin-trace.c | 4 +--
> tools/perf/tests/evsel-roundtrip-name.c | 4 +--
> tools/perf/tests/parse-events.c | 39 +++++++++----------------
> 6 files changed, 22 insertions(+), 33 deletions(-)
>
> diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
> index 9714327fd0ea..6fd95be5032b 100644
> --- a/tools/perf/builtin-kmem.c
> +++ b/tools/perf/builtin-kmem.c
> @@ -1408,7 +1408,7 @@ static int __cmd_kmem(struct perf_session *session)
> }
>
> evlist__for_each_entry(session->evlist, evsel) {
> - if (!strcmp(evsel__name(evsel), "kmem:mm_page_alloc") &&
> + if (evsel__name_is(evsel, "kmem:mm_page_alloc") &&
> evsel__field(evsel, "pfn")) {
> use_pfn = true;
> break;
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 1bfb22347371..0fce7d8986c0 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -2148,7 +2148,7 @@ static bool is_idle_sample(struct perf_sample *sample,
> struct evsel *evsel)
> {
> /* pid 0 == swapper == idle task */
> - if (strcmp(evsel__name(evsel), "sched:sched_switch") == 0)
> + if (evsel__name_is(evsel, "sched:sched_switch"))
> return evsel__intval(evsel, sample, "prev_pid") == 0;
>
> return sample->pid == 0;
> @@ -2375,7 +2375,7 @@ static bool timehist_skip_sample(struct perf_sched *sched,
> }
>
> if (sched->idle_hist) {
> - if (strcmp(evsel__name(evsel), "sched:sched_switch"))
> + if (!evsel__name_is(evsel, "sched:sched_switch"))
> rc = true;
> else if (evsel__intval(evsel, sample, "prev_pid") != 0 &&
> evsel__intval(evsel, sample, "next_pid") != 0)
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 37088cc0ff1b..cc981531ec00 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -3471,7 +3471,7 @@ static int check_ev_match(char *dir_name, char *scriptname,
>
> match = 0;
> evlist__for_each_entry(session->evlist, pos) {
> - if (!strcmp(evsel__name(pos), evname)) {
> + if (evsel__name_is(pos, evname)) {
> match = 1;
> break;
> }
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 90eaff8c0f6e..9b93807a1906 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -4902,7 +4902,7 @@ int cmd_trace(int argc, const char **argv)
> goto out;
> }
> trace.syscalls.events.bpf_output = evlist__last(trace.evlist);
> - assert(!strcmp(evsel__name(trace.syscalls.events.bpf_output), "__augmented_syscalls__"));
> + assert(evsel__name_is(trace.syscalls.events.bpf_output), "__augmented_syscalls__");
> skip_augmentation:
> #endif
> err = -1;
> @@ -4959,7 +4959,7 @@ int cmd_trace(int argc, const char **argv)
> */
> if (trace.syscalls.events.bpf_output) {
> evlist__for_each_entry(trace.evlist, evsel) {
> - bool raw_syscalls_sys_exit = strcmp(evsel__name(evsel), "raw_syscalls:sys_exit") == 0;
> + bool raw_syscalls_sys_exit = evsel__name_is(evsel, "raw_syscalls:sys_exit");
>
> if (raw_syscalls_sys_exit) {
> trace.raw_augmented_syscalls = true;
> diff --git a/tools/perf/tests/evsel-roundtrip-name.c b/tools/perf/tests/evsel-roundtrip-name.c
> index 15ff86f9da0b..1922cac13a24 100644
> --- a/tools/perf/tests/evsel-roundtrip-name.c
> +++ b/tools/perf/tests/evsel-roundtrip-name.c
> @@ -37,7 +37,7 @@ static int perf_evsel__roundtrip_cache_name_test(void)
> continue;
> }
> evlist__for_each_entry(evlist, evsel) {
> - if (strcmp(evsel__name(evsel), name)) {
> + if (!evsel__name_is(evsel, name)) {
> pr_debug("%s != %s\n", evsel__name(evsel), name);
> ret = TEST_FAIL;
> }
> @@ -71,7 +71,7 @@ static int perf_evsel__name_array_test(const char *const names[], int nr_names)
> continue;
> }
> evlist__for_each_entry(evlist, evsel) {
> - if (strcmp(evsel__name(evsel), names[i])) {
> + if (!evsel__name_is(evsel, names[i])) {
> pr_debug("%s != %s\n", evsel__name(evsel), names[i]);
> ret = TEST_FAIL;
> }
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index feb5727584d1..0b70451451b3 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -470,8 +470,7 @@ static int test__checkevent_breakpoint_modifier(struct evlist *evlist)
> TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
> TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
> - TEST_ASSERT_VAL("wrong name",
> - !strcmp(evsel__name(evsel), "mem:0:u"));
> + TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "mem:0:u"));
>
> return test__checkevent_breakpoint(evlist);
> }
> @@ -484,8 +483,7 @@ static int test__checkevent_breakpoint_x_modifier(struct evlist *evlist)
> TEST_ASSERT_VAL("wrong exclude_kernel", !evsel->core.attr.exclude_kernel);
> TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
> - TEST_ASSERT_VAL("wrong name",
> - !strcmp(evsel__name(evsel), "mem:0:x:k"));
> + TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "mem:0:x:k"));
>
> return test__checkevent_breakpoint_x(evlist);
> }
> @@ -498,8 +496,7 @@ static int test__checkevent_breakpoint_r_modifier(struct evlist *evlist)
> TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
> TEST_ASSERT_VAL("wrong exclude_hv", !evsel->core.attr.exclude_hv);
> TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
> - TEST_ASSERT_VAL("wrong name",
> - !strcmp(evsel__name(evsel), "mem:0:r:hp"));
> + TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "mem:0:r:hp"));
>
> return test__checkevent_breakpoint_r(evlist);
> }
> @@ -512,8 +509,7 @@ static int test__checkevent_breakpoint_w_modifier(struct evlist *evlist)
> TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
> TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
> - TEST_ASSERT_VAL("wrong name",
> - !strcmp(evsel__name(evsel), "mem:0:w:up"));
> + TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "mem:0:w:up"));
>
> return test__checkevent_breakpoint_w(evlist);
> }
> @@ -526,8 +522,7 @@ static int test__checkevent_breakpoint_rw_modifier(struct evlist *evlist)
> TEST_ASSERT_VAL("wrong exclude_kernel", !evsel->core.attr.exclude_kernel);
> TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
> - TEST_ASSERT_VAL("wrong name",
> - !strcmp(evsel__name(evsel), "mem:0:rw:kp"));
> + TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "mem:0:rw:kp"));
>
> return test__checkevent_breakpoint_rw(evlist);
> }
> @@ -540,8 +535,7 @@ static int test__checkevent_breakpoint_modifier_name(struct evlist *evlist)
> TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
> TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
> - TEST_ASSERT_VAL("wrong name",
> - !strcmp(evsel__name(evsel), "breakpoint"));
> + TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "breakpoint"));
>
> return test__checkevent_breakpoint(evlist);
> }
> @@ -554,8 +548,7 @@ static int test__checkevent_breakpoint_x_modifier_name(struct evlist *evlist)
> TEST_ASSERT_VAL("wrong exclude_kernel", !evsel->core.attr.exclude_kernel);
> TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> TEST_ASSERT_VAL("wrong precise_ip", !evsel->core.attr.precise_ip);
> - TEST_ASSERT_VAL("wrong name",
> - !strcmp(evsel__name(evsel), "breakpoint"));
> + TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "breakpoint"));
>
> return test__checkevent_breakpoint_x(evlist);
> }
> @@ -568,8 +561,7 @@ static int test__checkevent_breakpoint_r_modifier_name(struct evlist *evlist)
> TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
> TEST_ASSERT_VAL("wrong exclude_hv", !evsel->core.attr.exclude_hv);
> TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
> - TEST_ASSERT_VAL("wrong name",
> - !strcmp(evsel__name(evsel), "breakpoint"));
> + TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "breakpoint"));
>
> return test__checkevent_breakpoint_r(evlist);
> }
> @@ -582,8 +574,7 @@ static int test__checkevent_breakpoint_w_modifier_name(struct evlist *evlist)
> TEST_ASSERT_VAL("wrong exclude_kernel", evsel->core.attr.exclude_kernel);
> TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
> - TEST_ASSERT_VAL("wrong name",
> - !strcmp(evsel__name(evsel), "breakpoint"));
> + TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "breakpoint"));
>
> return test__checkevent_breakpoint_w(evlist);
> }
> @@ -596,8 +587,7 @@ static int test__checkevent_breakpoint_rw_modifier_name(struct evlist *evlist)
> TEST_ASSERT_VAL("wrong exclude_kernel", !evsel->core.attr.exclude_kernel);
> TEST_ASSERT_VAL("wrong exclude_hv", evsel->core.attr.exclude_hv);
> TEST_ASSERT_VAL("wrong precise_ip", evsel->core.attr.precise_ip);
> - TEST_ASSERT_VAL("wrong name",
> - !strcmp(evsel__name(evsel), "breakpoint"));
> + TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "breakpoint"));
>
> return test__checkevent_breakpoint_rw(evlist);
> }
> @@ -609,12 +599,12 @@ static int test__checkevent_breakpoint_2_events(struct evlist *evlist)
> TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->core.nr_entries);
>
> TEST_ASSERT_VAL("wrong type", PERF_TYPE_BREAKPOINT == evsel->core.attr.type);
> - TEST_ASSERT_VAL("wrong name", !strcmp(evsel__name(evsel), "breakpoint1"));
> + TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "breakpoint1"));
>
> evsel = evsel__next(evsel);
>
> TEST_ASSERT_VAL("wrong type", PERF_TYPE_BREAKPOINT == evsel->core.attr.type);
> - TEST_ASSERT_VAL("wrong name", !strcmp(evsel__name(evsel), "breakpoint2"));
> + TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "breakpoint2"));
>
> return TEST_OK;
> }
> @@ -691,15 +681,14 @@ static int test__checkevent_pmu_name(struct evlist *evlist)
> TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->core.nr_entries);
> TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type);
> TEST_ASSERT_VAL("wrong config", test_config(evsel, 1));
> - TEST_ASSERT_VAL("wrong name", !strcmp(evsel__name(evsel), "krava"));
> + TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "krava"));
>
> /* cpu/config=2/u" */
> evsel = evsel__next(evsel);
> TEST_ASSERT_VAL("wrong number of entries", 2 == evlist->core.nr_entries);
> TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW == evsel->core.attr.type);
> TEST_ASSERT_VAL("wrong config", test_config(evsel, 2));
> - TEST_ASSERT_VAL("wrong name",
> - !strcmp(evsel__name(evsel), "cpu/config=2/u"));
> + TEST_ASSERT_VAL("wrong name", evsel__name_is(evsel, "cpu/config=2/u"));
>
> return TEST_OK;
> }
> --
> 2.25.1
>
Hello,
Sorry, due to the new email settings, the last reply email was in html
format, resend it now.
On 3/29/24 00:02, Ian Rogers wrote:
> On Wed, Mar 27, 2024 at 10:59 PM Yang Jihong <[email protected]> wrote:
>>
>> When perf-sched enables the call-graph recording, sample_type of dummy
>> event does not have PERF_SAMPLE_CALLCHAIN, timehist_check_attr() checks
>> that the evsel does not have a callchain, and set show_callchain to 0.
>>
>> Currently perf sched timehist only saves callchain when processing
>> sched:sched_switch event, timehist_check_attr() only needs to determine
>> whether the event has PERF_SAMPLE_CALLCHAIN.
>>
>> Before:
>> # perf sched record -g true
>> [ perf record: Woken up 0 times to write data ]
>> [ perf record: Captured and wrote 4.153 MB perf.data (7536 samples) ]
>> # perf sched timehist
>> Samples do not have callchains.
>> time cpu task name wait time sch delay run time
>> [tid/pid] (msec) (msec) (msec)
>> --------------- ------ ------------------------------ --------- --------- ---------
>> 147851.826019 [0000] perf[285035] 0.000 0.000 0.000
>> 147851.826029 [0000] migration/0[15] 0.000 0.003 0.009
>> 147851.826063 [0001] perf[285035] 0.000 0.000 0.000
>> 147851.826069 [0001] migration/1[21] 0.000 0.003 0.006
>> <SNIP>
>>
>> After:
>> # perf sched record -g true
>> [ perf record: Woken up 1 times to write data ]
>> [ perf record: Captured and wrote 2.572 MB perf.data (822 samples) ]
>> # perf sched timehist
>> time cpu task name wait time sch delay run time
>> [tid/pid] (msec) (msec) (msec)
>> --------------- ------ ------------------------------ --------- --------- ---------
>> 144193.035164 [0000] perf[277062] 0.000 0.000 0.000 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
>> 144193.035174 [0000] migration/0[15] 0.000 0.003 0.009 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
>> 144193.035207 [0001] perf[277062] 0.000 0.000 0.000 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
>> 144193.035214 [0001] migration/1[21] 0.000 0.003 0.007 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
>> <SNIP>
>
> This looks good, should there be a Fixes tag for the sake of backports?
>
The direct cause is commit 9c95e4ef0657 ("perf evlist: Add
evlist__findnew_tracking_event() helper"). perf-record uses
evlist__add_aux_dummy() to replace evlist__add_dummy() to add a dummy
event. The difference is that evlist__add_aux_dummy() sets
no_aux_samples to true (this is expected behavior, for dummy event, no
need to sample aux data), resulting in evsel__config() not adding the
PERF_SAMPLE_CALLCHAIN bit to dummy's sample_type.
In summary, the direct cause is the problem introduced by commit
9c95e4ef0657 ("perf evlist: Add evlist__findnew_tracking_event()
helper"), but the root cause is the timehist_check_attr() logic problem,
The dummy event itself does not need to have PERF_SAMPLE_CALLCHAIN, so
there is no need to check it.
So, maybe add fixes-tag:
Fixes: 9c95e4ef0657 ("perf evlist: Add evlist__findnew_tracking_event()
helper")
If it is ok, I will send v2 version with this fixes-tag.
Thanks,
Yang
On Thu, Mar 28, 2024 at 8:02 PM Yang Jihong <[email protected]> wrote:
>
> Hello,
>
> Sorry, due to the new email settings, the last reply email was in html
> format, resend it now.
>
> On 3/29/24 00:02, Ian Rogers wrote:
> > On Wed, Mar 27, 2024 at 10:59 PM Yang Jihong <[email protected]> wrote:
> >>
> >> When perf-sched enables the call-graph recording, sample_type of dummy
> >> event does not have PERF_SAMPLE_CALLCHAIN, timehist_check_attr() checks
> >> that the evsel does not have a callchain, and set show_callchain to 0.
> >>
> >> Currently perf sched timehist only saves callchain when processing
> >> sched:sched_switch event, timehist_check_attr() only needs to determine
> >> whether the event has PERF_SAMPLE_CALLCHAIN.
> >>
> >> Before:
> >> # perf sched record -g true
> >> [ perf record: Woken up 0 times to write data ]
> >> [ perf record: Captured and wrote 4.153 MB perf.data (7536 samples) ]
> >> # perf sched timehist
> >> Samples do not have callchains.
> >> time cpu task name wait time sch delay run time
> >> [tid/pid] (msec) (msec) (msec)
> >> --------------- ------ ------------------------------ --------- --------- ---------
> >> 147851.826019 [0000] perf[285035] 0.000 0.000 0.000
> >> 147851.826029 [0000] migration/0[15] 0.000 0.003 0.009
> >> 147851.826063 [0001] perf[285035] 0.000 0.000 0.000
> >> 147851.826069 [0001] migration/1[21] 0.000 0.003 0.006
> >> <SNIP>
> >>
> >> After:
> >> # perf sched record -g true
> >> [ perf record: Woken up 1 times to write data ]
> >> [ perf record: Captured and wrote 2.572 MB perf.data (822 samples) ]
> >> # perf sched timehist
> >> time cpu task name wait time sch delay run time
> >> [tid/pid] (msec) (msec) (msec)
> >> --------------- ------ ------------------------------ --------- --------- ---------
> >> 144193.035164 [0000] perf[277062] 0.000 0.000 0.000 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
> >> 144193.035174 [0000] migration/0[15] 0.000 0.003 0.009 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
> >> 144193.035207 [0001] perf[277062] 0.000 0.000 0.000 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
> >> 144193.035214 [0001] migration/1[21] 0.000 0.003 0.007 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
> >> <SNIP>
> >
> > This looks good, should there be a Fixes tag for the sake of backports?
> >
> The direct cause is commit 9c95e4ef0657 ("perf evlist: Add
> evlist__findnew_tracking_event() helper"). perf-record uses
> evlist__add_aux_dummy() to replace evlist__add_dummy() to add a dummy
> event. The difference is that evlist__add_aux_dummy() sets
> no_aux_samples to true (this is expected behavior, for dummy event, no
> need to sample aux data), resulting in evsel__config() not adding the
> PERF_SAMPLE_CALLCHAIN bit to dummy's sample_type.
>
> In summary, the direct cause is the problem introduced by commit
> 9c95e4ef0657 ("perf evlist: Add evlist__findnew_tracking_event()
> helper"), but the root cause is the timehist_check_attr() logic problem,
> The dummy event itself does not need to have PERF_SAMPLE_CALLCHAIN, so
> there is no need to check it.
>
>
> So, maybe add fixes-tag:
>
> Fixes: 9c95e4ef0657 ("perf evlist: Add evlist__findnew_tracking_event()
> helper")
>
> If it is ok, I will send v2 version with this fixes-tag.
I think the maintainer can add the fixes tag when they add the reviewed-by tag:
Reviewed-by: Ian Rogers <[email protected]>
Thanks,
Ian
> Thanks,
> Yang
On Fri, Mar 29, 2024 at 09:08:01AM -0700, Ian Rogers wrote:
> On Thu, Mar 28, 2024 at 8:02 PM Yang Jihong <[email protected]> wrote:
> >
> > Hello,
> >
> > Sorry, due to the new email settings, the last reply email was in html
> > format, resend it now.
> >
> > On 3/29/24 00:02, Ian Rogers wrote:
> > > On Wed, Mar 27, 2024 at 10:59 PM Yang Jihong <[email protected]> wrote:
> > >>
> > >> When perf-sched enables the call-graph recording, sample_type of dummy
> > >> event does not have PERF_SAMPLE_CALLCHAIN, timehist_check_attr() checks
> > >> that the evsel does not have a callchain, and set show_callchain to 0.
> > >>
> > >> Currently perf sched timehist only saves callchain when processing
> > >> sched:sched_switch event, timehist_check_attr() only needs to determine
> > >> whether the event has PERF_SAMPLE_CALLCHAIN.
> > >>
> > >> Before:
> > >> # perf sched record -g true
> > >> [ perf record: Woken up 0 times to write data ]
> > >> [ perf record: Captured and wrote 4.153 MB perf.data (7536 samples) ]
> > >> # perf sched timehist
> > >> Samples do not have callchains.
> > >> time cpu task name wait time sch delay run time
> > >> [tid/pid] (msec) (msec) (msec)
> > >> --------------- ------ ------------------------------ --------- --------- ---------
> > >> 147851.826019 [0000] perf[285035] 0.000 0.000 0.000
> > >> 147851.826029 [0000] migration/0[15] 0.000 0.003 0.009
> > >> 147851.826063 [0001] perf[285035] 0.000 0.000 0.000
> > >> 147851.826069 [0001] migration/1[21] 0.000 0.003 0.006
> > >> <SNIP>
> > >>
> > >> After:
> > >> # perf sched record -g true
> > >> [ perf record: Woken up 1 times to write data ]
> > >> [ perf record: Captured and wrote 2.572 MB perf.data (822 samples) ]
> > >> # perf sched timehist
> > >> time cpu task name wait time sch delay run time
> > >> [tid/pid] (msec) (msec) (msec)
> > >> --------------- ------ ------------------------------ --------- --------- ---------
> > >> 144193.035164 [0000] perf[277062] 0.000 0.000 0.000 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
> > >> 144193.035174 [0000] migration/0[15] 0.000 0.003 0.009 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
> > >> 144193.035207 [0001] perf[277062] 0.000 0.000 0.000 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
> > >> 144193.035214 [0001] migration/1[21] 0.000 0.003 0.007 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
> > >> <SNIP>
> > >
> > > This looks good, should there be a Fixes tag for the sake of backports?
> > >
> > The direct cause is commit 9c95e4ef0657 ("perf evlist: Add
> > evlist__findnew_tracking_event() helper"). perf-record uses
> > evlist__add_aux_dummy() to replace evlist__add_dummy() to add a dummy
> > event. The difference is that evlist__add_aux_dummy() sets
> > no_aux_samples to true (this is expected behavior, for dummy event, no
> > need to sample aux data), resulting in evsel__config() not adding the
> > PERF_SAMPLE_CALLCHAIN bit to dummy's sample_type.
> >
> > In summary, the direct cause is the problem introduced by commit
> > 9c95e4ef0657 ("perf evlist: Add evlist__findnew_tracking_event()
> > helper"), but the root cause is the timehist_check_attr() logic problem,
> > The dummy event itself does not need to have PERF_SAMPLE_CALLCHAIN, so
> > there is no need to check it.
> >
> >
> > So, maybe add fixes-tag:
> >
> > Fixes: 9c95e4ef0657 ("perf evlist: Add evlist__findnew_tracking_event()
> > helper")
> >
> > If it is ok, I will send v2 version with this fixes-tag.
>
> I think the maintainer can add the fixes tag when they add the reviewed-by tag:
I usually do this, but if the submitter does it I'll have just to check
that it is the right tag, helps a bit in processing.
- Arnaldo
> Reviewed-by: Ian Rogers <[email protected]>
>
> Thanks,
> Ian
>
> > Thanks,
> > Yang
Hello,
On 3/30/24 22:48, Arnaldo Carvalho de Melo wrote:
> On Fri, Mar 29, 2024 at 09:08:01AM -0700, Ian Rogers wrote:
>> On Thu, Mar 28, 2024 at 8:02 PM Yang Jihong <[email protected]> wrote:
>>>
>>> Hello,
>>>
>>> Sorry, due to the new email settings, the last reply email was in html
>>> format, resend it now.
>>>
>>> On 3/29/24 00:02, Ian Rogers wrote:
>>>> On Wed, Mar 27, 2024 at 10:59 PM Yang Jihong <[email protected]> wrote:
>>>>>
>>>>> When perf-sched enables the call-graph recording, sample_type of dummy
>>>>> event does not have PERF_SAMPLE_CALLCHAIN, timehist_check_attr() checks
>>>>> that the evsel does not have a callchain, and set show_callchain to 0.
>>>>>
>>>>> Currently perf sched timehist only saves callchain when processing
>>>>> sched:sched_switch event, timehist_check_attr() only needs to determine
>>>>> whether the event has PERF_SAMPLE_CALLCHAIN.
>>>>>
>>>>> Before:
>>>>> # perf sched record -g true
>>>>> [ perf record: Woken up 0 times to write data ]
>>>>> [ perf record: Captured and wrote 4.153 MB perf.data (7536 samples) ]
>>>>> # perf sched timehist
>>>>> Samples do not have callchains.
>>>>> time cpu task name wait time sch delay run time
>>>>> [tid/pid] (msec) (msec) (msec)
>>>>> --------------- ------ ------------------------------ --------- --------- ---------
>>>>> 147851.826019 [0000] perf[285035] 0.000 0.000 0.000
>>>>> 147851.826029 [0000] migration/0[15] 0.000 0.003 0.009
>>>>> 147851.826063 [0001] perf[285035] 0.000 0.000 0.000
>>>>> 147851.826069 [0001] migration/1[21] 0.000 0.003 0.006
>>>>> <SNIP>
>>>>>
>>>>> After:
>>>>> # perf sched record -g true
>>>>> [ perf record: Woken up 1 times to write data ]
>>>>> [ perf record: Captured and wrote 2.572 MB perf.data (822 samples) ]
>>>>> # perf sched timehist
>>>>> time cpu task name wait time sch delay run time
>>>>> [tid/pid] (msec) (msec) (msec)
>>>>> --------------- ------ ------------------------------ --------- --------- ---------
>>>>> 144193.035164 [0000] perf[277062] 0.000 0.000 0.000 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
>>>>> 144193.035174 [0000] migration/0[15] 0.000 0.003 0.009 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
>>>>> 144193.035207 [0001] perf[277062] 0.000 0.000 0.000 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- preempt_schedule_common <- __cond_resched <- __wait_for_common <- wait_for_completion
>>>>> 144193.035214 [0001] migration/1[21] 0.000 0.003 0.007 __traceiter_sched_switch <- __traceiter_sched_switch <- __sched_text_start <- smpboot_thread_fn <- kthread <- ret_from_fork
>>>>> <SNIP>
>>>>
>>>> This looks good, should there be a Fixes tag for the sake of backports?
>>>>
>>> The direct cause is commit 9c95e4ef0657 ("perf evlist: Add
>>> evlist__findnew_tracking_event() helper"). perf-record uses
>>> evlist__add_aux_dummy() to replace evlist__add_dummy() to add a dummy
>>> event. The difference is that evlist__add_aux_dummy() sets
>>> no_aux_samples to true (this is expected behavior, for dummy event, no
>>> need to sample aux data), resulting in evsel__config() not adding the
>>> PERF_SAMPLE_CALLCHAIN bit to dummy's sample_type.
>>>
>>> In summary, the direct cause is the problem introduced by commit
>>> 9c95e4ef0657 ("perf evlist: Add evlist__findnew_tracking_event()
>>> helper"), but the root cause is the timehist_check_attr() logic problem,
>>> The dummy event itself does not need to have PERF_SAMPLE_CALLCHAIN, so
>>> there is no need to check it.
>>>
>>>
>>> So, maybe add fixes-tag:
>>>
>>> Fixes: 9c95e4ef0657 ("perf evlist: Add evlist__findnew_tracking_event()
>>> helper")
>>>
>>> If it is ok, I will send v2 version with this fixes-tag.
>>
>> I think the maintainer can add the fixes tag when they add the reviewed-by tag:
>
> I usually do this, but if the submitter does it I'll have just to check
> that it is the right tag, helps a bit in processing.
Okay, will send v2 version.
>
> - Arnaldo
>
>> Reviewed-by: Ian Rogers <[email protected]>
Thanks for reviewed-by tag.
Thanks,
Yang