diff mbox series

[v4] build: add platform meson option

Message ID 1617022234-13618-1-git-send-email-juraj.linkes@pantheon.tech (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series [v4] build: add platform meson option | expand

Checks

Context Check Description
ci/intel-Testing success Testing PASS
ci/Intel-compilation fail Compilation issues
ci/iol-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/github-robot success github build: passed
ci/travis-robot success travis build: passed
ci/checkpatch success coding style OK

Commit Message

Juraj Linkeš March 29, 2021, 12:50 p.m. UTC
The current meson option 'machine' should only specify the ISA, which is
not sufficient for Arm, where setting ISA implies other setting as well.
Add a new meson option, 'platform', which differentiates the type of the
build (native/generic) and sets machine accordingly, unless the user
chooses to override it.
The 'machine' option also doesn't describe very well what it sets, so
introduce a new option 'cpu_instruction_set', but keep 'machine' for
backwards compatibility.
These two new variables, taken together, achieve what 'machine' was
setting per architecture - setting the ISA in x86 build and setting
native/default 'build type' in aarch64 build - is now properly being set
for all architectures in a uniform manner.

Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
 config/arm/meson.build        |  6 ++---
 config/meson.build            | 47 +++++++++++++++++++++++++----------
 devtools/test-meson-builds.sh |  9 ++++---
 meson_options.txt             |  8 ++++--
 4 files changed, 48 insertions(+), 22 deletions(-)

Comments

Juraj Linkeš March 31, 2021, 12:16 p.m. UTC | #1
Bruce, what do you think of the patch now? Do we need to add/change anything else, like documentation?

One thing to note is that we're changing the default behavior in this patch from machine=native to machine=generic (or more accurately, to cpu_instruction_set=generic). Do we want to do that?

> -----Original Message-----
> From: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Sent: Monday, March 29, 2021 2:51 PM
> To: thomas@monjalon.net; david.marchand@redhat.com;
> bruce.richardson@intel.com; Honnappa.Nagarahalli@arm.com
> Cc: dev@dpdk.org; Juraj Linkeš <juraj.linkes@pantheon.tech>
> Subject: [PATCH v4] build: add platform meson option
> 
> The current meson option 'machine' should only specify the ISA, which is not
> sufficient for Arm, where setting ISA implies other setting as well.
> Add a new meson option, 'platform', which differentiates the type of the build
> (native/generic) and sets machine accordingly, unless the user chooses to
> override it.
> The 'machine' option also doesn't describe very well what it sets, so introduce a
> new option 'cpu_instruction_set', but keep 'machine' for backwards
> compatibility.
> These two new variables, taken together, achieve what 'machine' was setting
> per architecture - setting the ISA in x86 build and setting native/default 'build
> type' in aarch64 build - is now properly being set for all architectures in a
> uniform manner.
> 
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
>  config/arm/meson.build        |  6 ++---
>  config/meson.build            | 47 +++++++++++++++++++++++++----------
>  devtools/test-meson-builds.sh |  9 ++++---
>  meson_options.txt             |  8 ++++--
>  4 files changed, 48 insertions(+), 22 deletions(-)
>
Juraj Linkeš March 31, 2021, 12:19 p.m. UTC | #2
> -----Original Message-----
> From: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Sent: Wednesday, March 31, 2021 2:17 PM
> To: Juraj Linkeš <juraj.linkes@pantheon.tech>; thomas@monjalon.net;
> david.marchand@redhat.com; bruce.richardson@intel.com;
> Honnappa.Nagarahalli@arm.com
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v4] build: add platform meson option
> 
> Bruce, what do you think of the patch now? Do we need to add/change anything
> else, like documentation?
> 
> One thing to note is that we're changing the default behavior in this patch from
> machine=native to machine=generic (or more accurately, to
> cpu_instruction_set=generic). Do we want to do that?
> 

This change in behavior is likely behind the CI failure: http://patches.dpdk.org/project/dpdk/patch/1617022234-13618-1-git-send-email-juraj.linkes@pantheon.tech/

The generic build should work everywhere, which hints at something we may need to look at. 

> > -----Original Message-----
> > From: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Sent: Monday, March 29, 2021 2:51 PM
> > To: thomas@monjalon.net; david.marchand@redhat.com;
> > bruce.richardson@intel.com; Honnappa.Nagarahalli@arm.com
> > Cc: dev@dpdk.org; Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Subject: [PATCH v4] build: add platform meson option
> >
> > The current meson option 'machine' should only specify the ISA, which
> > is not sufficient for Arm, where setting ISA implies other setting as well.
> > Add a new meson option, 'platform', which differentiates the type of
> > the build
> > (native/generic) and sets machine accordingly, unless the user chooses
> > to override it.
> > The 'machine' option also doesn't describe very well what it sets, so
> > introduce a new option 'cpu_instruction_set', but keep 'machine' for
> > backwards compatibility.
> > These two new variables, taken together, achieve what 'machine' was
> > setting per architecture - setting the ISA in x86 build and setting
> > native/default 'build type' in aarch64 build - is now properly being
> > set for all architectures in a uniform manner.
> >
> > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > ---
> >  config/arm/meson.build        |  6 ++---
> >  config/meson.build            | 47 +++++++++++++++++++++++++----------
> >  devtools/test-meson-builds.sh |  9 ++++---
> >  meson_options.txt             |  8 ++++--
> >  4 files changed, 48 insertions(+), 22 deletions(-)
> >
Bruce Richardson March 31, 2021, 12:39 p.m. UTC | #3
On Wed, Mar 31, 2021 at 12:16:59PM +0000, Juraj Linkeš wrote:
> Bruce, what do you think of the patch now? Do we need to add/change anything else, like documentation?
> 
> One thing to note is that we're changing the default behavior in this patch from machine=native to machine=generic (or more accurately, to cpu_instruction_set=generic). Do we want to do that?
> 
The patch in general looks ok, but I am uncertain about this change indeed.
Especially since the -march flag we mirror to the pkg-config file. I'd like
to see something like [1] included along with such a change. It allows us
per-arch to select the flags to send to the pkg-config file, rather than
just always using machine_args blindly.

Feedback appreciated.

/Bruce

[1] http://patches.dpdk.org/project/dpdk/patch/20201211155111.145279-1-bruce.richardson@intel.com/
Juraj Linkeš April 15, 2021, 1:32 p.m. UTC | #4
> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Wednesday, March 31, 2021 2:39 PM
> To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Cc: thomas@monjalon.net; david.marchand@redhat.com;
> Honnappa.Nagarahalli@arm.com; dev@dpdk.org
> Subject: Re: [PATCH v4] build: add platform meson option
> 
> On Wed, Mar 31, 2021 at 12:16:59PM +0000, Juraj Linkeš wrote:
> > Bruce, what do you think of the patch now? Do we need to add/change
> anything else, like documentation?
> >
> > One thing to note is that we're changing the default behavior in this patch
> from machine=native to machine=generic (or more accurately, to
> cpu_instruction_set=generic). Do we want to do that?
> >
> The patch in general looks ok, but I am uncertain about this change indeed.
> Especially since the -march flag we mirror to the pkg-config file. I'd like to see
> something like [1] included along with such a change. It allows us per-arch to
> select the flags to send to the pkg-config file, rather than just always using
> machine_args blindly.
> 
> Feedback appreciated.
> 

I'm not sure what feedback do you want here. I'm not that familiar with pkg-config files. I think this is about DPDK being built a set of instructions and an app using DPDK being able to be built another set of instructions and the pkg-config file bridges these? Could you expand a bit? I read the other commit msg (in [1]) as well and I don't know how it's related to this patch.

> /Bruce
> 
> [1] http://patches.dpdk.org/project/dpdk/patch/20201211155111.145279-1-
> bruce.richardson@intel.com/
Bruce Richardson April 15, 2021, 1:51 p.m. UTC | #5
On Thu, Apr 15, 2021 at 01:32:08PM +0000, Juraj Linkeš wrote:
> 
> 
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Wednesday, March 31, 2021 2:39 PM
> > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Cc: thomas@monjalon.net; david.marchand@redhat.com;
> > Honnappa.Nagarahalli@arm.com; dev@dpdk.org
> > Subject: Re: [PATCH v4] build: add platform meson option
> > 
> > On Wed, Mar 31, 2021 at 12:16:59PM +0000, Juraj Linkeš wrote:
> > > Bruce, what do you think of the patch now? Do we need to add/change
> > anything else, like documentation?
> > >
> > > One thing to note is that we're changing the default behavior in this patch
> > from machine=native to machine=generic (or more accurately, to
> > cpu_instruction_set=generic). Do we want to do that?
> > >
> > The patch in general looks ok, but I am uncertain about this change indeed.
> > Especially since the -march flag we mirror to the pkg-config file. I'd like to see
> > something like [1] included along with such a change. It allows us per-arch to
> > select the flags to send to the pkg-config file, rather than just always using
> > machine_args blindly.
> > 
> > Feedback appreciated.
> > 
> 
> I'm not sure what feedback do you want here. I'm not that familiar with pkg-config files. I think this is about DPDK being built a set of instructions and an app using DPDK being able to be built another set of instructions and the pkg-config file bridges these? Could you expand a bit? I read the other commit msg (in [1]) as well and I don't know how it's related to this patch.
> 
The main thing I suppose I'm concerned about is that I'd like more input
from people on the change from "native" to "generic" build by default.

On top of that, the patch I linked to adjusts things a little further
allowing separation of the ISA level used for building DPDK and for
building the app using DPDK. For example, for x86, it would probably be
more respectful for applications to have "-msse4" in the pkg-config file,
rather than -march=native or -march=corei7. The fact that DPDK was compiled
for a particular micro-arch should not force the user to use the same
micro-arch. Instead, each architecture should be able to output a separate
value that is suitable - in x86 case, turning on SSE4.2 as the minimum
needed ISA level.

However, I'm prepared to accept that discussion of the latter change can be
dealt with separately from this patch. So long as others have no objection
to change in default machine flags, I'm ok with it too.

/Bruce
diff mbox series

Patch

diff --git a/config/arm/meson.build b/config/arm/meson.build
index 00bc4610a3..e3a23c4228 100644
--- a/config/arm/meson.build
+++ b/config/arm/meson.build
@@ -208,7 +208,7 @@  if dpdk_conf.get('RTE_ARCH_32')
 else
 	# aarch64 build
 	if not meson.is_cross_build()
-		if machine == 'default'
+		if platform == 'generic'
 			# default build
 			implementer_id = 'generic'
 			part_number = 'generic'
@@ -238,7 +238,7 @@  else
 	else
 		error('Unsupported Arm implementer: @0@. '.format(implementer_id) +
 		      'Please add support for it or use the generic ' +
-		      '(-Dmachine=generic) build.')
+		      '(-Dplatform=generic) build.')
 	endif
 
 	message('Arm implementer: ' + implementer_config['description'])
@@ -253,7 +253,7 @@  else
 		error('Unsupported part number @0@ of implementer @1@. '
 		      .format(part_number, implementer_id) +
 		      'Please add support for it or use the generic ' +
-		      '(-Dmachine=generic) build.')
+		      '(-Dplatform=generic) build.')
 	endif
 
 	# use default flags with implementer flags
diff --git a/config/meson.build b/config/meson.build
index 66a2edcc47..32c9e18c17 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -63,42 +63,63 @@  if not is_windows
 			pmd_subdir_opt)
 endif
 
-# set the machine type and cflags for it
+platform = get_option('platform')
+
+# set the cpu_instruction_set type and cflags for it
 if meson.is_cross_build()
-	machine = host_machine.cpu()
+	cpu_instruction_set = host_machine.cpu()
 else
-	machine = get_option('machine')
+	cpu_instruction_set = get_option('cpu_instruction_set')
+	if get_option('machine') != 'auto'
+		warning('The "machine" option is deprecated. ' +
+		        'Please use "cpu_instruction_set" instead.')
+		if cpu_instruction_set != 'auto'
+			error('Setting both "machine" and ' +
+			      '"cpu_instruction_set" is unsupported.')
+		endif
+		cpu_instruction_set = get_option('machine')
+	endif
+endif
+
+if platform == 'native'
+	if cpu_instruction_set == 'auto'
+		cpu_instruction_set = 'native'
+	endif
+elif platform == 'generic'
+	if cpu_instruction_set == 'auto'
+		cpu_instruction_set = 'default'
+	endif
 endif
 
-# machine type 'default' is special, it defaults to the per arch agreed common
+if cpu_instruction_set == 'default'
+# cpu_instruction_set type 'default' is special, it defaults to the per arch agreed common
 # minimal baseline needed for DPDK.
 # That might not be the most optimized, but the most portable version while
 # 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'
-if machine == 'default'
 	if host_machine.cpu_family().startswith('x86')
 		# matches the old pre-meson build systems default
-		machine = 'corei7'
+		cpu_instruction_set = 'corei7'
 	elif host_machine.cpu_family().startswith('arm')
-		machine = 'armv7-a'
+		cpu_instruction_set = 'armv7-a'
 	elif host_machine.cpu_family().startswith('aarch')
 		# arm64 manages defaults in config/arm/meson.build
-		machine = 'default'
+		cpu_instruction_set = 'default'
 	elif host_machine.cpu_family().startswith('ppc')
-		machine = 'power8'
+		cpu_instruction_set = 'power8'
 	endif
 endif
 
-dpdk_conf.set('RTE_MACHINE', machine)
+dpdk_conf.set('RTE_MACHINE', cpu_instruction_set)
 machine_args = []
 
 # ppc64 does not support -march= at all, use -mcpu and -mtune for that
 if host_machine.cpu_family().startswith('ppc')
-	machine_args += '-mcpu=' + machine
-	machine_args += '-mtune=' + machine
+	machine_args += '-mcpu=' + cpu_instruction_set
+	machine_args += '-mtune=' + cpu_instruction_set
 else
-	machine_args += '-march=' + machine
+	machine_args += '-march=' + cpu_instruction_set
 endif
 
 toolchain = cc.get_id()
diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
index c11ae87e0d..b413b9e96a 100755
--- a/devtools/test-meson-builds.sh
+++ b/devtools/test-meson-builds.sh
@@ -223,12 +223,13 @@  done
 # test compilation with minimal x86 instruction set
 # Set the install path for libraries to "lib" explicitly to prevent problems
 # with pkg-config prefixes if installed in "lib/x86_64-linux-gnu" later.
-default_machine='nehalem'
-if ! check_cc_flags "-march=$default_machine" ; then
-	default_machine='corei7'
+default_isa='nehalem'
+if ! check_cc_flags "-march=$default_isa" ; then
+	default_isa='corei7'
 fi
 build build-x86-default cc skipABI -Dcheck_includes=true \
-	-Dlibdir=lib -Dmachine=$default_machine $use_shared
+	-Dlibdir=lib -Dcpu_instruction_set=$default_isa \
+	$use_shared
 
 # 32-bit with default compiler
 if check_cc_flags '-m32' ; then
diff --git a/meson_options.txt b/meson_options.txt
index 3b8c5d316d..c6047f3405 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -2,6 +2,8 @@ 
 
 option('check_includes', type: 'boolean', value: false,
 	description: 'build "chkincs" to verify each header file can compile alone')
+option('cpu_instruction_set', type: 'string', value: 'auto',
+	description: 'Set the target machine ISA (instruction set architecture). Will be set according to the platform option by default.')
 option('disable_drivers', type: 'string', value: '',
 	description: 'Comma-separated list of drivers to explicitly disable.')
 option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VERSION>',
@@ -20,14 +22,16 @@  option('include_subdir_arch', type: 'string', value: '',
 	description: 'subdirectory where to install arch-dependent headers')
 option('kernel_dir', type: 'string', value: '',
 	description: 'Path to the kernel for building kernel modules. Headers must be in $kernel_dir or $kernel_dir/build. Modules will be installed in /lib/modules.')
-option('machine', type: 'string', value: 'native',
-	description: 'set the target machine type')
+option('machine', type: 'string', value: 'auto',
+	description: 'Set the target machine ISA (instruction set architecture).')
 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: 32,
 	description: 'maximum number of NUMA nodes supported by EAL')
+option('platform', type: 'string', value: 'generic',
+	description: 'Platform to build, either "native" or "generic".')
 option('enable_trace_fp', type: 'boolean', value: false,
 	description: 'enable fast path trace points.')
 option('tests', type: 'boolean', value: true,