From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Yang Jihong <yangjihong@bytedance.com>
Cc: peterz@infradead.org, mingo@redhat.com, namhyung@kernel.org,
mark.rutland@arm.com, alexander.shishkin@linux.intel.com,
jolsa@kernel.org, irogers@google.com, adrian.hunter@intel.com,
kan.liang@linux.intel.com, james.clark@arm.com,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] perf evsel: Use evsel__name_is() helper
Date: Tue, 2 Apr 2024 18:10:07 -0300 [thread overview]
Message-ID: <Zgx0Lwq_MIcRN9r5@x1> (raw)
In-Reply-To: <ZgxzmkCGNXLipMVg@x1>
On Tue, Apr 02, 2024 at 06:07:42PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Apr 01, 2024 at 02:27:24PM +0800, Yang Jihong wrote:
> > Code cleanup, replace strcmp(evsel__name(evsel, {NAME})) with
> > evsel__name_is() helper.
> >
> > No functional change.
>
> Well, its not building:
>
> builtin-trace.c: In function ‘cmd_trace’:
> builtin-trace.c:4919:90: error: macro "assert" passed 2 arguments, but takes just 1
> 4919 | assert(evsel__name_is(trace.syscalls.events.bpf_output), "__augmented_syscalls__");
> | ^
> In file included from /home/acme/git/perf-tools-next/tools/include/linux/kernel.h:7,
> from /home/acme/git/perf-tools-next/tools/include/linux/refcount.h:42,
> from util/cgroup.h:6,
> from builtin-trace.c:29:
> /usr/include/assert.h:50: note: macro "assert" defined here
>
> I'll fix it, but please test build things next time.
>
> - Arnaldo
>
> > Signed-off-by: Yang Jihong <yangjihong@bytedance.com>
> > Reviewed-by: Ian Rogers <irogers@google.com>
> > ---
> > 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__");
This:
⬢[acme@toolbox perf-tools-next]$ git diff
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index cdb64ed243c416bf..e5fef39c34bf40ff 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -4916,7 +4916,7 @@ int cmd_trace(int argc, const char **argv)
goto out;
}
trace.syscalls.events.bpf_output = evlist__last(trace.evlist);
- assert(evsel__name_is(trace.syscalls.events.bpf_output), "__augmented_syscalls__");
+ assert(evsel__name_is(trace.syscalls.events.bpf_output, "__augmented_syscalls__"));
skip_augmentation:
#endif
err = -1;
⬢[acme@toolbox perf-tools-next]$
> > 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
next prev parent reply other threads:[~2024-04-02 21:10 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-01 6:27 [PATCH v2 0/2] perf sched time -g/--call-graph option & perf Yang Jihong
2024-04-01 6:27 ` [PATCH v2 1/2] perf sched timehist: Fix -g/--call-graph option failure Yang Jihong
2024-04-01 6:27 ` [PATCH v2 2/2] perf evsel: Use evsel__name_is() helper Yang Jihong
2024-04-02 21:07 ` Arnaldo Carvalho de Melo
2024-04-02 21:10 ` Arnaldo Carvalho de Melo [this message]
2024-04-03 7:45 ` [External] " Yang Jihong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zgx0Lwq_MIcRN9r5@x1 \
--to=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=irogers@google.com \
--cc=james.clark@arm.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=yangjihong@bytedance.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).