[dpdk-dev] [PATCH v4 2/6] build: refactor Arm build

Honnappa Nagarahalli Honnappa.Nagarahalli at arm.com
Wed Oct 28 00:12:54 CET 2020
Previous message: [dpdk-dev] [PATCH v4 2/6] build: refactor Arm build
Next message: [dpdk-dev] [PATCH v4 2/6] build: refactor Arm build
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
<snip>

> > I think we need to split this further. Few suggestions below.
> >
> > >
> > > * Rename variables to have names that better describe what the
> > > variables store
> > This should be a separate commit.
> >
> > > * Remove unused or superfluous variables
> > Same here
> >
> > > * Change a list to dictionary where key lookup is needed
> > Same here
> >
> > > * Add informatory comments in the code
> >
> > > * Minor code restructure and reformatting
> > Same for this
> >
> 
> Ok, hopefully I'll be able to separate these cleanly.
> 
> > >
> > > Signed-off-by: Juraj Linkeš <juraj.linkes at pantheon.tech>
> > > ---
> > >  config/arm/arm64_armada_linux_gcc    |   2 +-
> > >  config/arm/arm64_armv8_linux_gcc     |   8 +-
> > >  config/arm/arm64_bluefield_linux_gcc |   4 +-
> > >  config/arm/arm64_dpaa_linux_gcc      |   2 +-
> > >  config/arm/arm64_emag_linux_gcc      |   2 +-
> > >  config/arm/arm64_n1sdp_linux_gcc     |   4 +-
> > >  config/arm/arm64_octeontx2_linux_gcc |   4 +-
> > >  config/arm/arm64_stingray_linux_gcc  |   4 +-
> > >  config/arm/arm64_thunderx2_linux_gcc |   4 +-
> > >  config/arm/arm64_thunderx_linux_gcc  |   2 +-
> > >  config/arm/meson.build               | 247 +++++++++++++++------------
> > >  11 files changed, 153 insertions(+), 130 deletions(-)
> > >
> > > diff --git a/config/arm/arm64_armada_linux_gcc
> > > b/config/arm/arm64_armada_linux_gcc
> > > index fa40c0398..52c5f4476 100644
> > > --- a/config/arm/arm64_armada_linux_gcc
> > > +++ b/config/arm/arm64_armada_linux_gcc
> > > @@ -14,4 +14,4 @@ cpu = 'armv8-a'
> > >  endian = 'little'
> > >
> > >  [properties]
> > > -implementor_id = '0x56'
> > > +implementer_id = '0x56'
> > Implementor and implementer mean the same. Looked at Arm specs, they
> > use 'implementer'. So, I am fine.
> >
> 
> That's where I got this and some of the other variable name changes from.
> 
> > > diff --git a/config/arm/arm64_armv8_linux_gcc
> > > b/config/arm/arm64_armv8_linux_gcc
> > > index 88f0ff9da..13ee8b223 100644
> > > --- a/config/arm/arm64_armv8_linux_gcc
> > > +++ b/config/arm/arm64_armv8_linux_gcc
> > > @@ -13,10 +13,10 @@ cpu = 'armv8-a'
> > >  endian = 'little'
> > >
> > >  [properties]
> > > -implementor_id = 'generic'
> > > +implementer_id = 'generic'
> > >
> > > -# Valid options for Arm's implementor_pn:
> > > -# 'default': valid for all armv8-a architectures (default value)
> > > +# Valid options for Arm's part_number:
> > > +# 'generic': valid for all armv8-a architectures (default value)
> > >  # '0xd03':   cortex-a53
> > >  # '0xd04':   cortex-a35
> > >  # '0xd05':   cortex-a55
> > > @@ -25,4 +25,4 @@ implementor_id = 'generic'
> > >  # '0xd09':   cortex-a73
> > >  # '0xd0a':   cortex-a75
> > >  # '0xd0b':   cortex-a76
> > > -implementor_pn = 'default'
> > > +part_number = 'generic'
> > Same here, Arm specs refer to this as 'PartNumber'. So, this should be fine.
> > I like 'generic' for part_number here.
> >
> > > diff --git a/config/arm/arm64_bluefield_linux_gcc
> > > b/config/arm/arm64_bluefield_linux_gcc
> > > index 86797d23c..b79389d85 100644
> > > --- a/config/arm/arm64_bluefield_linux_gcc
> > > +++ b/config/arm/arm64_bluefield_linux_gcc
> > > @@ -13,5 +13,5 @@ cpu = 'armv8-a'
> > >  endian = 'little'
> > >

[...]

> > > diff --git a/config/arm/meson.build b/config/arm/meson.build index
> > > 491842cad..6c31ab167 100644
> > > --- a/config/arm/meson.build
> > > +++ b/config/arm/meson.build
> > > @@ -3,12 +3,12 @@
> > >  # Copyright(c) 2017 Cavium, Inc
> > >  # Copyright(c) 2020 PANTHEON.tech s.r.o.
> > >
> > > -# for checking defines we need to use the correct compiler flags
> > > -march_opt = '-march=@0@'.format(machine)
> > > -

[...]

> > >
> > > +# implementer specific aarch64 flags, with middle priority # (will
> > > +overwrite common flags)
> > >  flags_generic = [
> > >  	['RTE_MACHINE', '"armv8a"'],
> > >  	['RTE_MAX_LCORE', 256],
> > >  	['RTE_USE_C11_MEM_MODEL', true],
> > > -	['RTE_CACHE_LINE_SIZE', 128]]
> > > +	['RTE_CACHE_LINE_SIZE', 128]
> > > +]
> > Any particular reason for this change? (and similar changes below)
> >
> 
> The first bracket is split from the second bracket so I did the same for the last
> two brackets. It makes it more apparent which brackets are paired, it's more
> consistent and also in line with how flags_common_default is formatted.
Ok

> 

[...]

> > >  flags_dpaa = [
> > >  	['RTE_MACHINE', '"dpaa"'],
> > >  	['RTE_USE_C11_MEM_MODEL', true],
> > >  	['RTE_CACHE_LINE_SIZE', 64],
> > >  	['RTE_MAX_NUMA_NODES', 1],
> > >  	['RTE_MAX_LCORE', 16],
> > > -	['RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', false]]
> > > +	['RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', false] ]
> > This is not needed
> >
> 
> Do you mean the space? It should be a line break. I'll check the exact
I meant the space. I guess it needs to be on the next line.

> characters, but I see this as adding a space in my local patch. Or do you mean
> the config option? It's set to true by default in config/meson.build and
> according to [1] it should be disabled.
> 
> [1] http://git.dpdk.org/dpdk/tree/config/defconfig_arm64-dpaa-linuxapp-
> gcc?h=v20.08
> 
> > >  flags_emag = [
> > >  	['RTE_MACHINE', '"emag"'],
> > > -	['RTE_CACHE_LINE_SIZE', 64],
> > >  	['RTE_MAX_NUMA_NODES', 1],
> > > -	['RTE_MAX_LCORE', 32]]
> > > +	['RTE_MAX_LCORE', 32],
> > > +	['RTE_CACHE_LINE_SIZE', 64]
> > > +]
> > >  flags_armada = [
> > >  	['RTE_MACHINE', '"armv8a"'],
> > > -	['RTE_CACHE_LINE_SIZE', 64],
> > >  	['RTE_MAX_NUMA_NODES', 1],
> > > -	['RTE_MAX_LCORE', 16]]
> > > +	['RTE_MAX_LCORE', 16],
> > > +	['RTE_CACHE_LINE_SIZE', 64]
> > > +]
> > Any reason for this change?
> >
> 
> The default (from flags_common_default) is 128 and I found here [0] that it
> should be set to 64 so I added it here. Should this also be in a separate patch
> (apart from those 4 already mention above)?
The RTE_CACHE_LINE_SIZE is already set to 64 for 'flags_armada'. It looks like you have moved the line down.

> 
> [0] http://git.dpdk.org/dpdk/tree/config/defconfig_arm64-armada-
> linuxapp-gcc?h=v20.08
> 
> > >
> > > -flags_default_extra = []
> > > +# part number specific aarch64 flags, with highest priority # (will
> > > +overwrite both common and implementer specific flags)
> > >  flags_n1sdp_extra = [
> > >  	['RTE_MACHINE', '"n1sdp"'],
> > >  	['RTE_MAX_NUMA_NODES', 1],
> > >  	['RTE_MAX_LCORE', 4],
> > >  	['RTE_EAL_NUMA_AWARE_HUGEPAGES', false],
> > > -	['RTE_LIBRTE_VHOST_NUMA', false]]
> > > +	['RTE_LIBRTE_VHOST_NUMA', false]
> > > +]

<snip>

> > > -
> > > -machine_args_emag = [
> > > -	['default', ['-march=armv8-a+crc+crypto', '-mtune=emag']],
> > > -	['native', ['-march=native']]]
> > > +	['RTE_USE_C11_MEM_MODEL', true]
> > > +]
> > > +# arm config (implementer 0x41) is the default config
> > > +pn_config_default
> > 'pn' here for 'part_number' is not consistent.
> >
> 
> Ok, I can rename it to part_number_config_default. Same for the other two
> pn variables.
Ok
> 

<snip>

> > > --
> > > 2.20.1
Previous message: [dpdk-dev] [PATCH v4 2/6] build: refactor Arm build
Next message: [dpdk-dev] [PATCH v4 2/6] build: refactor Arm build
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
More information about the dev mailing list