[PATCH 0/4] perf tools: Remove needless affinity setup/cleanup for pid targets

[PATCH 0/4] perf tools: Remove needless affinity setup/cleanup for pid targets

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Hi,

	Continuing with the removal of needless affinity setup/cleanup
for pid targets, please ack.

- Arnaldo

Arnaldo Carvalho de Melo (4):
  perf affinity: Allow passing a NULL arg to affinity__cleanup()
  perf stat: No need to setup affinities when starting a workload
  perf evlist: No need to setup affinities when enabling events for pid
    targets
  perf evlist: No need to setup affinities when disabling events for pid
    targets

 tools/perf/builtin-stat.c  | 17 ++++++++++-------
 tools/perf/util/affinity.c |  8 +++++++-
 tools/perf/util/evlist.c   | 28 ++++++++++++++++++----------
 3 files changed, 35 insertions(+), 18 deletions(-)

-- 
2.34.1

[PATCH 1/4] perf affinity: Allow passing a NULL arg to affinity__cleanup()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Just like with free(), NULL is checked to avoid having all callers do
it.

Its convenient for when not using affinity setup/cleanup for dummy CPU
maps, i.e. CPU maps for pid targets.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/affinity.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/affinity.c b/tools/perf/util/affinity.c
index f1e30d566db3c835..4d216c0dc4259b9e 100644
--- a/tools/perf/util/affinity.c
+++ b/tools/perf/util/affinity.c
@@ -62,7 +62,7 @@ void affinity__set(struct affinity *a, int cpu)
 	clear_bit(cpu, a->sched_cpus);
 }
 
-void affinity__cleanup(struct affinity *a)
+static void __affinity__cleanup(struct affinity *a)
 {
 	int cpu_set_size = get_cpu_set_size();
 
@@ -71,3 +71,9 @@ void affinity__cleanup(struct affinity *a)
 	zfree(&a->sched_cpus);
 	zfree(&a->orig_cpus);
 }
+
+void affinity__cleanup(struct affinity *a)
+{
+	if (a != NULL)
+		__affinity__cleanup(a);
+}
-- 
2.34.1

[PATCH 2/4] perf stat: No need to setup affinities when starting a workload

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

I.e. the simple:

  $ perf stat sleep 1

Uses a dummy CPU map and thus there is no need to setup/cleanup
affinities to avoid IPIs, etc.

With this we're down to a sched_getaffinity() call, in the libnuma
initialization, that probably can be removed in a followup patch.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-stat.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 973ade18b72a9479..934e992c966fb950 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -788,7 +788,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 	const bool forks = (argc > 0);
 	bool is_pipe = STAT_RECORD ? perf_stat.data.is_pipe : false;
 	struct evlist_cpu_iterator evlist_cpu_itr;
-	struct affinity affinity;
+	struct affinity saved_affinity, *affinity = NULL;
 	int err;
 	bool second_pass = false;
 
@@ -803,8 +803,11 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 	if (group)
 		evlist__set_leader(evsel_list);
 
-	if (affinity__setup(&affinity) < 0)
-		return -1;
+	if (!cpu_map__is_dummy(evsel_list->core.cpus)) {
+		if (affinity__setup(&saved_affinity) < 0)
+			return -1;
+		affinity = &saved_affinity;
+	}
 
 	evlist__for_each_entry(evsel_list, counter) {
 		if (bpf_counter__load(counter, &target))
@@ -813,7 +816,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 			all_counters_use_bpf = false;
 	}
 
-	evlist__for_each_cpu(evlist_cpu_itr, evsel_list, &affinity) {
+	evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) {
 		counter = evlist_cpu_itr.evsel;
 
 		/*
@@ -869,7 +872,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 		 */
 
 		/* First close errored or weak retry */
-		evlist__for_each_cpu(evlist_cpu_itr, evsel_list, &affinity) {
+		evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) {
 			counter = evlist_cpu_itr.evsel;
 
 			if (!counter->reset_group && !counter->errored)
@@ -878,7 +881,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 			perf_evsel__close_cpu(&counter->core, evlist_cpu_itr.cpu_map_idx);
 		}
 		/* Now reopen weak */
-		evlist__for_each_cpu(evlist_cpu_itr, evsel_list, &affinity) {
+		evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) {
 			counter = evlist_cpu_itr.evsel;
 
 			if (!counter->reset_group && !counter->errored)
@@ -904,7 +907,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 			counter->supported = true;
 		}
 	}
-	affinity__cleanup(&affinity);
+	affinity__cleanup(affinity);
 
 	evlist__for_each_entry(evsel_list, counter) {
 		if (!counter->supported) {
-- 
2.34.1

[PATCH 3/4] perf evlist: No need to setup affinities when enabling events for pid targets

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

When the target is a pid, not started by 'perf stat' we need to enable
the events, and in that case there is no need to setup affinities as we
use a dummy CPU map, with just one entry set to -1.

So stop doing it to avoid this needless call to sched_getaffinity():

  # strace -ke sched_getaffinity perf stat -e cycles -p 241957 sleep 1
  <SNIP>
  sched_getaffinity(0, 512, [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31]) = 8
   > /usr/lib64/libc-2.33.so(sched_getaffinity@@GLIBC_2.3.4+0x1a) [0xe6eea]
   > /var/home/acme/bin/perf(affinity__setup+0x6a) [0x5329ca]
   > /var/home/acme/bin/perf(__evlist__enable.constprop.0+0x23) [0x4b9693]
   > /var/home/acme/bin/perf(enable_counters+0x14d) [0x42de5d]
   > /var/home/acme/bin/perf(cmd_stat+0x2358) [0x4310c8]
   > /var/home/acme/bin/perf(run_builtin+0x6a) [0x4a2cfa]
   > /var/home/acme/bin/perf(main+0x612) [0x40f8c2]
   > /usr/lib64/libc-2.33.so(__libc_start_main+0xd4) [0x27b74]
   > /var/home/acme/bin/perf(_start+0x2d) [0x40fadd]
  <SNIP>

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 6e88d404b5b3e96f..ae6d4363da76ec56 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -487,12 +487,16 @@ static void __evlist__enable(struct evlist *evlist, char *evsel_name)
 {
 	struct evsel *pos;
 	struct evlist_cpu_iterator evlist_cpu_itr;
-	struct affinity affinity;
+	struct affinity saved_affinity, *affinity = NULL;
 
-	if (affinity__setup(&affinity) < 0)
-		return;
+	// See explanation in evlist__close()
+	if (!cpu_map__is_dummy(evlist->core.cpus)) {
+		if (affinity__setup(&saved_affinity) < 0)
+			return;
+		affinity = &saved_affinity;
+	}
 
-	evlist__for_each_cpu(evlist_cpu_itr, evlist, &affinity) {
+	evlist__for_each_cpu(evlist_cpu_itr, evlist, affinity) {
 		pos = evlist_cpu_itr.evsel;
 		if (evsel__strcmp(pos, evsel_name))
 			continue;
@@ -500,7 +504,7 @@ static void __evlist__enable(struct evlist *evlist, char *evsel_name)
 			continue;
 		evsel__enable_cpu(pos, evlist_cpu_itr.cpu_map_idx);
 	}
-	affinity__cleanup(&affinity);
+	affinity__cleanup(affinity);
 	evlist__for_each_entry(evlist, pos) {
 		if (evsel__strcmp(pos, evsel_name))
 			continue;
-- 
2.34.1

[PATCH 4/4] perf evlist: No need to setup affinities when disabling events for pid targets

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

When the target is a pid, not started by 'perf stat' we need to disable
the events, and in that case there is no need to setup affinities as we
use a dummy CPU map, with just one entry set to -1.

So stop doing it to avoid this needless call to sched_getaffinity():

  # strace -ke sched_getaffinity perf stat -e cycles -p 241957 sleep 1
  <SNIP>
  sched_getaffinity(0, 512, [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31]) = 8
   > /usr/lib64/libc-2.33.so(sched_getaffinity@@GLIBC_2.3.4+0x1a) [0xe6eea]
   > /var/home/acme/bin/perf(affinity__setup+0x6a) [0x532a2a]
   > /var/home/acme/bin/perf(__evlist__disable.constprop.0+0x27) [0x4b9827]
   > /var/home/acme/bin/perf(cmd_stat+0x29b5) [0x431725]
   > /var/home/acme/bin/perf(run_builtin+0x6a) [0x4a2cfa]
   > /var/home/acme/bin/perf(main+0x612) [0x40f8c2]
   > /usr/lib64/libc-2.33.so(__libc_start_main+0xd4) [0x27b74]
   > /var/home/acme/bin/perf(_start+0x2d) [0x40fadd]
  <SNIP>

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index ae6d4363da76ec56..eaad04e1672a4752 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -430,15 +430,19 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
 {
 	struct evsel *pos;
 	struct evlist_cpu_iterator evlist_cpu_itr;
-	struct affinity affinity;
+	struct affinity saved_affinity, *affinity = NULL;
 	bool has_imm = false;
 
-	if (affinity__setup(&affinity) < 0)
-		return;
+	// See explanation in evlist__close()
+	if (!cpu_map__is_dummy(evlist->core.cpus)) {
+		if (affinity__setup(&saved_affinity) < 0)
+			return;
+		affinity = &saved_affinity;
+	}
 
 	/* Disable 'immediate' events last */
 	for (int imm = 0; imm <= 1; imm++) {
-		evlist__for_each_cpu(evlist_cpu_itr, evlist, &affinity) {
+		evlist__for_each_cpu(evlist_cpu_itr, evlist, affinity) {
 			pos = evlist_cpu_itr.evsel;
 			if (evsel__strcmp(pos, evsel_name))
 				continue;
@@ -454,7 +458,7 @@ static void __evlist__disable(struct evlist *evlist, char *evsel_name)
 			break;
 	}
 
-	affinity__cleanup(&affinity);
+	affinity__cleanup(affinity);
 	evlist__for_each_entry(evlist, pos) {
 		if (evsel__strcmp(pos, evsel_name))
 			continue;
-- 
2.34.1

Re: [PATCH 0/4] perf tools: Remove needless affinity setup/cleanup for pid targets

[Ian Rogers]

On Mon, Jan 17, 2022 at 8:09 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> Hi,
>
>         Continuing with the removal of needless affinity setup/cleanup
> for pid targets, please ack.

The whole set:

Acked-by: Ian Rogers <irogers@google.com>

Thanks!
Ian

> - Arnaldo
>
> Arnaldo Carvalho de Melo (4):
>   perf affinity: Allow passing a NULL arg to affinity__cleanup()
>   perf stat: No need to setup affinities when starting a workload
>   perf evlist: No need to setup affinities when enabling events for pid
>     targets
>   perf evlist: No need to setup affinities when disabling events for pid
>     targets
>
>  tools/perf/builtin-stat.c  | 17 ++++++++++-------
>  tools/perf/util/affinity.c |  8 +++++++-
>  tools/perf/util/evlist.c   | 28 ++++++++++++++++++----------
>  3 files changed, 35 insertions(+), 18 deletions(-)
>
> --
> 2.34.1
>