Re: [dpdk-dev] [RFC PATCH v1] build: add platform meson option

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Nov 27, 2020 at 08:31:47AM +0000, Juraj Linkeš wrote:
> 
> 
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Thursday, November 26, 2020 5:03 PM
> > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Cc: thomas@monjalon.net; Honnappa.Nagarahalli@arm.com; dev@dpdk.org
> > Subject: Re: [RFC PATCH v1] build: add platform meson option
> > 
> > On Thu, Nov 26, 2020 at 04:47:29PM +0100, Juraj Linkeš wrote:
> > > 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.
> > >
> > > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > > ---
> > >  config/arm/meson.build |  2 +-
> > >  config/meson.build     | 14 +++++++++++++-
> > >  meson_options.txt      |  6 ++++--
> > >  3 files changed, 18 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/config/arm/meson.build b/config/arm/meson.build index
> > > 42b4e43c7..ac680956f 100644
> > > --- a/config/arm/meson.build
> > > +++ b/config/arm/meson.build
> > > @@ -6,7 +6,7 @@
> > >  march_opt = '-march=@0@'.format(machine)
> > >
> > >  arm_force_native_march = false
> > > -arm_force_default_march = (machine == 'default')
> > > +arm_force_default_march = (platform == 'generic')
> > >
> > >  flags_common_default = [
> > >  	# Accelarate rte_memcpy. Be sure to run unit test
> > > (memcpy_perf_autotest) diff --git a/config/meson.build
> > > b/config/meson.build index c02802c18..41d32e63e 100644
> > > --- a/config/meson.build
> > > +++ b/config/meson.build
> > > @@ -63,6 +63,8 @@ if not is_windows
> > >  			pmd_subdir_opt)
> > >  endif
> > >
> > > +platform = get_option('platform')
> > > +
> > >  # set the machine type and cflags for it  if meson.is_cross_build()
> > >  	machine = host_machine.cpu()
> > > @@ -70,13 +72,23 @@ else
> > >  	machine = get_option('machine')
> > >  endif
> > >
> > > +if platform == 'native'
> > > +	if machine == 'auto'
> > > +		machine = 'native'
> > > +	endif
> > > +elif platform == 'generic'
> > > +	if machine == 'auto'
> > > +		machine = 'default'
> > > +	endif
> > > +endif
> > > +
> > > +if machine == 'default'
> > >  # machine 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'
> > > diff --git a/meson_options.txt b/meson_options.txt index
> > > e384e6dbb..1a5e47fd3 100644
> > > --- a/meson_options.txt
> > > +++ b/meson_options.txt
> > > @@ -20,14 +20,16 @@ option('kernel_dir', type: 'string', value: '',
> > >  	description: 'Path to the kernel for building kernel modules.
> > > Headers must be in $kernel_dir/build. Modules will be installed in
> > > $DEST_DIR/$kernel_dir/extra/dpdk.')
> > >  option('lib_musdk_dir', type: 'string', value: '',
> > >  	description: 'path to the MUSDK library installation directory')
> > > -option('machine', type: 'string', value: 'native',
> > > -	description: 'set the target machine type')
> > > +option('machine', type: 'string', value: 'auto',
> > > +	description: 'set the target machine type/ISA')
> > >  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('platform', type: 'string', value: 'generic',
> > > +	description: 'Platform to build for, either "native" or "generic".')
> > 
> > As well as this short description option, I think we need more comprehensive
> > coverage of this option in the docs.
> 
> Where should this be documented? In doc/guides/linux_gsg/build_dpdk.rst or somewhere else?
> 
I'll need to look into this, but I think that we need to expand that
section to cover in more detail all build options as we rework them.

> > Presumably for ARM systems this will have
> > other options for various SOC's rather than just generic/native?
> > 
> 
> Yes, I'm planning on using the platform option for this. Since we've separated the changes into their own patch sets, only this small change is in this patch set. It'll affect how we're doing the automatic numa/core detection as well as how we're choosic which Arm SoC to build for, but those changes are in different patch sets.
> 
> I mainly want feedback on naming - I like 'platform' and I don't think we need to rename 'machine', maybe just document it better - and on the overall idea. I think this is in line with what you had in mind, right?
> 
Yes this is in line with what I had in mind, thanks for doing the work.

I'm still of a mind to rename "machine", since it's a very ambiguous term.
I think something like "cpu_instruction_set" would be a lot clearer. With
choice of a good name, the amount of documentation needed around it is much
reduced.

We should probably get feedback from a number of people before making a
decision.

Regards,
/Bruce