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

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

On Wed, Apr 14, 2021 at 09:02:16AM +0000, Juraj Linkeš wrote:
> 
> 
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Friday, April 9, 2021 4:32 PM
> > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Cc: Ruifeng.Wang@arm.com; Honnappa.Nagarahalli@arm.com;
> > Phil.Yang@arm.com; vcchunga@amazon.com; Dharmik.Thakkar@arm.com;
> > jerinjacobk@gmail.com; hemant.agrawal@nxp.com;
> > ajit.khaparde@broadcom.com; ferruh.yigit@intel.com; aboyer@pensando.io;
> > dev@dpdk.org
> > Subject: Re: [PATCH v19 1/3] build: disable/enable drivers in Arm builds
> > 
> > On Fri, Apr 09, 2021 at 02:10:08PM +0000, Juraj Linkeš wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Bruce Richardson <bruce.richardson@intel.com>
> > > > Sent: Friday, April 9, 2021 12:03 PM
> > > > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > > > Cc: Ruifeng.Wang@arm.com; Honnappa.Nagarahalli@arm.com;
> > > > Phil.Yang@arm.com; vcchunga@amazon.com; Dharmik.Thakkar@arm.com;
> > > > jerinjacobk@gmail.com; hemant.agrawal@nxp.com;
> > > > ajit.khaparde@broadcom.com; ferruh.yigit@intel.com;
> > > > aboyer@pensando.io; dev@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@pantheon.tech>
> > > > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > >
> > > > I think this patch has changed since I last acked it. Further review below.
> > > >
> > > > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@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')
> > 
> 
> Great, this is the sort of review I was hoping to get. This is much more elegant.
> 
> > >
> > > > > +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.
> > 
> 
> Right, it should.
> 
> > > > > +
> > > > > +# 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?
> 
> I think it would work and it would mean that we'd allow using both enable lists and disable lists for all scenarios. Is that intentional?
> 

I think it's necessary to have sane behaviour myself. However, we can still
limit the use to not both together if it causes problems. My main concern
is how to manage overriding via options any values in the cross-files,
ideally without modifying the files, and this seemed a good way.

/Bruce