Skip to contentSkip to conversation

perf bench futex: cache align the worer struct

PATCH In review Sebastian Andrzej Siewior 5 participants 16 messages Oct 16, 2016 – Oct 24, 2016

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/2 perf bench futex: cache align the worer struct

It popped up in perf testing that the worker consumes some amount of CPU. It boils down to the increment of ops which causes cache line bouncing between the individual threads.
The patch aligns the struct by 256 bytes to ensure that not a cache line is shared among CPUs. 128 byte is the x86 worst case and grep says that L1_CACHE_SHIFT is set to 8 on s390.

---
 tools/perf/bench/futex-hash.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
tools/perf/bench/futex-hash.c +4 -1
index 8024cd5febd2..d9e5e80bb4d0 100644
@@ -39,12 +39,15 @@ static unsigned int threads_starting;
 static struct stats throughput_stats;
 static pthread_cond_t thread_parent, thread_worker;
 
+#define SMP_CACHE_BYTES 256
+#define __cacheline_aligned __attribute__ ((aligned (SMP_CACHE_BYTES)))
+
 struct worker {
 	int tid;
 	u_int32_t *futex;
 	pthread_t thread;
 	unsigned long ops;
-};
+} __cacheline_aligned;
 
 static const struct option options[] = {
 	OPT_UINTEGER('t', "threads", &nthreads, "Specify amount of threads"),
Signature · 1 line
2.9.3
#1 lore raw

PATCH 2/2 perf bench futex: add NUMA support

By default the application uses malloc() and all available CPUs. This patch introduces NUMA support which means:
- memory is allocated node local via numa_alloc_local()
- all CPUs of the specified NUMA node are used. This is also true if the number of threads set is greater than the number of CPUs available on this node.

---
 tools/perf/bench/Build        |  4 ++
 tools/perf/bench/futex-hash.c | 87 ++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 81 insertions(+), 10 deletions(-)
tools/perf/bench/Build +4 -0
index 60bf11943047..9e6e518d7d62 100644
@@ -1,3 +1,7 @@
+ifdef CONFIG_NUMA
+CFLAGS_futex-hash.o   += -DCONFIG_NUMA=1
+endif
+
 perf-y += sched-messaging.o
 perf-y += sched-pipe.o
 perf-y += mem-functions.o
tools/perf/bench/futex-hash.c +77 -10

148 lines not on this page · raw message

Signature · 1 line
2.9.3
#2 lore raw

Re: [PATCH 2/2] perf bench futex: add NUMA support

Em Sun, Oct 16, 2016 at 09:08:03PM +0200, Sebastian Andrzej Siewior escreveu:

Quoted 18 lines

By default the application uses malloc() and all available CPUs. This patch introduces NUMA support which means:
- memory is allocated node local via numa_alloc_local()
- all CPUs of the specified NUMA node are used. This is also true if the number of threads set is greater than the number of CPUs available on this node.

---
 tools/perf/bench/Build        |  4 ++
 tools/perf/bench/futex-hash.c | 87 ++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 81 insertions(+), 10 deletions(-)
tools/perf/bench/Build +0 -0
index 60bf11943047..9e6e518d7d62 100644

tools/perf/bench/Build line 3

Quoted 3 lines
tools/perf/bench/Build +3 -0
@@ -1,3 +1,7 @@
+ifdef CONFIG_NUMA
+CFLAGS_futex-hash.o   += -DCONFIG_NUMA=1
+endif

Jiri, do we really need this? I.e. aren't the CONFIG_FOO defines available to tools?

Quoted 164 lines
tools/perf/bench/Build +0 -0
 perf-y += sched-messaging.o
 perf-y += sched-pipe.o
 perf-y += mem-functions.o
tools/perf/bench/futex-hash.c +77 -10
index d9e5e80bb4d0..8db4a5bd6a4e 100644
@@ -25,6 +25,9 @@
 
 #include <err.h>
 #include <sys/time.h>
+#ifdef CONFIG_NUMA
+#include <numa.h>
+#endif
 
 static unsigned int nthreads = 0;
 static unsigned int nsecs    = 10;
@@ -32,6 +35,7 @@ static unsigned int nsecs    = 10;
 static unsigned int nfutexes = 1024;
 static bool fshared = false, done = false, silent = false;
 static int futex_flag = 0;
+static int numa_node = -1;
 
 struct timeval start, end, runtime;
 static pthread_mutex_t thread_lock;
@@ -55,9 +59,28 @@ static const struct option options[] = {
 	OPT_UINTEGER('f', "futexes", &nfutexes, "Specify amount of futexes per threads"),
 	OPT_BOOLEAN( 's', "silent",  &silent,   "Silent mode: do not display data/details"),
 	OPT_BOOLEAN( 'S', "shared",  &fshared,  "Use shared futexes instead of private ones"),
+#ifdef CONFIG_NUMA
+	OPT_INTEGER( 'n', "numa",   &numa_node,  "Specify the NUMA node"),
+#endif
 	OPT_END()
 };
 
+#ifndef CONFIG_NUMA
+static int numa_run_on_node(int node __maybe_unused) { return 0; }
+static int numa_node_of_cpu(int node __maybe_unused) { return 0; }
+static void *numa_alloc_local(size_t size) { return malloc(size); }
+static void numa_free(void *p, size_t size __maybe_unused) { return free(p); }
+#endif
+
+static bool cpu_is_local(int cpu)
+{
+	if (numa_node < 0)
+		return true;
+	if (numa_node_of_cpu(cpu) == numa_node)
+		return true;
+	return false;
+}
+
 static const char * const bench_futex_hash_usage[] = {
 	"perf bench futex hash <options>",
 	NULL
@@ -123,6 +146,8 @@ int bench_futex_hash(int argc, const char **argv,
 	unsigned int i, ncpus;
 	pthread_attr_t thread_attr;
 	struct worker *worker = NULL;
+	char *node_str = NULL;
+	unsigned int cpunum;
 
 	argc = parse_options(argc, argv, options, bench_futex_hash_usage, 0);
 	if (argc) {
@@ -136,18 +161,50 @@ int bench_futex_hash(int argc, const char **argv,
 	act.sa_sigaction = toggle_done;
 	sigaction(SIGINT, &act, NULL);
 
-	if (!nthreads) /* default to the number of CPUs */
-		nthreads = ncpus;
+	if (!nthreads) {
+		/* default to the number of CPUs per NUMA node */
+		if (numa_node < 0) {
+			nthreads = ncpus;
+		} else {
+			for (i = 0; i < ncpus; i++) {
+				if (cpu_is_local(i))
+					nthreads++;
+			}
+			if (!nthreads)
+				err(EXIT_FAILURE, "No online CPUs for this node");
+		}
+	} else {
+		int cpu_available = 0;
 
-	worker = calloc(nthreads, sizeof(*worker));
+		for (i = 0; i < ncpus && !cpu_available; i++) {
+			if (cpu_is_local(i))
+				cpu_available = 1;
+		}
+		if (!cpu_available)
+			err(EXIT_FAILURE, "No online CPUs for this node");
+	}
+
+	if (numa_node >= 0) {
+		ret = numa_run_on_node(numa_node);
+		if (ret < 0)
+			err(EXIT_FAILURE, "numa_run_on_node");
+		ret = asprintf(&node_str, " on node %d", numa_node);
+		if (ret < 0)
+			err(EXIT_FAILURE, "numa_node, asprintf");
+	}
+
+	worker = numa_alloc_local(nthreads * sizeof(*worker));
 	if (!worker)
 		goto errmem;
 
 	if (!fshared)
 		futex_flag = FUTEX_PRIVATE_FLAG;
 
-	printf("Run summary [PID %d]: %d threads, each operating on %d [%s] futexes for %d secs.\n\n",
-	       getpid(), nthreads, nfutexes, fshared ? "shared":"private", nsecs);
+	printf("Run summary [PID %d]: %d threads%s, each operating on %d [%s] futexes for %d secs.\n\n",
+	       getpid(), nthreads,
+	       node_str ? : "",
+	       nfutexes, fshared ? "shared":"private",
+	       nsecs);
 
 	init_stats(&throughput_stats);
 	pthread_mutex_init(&thread_lock, NULL);
@@ -157,14 +214,24 @@ int bench_futex_hash(int argc, const char **argv,
 	threads_starting = nthreads;
 	pthread_attr_init(&thread_attr);
 	gettimeofday(&start, NULL);
-	for (i = 0; i < nthreads; i++) {
+	for (cpunum = 0, i = 0; i < nthreads; i++, cpunum++) {
+
+		do {
+			if (cpu_is_local(cpunum))
+				break;
+			cpunum++;
+			if (cpunum > ncpus)
+				cpunum = 0;
+		} while (1);
+
 		worker[i].tid = i;
-		worker[i].futex = calloc(nfutexes, sizeof(*worker[i].futex));
+		worker[i].futex = numa_alloc_local(nfutexes *
+						   sizeof(*worker[i].futex));
 		if (!worker[i].futex)
 			goto errmem;
 
 		CPU_ZERO(&cpu);
-		CPU_SET(i % ncpus, &cpu);
+		CPU_SET(cpunum % ncpus, &cpu);
 
 		ret = pthread_attr_setaffinity_np(&thread_attr, sizeof(cpu_set_t), &cpu);
 		if (ret)
@@ -211,12 +278,12 @@ int bench_futex_hash(int argc, const char **argv,
 				       &worker[i].futex[nfutexes-1], t);
 		}
 
-		free(worker[i].futex);
+		numa_free(worker[i].futex, nfutexes * sizeof(*worker[i].futex));
 	}
 
 	print_summary();
 
-	free(worker);
+	numa_free(worker, nthreads * sizeof(*worker));
 	return ret;
 errmem:
 	err(EXIT_FAILURE, "calloc");
Signature · 1 line
2.9.3
#3 lore raw

Re: [PATCH 2/2] perf bench futex: add NUMA support

Quoted 14 lines · Arnaldo Carvalho de Melo

Em Sun, Oct 16, 2016 at 09:08:03PM +0200, Sebastian Andrzej Siewior escreveu:

Quoted 13 lines

By default the application uses malloc() and all available CPUs. This patch introduces NUMA support which means:
- memory is allocated node local via numa_alloc_local()
- all CPUs of the specified NUMA node are used. This is also true if the number of threads set is greater than the number of CPUs available on this node.

---
 tools/perf/bench/Build        |  4 ++
 tools/perf/bench/futex-hash.c | 87 ++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 81 insertions(+), 10 deletions(-)
Quoted 11 lines

tools/perf/bench/Build line 3

Quoted 8 lines
tools/perf/bench/Build +3 -0
index 60bf11943047..9e6e518d7d62 100644
@@ -1,3 +1,7 @@
+ifdef CONFIG_NUMA
+CFLAGS_futex-hash.o   += -DCONFIG_NUMA=1
+endif

Jiri, do we really need this? I.e. aren't the CONFIG_FOO defines available to tools?

not directly ATM.. it's prepared for the .config customary setting feature

meanwhile we set HAVE_* defines, like for CONFIG_NUMA we have:
-DHAVE_LIBNUMA_SUPPORT

you can check it in Makefile.config

jirka

#4 lore raw

Re: [PATCH 2/2] perf bench futex: add NUMA support

Em Mon, Oct 17, 2016 at 05:01:23PM +0200, Jiri Olsa escreveu:

Quoted 27 lines · Arnaldo Carvalho de Melo
Quoted 14 lines

Em Sun, Oct 16, 2016 at 09:08:03PM +0200, Sebastian Andrzej Siewior escreveu:

Quoted 13 lines

By default the application uses malloc() and all available CPUs. This patch introduces NUMA support which means:
- memory is allocated node local via numa_alloc_local()
- all CPUs of the specified NUMA node are used. This is also true if the number of threads set is greater than the number of CPUs available on this node.

---
 tools/perf/bench/Build        |  4 ++
 tools/perf/bench/futex-hash.c | 87 ++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 81 insertions(+), 10 deletions(-)
Quoted 11 lines

tools/perf/bench/Build line 3

Quoted 8 lines
tools/perf/bench/Build +3 -0
index 60bf11943047..9e6e518d7d62 100644
@@ -1,3 +1,7 @@
+ifdef CONFIG_NUMA
+CFLAGS_futex-hash.o   += -DCONFIG_NUMA=1
+endif

Jiri, do we really need this? I.e. aren't the CONFIG_FOO defines available to tools?

Quoted 6 lines

not directly ATM.. it's prepared for the .config customary setting feature

meanwhile we set HAVE_* defines, like for CONFIG_NUMA we have:
-DHAVE_LIBNUMA_SUPPORT

you can check it in Makefile.config

So, Andrzej, can you use that instead? I merged the first patch already.

- Arnaldo

#5 lore raw

PATCH v2 2/2 perf bench futex: add NUMA support

By default the application uses malloc() and all available CPUs. This patch introduces NUMA support which means:
- memory is allocated node local via numa_alloc_local()
- all CPUs of the specified NUMA node are used. This is also true if the number of threads set is greater than the number of CPUs available on this node.

---
v1…v2: use HAVE_LIBNUMA_SUPPORT instead the custom variant

 tools/perf/bench/futex-hash.c | 87 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 77 insertions(+), 10 deletions(-)
tools/perf/bench/futex-hash.c +77 -10
index d9e5e80bb4d0..17d38ce829f8 100644
@@ -25,6 +25,9 @@
 
 #include <err.h>
 #include <sys/time.h>
+#ifdef HAVE_LIBNUMA_SUPPORT
+#include <numa.h>
+#endif
 
 static unsigned int nthreads = 0;
 static unsigned int nsecs    = 10;
@@ -32,6 +35,7 @@ static unsigned int nsecs    = 10;
 static unsigned int nfutexes = 1024;
 static bool fshared = false, done = false, silent = false;
 static int futex_flag = 0;
+static int numa_node = -1;
 
 struct timeval start, end, runtime;
 static pthread_mutex_t thread_lock;
@@ -55,9 +59,28 @@ static const struct option options[] = {
 	OPT_UINTEGER('f', "futexes", &nfutexes, "Specify amount of futexes per threads"),
 	OPT_BOOLEAN( 's', "silent",  &silent,   "Silent mode: do not display data/details"),
 	OPT_BOOLEAN( 'S', "shared",  &fshared,  "Use shared futexes instead of private ones"),
+#ifdef HAVE_LIBNUMA_SUPPORT
+	OPT_INTEGER( 'n', "numa",   &numa_node,  "Specify the NUMA node"),
+#endif
 	OPT_END()
 };
 
+#ifndef HAVE_LIBNUMA_SUPPORT
+static int numa_run_on_node(int node __maybe_unused) { return 0; }
+static int numa_node_of_cpu(int node __maybe_unused) { return 0; }
+static void *numa_alloc_local(size_t size) { return malloc(size); }
+static void numa_free(void *p, size_t size __maybe_unused) { return free(p); }
+#endif
+
+static bool cpu_is_local(int cpu)
+{
+	if (numa_node < 0)
+		return true;
+	if (numa_node_of_cpu(cpu) == numa_node)
+		return true;
+	return false;
+}
+
 static const char * const bench_futex_hash_usage[] = {
 	"perf bench futex hash <options>",
 	NULL
@@ -123,6 +146,8 @@ int bench_futex_hash(int argc, const char **argv,
 	unsigned int i, ncpus;
 	pthread_attr_t thread_attr;
 	struct worker *worker = NULL;
+	char *node_str = NULL;
+	unsigned int cpunum;
 
 	argc = parse_options(argc, argv, options, bench_futex_hash_usage, 0);
 	if (argc) {
@@ -136,18 +161,50 @@ int bench_futex_hash(int argc, const char **argv,
 	act.sa_sigaction = toggle_done;
 	sigaction(SIGINT, &act, NULL);
 
-	if (!nthreads) /* default to the number of CPUs */
-		nthreads = ncpus;
+	if (!nthreads) {
+		/* default to the number of CPUs per NUMA node */
+		if (numa_node < 0) {
+			nthreads = ncpus;
+		} else {
+			for (i = 0; i < ncpus; i++) {
+				if (cpu_is_local(i))
+					nthreads++;
+			}
+			if (!nthreads)
+				err(EXIT_FAILURE, "No online CPUs for this node");
+		}
+	} else {
+		int cpu_available = 0;
 
-	worker = calloc(nthreads, sizeof(*worker));
+		for (i = 0; i < ncpus && !cpu_available; i++) {
+			if (cpu_is_local(i))
+				cpu_available = 1;
+		}
+		if (!cpu_available)
+			err(EXIT_FAILURE, "No online CPUs for this node");
+	}
+
+	if (numa_node >= 0) {
+		ret = numa_run_on_node(numa_node);
+		if (ret < 0)
+			err(EXIT_FAILURE, "numa_run_on_node");
+		ret = asprintf(&node_str, " on node %d", numa_node);
+		if (ret < 0)
+			err(EXIT_FAILURE, "numa_node, asprintf");
+	}
+
+	worker = numa_alloc_local(nthreads * sizeof(*worker));
 	if (!worker)
 		goto errmem;
 
 	if (!fshared)
 		futex_flag = FUTEX_PRIVATE_FLAG;
 
-	printf("Run summary [PID %d]: %d threads, each operating on %d [%s] futexes for %d secs.\n\n",
-	       getpid(), nthreads, nfutexes, fshared ? "shared":"private", nsecs);
+	printf("Run summary [PID %d]: %d threads%s, each operating on %d [%s] futexes for %d secs.\n\n",
+	       getpid(), nthreads,
+	       node_str ? : "",
+	       nfutexes, fshared ? "shared":"private",
+	       nsecs);
 
 	init_stats(&throughput_stats);
 	pthread_mutex_init(&thread_lock, NULL);
@@ -157,14 +214,24 @@ int bench_futex_hash(int argc, const char **argv,
 	threads_starting = nthreads;
 	pthread_attr_init(&thread_attr);
 	gettimeofday(&start, NULL);
-	for (i = 0; i < nthreads; i++) {
+	for (cpunum = 0, i = 0; i < nthreads; i++, cpunum++) {
+
+		do {
+			if (cpu_is_local(cpunum))
+				break;
+			cpunum++;
+			if (cpunum > ncpus)
+				cpunum = 0;
+		} while (1);
+
 		worker[i].tid = i;
-		worker[i].futex = calloc(nfutexes, sizeof(*worker[i].futex));
+		worker[i].futex = numa_alloc_local(nfutexes *
+						   sizeof(*worker[i].futex));
 		if (!worker[i].futex)
 			goto errmem;
 
 		CPU_ZERO(&cpu);
-		CPU_SET(i % ncpus, &cpu);
+		CPU_SET(cpunum % ncpus, &cpu);
 
 		ret = pthread_attr_setaffinity_np(&thread_attr, sizeof(cpu_set_t), &cpu);
 		if (ret)
@@ -211,12 +278,12 @@ int bench_futex_hash(int argc, const char **argv,
 				       &worker[i].futex[nfutexes-1], t);
 		}
 
-		free(worker[i].futex);
+		numa_free(worker[i].futex, nfutexes * sizeof(*worker[i].futex));
 	}
 
 	print_summary();
 
-	free(worker);
+	numa_free(worker, nthreads * sizeof(*worker));
 	return ret;
 errmem:
 	err(EXIT_FAILURE, "calloc");
Signature · 1 line
2.9.3
#6 lore raw

Re: [PATCH 2/2 v2] perf bench futex: add NUMA support

Quoted 6 lines · Sebastian Andrzej Siewior

By default the application uses malloc() and all available CPUs. This patch introduces NUMA support which means:
- memory is allocated node local via numa_alloc_local()
- all CPUs of the specified NUMA node are used. This is also true if the number of threads set is greater than the number of CPUs available on this node.

Can't we just use numactl to bind cpus and memory to be node-local?

Thanks,
Davidlohr

#7 lore raw

Re: [PATCH 2/2 v2] perf bench futex: add NUMA support

Quoted 10 lines · Davidlohr Bueso
Quoted 6 lines · Sebastian Andrzej Siewior

By default the application uses malloc() and all available CPUs. This patch introduces NUMA support which means:
- memory is allocated node local via numa_alloc_local()
- all CPUs of the specified NUMA node are used. This is also true if the number of threads set is greater than the number of CPUs available on this node.

Can't we just use numactl to bind cpus and memory to be node-local?

something like

	numactl --cpunodebind=$NODE --membind=$NODE perf …
?
This should work for memory however since we use
	pthread_attr_setaffinity_np(&thread_attr, sizeof(cpu_set_t), &cpu);

we would need to query the affinity mask, and deploy threads based on that mask.
Using NUMA support within this bench-tool has also the side effect that the output gives all the option used.

Quoted 2 lines

Thanks,
Davidlohr

Sebastian

#8 lore raw

Re: [PATCH 2/2 v2] perf bench futex: add NUMA support

Quoted 10 lines · Sebastian Andrzej Siewior
Quoted 9 lines · Davidlohr Bueso
Quoted 6 lines · Sebastian Andrzej Siewior

By default the application uses malloc() and all available CPUs. This patch introduces NUMA support which means:
- memory is allocated node local via numa_alloc_local()
- all CPUs of the specified NUMA node are used. This is also true if the number of threads set is greater than the number of CPUs available on this node.

Quoted 5 lines
Quoted 1 line

Can't we just use numactl to bind cpus and memory to be node-local?

something like
numactl --cpunodebind=$NODE --membind=$NODE perf ???
?

Yes.

Quoted 4 lines

This should work for memory however since we use

	pthread_attr_setaffinity_np(&thread_attr, sizeof(cpu_set_t), &cpu);

we would need to query the affinity mask, and deploy threads based on that mask.

Ah right. I also considered getting rid of the affinity, but that would probably hurt more than help (or at least alter) for non-numa options.

Quoted 2 lines

Using NUMA support within this bench-tool has also the side effect that the output gives all the option used.

So if we are going to support the numa option for the benchmark, could you please move the new code into futex.h instead of futex-hash.c? That way we can integrate the support for the other futex programs as well.

Thanks,
Davidlohr

#9 lore raw

Re: [PATCH 2/2 v2] perf bench futex: add NUMA support

In reply to Sebastian Andrzej Siewior

tools/perf/bench/futex-hash.c line 30

Quoted 3 lines · Sebastian Andrzej Siewior
tools/perf/bench/futex-hash.c +3 -0
+#ifdef HAVE_LIBNUMA_SUPPORT
+#include <numa.h>
+#endif

In futex.h

Quoted 1 line

+static int numa_node = -1;

In futex.h (perhaps rename to futexbench_numa_node?)

Quoted 5 lines
tools/perf/bench/futex-hash.c +5 -0
+#ifndef HAVE_LIBNUMA_SUPPORT
+static int numa_run_on_node(int node __maybe_unused) { return 0; }
+static int numa_node_of_cpu(int node __maybe_unused) { return 0; }
+static void *numa_alloc_local(size_t size) { return malloc(size); }
+static void numa_free(void *p, size_t size __maybe_unused) { return free(p); }

tools/perf/bench/futex-hash.c static const struct option options[] = { · line 82

Quoted 10 lines
tools/perf/bench/futex-hash.c +10 -0
+#endif
+
+static bool cpu_is_local(int cpu)
+{
+	if (numa_node < 0)
+		return true;
+	if (numa_node_of_cpu(cpu) == numa_node)
+		return true;
+	return false;
+}

In futex.h

tools/perf/bench/futex-hash.c int bench_futex_hash(int argc, const char **argv, · line 165

Quoted 4 lines
tools/perf/bench/futex-hash.c +2 -2
-	if (!nthreads) /* default to the number of CPUs */
-		nthreads = ncpus;
+	if (!nthreads) {
+		/* default to the number of CPUs per NUMA node */

This comment should go...

tools/perf/bench/futex-hash.c int bench_futex_hash(int argc, const char **argv, · line 168

Quoted 3 lines
tools/perf/bench/futex-hash.c +3 -0
+		if (numa_node < 0) {
+			nthreads = ncpus;
+		} else {

here.

Quoted 5 lines
tools/perf/bench/futex-hash.c +5 -0
+			for (i = 0; i < ncpus; i++) {
+				if (cpu_is_local(i))
+					nthreads++;
+			}
+			if (!nthreads)

tools/perf/bench/futex-hash.c int bench_futex_hash(int argc, const char **argv, · line 182

Quoted 10 lines
tools/perf/bench/futex-hash.c +8 -1
+				err(EXIT_FAILURE, "No online CPUs for this node");
+		}
+	} else {
+		int cpu_available = 0;
 
-	worker = calloc(nthreads, sizeof(*worker));
+		for (i = 0; i < ncpus && !cpu_available; i++) {
+			if (cpu_is_local(i))
+				cpu_available = 1;
+		}

Is this really necessary? If the user passes the number of threads, then we shouldn't care about ncpus; we just run all the threads on the specified node. Wouldn't the below numa_run_on_node() ensure that the node is not, for example, CPU-less?

tools/perf/bench/futex-hash.c int bench_futex_hash(int argc, const char **argv, · line 190

Quoted 8 lines
tools/perf/bench/futex-hash.c +8 -0
+		if (!cpu_available)
+			err(EXIT_FAILURE, "No online CPUs for this node");
+	}
+
+	if (numa_node >= 0) {
+		ret = numa_run_on_node(numa_node);
+		if (ret < 0)
+			err(EXIT_FAILURE, "numa_run_on_node");

Thanks,
Davidlohr

#10 lore raw
In reply to Sebastian Andrzej Siewior
Quoted 3 lines · Sebastian Andrzej Siewior

It popped up in perf testing that the worker consumes some amount of CPU. It boils down to the increment of ops which causes cache line bouncing between the individual threads.

Are you referring to this?

      │         for (i = 0; i < nfutexes; i++, w->ops++) {
      │ be:   add    $0x1,%ebx

65.87 │ addq $0x1,0x18(%r12)

(which is like 65% of 13% on my box with a default futex-hash run).

Even better, could we get rid entirely of the ops increments and just use a local variable, then update the worker at the end of the function. The following makes 'perf' pretty much disappear in the profile.

Thanks,
Davidlohr

----8<--------------
From: Davidlohr Bueso <[email protected]>
Subject: [PATCH] perf/bench-futex: Avoid worker cacheline bouncing

Sebastian noted that overhead for worker thread ops (throughput) accounting was producing 'perf' to appear in the profiles, consuming a non-trivial (ie 13%) amount of CPU. This is due to cacheline bouncing due to the increment of w->ops. We can easily fix this by just working on a local copy and updating the actual worker once done running, and ready to show the program summary. There is no danger of the worker being concurrent, so we can trust that no stale value is being seen by another thread.

---
tools/perf/bench/futex-hash.c | 8 ++++++--

1 file changed, 6 insertions(+), 2 deletions(-)
tools/perf/bench/futex-hash.c +0 -0
index 8024cd5febd2..0b9583164da0 100644
@@ -63,8 +63,10 @@ static const char * const bench_futex_hash_usage[] = {

static void *workerfn(void *arg)

{
	int ret;
-	unsigned int i;
	struct worker *w = (struct worker *) arg;

+ unsigned int i;
+ unsigned long ops = w->ops; /* avoid cacheline-bouncing */
+

	pthread_mutex_lock(&thread_lock);
	threads_starting--;
@@ -74,7 +76,7 @@ static void *workerfn(void *arg)
	pthread_mutex_unlock(&thread_lock);

	do {

- for (i = 0; i < nfutexes; i++, w->ops++) {
+ for (i = 0; i < nfutexes; i++, ops++) {
/*
* We want the futex calls to fail in order to stress
* the hashing of uaddr and not measure other steps,
@@ -88,6 +90,8 @@ static void *workerfn(void *arg)

		}
	}  while (!done);

+ w->ops = ops;
+

	return NULL;
}
Signature · 1 line
2.6.6
#11 lore raw
Quoted 6 lines · Davidlohr Bueso
Quoted 3 lines · Sebastian Andrzej Siewior

It popped up in perf testing that the worker consumes some amount of CPU. It boils down to the increment of ops which causes cache line bouncing between the individual threads.

Quoted 7 lines

Are you referring to this?

      │         for (i = 0; i < nfutexes; i++, w->ops++) {
      │ be:   add    $0x1,%ebx

65.87 │ addq $0x1,0x18(%r12)

(which is like 65% of 13% on my box with a default futex-hash run).

correct.

Quoted 3 lines

Even better, could we get rid entirely of the ops increments and just use a local variable, then update the worker at the end of the function. The following makes 'perf' pretty much disappear in the profile.

this should do it, too. So what remains is the read access for w->futex but since it does not pop up in perf, it is probably not that important.

Quoted 2 lines

Thanks,
Davidlohr

Sebastian

#12 lore raw

PATCH 1/1 perf/bench-futex: Avoid worker cacheline bouncing

Sebastian noted that overhead for worker thread ops (throughput) accounting was producing 'perf' to appear in the profiles, consuming a non-trivial (ie 13%) amount of CPU. This is due to cacheline bouncing due to the increment of w->ops. We can easily fix this by just working on a local copy and updating the actual worker once done running, and ready to show the program summary. There is no danger of the worker being concurrent, so we can trust that no stale value is being seen by another thread.

---
This is an updated version as I noticed lock-pi was also doing this. acme, could you please pick up if no objection? Thanks.

tools/perf/bench/futex-hash.c | 6 ++++--
tools/perf/bench/futex-lock-pi.c | 4 +++-

2 files changed, 7 insertions(+), 3 deletions(-)
tools/perf/bench/futex-hash.c +0 -0
index 8024cd5febd2..da04b8c5568a 100644
@@ -63,8 +63,9 @@ static const char * const bench_futex_hash_usage[] = {

static void *workerfn(void *arg)

{
	int ret;
-	unsigned int i;
	struct worker *w = (struct worker *) arg;

+ unsigned int i;
+ unsigned long ops = w->ops; /* avoid cacheline bouncing */

	pthread_mutex_lock(&thread_lock);
	threads_starting--;
@@ -74,7 +75,7 @@ static void *workerfn(void *arg)
	pthread_mutex_unlock(&thread_lock);

	do {

- for (i = 0; i < nfutexes; i++, w->ops++) {
+ for (i = 0; i < nfutexes; i++, ops++) {
/*
* We want the futex calls to fail in order to stress
* the hashing of uaddr and not measure other steps,
@@ -88,6 +89,7 @@ static void *workerfn(void *arg)

		}
	}  while (!done);
+	w->ops = ops;
	return NULL;
}
tools/perf/bench/futex-lock-pi.c +0 -0
index 936d89d30483..7032e4643c65 100644
@@ -75,6 +75,7 @@ static void toggle_done(int sig __maybe_unused,

static void *workerfn(void *arg)

{
	struct worker *w = (struct worker *) arg;

+ unsigned long ops = w->ops;

	pthread_mutex_lock(&thread_lock);
	threads_starting--;
@@ -103,9 +104,10 @@ static void *workerfn(void *arg)
		if (ret && !silent)
			warn("thread %d: Could not unlock pi-lock for %p (%d)",
			     w->tid, w->futex, ret);
-		w->ops++; /* account for thread's share of work */
+		ops++; /* account for thread's share of work */
	}  while (!done);
+	w->ops = ops;
	return NULL;
}
Signature · 1 line
2.6.6
author Acked-by on v1 1/1
#13 lore raw

Re: [PATCH] perf/bench-futex: Avoid worker cacheline bouncing

Quoted 10 lines · Davidlohr Bueso

Sebastian noted that overhead for worker thread ops (throughput) accounting was producing 'perf' to appear in the profiles, consuming a non-trivial (ie 13%) amount of CPU. This is due to cacheline bouncing due to the increment of w->ops. We can easily fix this by just working on a local copy and updating the actual worker once done running, and ready to show the program summary. There is no danger of the worker being concurrent, so we can trust that no stale value is being seen by another thread.

tools/perf/bench/futex-hash.c static const char * const bench_futex_hash_usage[] = { · line 68

Quoted 10 lines
tools/perf/bench/futex-hash.c +2 -1
@@ -63,8 +63,9 @@ static const char * const bench_futex_hash_usage[] = {
 static void *workerfn(void *arg)
 {
 	int ret;
-	unsigned int i;
 	struct worker *w = (struct worker *) arg;
+	unsigned int i;
+	unsigned long ops = w->ops; /* avoid cacheline bouncing */

we start at 0 so there is probably no need to init it with w->ops.

Sebastian

#14 lore raw

Re: [PATCH] perf/bench-futex: Avoid worker cacheline bouncing

Quoted 10 lines · Sebastian Andrzej Siewior
Quoted 9 lines · Davidlohr Bueso

Sebastian noted that overhead for worker thread ops (throughput) accounting was producing 'perf' to appear in the profiles, consuming a non-trivial (ie 13%) amount of CPU. This is due to cacheline bouncing due to the increment of w->ops. We can easily fix this by just working on a local copy and updating the actual worker once done running, and ready to show the program summary. There is no danger of the worker being concurrent, so we can trust that no stale value is being seen by another thread.

Quoted 2 lines
Quoted 1 line

Thanks.

Quoted 13 lines

tools/perf/bench/futex-hash.c static const char * const bench_futex_hash_usage[] = { · line 68

Quoted 11 lines
tools/perf/bench/futex-hash.c +2 -1
@@ -63,8 +63,9 @@ static const char * const bench_futex_hash_usage[] = {
 static void *workerfn(void *arg)
 {
 	int ret;
-	unsigned int i;
 	struct worker *w = (struct worker *) arg;
+	unsigned int i;
+	unsigned long ops = w->ops; /* avoid cacheline bouncing */

we start at 0 so there is probably no need to init it with w->ops.

Yeah, but I prefer having it this way - separates the init from the actual work (although no big deal here). The extra load happens ncpu times, so also no big deal.

#15 lore raw

[tip:perf/core] perf bench futex: Cache align the worker struct

In reply to Sebastian Andrzej Siewior
perf bench futex: Cache align the worker struct
Commit-ID:  34b753007d646482a4125a7095e1d1986d395f95
Gitweb:     http://git.kernel.org/tip/34b753007d646482a4125a7095e1d1986d395f95
Author:     Sebastian Andrzej Siewior <[email protected]>
AuthorDate: Sun, 16 Oct 2016 21:08:02 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 24 Oct 2016 11:07:45 -0300

perf bench futex: Cache align the worker struct

It popped up in perf testing that the worker consumes some amount of CPU. It boils down to the increment of ops which causes cache line bouncing between the individual threads.

This patch aligns the struct by 256 bytes to ensure that not a cache line is shared among CPUs. 128 byte is the x86 worst case and grep says that L1_CACHE_SHIFT is set to 8 on s390.

---
 tools/perf/bench/futex-hash.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
tools/perf/bench/futex-hash.c +4 -1
index 8024cd5..d9e5e80 100644
@@ -39,12 +39,15 @@ static unsigned int threads_starting;
 static struct stats throughput_stats;
 static pthread_cond_t thread_parent, thread_worker;
 
+#define SMP_CACHE_BYTES 256
+#define __cacheline_aligned __attribute__ ((aligned (SMP_CACHE_BYTES)))
+
 struct worker {
 	int tid;
 	u_int32_t *futex;
 	pthread_t thread;
 	unsigned long ops;
-};
+} __cacheline_aligned;
 
 static const struct option options[] = {
 	OPT_UINTEGER('t', "threads", &nthreads, "Specify amount of threads"),