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

[Davidlohr Bueso]

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