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

[Davidlohr Bueso <dave@stgolabs.net>]

On Mon, 17 Oct 2016, Sebastian Andrzej Siewior wrote:

>+#ifdef HAVE_LIBNUMA_SUPPORT
>+#include <numa.h>
>+#endif

In futex.h

>+static int numa_node = -1;

In futex.h (perhaps rename to futexbench_numa_node?)

>+#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;
>+}

In futex.h

>-	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...

>+		if (numa_node < 0) {
>+			nthreads = ncpus;
>+		} else {

here.

>+			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;
>+		}

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?

>+		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