From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 5E427A04DF;
	Wed, 21 Oct 2020 14:02:33 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id 00030ACEF;
	Wed, 21 Oct 2020 14:02:30 +0200 (CEST)
Received: from mga18.intel.com (mga18.intel.com [134.134.136.126])
 by dpdk.org (Postfix) with ESMTP id 7D4C4ACEE
 for <dev@dpdk.org>; Wed, 21 Oct 2020 14:02:28 +0200 (CEST)
IronPort-SDR: UC1r8HgkVQdJc7ZyLJvmFs7A/mT1yzNtIULXTrKhPKn+XX6gKgw7ZH5pxOBCoa6jYl/NSIsjtF
 zn6yH3kR+F5Q==
X-IronPort-AV: E=McAfee;i="6000,8403,9780"; a="155139737"
X-IronPort-AV: E=Sophos;i="5.77,401,1596524400"; d="scan'208";a="155139737"
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from orsmga005.jf.intel.com ([10.7.209.41])
 by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
 21 Oct 2020 05:02:26 -0700
IronPort-SDR: 0O6Vbm1ZWEHFRWzs8ly4itq1OCxVFhWgcecyYujhtAG3/6yLJ2N1mi63pGiQgLRspMJErJn8N7
 8XWIE/onVIBQ==
X-IronPort-AV: E=Sophos;i="5.77,401,1596524400"; d="scan'208";a="533499472"
Received: from bricha3-mobl.ger.corp.intel.com ([10.213.249.97])
 by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA;
 21 Oct 2020 05:02:24 -0700
Date: Wed, 21 Oct 2020 13:02:20 +0100
From: Bruce Richardson <bruce.richardson@intel.com>
To: Juraj =?utf-8?Q?Linke=C5=A1?= <juraj.linkes@pantheon.tech>
Cc: Ruifeng.Wang@arm.com, Honnappa.Nagarahalli@arm.com, Phil.Yang@arm.com,
 vcchunga@amazon.com, Dharmik.Thakkar@arm.com, jerinjacobk@gmail.com,
 hemant.agrawal@nxp.com, dev@dpdk.org
Message-ID: <20201021120220.GC592@bricha3-MOBL.ger.corp.intel.com>
References: <1602600882-695-1-git-send-email-juraj.linkes@pantheon.tech>
 <1603280261-20206-1-git-send-email-juraj.linkes@pantheon.tech>
 <1603280261-20206-4-git-send-email-juraj.linkes@pantheon.tech>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1603280261-20206-4-git-send-email-juraj.linkes@pantheon.tech>
Subject: Re: [dpdk-dev] [RFC PATCH v3 3/6] build: automatic NUMA and cpu
	counts detection
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On Wed, Oct 21, 2020 at 01:37:38PM +0200, Juraj Linkeš wrote:
> The build machine's number of cpus and numa nodes vary, resulting in
> mismatched counts of RTE_MAX_LCORE and RTE_MAX_NUMA_NODES for many
> builds. Automatically discover the host's numa and cpu counts to remove
> this mismatch for native builds. Use current defaults for default builds.
> Force the users to specify the counts for cross build in cross files or
> on the command line.
> Give users the option to override the discovery 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@pantheon.tech>
> ---
>  buildtools/get_cpu_count.py  |  7 ++++++
>  buildtools/get_numa_count.py | 22 +++++++++++++++++++
>  buildtools/meson.build       |  2 ++
>  config/meson.build           | 42 ++++++++++++++++++++++++++++++++++--
>  meson_options.txt            |  8 +++----
>  5 files changed, 75 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
> +# 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..f0c49973a
> --- /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:
> +        print(subprocess.run(['sysctl', 'vm.ndomains'], capture_output=True).stdout)
> +
> +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 a57c8ae9e..c4477f977 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -74,7 +74,11 @@ endif
>  # still being able to support the CPU features required for DPDK.
>  # This can be bumped up by the DPDK project, but it can never be an
>  # invariant like 'native'
> +max_lcores = get_option('max_lcores')
> +max_numa_nodes = get_option('max_numa_nodes')
>  if machine == 'default'
> +	max_numa_nodes = 4
> +	max_lcores = 128

This doesn't seem right, since you are overriding the user-specified values
with hard-coded ones.

>  	if host_machine.cpu_family().startswith('x86')
>  		# matches the old pre-meson build systems default
>  		machine = 'corei7'
> @@ -83,6 +87,22 @@ if machine == 'default'
>  	elif host_machine.cpu_family().startswith('ppc')
>  		machine = 'power8'
>  	endif
> +elif not meson.is_cross_build()
> +	# find host core count and numa node count for native builds
> +	if max_lcores == 0
> +		max_lcores = run_command(get_cpu_count_cmd).stdout().to_int()
> +		min_lcores = 2
> +		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
> +	endif
> +	if max_numa_nodes == 0
> +		max_numa_nodes = run_command(get_numa_count_cmd).stdout().to_int()
> +		message('Found @0@ numa nodes'.format(max_numa_nodes))
> +	endif
>  endif
>  
>  dpdk_conf.set('RTE_MACHINE', machine)
> @@ -227,8 +247,10 @@ 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'))
> +if not meson.is_cross_build()
> +	dpdk_conf.set('RTE_MAX_LCORE', max_lcores)
> +	dpdk_conf.set('RTE_MAX_NUMA_NODES', max_numa_nodes)
> +endif

Rather than conditionally setting the value here, you should move the
checks below up above this to simplify things.

>  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'))
> @@ -247,6 +269,22 @@ compile_time_cpuflags = []
>  subdir(arch_subdir)
>  dpdk_conf.set('RTE_COMPILE_TIME_CPUFLAGS', ','.join(compile_time_cpuflags))
>  
> +# check that cpu and numa count is set in cross builds
> +if meson.is_cross_build()
> +    	if max_lcores > 0
> +		# specified on the cmdline
> +		dpdk_conf.set('RTE_MAX_LCORE', max_lcores)
> +	elif 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 max_numa_nodes > 0
> +		# specified on the cmdline
> +		dpdk_conf.set('RTE_MAX_NUMA_NODES', max_numa_nodes)
> +	elif 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
> +
>  # set the install path for the drivers
>  dpdk_conf.set_quoted('RTE_EAL_PMD_PATH', eal_pmd_path)
>  
> diff --git a/meson_options.txt b/meson_options.txt
> index 9bf18ab6b..01b0c45c3 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -26,10 +26,10 @@ option('machine', type: 'string', value: 'native',
>  	description: 'set the target machine type')
>  option('max_ethports', type: 'integer', value: 32,
>  	description: 'maximum number of Ethernet devices')
> -option('max_lcores', type: 'integer', value: 128,
> -	description: 'maximum number of cores/threads supported by EAL')
> -option('max_numa_nodes', type: 'integer', value: 4,
> -	description: 'maximum number of NUMA nodes supported by EAL')
> +option('max_lcores', type: 'integer', value: 0,
> +	description: 'maximum number of cores/threads supported by EAL. Value 0 means the number of cpus on the host will be used. For cross build, set to non-zero to overwrite the cross-file value.')
> +option('max_numa_nodes', type: 'integer', value: 0,
> +	description: 'maximum number of NUMA nodes supported by EAL. Value 0 means the number of numa nodes on the host will be used. For cross build, set to non-zero to overwrite the cross-file value.')

I don't like this change, because it very much assumes for
non-cross-compiles that people will be running DPDK on the system they
build it on. That's a very, very big assumption! I'm ok with having zero as
a "detect" option, and having the values overridden from cross-files, but
not with detection as the default out-of-the-box option! Lots of users may
pull builds from a CI based on VMs with just a few cores, for instance.

>  option('enable_trace_fp', type: 'boolean', value: false,
>  	description: 'enable fast path trace points.')
>  option('tests', type: 'boolean', value: true,
> -- 
> 2.20.1
>