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

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Friday, November 27, 2020 3:07 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 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.
> 

Any news on this? If we want to document this in just doc/guides/linux_gsg/build_dpdk.rst I can propose some changes.

> > > 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.
> 

I wanted to keep machine mainly for backwards compatibilty, but we can do that anyway, as in keep machine for a release a two with a deprecation notice and then remove it while using a differently named (but same in function) option.

What's were doing with the option is setting the appropriate "machine args", as the variable  is called. For x86, that means setting -march (which is just the instruction set according to the docs [0]), for ppc it sets -mcpu and -mtune (architecture type, register usage, and instruction scheduling parameters according to [1]) and we're not using it to set machine args for arm at all (we are setting machine args based on other criteria; when this new option along with platform are in effect, we can think about using cpu_instruction_set somehow).

With this in mind, cpu_instruction_set seems like the best candidate. I'll submit a new version with both cpu_instruction_set and machine if there aren't any objections in some time.

[0] https://gcc.gnu.org/onlinedocs/gcc-8.4.0/gcc/x86-Options.html#x86-Options
[1] https://gcc.gnu.org/onlinedocs/gcc-8.4.0/gcc/RS_002f6000-and-PowerPC-Options.html#RS_002f6000-and-PowerPC-Options

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

Unfortunately we didn't get any feedback. Do you have anyone in particular in mind?

> Regards,
> /Bruce