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

[Bruce Richardson]

On Fri, Apr 09, 2021 at 02:10:08PM +0000, Juraj Linkeš wrote:
> 
> 
> > -----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.

Ok, I did not realise that. I need to look at how they are used in that
file.

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

Ok, didn't realise that the trailing "," will still be present. However, I
think a better fix for this, and the issue below with "." being printed in
the empty case is to add the following to buildtools/list-dir-globs.py:

@@ -14,6 +14,8 @@
                     os.getenv('MESON_SUBDIR', '.'))

 for path in sys.argv[1].split(','):
+    if not path:
+        continue
     for p in iglob(os.path.join(root, path)):
         if os.path.isdir(p):
             print(os.path.relpath(p))

With that added, we don't need to worry about null strings or and trailing
commas and can just do:
disabled_drivers += ',' + get_option('disable_drivers')

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

I think the above two-line change to the globs script should fix this.

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

It's got quite a complicated number of scenarios, I admit.
One thought, though not sure if it would work, is to always check
enabled_drivers and disabled_drivers. If no enable_drivers option in
cross-file or cmdline, we initialize the value to "list_dir_globs *.*",
i.e. everything enabled. Similarly, if no disable_driver options provided,
we use an empty list.

Thereafter in the main loop we just check for each driver that it is in the
enable list and not in the disabled one.

Does that seem like it would work?
 
/Bruce