[dpdk-dev] [PATCH v19 1/3] build: disable/enable drivers in Arm builds

Juraj Linkeš juraj.linkes at pantheon.tech
Fri Apr 9 16:10:08 CEST 2021



> -----Original Message-----
> From: Bruce Richardson <bruce.richardson at intel.com>
> Sent: Friday, April 9, 2021 12:03 PM
> To: Juraj Linkeš <juraj.linkes at pantheon.tech>
> Cc: Ruifeng.Wang at arm.com; Honnappa.Nagarahalli at arm.com;
> Phil.Yang at arm.com; vcchunga at amazon.com; Dharmik.Thakkar at arm.com;
> jerinjacobk at gmail.com; hemant.agrawal at nxp.com;
> ajit.khaparde at broadcom.com; ferruh.yigit at intel.com; aboyer at pensando.io;
> dev at dpdk.org
> Subject: Re: [PATCH v19 1/3] build: disable/enable drivers in Arm builds
> 
> On Fri, Apr 09, 2021 at 10:41:17AM +0200, Juraj Linkeš wrote:
> > Add support for enabling or disabling drivers for Arm cross build. Do
> > not implement any enable/disable lists yet.
> >
> > Enabling drivers is useful when building for an SoC where we only want
> > to build a few drivers. That way the list won't be too long.
> >
> > Similarly, disabling drivers is useful when we want to disable only a
> > few drivers.
> >
> > Both of these are advantageous mainly in aarch64 -> aarch64 (or arch
> > -> same arch) builds, where the build machine may have the required
> > driver dependencies, yet we don't want to build drivers for a specific SoC.
> >
> > By default, build all drivers for which dependencies are found. If
> > enabled_drivers is a non-empty list, build only those drivers.  If
> > disabled_drivers is non-empty list, build all drivers except those in
> > disabled_drivers. Error out if both are specified (i.e. do not support
> > that case).
> >
> > There are two drivers, bus/pci and bus/vdev, which break the build if
> > not enabled. Address this by always enabling these if the user
> > disables them or doesn't specify in their allowlist.
> >
> > Also remove the old Makefile arm configuration options which don't do
> > anything in Meson.
> >
> > Signed-off-by: Juraj Linkeš <juraj.linkes at pantheon.tech>
> > Acked-by: Bruce Richardson <bruce.richardson at intel.com>
> 
> I think this patch has changed since I last acked it. Further review below.
> 
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli at arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang at arm.com>
> > ---
> >  config/arm/meson.build                        |  4 --
> >  .../linux_gsg/cross_build_dpdk_for_arm64.rst  |  8 +++
> >  drivers/meson.build                           | 49 +++++++++++++++++--
> >  meson.build                                   |  2 +
> >  meson_options.txt                             |  2 +
> >  5 files changed, 56 insertions(+), 9 deletions(-)
> >
> > diff --git a/config/arm/meson.build b/config/arm/meson.build index
> > 00bc4610a3..a241c45d13 100644
> > --- a/config/arm/meson.build
> > +++ b/config/arm/meson.build
> > @@ -16,9 +16,6 @@ flags_common = [
> >  	#	['RTE_ARM64_MEMCPY_ALIGN_MASK', 0xF],
> >  	#	['RTE_ARM64_MEMCPY_STRICT_ALIGN', false],
> >
> > -	['RTE_NET_FM10K', false],
> > -	['RTE_NET_AVP', false],
> > -
> >  	['RTE_SCHED_VECTOR', false],
> >  	['RTE_ARM_USE_WFE', false],
> >  	['RTE_ARCH_ARM64', true],
> > @@ -125,7 +122,6 @@ implementer_cavium = {
> >  				['RTE_MACHINE', '"octeontx2"'],
> >  				['RTE_ARM_FEATURE_ATOMICS', true],
> >  				['RTE_USE_C11_MEM_MODEL', true],
> > -				['RTE_EAL_IGB_UIO', false],
> >  				['RTE_MAX_LCORE', 36],
> >  				['RTE_MAX_NUMA_NODES', 1]
> >  			]
> > diff --git a/doc/guides/linux_gsg/cross_build_dpdk_for_arm64.rst
> > b/doc/guides/linux_gsg/cross_build_dpdk_for_arm64.rst
> > index faaf24b95b..1504dbfef0 100644
> > --- a/doc/guides/linux_gsg/cross_build_dpdk_for_arm64.rst
> > +++ b/doc/guides/linux_gsg/cross_build_dpdk_for_arm64.rst
> > @@ -234,3 +234,11 @@ There are other options you may specify in a cross
> file to tailor the build::
> >        numa = false        # set to false to force building for a non-NUMA system
> >           # if not set or set to true, the build system will build for a NUMA
> >           # system only if libnuma is installed
> > +
> > +      disabled_drivers = ['bus/dpaa', 'crypto/*']  # add disabled drivers
> > +         # valid values are dir/subdirs in the drivers directory
> > +         # wildcards are allowed
> > +
> > +      enabled_drivers = ['common/*', 'bus/*']  # build only these drivers
> > +         # valid values are dir/subdirs in the drivers directory
> > +         # wildcards are allowed
> > diff --git a/drivers/meson.build b/drivers/meson.build index
> > 9c8eded697..be5fd2df88 100644
> > --- a/drivers/meson.build
> > +++ b/drivers/meson.build
> > @@ -19,8 +19,39 @@ subdirs = [
> >  	'baseband', # depends on common and bus.
> >  ]
> >
> > -disabled_drivers = run_command(list_dir_globs, get_option('disable_drivers'),
> > -		).stdout().split()
> > +always_enabled = ['bus/pci', 'bus/vdev']
> > +
> > +if meson.is_cross_build()
> > +	disabled_drivers += meson.get_cross_property('disabled_drivers', [])
> > +	enabled_drivers += meson.get_cross_property('enabled_drivers', [])
> > +endif
> 
> I don't think "+=" is correct here. This is the first use of the disabled_drivers
> variable. [Sorry, correction, I see you've added a new definition of it in the top-
> level meson.build. I think it's better to move that to this file]
> 

This need not be true. It's added in config/arm/meson.build in the subsequent patch, which comes before drivers/meson.build, which is why I structured it this way - I don't think there's a reason to move the initialization around in the same series, but I could do that.

> Also, would it not be better to have the cross-file option the same as that used
> in the parameter option? Right now you have the cross-file option as
> "disabled_drivers" vs cmdline option "disable_drivers" and the types being list
> and string respectively too. Why not have the cross-file option be a string called
> "disable_drivers" too? It would save some joining an array/string conversion
> below and simplify things.
> 

The name can be the same. The reason I have two different types is that it struck me as more user friendly - specifying something in code is more intuitive as list as opposed to comma-delimited string, whereas it's more intuitive as comma-delimited string on cmdline. It's possible that having a comma-delimited string everywhere is actually better anyway - I'll change it.

> > +
> > +# add cmdline disabled drivers (comma separated string) # and meson
> > +disabled drivers (list) # together into a comma separated string
> > +disabled_drivers = ','.join([get_option('disable_drivers'),
> > +','.join(disabled_drivers)]).strip(',')
> 
> Do we need the string at the end here? I would think that join never adds a
> trailing comma? As stated above if these were both strings it might make things
> shorter and easier.
> 

If we're joining two empty strings (which happens when neither the cmdline nor the code list is specified), then we'll end up with a singular comma instead of an empty string.

Even then both of these are strings (which I'll change), we'll still need the strip, as the above scenario would still happen.

> > +if disabled_drivers != ''
> > +	disabled_drivers = run_command(list_dir_globs,
> > +		disabled_drivers).stdout().split()
> > +else
> > +	disabled_drivers = []
> > +endif
> 
> Don't think we need the "if/else" here. The existing code works fine with taking
> an empty array.
> 

An empty string results in ['.'], not in []. I ran into problems with allowlists when ['.'] is returned - I'm assuming that the allowlist is either empty or whathever is in the allowlist means something and '.' is meaningless since it's not a driver. This was the most straightforward solution I found. For disabled drivers we don't need this, but I did the same thing for consistency.

> > +
> > +# add cmdline enabled drivers (comma separated string) # and meson
> > +enabled drivers (list) # together into a comma separated string
> > +enabled_drivers = ','.join([get_option('enable_drivers'),
> > +','.join(enabled_drivers)]).strip(',')
> > +if enabled_drivers != ''
> > +	enabled_drivers = run_command(list_dir_globs,
> > +		enabled_drivers).stdout().split()
> > +else
> > +	enabled_drivers = []
> > +endif
> > +
> > +if disabled_drivers != [] and enabled_drivers != []
> > +	error('Simultaneous disabled drivers and enabled drivers ' +
> > +	      'configuration is not supported.') endif
> 
> For use in cross-files this makes sense, but I'm not sure it's the correct approach
> for when a cross-file specifies enable and the user specifies disable on the
> cmdline. In that case, the enable list should just have the disabled values
> removed from it. Therefore, I suggest having this check only in the cross-build
> section.
> 

Do you want to distinguish between enabling/disabling driver when cross compiling and when doing a regular build? From the previous discussion, I thought we didn't want to mix enable/disable lists no matter what the build or source is. The different scenarios in my mind are combinations of:
1. cross/regular build
2. no list, just enable list, just disable list, both lists
3. where a list is specified - cmdline or meson code (soc config or cross file)

These give us quite a number of different scenarios. When do we want to allow the mixing of enable/disable lists and when not? It's not clear to me from your description, but it seems that specifying a list on the cmdline should overwrite or supplement either an enable or disable list specified in code - we would allow just one of these in code and then augment that with cmdline.

> >
> >  default_cflags = machine_args
> >  default_cflags += ['-DALLOW_EXPERIMENTAL_API'] @@ -49,7 +80,7 @@
> > foreach subpath:subdirs
> >  		dpdk_driver_classes += class
> >  	endif
> >  	# get already enabled drivers of the same class
> > -	enabled_drivers = get_variable(class + '_drivers', [])
> > +	enabled_class_drivers = get_variable(class + '_drivers', [])
> >
> >  	foreach drv:drivers
> >  		drv_path = join_paths(class, drv)
> > @@ -77,11 +108,19 @@ foreach subpath:subdirs
> >  		if disabled_drivers.contains(drv_path)
> >  			build = false
> >  			reason = 'explicitly disabled via build config'
> > +		elif enabled_drivers.length() > 0 and not
> enabled_drivers.contains(drv_path)
> > +			build = false
> > +			reason = 'not in enabled drivers build config'
> >  		else
> >  			# pull in driver directory which should update all the
> local variables
> >  			subdir(drv_path)
> >  		endif
> >
> > +		if not build and always_enabled.contains(drv_path)
> > +			build = true
> > +			message('Driver @0@ cannot be disabled,
> enabling.'.format(drv_path))
> > +		endif
> > +
> >  		if build
> >  			# get dependency objs from strings
> >  			shared_deps = ext_deps
> > @@ -109,7 +148,7 @@ foreach subpath:subdirs
> >  						'_disable_reason', reason)
> >  			endif
> >  		else
> > -			enabled_drivers += name
> > +			enabled_class_drivers += name
> >  			lib_name = '_'.join(['rte', class, name])
> >  			dpdk_conf.set(lib_name.to_upper(), 1)
> >
> > @@ -214,5 +253,5 @@ foreach subpath:subdirs
> >  		endif # build
> >  	endforeach
> >
> > -	set_variable(class + '_drivers', enabled_drivers)
> > +	set_variable(class + '_drivers', enabled_class_drivers)
> >  endforeach
> > diff --git a/meson.build b/meson.build index 7778e18200..38a2bd5416
> > 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -22,6 +22,8 @@ dpdk_drivers = []
> >  dpdk_extra_ldflags = []
> >  dpdk_libs_disabled = []
> >  dpdk_drvs_disabled = []
> > +disabled_drivers = []
> > +enabled_drivers = []
> 
> These variables don't need to be declared here. They are only used in
> drivers/meson.build and nowhere else.
> 

As mentioned above, this is in anticipation of the subsequent patch. The other place that makes sense is in config/meson.build. I could put it into drivers/meson.build in this commit and then move it to either the top-level meson.build or to config/meson.build. What do you think?

> >  abi_version_file = files('ABI_VERSION')
> >
> >  if host_machine.cpu_family().startswith('x86')
> > diff --git a/meson_options.txt b/meson_options.txt index
> > 86bc6c88f6..d2d24a1424 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -8,6 +8,8 @@ option('drivers_install_subdir', type: 'string', value:
> 'dpdk/pmds-<VERSION>',
> >  	description: 'Subdirectory of libdir where to install PMDs. Defaults
> > to using a versioned subdirectory.')  option('enable_docs', type: 'boolean',
> value: false,
> >  	description: 'build documentation')
> > +option('enable_drivers', type: 'string', value: '',
> > +	description: 'Comma-separated list of drivers to build. If
> > +unspecified, build all drivers.')
> >  option('enable_driver_sdk', type: 'boolean', value: false,
> >  	description: 'Install headers to build drivers.')
> > option('enable_kmods', type: 'boolean', value: false,
> > --
> > 2.20.1
> >



More information about the dev mailing list