Re: [dpdk-dev] [PATCH v8 06/14] build: organize Arm config into dict

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>

> >
> > >
> > > Use dictionary lookup instead of checking for existing variables,
> > > iterating over all elements in the list or checking lists for
> > > optional configuration. Move variable contents into the dictionary
> > > for variables that would be referenced only once.
> > > Fallback to generic part number if the discovered part number is unknown.
> > >
> > > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > > ---
> > >  config/arm/meson.build | 282
> > > +++++++++++++++++++++++------------------
> > >  1 file changed, 160 insertions(+), 122 deletions(-)
> > >
> > > diff --git a/config/arm/meson.build b/config/arm/meson.build index
> > > eda485e7f..5d232f1c4 100644
> > > --- a/config/arm/meson.build
> > > +++ b/config/arm/meson.build
> > > @@ -28,115 +28,146 @@ flags_common_default = [
> > >  	['RTE_CACHE_LINE_SIZE', 128]
> > >  ]
> > >
> > > -# implementer specific aarch64 flags, with middle priority -# (will
> > > overwrite common flags) -flags_implementer_generic = [
> > > -	['RTE_MACHINE', '"armv8a"'],
> > > -	['RTE_USE_C11_MEM_MODEL', true],
> > > -	['RTE_CACHE_LINE_SIZE', 128],
> > > -	['RTE_MAX_LCORE', 256]
> > > -]
> > > -flags_implementer_arm = [
> > > -	['RTE_MACHINE', '"armv8a"'],
> > > -	['RTE_USE_C11_MEM_MODEL', true],
> > > -	['RTE_CACHE_LINE_SIZE', 64],
> > > -	['RTE_MAX_LCORE', 16]
> > > -]
> > > -flags_implementer_cavium = [
> > > -	['RTE_MAX_VFIO_GROUPS', 128],
> > > -	['RTE_CACHE_LINE_SIZE', 128],
> > > -	['RTE_MAX_LCORE', 96],
> > > -	['RTE_MAX_NUMA_NODES', 2]
> > > -]
> > > -flags_implementer_dpaa = [
> > > -	['RTE_MACHINE', '"dpaa"'],
> > > -	['RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', false],
> > > -	['RTE_USE_C11_MEM_MODEL', true],
> > > -	['RTE_CACHE_LINE_SIZE', 64],
> > > -	['RTE_MAX_LCORE', 16],
> > > -	['RTE_MAX_NUMA_NODES', 1]
> > > -]
> > > -flags_implementer_emag = [
> > > -	['RTE_MACHINE', '"emag"'],
> > > -	['RTE_CACHE_LINE_SIZE', 64],
> > > -	['RTE_MAX_LCORE', 32],
> > > -	['RTE_MAX_NUMA_NODES', 1]
> > > -]
> > > -flags_implementer_armada = [
> > > -	['RTE_MACHINE', '"armv8a"'],
> > > -	['RTE_CACHE_LINE_SIZE', 64],
> > > -	['RTE_MAX_LCORE', 16],
> > > -	['RTE_MAX_NUMA_NODES', 1]
> > > -]
> > > -
> > > -# part number specific aarch64 flags, with highest priority -#
> > > (will overwrite both common and implementer specific flags)
> > > flags_part_number_thunderx = [
> > >  	['RTE_MACHINE', '"thunderx"'],
> > >  	['RTE_USE_C11_MEM_MODEL', false]
> > >  ]
> > > -flags_part_number_thunderx2 = [
> > > -	['RTE_MACHINE', '"thunderx2"'],
> > > -	['RTE_ARM_FEATURE_ATOMICS', true],
> > > -	['RTE_USE_C11_MEM_MODEL', true],
> > > -	['RTE_CACHE_LINE_SIZE', 64],
> > > -	['RTE_MAX_LCORE', 256],
> > > -	['RTE_MAX_NUMA_NODES', 2]
> > > -]
> > > -flags_part_number_octeontx2 = [
> > > -	['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]
> > > -]
> > > -flags_part_number_n1generic = [
> > > -	['RTE_MACHINE', '"neoverse-n1"'],
> > > -	['RTE_ARM_FEATURE_ATOMICS', true],
> > > -	['RTE_USE_C11_MEM_MODEL', true],
> > > -	['RTE_EAL_NUMA_AWARE_HUGEPAGES', false],
> > > -	['RTE_LIBRTE_VHOST_NUMA', false],
> > > -	['RTE_MAX_MEM_MB', 1048576],
> > > -	['RTE_CACHE_LINE_SIZE', 64],
> > > -	['RTE_MAX_LCORE', 64],
> > > -	['RTE_MAX_NUMA_NODES', 1]
> > > -]
> > > -
> > > -# arm config (implementer 0x41) is the default config -
> > > part_number_config_arm = [
> > > -	['generic', ['-march=armv8-a+crc', '-moutline-atomics']],
> > > -	['native', ['-march=native']],
> > > -	['0xd03', ['-mcpu=cortex-a53']],
> > > -	['0xd04', ['-mcpu=cortex-a35']],
> > > -	['0xd07', ['-mcpu=cortex-a57']],
> > > -	['0xd08', ['-mcpu=cortex-a72']],
> > > -	['0xd09', ['-mcpu=cortex-a73']],
> > > -	['0xd0a', ['-mcpu=cortex-a75']],
> > > -	['0xd0b', ['-mcpu=cortex-a76']],
> > > -	['0xd0c', ['-march=armv8.2-a+crypto', '-mcpu=neoverse-n1'],
> > > flags_part_number_n1generic]
> > > -]
> > > -part_number_config_cavium = [
> > > -	['generic', ['-march=armv8-a+crc+crypto','-mcpu=thunderx']],
> > > -	['native', ['-march=native']],
> > > -	['0xa1', ['-mcpu=thunderxt88'], flags_part_number_thunderx],
> > > -	['0xa2', ['-mcpu=thunderxt81'], flags_part_number_thunderx],
> > > -	['0xa3', ['-mcpu=thunderxt83'], flags_part_number_thunderx],
> > > -	['0xaf', ['-march=armv8.1-a+crc+crypto','-mcpu=thunderx2t99'],
> > > flags_part_number_thunderx2],
> > > -	['0xb2', ['-march=armv8.2-a+crc+crypto+lse','-mcpu=octeontx2'],
> > > flags_part_number_octeontx2]
> > > -]
> > > -part_number_config_emag = [
> > > -	['generic', ['-march=armv8-a+crc+crypto', '-mtune=emag']],
> > > -	['native', ['-march=native']]
> > > -]
> > > +part_number_config_arm = {
> > > +	'generic': {'machine_args':  ['-march=armv8-a+crc', '-moutline-
> > > atomics']},
> > > +	'native': {'machine_args':  ['-march=native']},
> > > +	'0xd03': {'machine_args':  ['-mcpu=cortex-a53']},
> > > +	'0xd04': {'machine_args':  ['-mcpu=cortex-a35']},
> > > +	'0xd07': {'machine_args':  ['-mcpu=cortex-a57']},
> > > +	'0xd08': {'machine_args':  ['-mcpu=cortex-a72']},
> > > +	'0xd09': {'machine_args':  ['-mcpu=cortex-a73']},
> > > +	'0xd0a': {'machine_args':  ['-mcpu=cortex-a75']},
> > > +	'0xd0b': {'machine_args':  ['-mcpu=cortex-a76']},
> > > +	'0xd0c': {
> > > +		'machine_args':  ['-march=armv8.2-a+crypto', '-
> > > mcpu=neoverse-n1'],
> > > +		'flags': [
> > > +			['RTE_MACHINE', '"neoverse-n1"'],
> > > +			['RTE_ARM_FEATURE_ATOMICS', true],
> > > +			['RTE_USE_C11_MEM_MODEL', true],
> > > +			['RTE_EAL_NUMA_AWARE_HUGEPAGES', false],
> > > +			['RTE_LIBRTE_VHOST_NUMA', false],
> > > +			['RTE_MAX_MEM_MB', 1048576],
> > > +			['RTE_CACHE_LINE_SIZE', 64],
> > > +			['RTE_MAX_LCORE', 64],
> > > +			['RTE_MAX_NUMA_NODES', 1]
> > > +		]
> > > +	}
> > > +}
> > >
> > > -## Arm implementer ID (MIDR in Arm Architecture Reference Manual) -
> > > implementer_generic = ['Generic armv8', flags_implementer_generic,
> > > part_number_config_arm]
> > > -implementer_0x41 = ['Arm', flags_implementer_arm,
> > > part_number_config_arm]
> > > -implementer_0x43 = ['Cavium', flags_implementer_cavium,
> > > part_number_config_cavium]
> > > -implementer_0x50 = ['Ampere Computing', flags_implementer_emag,
> > > part_number_config_emag]
> > > -implementer_0x56 = ['Marvell ARMADA', flags_implementer_armada,
> > > part_number_config_arm] -implementer_dpaa = ['NXP DPAA',
> > > flags_implementer_dpaa, part_number_config_arm]
> > > +## Arm implementers (ID from MIDR in Arm Architecture Reference
> > > +Manual) ## Part numbers are specific to Arm implementers #
> > > +implementer specific
> > > +aarch64 flags have middle priority
> > > +#     (will overwrite common flags)
> > > +# part number specific aarch64 flags have the highest priority
> > > +#     (will overwrite both common and implementer specific flags)
> > > +implementers = {
> > I think this one is big. It will grow further in the future. I like
> > the existing one, which is dis-integrated into smaller chunks and is easy to
> maintain in the future.
> >
> 
> My main concern was with the readability/understandability of the data
> (which affects maintenance), at least when a newcomer tries to wrap their
> head around them. The data are hierarchical and the relationship between
> implementer ID and part number and the order of flag application is more
> apparent when organized this way, although it could because of better
> formatting and better variable names. The bigger difference is in the code -
> using implementer_config['part_number_config'] immediately tells you what
> data are you working with as opposed to implementer_config[2].
> 
> I'm also looking at maintenance in terms of "where in this file do I need to
> change/add things for this implementer or their part number", which informed
> my motivation for (almost) removing the fragmentation, which we don't have
> to do, but it made sense to me. This way, if I'm looking for some configuration
> I know exactly where to find it in the file (under particular implementer
> ID/part number) and I don't have to chase variables around, although this is
> somewhat alleviated by better variable names. Or in other words having
> related configuration in one place rather than fragmented in multiple places is
> better organization of the data in my view, both in terms of readability and
> maintainability.
> 
> I don't actually see how having fragmented configuration is better when it's
> going to grow. The disjointed parts which are logically connected (implemeter
> configuration connected to its part number config) are only going to be further
> apart in the code, causing more potential confusion. When we have
> configuration organized by their relationship new additions won't really affect
> existing organization - e.g. when we add a new implementer, it won't move
> the configuration within other implementers; the other implementer's
> configuration won't be affected.
> 
> If I understand correctly, your gripe is not with using a dictionary (or how I've
> formatted the data in it), but rather with the removal of fragmentation. I'd like
> to keep the dictionaries (and formatting), since it results in more readable
> code. I could put the fragmentetion back in place, but I don't think it's better
> maintanence-wise.
I would not call it gripe, just trying to figure out if there is another way.
I like the dictionary method.  The problem I see is that, this structure is ~100 lines (that of SoC is ~100) and does not fit in one screen. I have to scroll back and forth to the top to understand the various fields. Some of the lines are beyond 80 characters (I guess this can be fixed).

IMO, splitting each implementation into its own structures and then combining them together might be easier. For ex: (I have not paid attention to syntax)

implementer_generic = {
        'generic': {
                'description': 'Generic armv8',
                'flags': [
                        ['RTE_MACHINE', '"armv8a"'],
                        ['RTE_USE_C11_MEM_MODEL', true],
                        ['RTE_CACHE_LINE_SIZE', 128],
                        ['RTE_MAX_LCORE', 256],
                        ['RTE_MAX_NUMA_NODES', 4]
                ],
                'part_number_config': {
                        'generic': {'machine_args':  ['-march=armv8-a+crc', '-moutline-atomics']}
                }
        }
}

implementer_arm = {
        '0x41': {
                'description': 'Arm',
                'flags': [
                        ['RTE_MACHINE', '"armv8a"'],
                        ['RTE_USE_C11_MEM_MODEL', true],
                        ['RTE_CACHE_LINE_SIZE', 64],
                        ['RTE_MAX_LCORE', 16],
                        ['RTE_MAX_NUMA_NODES', 1]
                ],
                'part_number_config': part_number_config_arm
        },
}
......

implementers = {
    implementer_generic,
    implementer_arm,
    ....}

> 
> > > +	'generic': {
> > > +		'description': 'Generic armv8',
> > > +		'flags': [
> > > +			['RTE_MACHINE', '"armv8a"'],
> > > +			['RTE_USE_C11_MEM_MODEL', true],
> > > +			['RTE_CACHE_LINE_SIZE', 128],
> > > +			['RTE_MAX_LCORE', 256]
> > > +		],
> > > +		'part_number_config': part_number_config_arm
> > > +	},
> > > +	'0x41': {
> > > +		'description': 'Arm',
> > > +		'flags': [
> > > +			['RTE_MACHINE', '"armv8a"'],
> > > +			['RTE_USE_C11_MEM_MODEL', true],
> > > +			['RTE_CACHE_LINE_SIZE', 64],
> > > +			['RTE_MAX_LCORE', 16]
> > > +		],
> > > +		'part_number_config': part_number_config_arm
> > > +	},
> > > +	'0x43': {
> > > +		'description': 'Cavium',
> > > +		'flags': [
> > > +			['RTE_MAX_VFIO_GROUPS', 128],
> > > +			['RTE_CACHE_LINE_SIZE', 128],
> > > +			['RTE_MAX_LCORE', 96],
> > > +			['RTE_MAX_NUMA_NODES', 2]
> > > +		],
> > > +		'part_number_config': {
> > > +			'generic': {'machine_args': ['-march=armv8-
> > > a+crc+crypto', '-mcpu=thunderx']},
> > > +			'native': {'machine_args': ['-march=native']},
> > > +			'0xa1': {
> > > +				'machine_args': ['-mcpu=thunderxt88'],
> > > +				'flags': flags_part_number_thunderx
> > > +			},
> > > +			'0xa2': {
> > > +				'machine_args': ['-mcpu=thunderxt81'],
> > > +				'flags': flags_part_number_thunderx
> > > +			},
> > > +			'0xa3': {
> > > +				'machine_args': ['-mcpu=thunderxt83'],
> > > +				'flags': flags_part_number_thunderx
> > > +			},
> > > +			'0xaf': {
> > > +				'machine_args': ['-march=armv8.1-
> > > a+crc+crypto','-mcpu=thunderx2t99'],
> > > +				'flags': [
> > > +					['RTE_MACHINE', '"thunderx2"'],
> > > +					['RTE_ARM_FEATURE_ATOMICS',
> > > true],
> > > +					['RTE_USE_C11_MEM_MODEL', true],
> > > +					['RTE_CACHE_LINE_SIZE', 64],
> > > +					['RTE_MAX_LCORE', 256],
> > > +					['RTE_MAX_NUMA_NODES', 2]
> > > +				]
> > > +			},
> > > +			'0xb2': {
> > > +				'machine_args': ['-march=armv8.2-
> > > a+crc+crypto+lse','-mcpu=octeontx2'],
> > > +				'flags': [
> > > +					['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]
> > > +				]
> > > +			}
> > > +		}
> > > +	},
> > > +	'0x50': {
> > > +		'description': 'Ampere Computing',
> > > +		'flags': [
> > > +			['RTE_MACHINE', '"emag"'],
> > > +			['RTE_CACHE_LINE_SIZE', 64],
> > > +			['RTE_MAX_LCORE', 32],
> > > +			['RTE_MAX_NUMA_NODES', 1]
> > > +		],
> > > +		'part_number_config': {
> > > +			'generic': {'machine_args':  ['-march=armv8-
> > > a+crc+crypto', '-mtune=emag']},
> > > +			'native': {'machine_args':  ['-march=native']}
> > > +		}
> > > +	},
> > > +	'0x56': {
> > > +		'description': 'Marvell ARMADA',
> > > +		'flags': [
> > > +			['RTE_MACHINE', '"armv8a"'],
> > > +			['RTE_CACHE_LINE_SIZE', 64],
> > > +			['RTE_MAX_LCORE', 16],
> > > +			['RTE_MAX_NUMA_NODES', 1]
> > > +		],
> > > +		'part_number_config': part_number_config_arm
> > > +	},
> > > +	'dpaa': {
> > > +		'description': 'NXP DPAA',
> > > +		'flags': [
> > > +			['RTE_MACHINE', '"dpaa"'],
> > > +			['RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', false],
> > > +			['RTE_USE_C11_MEM_MODEL', true],
> > > +			['RTE_CACHE_LINE_SIZE', 64],
> > > +			['RTE_MAX_LCORE', 16],
> > > +			['RTE_MAX_NUMA_NODES', 1]
> > > +		],
> > > +		'part_number_config': part_number_config_arm
> > > +	}
> > > +}
> > >
> > >  dpdk_conf.set('RTE_ARCH_ARM', 1)
> > >  dpdk_conf.set('RTE_FORCE_INTRINSICS', 1) @@ -152,7 +183,7 @@ else
> > >  	implementer_id = 'generic'
> > >  	if machine == 'generic' and not meson.is_cross_build()
> > >  		# generic build
> > > -		implementer_config = implementer_generic
> > > +		implementer_config = implementer['generic']
> > >  		part_number = 'generic'
> > >  	elif not meson.is_cross_build()
> > >  		# native build
> > > @@ -167,9 +198,9 @@ else
> > >  			part_number = cmd_output[3]
> > >  		endif
> > >  		# Set to generic if variable is not found
> > > -		implementer_config = get_variable('implementer_' +
> > > implementer_id, ['generic'])
> > > +		implementer_config = implementers.get(implementer_id,
> > > ['generic'])
> > >  		if implementer_config[0] == 'generic'
> > > -			implementer_config = implementer_generic
> > > +			implementer_config = implementer['generic']
> > >  			part_number = 'generic'
> > >  		endif
> > >  		if arm_force_native_march == true @@ -179,28 +210,35 @@
> else
> > >  		# cross build
> > >  		implementer_id =
> > > meson.get_cross_property('implementer_id', 'generic')
> > >  		part_number = meson.get_cross_property('part_number',
> > > 'generic')
> > > -		implementer_config = get_variable('implementer_' +
> > > implementer_id)
> > > +		implementer_config = implementers.get(implementer_id)
> > >  	endif
> > >
> > > -	message('Arm implementer: ' + implementer_config[0])
> > > +	message('Arm implementer: ' + implementer_config['description'])
> > >  	message('Arm part number: ' + part_number)
> > >
> > > +	part_number_config = implementer_config['part_number_config']
> > > +	if part_number_config.has_key(part_number)
> > > +		# use the specified part_number machine args if found
> > > +		part_number_config = part_number_config[part_number]
> > > +	elif not meson.is_cross_build()
> > > +		# default to generic machine args if part_number is not found
> > > +		# and not forcing native machine args
> > > +		# but don't default in cross-builds; if part_number is specified
> > > +		# incorrectly in a cross-file, it needs to be fixed there
> > > +		part_number_config = part_number_config['generic']
> > > +	else
> > > +		# doing cross build and part number is not in
> > > part_number_config
> > > +		error('Cross build part number 0@0 not
> > > found.'.format(part_number))
> > > +	endif
> > > +
> > >  	# use default flags with implementer flags
> > > -	dpdk_flags = flags_common_default + implementer_config[1]
> > > +	dpdk_flags = flags_common_default + implementer_config['flags'] +
> > > +part_number_config.get('flags', [])
> > >
> > > +	# apply supported machine args
> > >  	machine_args = [] # Clear previous machine args
> > > -	foreach marg: implementer_config[2]
> > > -		if marg[0] == part_number
> > > -			# apply supported machine args
> > > -			foreach flag: marg[1]
> > > -				if cc.has_argument(flag)
> > > -					machine_args += flag
> > > -				endif
> > > -			endforeach
> > > -			if marg.length() > 2
> > > -				# add extra flags for the part
> > > -				dpdk_flags += marg[2]
> > > -			endif
> > > +	foreach flag: part_number_config['machine_args']
> > > +		if cc.has_argument(flag)
> > > +			machine_args += flag
> > >  		endif
> > >  	endforeach
> > >
> > > --
> > > 2.20.1