public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if the evsel name is equal to a given string
       [not found] <ZEGLM8VehJbS0gP2@kernel.org>
@ 2023-04-20 19:10 ` Arnaldo Carvalho de Melo
  2023-04-20 19:28   ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-04-20 19:10 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Linux Kernel Mailing List, Adrian Hunter, Ian Rogers, Jiri Olsa,
	Liang, Kan, Namhyung Kim

Em Thu, Apr 20, 2023 at 03:57:55PM -0300, Arnaldo Carvalho de Melo escreveu:
> This makes the logic a bit clear by avoiding the !strcmp() pattern and
> also a way to intercept the pointer if we need to do extra validation on
> it or to do lazy setting of evsel->name via evsel__name(evsel).

+ this, looking if there are others...

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index d3cbee7460fcc48e..e95a545f2c600810 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2169,10 +2169,8 @@ static void setup_system_wide(int forks)
 		struct evsel *counter;
 
 		evlist__for_each_entry(evsel_list, counter) {
-			if (!counter->core.requires_cpu &&
-			    strcmp(counter->name, "duration_time")) {
+			if (!counter->core.requires_cpu && evsel__name_is(counter, "duration_time"))
 				return;
-			}
 		}
 
 		if (evsel_list->core.nr_entries)

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if the evsel name is equal to a given string
  2023-04-20 19:10 ` [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if the evsel name is equal to a given string Arnaldo Carvalho de Melo
@ 2023-04-20 19:28   ` Arnaldo Carvalho de Melo
  2023-04-20 21:16     ` Liang, Kan
  0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-04-20 19:28 UTC (permalink / raw)
  To: Linux Kernel Mailing List
  Cc: Adrian Hunter, Ian Rogers, Jiri Olsa, Liang, Kan, Namhyung Kim

Em Thu, Apr 20, 2023 at 04:10:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Apr 20, 2023 at 03:57:55PM -0300, Arnaldo Carvalho de Melo escreveu:
> > This makes the logic a bit clear by avoiding the !strcmp() pattern and
> > also a way to intercept the pointer if we need to do extra validation on
> > it or to do lazy setting of evsel->name via evsel__name(evsel).
> 
> + this, looking if there are others...

Somehow the first message didn't go thru, so below is the combined
patch, this is an effort to avoid accessing evsel->name directly as the
preferred way to get an evsel name is evsel__name(), so looking for
direct access and providing accessors that avoid that.

- Arnaldo

From e60455d6a4e35ba0c376966443294586a1adc3ec Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Thu, 20 Apr 2023 15:54:11 -0300
Subject: [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if
 the evsel name is equal to a given string

This makes the logic a bit clear by avoiding the !strcmp() pattern and
also a way to intercept the pointer if we need to do extra validation on
it or to do lazy setting of evsel->name via evsel__name(evsel).

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: "Liang, Kan" <kan.liang@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Link: https://lore.kernel.org/lkml/ZEGLM8VehJbS0gP2@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/arch/arm64/util/kvm-stat.c   |  4 ++--
 tools/perf/arch/powerpc/util/kvm-stat.c |  4 ++--
 tools/perf/arch/x86/util/kvm-stat.c     |  8 ++++----
 tools/perf/builtin-kvm.c                |  6 +++---
 tools/perf/builtin-stat.c               |  2 +-
 tools/perf/tests/expand-cgroup.c        |  2 +-
 tools/perf/tests/parse-events.c         | 12 ++++++------
 tools/perf/tests/parse-metric.c         |  2 +-
 tools/perf/tests/pmu-events.c           |  2 +-
 tools/perf/util/evsel.c                 |  7 ++++++-
 tools/perf/util/evsel.h                 |  1 +
 11 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/tools/perf/arch/arm64/util/kvm-stat.c b/tools/perf/arch/arm64/util/kvm-stat.c
index 72ca9bb45804d109..6611aa21cba957d9 100644
--- a/tools/perf/arch/arm64/util/kvm-stat.c
+++ b/tools/perf/arch/arm64/util/kvm-stat.c
@@ -44,14 +44,14 @@ static bool event_begin(struct evsel *evsel,
 			struct perf_sample *sample __maybe_unused,
 			struct event_key *key __maybe_unused)
 {
-	return !strcmp(evsel->name, kvm_entry_trace);
+	return evsel__name_is(evsel, kvm_entry_trace);
 }
 
 static bool event_end(struct evsel *evsel,
 		      struct perf_sample *sample,
 		      struct event_key *key)
 {
-	if (!strcmp(evsel->name, kvm_exit_trace)) {
+	if (evsel__name_is(evsel, kvm_exit_trace)) {
 		event_get_key(evsel, sample, key);
 		return true;
 	}
diff --git a/tools/perf/arch/powerpc/util/kvm-stat.c b/tools/perf/arch/powerpc/util/kvm-stat.c
index d04a08c9fd19c58c..ea1220d66b6758b2 100644
--- a/tools/perf/arch/powerpc/util/kvm-stat.c
+++ b/tools/perf/arch/powerpc/util/kvm-stat.c
@@ -60,13 +60,13 @@ static bool hcall_event_end(struct evsel *evsel,
 			    struct perf_sample *sample __maybe_unused,
 			    struct event_key *key __maybe_unused)
 {
-	return (!strcmp(evsel->name, kvm_events_tp[3]));
+	return (evsel__name_is(evsel, kvm_events_tp[3]));
 }
 
 static bool hcall_event_begin(struct evsel *evsel,
 			      struct perf_sample *sample, struct event_key *key)
 {
-	if (!strcmp(evsel->name, kvm_events_tp[2])) {
+	if (evsel__name_is(evsel, kvm_events_tp[2])) {
 		hcall_event_get_key(evsel, sample, key);
 		return true;
 	}
diff --git a/tools/perf/arch/x86/util/kvm-stat.c b/tools/perf/arch/x86/util/kvm-stat.c
index ef513def03bac71c..424716518b755915 100644
--- a/tools/perf/arch/x86/util/kvm-stat.c
+++ b/tools/perf/arch/x86/util/kvm-stat.c
@@ -46,7 +46,7 @@ static bool mmio_event_begin(struct evsel *evsel,
 		return true;
 
 	/* MMIO write begin event in kernel. */
-	if (!strcmp(evsel->name, "kvm:kvm_mmio") &&
+	if (evsel__name_is(evsel, "kvm:kvm_mmio") &&
 	    evsel__intval(evsel, sample, "type") == KVM_TRACE_MMIO_WRITE) {
 		mmio_event_get_key(evsel, sample, key);
 		return true;
@@ -63,7 +63,7 @@ static bool mmio_event_end(struct evsel *evsel, struct perf_sample *sample,
 		return true;
 
 	/* MMIO read end event in kernel.*/
-	if (!strcmp(evsel->name, "kvm:kvm_mmio") &&
+	if (evsel__name_is(evsel, "kvm:kvm_mmio") &&
 	    evsel__intval(evsel, sample, "type") == KVM_TRACE_MMIO_READ) {
 		mmio_event_get_key(evsel, sample, key);
 		return true;
@@ -101,7 +101,7 @@ static bool ioport_event_begin(struct evsel *evsel,
 			       struct perf_sample *sample,
 			       struct event_key *key)
 {
-	if (!strcmp(evsel->name, "kvm:kvm_pio")) {
+	if (evsel__name_is(evsel, "kvm:kvm_pio")) {
 		ioport_event_get_key(evsel, sample, key);
 		return true;
 	}
@@ -145,7 +145,7 @@ static bool msr_event_begin(struct evsel *evsel,
 			       struct perf_sample *sample,
 			       struct event_key *key)
 {
-	if (!strcmp(evsel->name, "kvm:kvm_msr")) {
+	if (evsel__name_is(evsel, "kvm:kvm_msr")) {
 		msr_event_get_key(evsel, sample, key);
 		return true;
 	}
diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 747d19336340f28f..71165036e4cac89b 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -625,7 +625,7 @@ void exit_event_get_key(struct evsel *evsel,
 
 bool kvm_exit_event(struct evsel *evsel)
 {
-	return !strcmp(evsel->name, kvm_exit_trace);
+	return evsel__name_is(evsel, kvm_exit_trace);
 }
 
 bool exit_event_begin(struct evsel *evsel,
@@ -641,7 +641,7 @@ bool exit_event_begin(struct evsel *evsel,
 
 bool kvm_entry_event(struct evsel *evsel)
 {
-	return !strcmp(evsel->name, kvm_entry_trace);
+	return evsel__name_is(evsel, kvm_entry_trace);
 }
 
 bool exit_event_end(struct evsel *evsel,
@@ -878,7 +878,7 @@ static bool is_child_event(struct perf_kvm_stat *kvm,
 		return false;
 
 	for (; child_ops->name; child_ops++) {
-		if (!strcmp(evsel->name, child_ops->name)) {
+		if (evsel__name_is(evsel, child_ops->name)) {
 			child_ops->get_key(evsel, sample, key);
 			return true;
 		}
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index d3cbee7460fcc48e..efda63f6bf329b51 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2170,7 +2170,7 @@ static void setup_system_wide(int forks)
 
 		evlist__for_each_entry(evsel_list, counter) {
 			if (!counter->core.requires_cpu &&
-			    strcmp(counter->name, "duration_time")) {
+			    !evsel__name_is(counter, "duration_time")) {
 				return;
 			}
 		}
diff --git a/tools/perf/tests/expand-cgroup.c b/tools/perf/tests/expand-cgroup.c
index ec340880a848d907..9c1a1f18db750607 100644
--- a/tools/perf/tests/expand-cgroup.c
+++ b/tools/perf/tests/expand-cgroup.c
@@ -61,7 +61,7 @@ static int test_expand_events(struct evlist *evlist,
 
 	i = 0;
 	evlist__for_each_entry(evlist, evsel) {
-		if (strcmp(evsel->name, ev_name[i % nr_events])) {
+		if (!evsel__name_is(evsel, ev_name[i % nr_events])) {
 			pr_debug("event name doesn't match:\n");
 			pr_debug("  evsel[%d]: %s\n  expected: %s\n",
 				 i, evsel->name, ev_name[i % nr_events]);
diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
index 6eb1400443adddee..8068cfd89b84f723 100644
--- a/tools/perf/tests/parse-events.c
+++ b/tools/perf/tests/parse-events.c
@@ -1401,7 +1401,7 @@ static int test__checkevent_config_symbol(struct evlist *evlist)
 {
 	struct evsel *evsel = evlist__first(evlist);
 
-	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "insn") == 0);
+	TEST_ASSERT_VAL("wrong name setting", evsel__name_is(evsel, "insn"));
 	return TEST_OK;
 }
 
@@ -1409,7 +1409,7 @@ static int test__checkevent_config_raw(struct evlist *evlist)
 {
 	struct evsel *evsel = evlist__first(evlist);
 
-	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "rawpmu") == 0);
+	TEST_ASSERT_VAL("wrong name setting", evsel__name_is(evsel, "rawpmu"));
 	return TEST_OK;
 }
 
@@ -1417,7 +1417,7 @@ static int test__checkevent_config_num(struct evlist *evlist)
 {
 	struct evsel *evsel = evlist__first(evlist);
 
-	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "numpmu") == 0);
+	TEST_ASSERT_VAL("wrong name setting", evsel__name_is(evsel, "numpmu"));
 	return TEST_OK;
 }
 
@@ -1425,7 +1425,7 @@ static int test__checkevent_config_cache(struct evlist *evlist)
 {
 	struct evsel *evsel = evlist__first(evlist);
 
-	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "cachepmu") == 0);
+	TEST_ASSERT_VAL("wrong name setting", evsel__name_is(evsel, "cachepmu"));
 	return TEST_OK;
 }
 
@@ -1438,7 +1438,7 @@ static int test__intel_pt(struct evlist *evlist)
 {
 	struct evsel *evsel = evlist__first(evlist);
 
-	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "intel_pt//u") == 0);
+	TEST_ASSERT_VAL("wrong name setting", evsel__name_is(evsel, "intel_pt//u"));
 	return TEST_OK;
 }
 
@@ -1446,7 +1446,7 @@ static int test__checkevent_complex_name(struct evlist *evlist)
 {
 	struct evsel *evsel = evlist__first(evlist);
 
-	TEST_ASSERT_VAL("wrong complex name parsing", strcmp(evsel->name, "COMPLEX_CYCLES_NAME:orig=cycles,desc=chip-clock-ticks") == 0);
+	TEST_ASSERT_VAL("wrong complex name parsing", evsel__name_is(evsel, "COMPLEX_CYCLES_NAME:orig=cycles,desc=chip-clock-ticks"));
 	return TEST_OK;
 }
 
diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
index c43b056f9fa395f4..1185b79e6274886e 100644
--- a/tools/perf/tests/parse-metric.c
+++ b/tools/perf/tests/parse-metric.c
@@ -39,7 +39,7 @@ static void load_runtime_stat(struct evlist *evlist, struct value *vals)
 	evlist__for_each_entry(evlist, evsel) {
 		count = find_value(evsel->name, vals);
 		evsel->stats->aggr->counts.val = count;
-		if (!strcmp(evsel->name, "duration_time"))
+		if (evsel__name_is(evsel, "duration_time"))
 			update_stats(&walltime_nsecs_stats, count);
 	}
 }
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index 7f8e864525271483..1dff863b9711cf6d 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -866,7 +866,7 @@ static int test__parsing_callback(const struct pmu_metric *pm,
 	evlist__alloc_aggr_stats(evlist, 1);
 	evlist__for_each_entry(evlist, evsel) {
 		evsel->stats->aggr->counts.val = k;
-		if (!strcmp(evsel->name, "duration_time"))
+		if (evsel__name_is(evsel, "duration_time"))
 			update_stats(&walltime_nsecs_stats, k);
 		k++;
 	}
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index a85a987128aad281..81b854650160c2b0 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -821,6 +821,11 @@ const char *evsel__name(struct evsel *evsel)
 	return "unknown";
 }
 
+bool evsel__name_is(struct evsel *evsel, const char *name)
+{
+	return !strcmp(evsel->name, name);
+}
+
 const char *evsel__group_pmu_name(const struct evsel *evsel)
 {
 	const struct evsel *leader;
@@ -1146,7 +1151,7 @@ static void evsel__set_default_freq_period(struct record_opts *opts,
 
 static bool evsel__is_offcpu_event(struct evsel *evsel)
 {
-	return evsel__is_bpf_output(evsel) && !strcmp(evsel->name, OFFCPU_EVENT);
+	return evsel__is_bpf_output(evsel) && evsel__name_is(evsel, OFFCPU_EVENT);
 }
 
 /*
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 68072ec655ce9fff..1e5d640e4a9bd0f1 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -282,6 +282,7 @@ int arch_evsel__hw_name(struct evsel *evsel, char *bf, size_t size);
 
 int __evsel__hw_cache_type_op_res_name(u8 type, u8 op, u8 result, char *bf, size_t size);
 const char *evsel__name(struct evsel *evsel);
+bool evsel__name_is(struct evsel *evsel, const char *name);
 const char *evsel__group_pmu_name(const struct evsel *evsel);
 const char *evsel__metric_id(const struct evsel *evsel);
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if the evsel name is equal to a given string
  2023-04-20 19:28   ` Arnaldo Carvalho de Melo
@ 2023-04-20 21:16     ` Liang, Kan
  2023-04-24 17:25       ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 6+ messages in thread
From: Liang, Kan @ 2023-04-20 21:16 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Linux Kernel Mailing List
  Cc: Adrian Hunter, Ian Rogers, Jiri Olsa, Namhyung Kim



On 2023-04-20 3:28 p.m., Arnaldo Carvalho de Melo wrote:
> Em Thu, Apr 20, 2023 at 04:10:30PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Thu, Apr 20, 2023 at 03:57:55PM -0300, Arnaldo Carvalho de Melo escreveu:
>>> This makes the logic a bit clear by avoiding the !strcmp() pattern and
>>> also a way to intercept the pointer if we need to do extra validation on
>>> it or to do lazy setting of evsel->name via evsel__name(evsel).
>>
>> + this, looking if there are others...
> 
> Somehow the first message didn't go thru, so below is the combined
> patch, this is an effort to avoid accessing evsel->name directly as the
> preferred way to get an evsel name is evsel__name(), so looking for
> direct access and providing accessors that avoid that.

One more

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 2260e27adf44..3a960a3f6962 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -467,7 +467,7 @@ static int evsel__strcmp(struct evsel *pos, char
*evsel_name)
 		return 0;
 	if (evsel__is_dummy_event(pos))
 		return 1;
-	return strcmp(pos->name, evsel_name);
+	return !evsel__name_is(pos, evsel_name);
 }

 static int evlist__is_enabled(struct evlist *evlist)


> 
> - Arnaldo
> 
> From e60455d6a4e35ba0c376966443294586a1adc3ec Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Thu, 20 Apr 2023 15:54:11 -0300
> Subject: [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if
>  the evsel name is equal to a given string
> 
> This makes the logic a bit clear by avoiding the !strcmp() pattern and
> also a way to intercept the pointer if we need to do extra validation on
> it or to do lazy setting of evsel->name via evsel__name(evsel).
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: "Liang, Kan" <kan.liang@linux.intel.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Link: https://lore.kernel.org/lkml/ZEGLM8VehJbS0gP2@kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

With the above one,

Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan
> ---
>  tools/perf/arch/arm64/util/kvm-stat.c   |  4 ++--
>  tools/perf/arch/powerpc/util/kvm-stat.c |  4 ++--
>  tools/perf/arch/x86/util/kvm-stat.c     |  8 ++++----
>  tools/perf/builtin-kvm.c                |  6 +++---
>  tools/perf/builtin-stat.c               |  2 +-
>  tools/perf/tests/expand-cgroup.c        |  2 +-
>  tools/perf/tests/parse-events.c         | 12 ++++++------
>  tools/perf/tests/parse-metric.c         |  2 +-
>  tools/perf/tests/pmu-events.c           |  2 +-
>  tools/perf/util/evsel.c                 |  7 ++++++-
>  tools/perf/util/evsel.h                 |  1 +
>  11 files changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/perf/arch/arm64/util/kvm-stat.c b/tools/perf/arch/arm64/util/kvm-stat.c
> index 72ca9bb45804d109..6611aa21cba957d9 100644
> --- a/tools/perf/arch/arm64/util/kvm-stat.c
> +++ b/tools/perf/arch/arm64/util/kvm-stat.c
> @@ -44,14 +44,14 @@ static bool event_begin(struct evsel *evsel,
>  			struct perf_sample *sample __maybe_unused,
>  			struct event_key *key __maybe_unused)
>  {
> -	return !strcmp(evsel->name, kvm_entry_trace);
> +	return evsel__name_is(evsel, kvm_entry_trace);
>  }
>  
>  static bool event_end(struct evsel *evsel,
>  		      struct perf_sample *sample,
>  		      struct event_key *key)
>  {
> -	if (!strcmp(evsel->name, kvm_exit_trace)) {
> +	if (evsel__name_is(evsel, kvm_exit_trace)) {
>  		event_get_key(evsel, sample, key);
>  		return true;
>  	}
> diff --git a/tools/perf/arch/powerpc/util/kvm-stat.c b/tools/perf/arch/powerpc/util/kvm-stat.c
> index d04a08c9fd19c58c..ea1220d66b6758b2 100644
> --- a/tools/perf/arch/powerpc/util/kvm-stat.c
> +++ b/tools/perf/arch/powerpc/util/kvm-stat.c
> @@ -60,13 +60,13 @@ static bool hcall_event_end(struct evsel *evsel,
>  			    struct perf_sample *sample __maybe_unused,
>  			    struct event_key *key __maybe_unused)
>  {
> -	return (!strcmp(evsel->name, kvm_events_tp[3]));
> +	return (evsel__name_is(evsel, kvm_events_tp[3]));
>  }
>  
>  static bool hcall_event_begin(struct evsel *evsel,
>  			      struct perf_sample *sample, struct event_key *key)
>  {
> -	if (!strcmp(evsel->name, kvm_events_tp[2])) {
> +	if (evsel__name_is(evsel, kvm_events_tp[2])) {
>  		hcall_event_get_key(evsel, sample, key);
>  		return true;
>  	}
> diff --git a/tools/perf/arch/x86/util/kvm-stat.c b/tools/perf/arch/x86/util/kvm-stat.c
> index ef513def03bac71c..424716518b755915 100644
> --- a/tools/perf/arch/x86/util/kvm-stat.c
> +++ b/tools/perf/arch/x86/util/kvm-stat.c
> @@ -46,7 +46,7 @@ static bool mmio_event_begin(struct evsel *evsel,
>  		return true;
>  
>  	/* MMIO write begin event in kernel. */
> -	if (!strcmp(evsel->name, "kvm:kvm_mmio") &&
> +	if (evsel__name_is(evsel, "kvm:kvm_mmio") &&
>  	    evsel__intval(evsel, sample, "type") == KVM_TRACE_MMIO_WRITE) {
>  		mmio_event_get_key(evsel, sample, key);
>  		return true;
> @@ -63,7 +63,7 @@ static bool mmio_event_end(struct evsel *evsel, struct perf_sample *sample,
>  		return true;
>  
>  	/* MMIO read end event in kernel.*/
> -	if (!strcmp(evsel->name, "kvm:kvm_mmio") &&
> +	if (evsel__name_is(evsel, "kvm:kvm_mmio") &&
>  	    evsel__intval(evsel, sample, "type") == KVM_TRACE_MMIO_READ) {
>  		mmio_event_get_key(evsel, sample, key);
>  		return true;
> @@ -101,7 +101,7 @@ static bool ioport_event_begin(struct evsel *evsel,
>  			       struct perf_sample *sample,
>  			       struct event_key *key)
>  {
> -	if (!strcmp(evsel->name, "kvm:kvm_pio")) {
> +	if (evsel__name_is(evsel, "kvm:kvm_pio")) {
>  		ioport_event_get_key(evsel, sample, key);
>  		return true;
>  	}
> @@ -145,7 +145,7 @@ static bool msr_event_begin(struct evsel *evsel,
>  			       struct perf_sample *sample,
>  			       struct event_key *key)
>  {
> -	if (!strcmp(evsel->name, "kvm:kvm_msr")) {
> +	if (evsel__name_is(evsel, "kvm:kvm_msr")) {
>  		msr_event_get_key(evsel, sample, key);
>  		return true;
>  	}
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 747d19336340f28f..71165036e4cac89b 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -625,7 +625,7 @@ void exit_event_get_key(struct evsel *evsel,
>  
>  bool kvm_exit_event(struct evsel *evsel)
>  {
> -	return !strcmp(evsel->name, kvm_exit_trace);
> +	return evsel__name_is(evsel, kvm_exit_trace);
>  }
>  
>  bool exit_event_begin(struct evsel *evsel,
> @@ -641,7 +641,7 @@ bool exit_event_begin(struct evsel *evsel,
>  
>  bool kvm_entry_event(struct evsel *evsel)
>  {
> -	return !strcmp(evsel->name, kvm_entry_trace);
> +	return evsel__name_is(evsel, kvm_entry_trace);
>  }
>  
>  bool exit_event_end(struct evsel *evsel,
> @@ -878,7 +878,7 @@ static bool is_child_event(struct perf_kvm_stat *kvm,
>  		return false;
>  
>  	for (; child_ops->name; child_ops++) {
> -		if (!strcmp(evsel->name, child_ops->name)) {
> +		if (evsel__name_is(evsel, child_ops->name)) {
>  			child_ops->get_key(evsel, sample, key);
>  			return true;
>  		}
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index d3cbee7460fcc48e..efda63f6bf329b51 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -2170,7 +2170,7 @@ static void setup_system_wide(int forks)
>  
>  		evlist__for_each_entry(evsel_list, counter) {
>  			if (!counter->core.requires_cpu &&
> -			    strcmp(counter->name, "duration_time")) {
> +			    !evsel__name_is(counter, "duration_time")) {
>  				return;
>  			}
>  		}
> diff --git a/tools/perf/tests/expand-cgroup.c b/tools/perf/tests/expand-cgroup.c
> index ec340880a848d907..9c1a1f18db750607 100644
> --- a/tools/perf/tests/expand-cgroup.c
> +++ b/tools/perf/tests/expand-cgroup.c
> @@ -61,7 +61,7 @@ static int test_expand_events(struct evlist *evlist,
>  
>  	i = 0;
>  	evlist__for_each_entry(evlist, evsel) {
> -		if (strcmp(evsel->name, ev_name[i % nr_events])) {
> +		if (!evsel__name_is(evsel, ev_name[i % nr_events])) {
>  			pr_debug("event name doesn't match:\n");
>  			pr_debug("  evsel[%d]: %s\n  expected: %s\n",
>  				 i, evsel->name, ev_name[i % nr_events]);
> diff --git a/tools/perf/tests/parse-events.c b/tools/perf/tests/parse-events.c
> index 6eb1400443adddee..8068cfd89b84f723 100644
> --- a/tools/perf/tests/parse-events.c
> +++ b/tools/perf/tests/parse-events.c
> @@ -1401,7 +1401,7 @@ static int test__checkevent_config_symbol(struct evlist *evlist)
>  {
>  	struct evsel *evsel = evlist__first(evlist);
>  
> -	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "insn") == 0);
> +	TEST_ASSERT_VAL("wrong name setting", evsel__name_is(evsel, "insn"));
>  	return TEST_OK;
>  }
>  
> @@ -1409,7 +1409,7 @@ static int test__checkevent_config_raw(struct evlist *evlist)
>  {
>  	struct evsel *evsel = evlist__first(evlist);
>  
> -	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "rawpmu") == 0);
> +	TEST_ASSERT_VAL("wrong name setting", evsel__name_is(evsel, "rawpmu"));
>  	return TEST_OK;
>  }
>  
> @@ -1417,7 +1417,7 @@ static int test__checkevent_config_num(struct evlist *evlist)
>  {
>  	struct evsel *evsel = evlist__first(evlist);
>  
> -	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "numpmu") == 0);
> +	TEST_ASSERT_VAL("wrong name setting", evsel__name_is(evsel, "numpmu"));
>  	return TEST_OK;
>  }
>  
> @@ -1425,7 +1425,7 @@ static int test__checkevent_config_cache(struct evlist *evlist)
>  {
>  	struct evsel *evsel = evlist__first(evlist);
>  
> -	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "cachepmu") == 0);
> +	TEST_ASSERT_VAL("wrong name setting", evsel__name_is(evsel, "cachepmu"));
>  	return TEST_OK;
>  }
>  
> @@ -1438,7 +1438,7 @@ static int test__intel_pt(struct evlist *evlist)
>  {
>  	struct evsel *evsel = evlist__first(evlist);
>  
> -	TEST_ASSERT_VAL("wrong name setting", strcmp(evsel->name, "intel_pt//u") == 0);
> +	TEST_ASSERT_VAL("wrong name setting", evsel__name_is(evsel, "intel_pt//u"));
>  	return TEST_OK;
>  }
>  
> @@ -1446,7 +1446,7 @@ static int test__checkevent_complex_name(struct evlist *evlist)
>  {
>  	struct evsel *evsel = evlist__first(evlist);
>  
> -	TEST_ASSERT_VAL("wrong complex name parsing", strcmp(evsel->name, "COMPLEX_CYCLES_NAME:orig=cycles,desc=chip-clock-ticks") == 0);
> +	TEST_ASSERT_VAL("wrong complex name parsing", evsel__name_is(evsel, "COMPLEX_CYCLES_NAME:orig=cycles,desc=chip-clock-ticks"));
>  	return TEST_OK;
>  }
>  
> diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
> index c43b056f9fa395f4..1185b79e6274886e 100644
> --- a/tools/perf/tests/parse-metric.c
> +++ b/tools/perf/tests/parse-metric.c
> @@ -39,7 +39,7 @@ static void load_runtime_stat(struct evlist *evlist, struct value *vals)
>  	evlist__for_each_entry(evlist, evsel) {
>  		count = find_value(evsel->name, vals);
>  		evsel->stats->aggr->counts.val = count;
> -		if (!strcmp(evsel->name, "duration_time"))
> +		if (evsel__name_is(evsel, "duration_time"))
>  			update_stats(&walltime_nsecs_stats, count);
>  	}
>  }
> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> index 7f8e864525271483..1dff863b9711cf6d 100644
> --- a/tools/perf/tests/pmu-events.c
> +++ b/tools/perf/tests/pmu-events.c
> @@ -866,7 +866,7 @@ static int test__parsing_callback(const struct pmu_metric *pm,
>  	evlist__alloc_aggr_stats(evlist, 1);
>  	evlist__for_each_entry(evlist, evsel) {
>  		evsel->stats->aggr->counts.val = k;
> -		if (!strcmp(evsel->name, "duration_time"))
> +		if (evsel__name_is(evsel, "duration_time"))
>  			update_stats(&walltime_nsecs_stats, k);
>  		k++;
>  	}
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index a85a987128aad281..81b854650160c2b0 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -821,6 +821,11 @@ const char *evsel__name(struct evsel *evsel)
>  	return "unknown";
>  }
>  
> +bool evsel__name_is(struct evsel *evsel, const char *name)
> +{
> +	return !strcmp(evsel->name, name);
> +}
> +
>  const char *evsel__group_pmu_name(const struct evsel *evsel)
>  {
>  	const struct evsel *leader;
> @@ -1146,7 +1151,7 @@ static void evsel__set_default_freq_period(struct record_opts *opts,
>  
>  static bool evsel__is_offcpu_event(struct evsel *evsel)
>  {
> -	return evsel__is_bpf_output(evsel) && !strcmp(evsel->name, OFFCPU_EVENT);
> +	return evsel__is_bpf_output(evsel) && evsel__name_is(evsel, OFFCPU_EVENT);
>  }
>  
>  /*
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 68072ec655ce9fff..1e5d640e4a9bd0f1 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -282,6 +282,7 @@ int arch_evsel__hw_name(struct evsel *evsel, char *bf, size_t size);
>  
>  int __evsel__hw_cache_type_op_res_name(u8 type, u8 op, u8 result, char *bf, size_t size);
>  const char *evsel__name(struct evsel *evsel);
> +bool evsel__name_is(struct evsel *evsel, const char *name);
>  const char *evsel__group_pmu_name(const struct evsel *evsel);
>  const char *evsel__metric_id(const struct evsel *evsel);
>  

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if the evsel name is equal to a given string
  2023-04-20 21:16     ` Liang, Kan
@ 2023-04-24 17:25       ` Arnaldo Carvalho de Melo
  2023-04-24 17:27         ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-04-24 17:25 UTC (permalink / raw)
  To: Liang, Kan
  Cc: Linux Kernel Mailing List, Adrian Hunter, Ian Rogers, Jiri Olsa,
	Namhyung Kim

Em Thu, Apr 20, 2023 at 05:16:18PM -0400, Liang, Kan escreveu:
> 
> 
> On 2023-04-20 3:28 p.m., Arnaldo Carvalho de Melo wrote:
> > Em Thu, Apr 20, 2023 at 04:10:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> >> Em Thu, Apr 20, 2023 at 03:57:55PM -0300, Arnaldo Carvalho de Melo escreveu:
> >>> This makes the logic a bit clear by avoiding the !strcmp() pattern and
> >>> also a way to intercept the pointer if we need to do extra validation on
> >>> it or to do lazy setting of evsel->name via evsel__name(evsel).
> >>
> >> + this, looking if there are others...
> > 
> > Somehow the first message didn't go thru, so below is the combined
> > patch, this is an effort to avoid accessing evsel->name directly as the
> > preferred way to get an evsel name is evsel__name(), so looking for
> > direct access and providing accessors that avoid that.
> 
> One more
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 2260e27adf44..3a960a3f6962 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -467,7 +467,7 @@ static int evsel__strcmp(struct evsel *pos, char
> *evsel_name)
>  		return 0;
>  	if (evsel__is_dummy_event(pos))
>  		return 1;
> -	return strcmp(pos->name, evsel_name);
> +	return !evsel__name_is(pos, evsel_name);
>  }
> 
>  static int evlist__is_enabled(struct evlist *evlist)

Added
 
> > 
> > From e60455d6a4e35ba0c376966443294586a1adc3ec Mon Sep 17 00:00:00 2001
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Date: Thu, 20 Apr 2023 15:54:11 -0300
> > Subject: [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if
> >  the evsel name is equal to a given string
> > 
> > This makes the logic a bit clear by avoiding the !strcmp() pattern and
> > also a way to intercept the pointer if we need to do extra validation on
> > it or to do lazy setting of evsel->name via evsel__name(evsel).
> > 
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Ian Rogers <irogers@google.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: "Liang, Kan" <kan.liang@linux.intel.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Link: https://lore.kernel.org/lkml/ZEGLM8VehJbS0gP2@kernel.org
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> With the above one,
> 
> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Added these extra ones and actually made evsel__name_is() use
evsel__name().

Does your reviewed-by stands after these extra changes?

Thanks,

- Arnaldo


diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 2260e27adf44c579..a0504316b06fbcba 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -467,7 +467,7 @@ static int evsel__strcmp(struct evsel *pos, char *evsel_name)
 		return 0;
 	if (evsel__is_dummy_event(pos))
 		return 1;
-	return strcmp(pos->name, evsel_name);
+	return !evsel__name_is(pos, evsel_name);
 }
 
 static int evlist__is_enabled(struct evlist *evlist)
@@ -1706,7 +1706,7 @@ struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str)
 	evlist__for_each_entry(evlist, evsel) {
 		if (!evsel->name)
 			continue;
-		if (strcmp(str, evsel->name) == 0)
+		if (evsel__name_is(evsel, str))
 			return evsel;
 	}
 
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 81b854650160c2b0..356c07f03be6bfce 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -823,7 +823,7 @@ const char *evsel__name(struct evsel *evsel)
 
 bool evsel__name_is(struct evsel *evsel, const char *name)
 {
-	return !strcmp(evsel->name, name);
+	return !strcmp(evsel__name(evsel), name);
 }
 
 const char *evsel__group_pmu_name(const struct evsel *evsel)
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 31b1cd0935e277ba..dae81d8e1769c763 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -2893,7 +2893,7 @@ static struct evsel *find_evsel(struct evlist *evlist, char *event_name)
 	full_name = !!strchr(event_name, ':');
 	evlist__for_each_entry(evlist, pos) {
 		/* case 2 */
-		if (full_name && !strcmp(pos->name, event_name))
+		if (full_name && evsel__name_is(pos->name, event_name))
 			return pos;
 		/* case 3 */
 		if (!full_name && strstr(pos->name, event_name)) {

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if the evsel name is equal to a given string
  2023-04-24 17:25       ` Arnaldo Carvalho de Melo
@ 2023-04-24 17:27         ` Arnaldo Carvalho de Melo
  2023-04-24 17:50           ` Liang, Kan
  0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-04-24 17:27 UTC (permalink / raw)
  To: Liang, Kan
  Cc: Linux Kernel Mailing List, Adrian Hunter, Ian Rogers, Jiri Olsa,
	Namhyung Kim

Em Mon, Apr 24, 2023 at 02:25:27PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Apr 20, 2023 at 05:16:18PM -0400, Liang, Kan escreveu:
> > 
> > 
> > On 2023-04-20 3:28 p.m., Arnaldo Carvalho de Melo wrote:
> > > Em Thu, Apr 20, 2023 at 04:10:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> > >> Em Thu, Apr 20, 2023 at 03:57:55PM -0300, Arnaldo Carvalho de Melo escreveu:
> > >>> This makes the logic a bit clear by avoiding the !strcmp() pattern and
> > >>> also a way to intercept the pointer if we need to do extra validation on
> > >>> it or to do lazy setting of evsel->name via evsel__name(evsel).
> > >>
> > >> + this, looking if there are others...
> > > 
> > > Somehow the first message didn't go thru, so below is the combined
> > > patch, this is an effort to avoid accessing evsel->name directly as the
> > > preferred way to get an evsel name is evsel__name(), so looking for
> > > direct access and providing accessors that avoid that.
> > 
> > One more
> > 
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index 2260e27adf44..3a960a3f6962 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -467,7 +467,7 @@ static int evsel__strcmp(struct evsel *pos, char
> > *evsel_name)
> >  		return 0;
> >  	if (evsel__is_dummy_event(pos))
> >  		return 1;
> > -	return strcmp(pos->name, evsel_name);
> > +	return !evsel__name_is(pos, evsel_name);
> >  }
> > 
> >  static int evlist__is_enabled(struct evlist *evlist)
> 
> Added
>  
> > > 
> > > From e60455d6a4e35ba0c376966443294586a1adc3ec Mon Sep 17 00:00:00 2001
> > > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > Date: Thu, 20 Apr 2023 15:54:11 -0300
> > > Subject: [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if
> > >  the evsel name is equal to a given string
> > > 
> > > This makes the logic a bit clear by avoiding the !strcmp() pattern and
> > > also a way to intercept the pointer if we need to do extra validation on
> > > it or to do lazy setting of evsel->name via evsel__name(evsel).
> > > 
> > > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > > Cc: Ian Rogers <irogers@google.com>
> > > Cc: Jiri Olsa <jolsa@kernel.org>
> > > Cc: "Liang, Kan" <kan.liang@linux.intel.com>
> > > Cc: Namhyung Kim <namhyung@kernel.org>
> > > Link: https://lore.kernel.org/lkml/ZEGLM8VehJbS0gP2@kernel.org
> > > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > With the above one,
> > 
> > Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
> 
> Added these extra ones and actually made evsel__name_is() use
> evsel__name().
> 
> Does your reviewed-by stands after these extra changes?


oops, fix that pos->name leftover:


diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 2260e27adf44c579..a0504316b06fbcba 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -467,7 +467,7 @@ static int evsel__strcmp(struct evsel *pos, char *evsel_name)
 		return 0;
 	if (evsel__is_dummy_event(pos))
 		return 1;
-	return strcmp(pos->name, evsel_name);
+	return !evsel__name_is(pos, evsel_name);
 }
 
 static int evlist__is_enabled(struct evlist *evlist)
@@ -1706,7 +1706,7 @@ struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str)
 	evlist__for_each_entry(evlist, evsel) {
 		if (!evsel->name)
 			continue;
-		if (strcmp(str, evsel->name) == 0)
+		if (evsel__name_is(evsel, str))
 			return evsel;
 	}
 
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 81b854650160c2b0..356c07f03be6bfce 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -823,7 +823,7 @@ const char *evsel__name(struct evsel *evsel)
 
 bool evsel__name_is(struct evsel *evsel, const char *name)
 {
-	return !strcmp(evsel->name, name);
+	return !strcmp(evsel__name(evsel), name);
 }
 
 const char *evsel__group_pmu_name(const struct evsel *evsel)
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 31b1cd0935e277ba..650cd8df40412a38 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -2893,7 +2893,7 @@ static struct evsel *find_evsel(struct evlist *evlist, char *event_name)
 	full_name = !!strchr(event_name, ':');
 	evlist__for_each_entry(evlist, pos) {
 		/* case 2 */
-		if (full_name && !strcmp(pos->name, event_name))
+		if (full_name && evsel__name_is(pos, event_name))
 			return pos;
 		/* case 3 */
 		if (!full_name && strstr(pos->name, event_name)) {

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if the evsel name is equal to a given string
  2023-04-24 17:27         ` Arnaldo Carvalho de Melo
@ 2023-04-24 17:50           ` Liang, Kan
  0 siblings, 0 replies; 6+ messages in thread
From: Liang, Kan @ 2023-04-24 17:50 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Linux Kernel Mailing List, Adrian Hunter, Ian Rogers, Jiri Olsa,
	Namhyung Kim



On 2023-04-24 1:27 p.m., Arnaldo Carvalho de Melo wrote:
> Em Mon, Apr 24, 2023 at 02:25:27PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Thu, Apr 20, 2023 at 05:16:18PM -0400, Liang, Kan escreveu:
>>>
>>>
>>> On 2023-04-20 3:28 p.m., Arnaldo Carvalho de Melo wrote:
>>>> Em Thu, Apr 20, 2023 at 04:10:30PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>> Em Thu, Apr 20, 2023 at 03:57:55PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>> This makes the logic a bit clear by avoiding the !strcmp() pattern and
>>>>>> also a way to intercept the pointer if we need to do extra validation on
>>>>>> it or to do lazy setting of evsel->name via evsel__name(evsel).
>>>>>
>>>>> + this, looking if there are others...
>>>>
>>>> Somehow the first message didn't go thru, so below is the combined
>>>> patch, this is an effort to avoid accessing evsel->name directly as the
>>>> preferred way to get an evsel name is evsel__name(), so looking for
>>>> direct access and providing accessors that avoid that.
>>>
>>> One more
>>>
>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>> index 2260e27adf44..3a960a3f6962 100644
>>> --- a/tools/perf/util/evlist.c
>>> +++ b/tools/perf/util/evlist.c
>>> @@ -467,7 +467,7 @@ static int evsel__strcmp(struct evsel *pos, char
>>> *evsel_name)
>>>  		return 0;
>>>  	if (evsel__is_dummy_event(pos))
>>>  		return 1;
>>> -	return strcmp(pos->name, evsel_name);
>>> +	return !evsel__name_is(pos, evsel_name);
>>>  }
>>>
>>>  static int evlist__is_enabled(struct evlist *evlist)
>>
>> Added
>>  
>>>>
>>>> From e60455d6a4e35ba0c376966443294586a1adc3ec Mon Sep 17 00:00:00 2001
>>>> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>>>> Date: Thu, 20 Apr 2023 15:54:11 -0300
>>>> Subject: [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if
>>>>  the evsel name is equal to a given string
>>>>
>>>> This makes the logic a bit clear by avoiding the !strcmp() pattern and
>>>> also a way to intercept the pointer if we need to do extra validation on
>>>> it or to do lazy setting of evsel->name via evsel__name(evsel).
>>>>
>>>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>>>> Cc: Ian Rogers <irogers@google.com>
>>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>>> Cc: "Liang, Kan" <kan.liang@linux.intel.com>
>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>> Link: https://lore.kernel.org/lkml/ZEGLM8VehJbS0gP2@kernel.org
>>>> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>>>
>>> With the above one,
>>>
>>> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
>>
>> Added these extra ones and actually made evsel__name_is() use
>> evsel__name().
>>
>> Does your reviewed-by stands after these extra changes?
>

Yes, the below extra changes look good to me as well.

Thanks,
Kan

> 
> oops, fix that pos->name leftover:
> 
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 2260e27adf44c579..a0504316b06fbcba 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -467,7 +467,7 @@ static int evsel__strcmp(struct evsel *pos, char *evsel_name)
>  		return 0;
>  	if (evsel__is_dummy_event(pos))
>  		return 1;
> -	return strcmp(pos->name, evsel_name);
> +	return !evsel__name_is(pos, evsel_name);
>  }
>  
>  static int evlist__is_enabled(struct evlist *evlist)
> @@ -1706,7 +1706,7 @@ struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const char *str)
>  	evlist__for_each_entry(evlist, evsel) {
>  		if (!evsel->name)
>  			continue;
> -		if (strcmp(str, evsel->name) == 0)
> +		if (evsel__name_is(evsel, str))
>  			return evsel;
>  	}
>  
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 81b854650160c2b0..356c07f03be6bfce 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -823,7 +823,7 @@ const char *evsel__name(struct evsel *evsel)
>  
>  bool evsel__name_is(struct evsel *evsel, const char *name)
>  {
> -	return !strcmp(evsel->name, name);
> +	return !strcmp(evsel__name(evsel), name);
>  }
>  
>  const char *evsel__group_pmu_name(const struct evsel *evsel)
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 31b1cd0935e277ba..650cd8df40412a38 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -2893,7 +2893,7 @@ static struct evsel *find_evsel(struct evlist *evlist, char *event_name)
>  	full_name = !!strchr(event_name, ':');
>  	evlist__for_each_entry(evlist, pos) {
>  		/* case 2 */
> -		if (full_name && !strcmp(pos->name, event_name))
> +		if (full_name && evsel__name_is(pos, event_name))
>  			return pos;
>  		/* case 3 */
>  		if (!full_name && strstr(pos->name, event_name)) {

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-04-24 17:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <ZEGLM8VehJbS0gP2@kernel.org>
2023-04-20 19:10 ` [PATCH 1/1] perf evsel: Introduce evsel__name_is() method to check if the evsel name is equal to a given string Arnaldo Carvalho de Melo
2023-04-20 19:28   ` Arnaldo Carvalho de Melo
2023-04-20 21:16     ` Liang, Kan
2023-04-24 17:25       ` Arnaldo Carvalho de Melo
2023-04-24 17:27         ` Arnaldo Carvalho de Melo
2023-04-24 17:50           ` Liang, Kan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox