[dpdk-dev] [PATCH v5 08/11] build: optional NUMA and cpu counts detection

[Bruce Richardson]

On Wed, Oct 28, 2020 at 03:04:02PM +0100, Juraj Linkeš wrote:
> Add an option to automatically discover the host's numa and cpu counts
> and use those values for a non cross-build.
> Give users the option to override the per-arch default values or values
> from cross files by specifying them on the command line with -Dmax_lcores
> and -Dmax_numa_nodes.
> 
> Signed-off-by: Juraj Linkeš <juraj.linkes at pantheon.tech>
> ---
>  buildtools/get_cpu_count.py  |  7 ++++++
>  buildtools/get_numa_count.py | 22 +++++++++++++++++
>  buildtools/meson.build       |  2 ++
>  config/meson.build           | 48 ++++++++++++++++++++++++++++++++++--
>  config/x86/meson.build       |  2 ++
>  meson_options.txt            |  8 +++---
>  6 files changed, 83 insertions(+), 6 deletions(-)
>  create mode 100644 buildtools/get_cpu_count.py
>  create mode 100644 buildtools/get_numa_count.py
> 
> diff --git a/buildtools/get_cpu_count.py b/buildtools/get_cpu_count.py
> new file mode 100644
> index 000000000..386f85f8b
> --- /dev/null
> +++ b/buildtools/get_cpu_count.py
> @@ -0,0 +1,7 @@
> +#!/usr/bin/python3

I think for DPDK python scripts we standardized on "/usr/bin/env python3"
[Someone please correct me if I'm wrong here!]

> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright (c) 2020 PANTHEON.tech s.r.o.
> +
> +import os
> +
> +print(os.cpu_count())
> diff --git a/buildtools/get_numa_count.py b/buildtools/get_numa_count.py
> new file mode 100644
> index 000000000..e7f3de6b0
> --- /dev/null
> +++ b/buildtools/get_numa_count.py
> @@ -0,0 +1,22 @@
> +#!/usr/bin/python3
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright (c) 2020 PANTHEON.tech s.r.o.
> +
> +import ctypes
> +import glob
> +import os
> +import subprocess
> +
> +if os.name == 'posix':
> +    if os.path.isdir('/sys/devices/system/node'):
> +        print(len(glob.glob('/sys/devices/system/node/node*')))
> +    else:
> +        subprocess.run(['sysctl', 'vm.ndomains'])
> +

I just realised that by default sysctl outputs the name of the element as
well as the value, which is not what we want. Using "-n" flag suppresses
the name, so please add that to the command. [Sorry for not spotting this
in previous versions].

> +elif os.name == 'nt':
> +    libkernel32 = ctypes.windll.kernel32
> +
> +    count = ctypes.c_ulong()
> +
> +    libkernel32.GetNumaHighestNodeNumber(ctypes.pointer(count))
> +    print(count.value + 1)
> diff --git a/buildtools/meson.build b/buildtools/meson.build
> index 04808dabc..925e733b1 100644
> --- a/buildtools/meson.build
> +++ b/buildtools/meson.build
> @@ -17,3 +17,5 @@ else
>  endif
>  map_to_win_cmd = py3 + files('map_to_win.py')
>  sphinx_wrapper = py3 + files('call-sphinx-build.py')
> +get_cpu_count_cmd = py3 + files('get_cpu_count.py')
> +get_numa_count_cmd = py3 + files('get_numa_count.py')
> diff --git a/config/meson.build b/config/meson.build
> index c7f7aa6e2..a51000ab2 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -231,8 +231,6 @@ foreach arg: warning_flags
>  endforeach
>  
>  # set other values pulled from the build options
> -dpdk_conf.set('RTE_MAX_LCORE', get_option('max_lcores'))
> -dpdk_conf.set('RTE_MAX_NUMA_NODES', get_option('max_numa_nodes'))
>  dpdk_conf.set('RTE_MAX_ETHPORTS', get_option('max_ethports'))
>  dpdk_conf.set('RTE_LIBEAL_USE_HPET', get_option('use_hpet'))
>  dpdk_conf.set('RTE_ENABLE_TRACE_FP', get_option('enable_trace_fp'))
> @@ -251,6 +249,52 @@ compile_time_cpuflags = []
>  subdir(arch_subdir)
>  dpdk_conf.set('RTE_COMPILE_TIME_CPUFLAGS', ','.join(compile_time_cpuflags))
>  
> +max_lcores = get_option('max_lcores')
> +max_numa_nodes = get_option('max_numa_nodes')
> +if max_lcores > 0
> +	# Overwrite the default value from arch_subdir with user input
> +	dpdk_conf.set('RTE_MAX_LCORE', max_lcores)
> +elif max_lcores == -1
> +	# Overwrite the default value with discovered values
> +	if not meson.is_cross_build()

I'd suggest changing this to the inverse and erroring out, since I assume
that having -1 for cross-compilation should be an error?

	if meson.is_cross_build()
		error('....')
	endif

Thereafter you save a level of indentation for the rest of the block.

> +		# Discovery makes sense only for non-cross builds
> +		max_lcores = run_command(get_cpu_count_cmd).stdout().to_int()
> +		min_lcores = 2
> +		# DPDK must be build for at least 2 cores

Even 2 seems rather low. I'd personally look to set the minimum at 4.

> +		if max_lcores < min_lcores
> +			message('Found less than @0@ cores, building for @0@ cores'.format(min_lcores))
> +			max_lcores = min_lcores
> +		else
> +			message('Found @0@ cores'.format(max_lcores))
> +		endif
> +		dpdk_conf.set('RTE_MAX_LCORE', max_lcores)
> +	endif
> +endif
> +
> +if max_numa_nodes > 0
> +	# Overwrite the default value from arch_subdir with user input
> +	dpdk_conf.set('RTE_MAX_NUMA_NODES', max_numa_nodes)
> +elif max_numa_nodes == -1
> +	# Overwrite the default value with discovered values
> +	if not meson.is_cross_build()
> +		# Discovery makes sense only for non-cross builds
> +		max_numa_nodes = run_command(get_numa_count_cmd).stdout().to_int()
> +		message('Found @0@ numa nodes'.format(max_numa_nodes))
> +		dpdk_conf.set('RTE_MAX_NUMA_NODES', max_numa_nodes)
> +	endif
> +endif
> +
> +# check that cpu and numa count is set in cross builds
> +# and error out if it's not set
> +if meson.is_cross_build()
> +	if not dpdk_conf.has('RTE_MAX_LCORE')
> +		error('Number of cores for cross build not specified in @0@ subdir (e.g. in a cross-file) nor on the cmdline'.format(arch_subdir))
> +	endif
> +	if not dpdk_conf.has('RTE_MAX_NUMA_NODES')
> +		error('Number of numa nodes for cross build not specified in @0@ subdir (e.g. in a cross-file) nor on the cmdline'.format(arch_subdir))
> +	endif
> +endif
> +

Not sure you need the is_cross_build bit, since if we don't have
RTE_MAX_LCORES set at all, then it's an error too. It just so happens that
the code paths to this point prevent that from ever happening.

Is the intention here that the subdir meson.build files will just literally
take the value from the cross-file, or are you doing other processing in
the ARM case, e.g. to compute it based on other info. If it's just the
former where it's always read from the cross-files, I'd have the reading
done in this file (above the other assignment work), since it would be
architecture agnostic.

>  # set the install path for the drivers
>  dpdk_conf.set_quoted('RTE_EAL_PMD_PATH', eal_pmd_path)
>  
<snip>