Re: [dpdk-dev] [EXT] [PATCH] config: change default cache line size for ARMv8 with meson

[Yongseok Koh <yskoh@mellanox.com>]

On Wed, Jan 16, 2019 at 02:02:26AM +0000, Honnappa Nagarahalli wrote:
> > > >>>>>> On Wed, 2019-01-09 at 10:22 +0000, Yongseok Koh wrote:
> > > >>>>>>> On Jan 9, 2019, at 2:09 AM, Jerin Jacob Kollanukkaran wrote:
> > > >>>>>>>> I think, I way forward is to add
> > > >>>>>>>> config/arm/arm64_a72_linuxapp_gcc for meson. This config can
> > > >>>>>>>> be used for all SoC with A72
> > > >>>>>>>> armv8
> > > >>>>>>>> implementation and may have sym link to specfific SoC to
> > > >>>>>>>> avoid confusion to end users.
> > > >>>>>>>
> > > >>>>>>> Is config/arm/arm64_a72_linuxapp_gcc valid? Others have
> > > >>>>>>
> > > >>>>>> Yes. For cross compiling for A72.
> > > >>>>>
> > > >>>>> Any cross-compilation with meson requires a config file.
> > > >>>>> The default Arm cross-compilation is done with
> > > >>>>> 	config/arm/arm64_armv8_linuxapp_gcc
> > > >>>>> which set implementor_id = 'generic'
> > > >>>>>
> > > >>>>> For native compilation, implementor_id is detected from
> > > >>>>> /sys/devices/system/cpu/cpu0/regs/identification/midr_el1
> > > >>>>>
> > > >>>>> So each Arm machine needs 2 things:
> > > >>>>> 	- a cross-compilation file
> > > >>>>> 	- settings based on implementor_id in
> > config/arm/meson.build
> > > >>>>
> > > >>>> Yes. config/arm/arm64_armv8_linuxapp_gcc sets the implementor_id
> > > >>>> = 'generic' which assumed to generic across all the armv8 platform.
> > > >>>> If tomorrow there is new core from ARM which A100 with armv8.2
> > > >>>> specific we can not tune the generic params armv8.2 as it will
> > > >>>> break other CPU.
> > > >>>>
> > > >>>>
> > > >>>>>> Having not seperate IMPLEMENTOR ID is a chip design issue.
> > > >>>>>
> > > >>>>> No I don't think it's a design issue.
> > > >>>>> If the Arm core has no modification, it does not need to be
> > > >>>>> specially identified.
> > > >>>>
> > > >>>> Thats right. It does not need to be specially identified, then
> > > >>>> should have default config is enough.
> > > >>>>
> > > >>>>
> > > >>>>>> I think it can work around by creating
> > > >>>>>> config/arm/arm64_<your_soc_name>_linuxapp_gcc
> > > >>>>>> and build on x86 or arm64 through
> > > >>>>>>
> > > >>>>>> meson build --cross-file
> > > >>>>>> config/arm/arm64_<your_soc_name>_linuxapp_gcc
> > > >>>>>
> > > >>>>> No, it is a real A72, so it should work with default settings.
> > > >>>>>
> > > >>>>> The only issue we have is that the default cache line size for
> > > >>>>> Aarch64
> > > >>>>> is set to 128 in config/arm/meson.build, and this is wrong.
> > > >>>>> The default cache line is 64 bits.
> > > >>>>
> > > >>>> The cache line size as per ARM spec it is IMPLEMENTATION DEFINED.
> > > >>>
> > > >>> In A72 spec, it is said
> > > >>> "Returns 0b010 to indicate that the cache line size is 64 bytes."
> > > >>> But I guess we cannot say it is always true for all models.
> > > >>> So let's assume there is no default.
> > > >>
> > > >> Please note, A72 is not armv8 spec. A72 is just an IMPLEMENTATION
> > > >> of armv8.
> > > >
> > > > Yes, this my understanding. That's why I agree with you.
> > > >
> > > >>>> So no default there. So the default is something work on all
> > > >>>> platforms.
> > > >>>> Actually Cavium has machine with 64B and 128B CL and same image
> > > >>>> should work on both for generic build.
> > > >>>>
> > > >>>>> This is already overriden for Cavium machines which have 128-bit
> > > >>>>> cache lines.
> > > >>>>> It may be needed to do the same change for other machines
> > > >>>>> (Qualcomm?)
> > > >>>>> having Arm core modified to 128-bit cache lines.
> > > >>>>
> > > >>>> Assume you meant 128B here.
> > > >>>
> > > >>> Yes, sorry I mixed bits and bytes :)
> > > >>>
> > > >>>> Building the image Naively(on 128B CL
> > > >>>> machine) and cross compile (on x86) is not an issue.
> > > >>>>
> > > >>>>> The other concern is about running a generic Arm build.
> > > >>>>
> > > >>>> Yes. That's the ONLY concern.
> > > >>>>
> > > >>>>> Given 64-bit should be the default, generic builds will have
> > > >>>>> this value.
> > > >>>>> Is it a big issue for running generic 64-bit build on Cavium
> > > >>>>> machines?
> > > >>>>
> > > >>>> Cavium has both 64B and 128B CL machines. So putting generic
> > > >>>> form,
> > > >>>>
> > > >>>> You can run 128B configured image on 64B machine, It will waste
> > > >>>> some memory not beyond that. Other way around will result in HW
> > > >>>> misbehavior.
> > > >>>> ie Running 64B CL image on 128B target.
> > > >>>
> > > >>> Indeed it is the main concern.
> > > >>> Running DPDK tuned for 128 bytes on a core having 64 bytes cache
> > > >>> line will result in lower performances. It is less an issue than
> > > >>> HW misbehavior.
> > > >>
> > > >> Do you see performance issue or it more memory usage? It nothing do
> > > >> with thread just of out curosity. Becase, our 64CL machine does
> > > >> take more memory, performance seems to same for both. Note we are
> > > >> using 512MB hugepage size.
> > > >
> > > > Yes, we see better performance with 64B cache line on Bluefield.
> > > >
> > > >>> If we agree to keep 128 bytes as generic cache line size for Arm,
> > > >>> we need a way to get 64 bytes size for unmodified cores.
> > > >>> In other words, the generic build settings must be different of
> > > >>> the default settings.
> > > >>
> > > >> Please send a patch.
> > > >>
> > > >> If MIDR value is set to A72, we can set to 64B cache, no issue.
> > > >>
> > > >>> Please make a difference between default 'armv8' and 'generic'
> > > >>> as implementor_id in config/arm/meson.build.
> > > >>> I propose arm64_armv8_linuxapp_gcc being the default config (for
> > > >>> armv8)
> > > >>> and creating arm64_generic_linuxapp_gcc for the generic build (for
> > > >>> distros).
> > > >>
> > > >> It should be inline with how distro guys build the image. I guess
> > > >> we dont want DPDK to be a exception.
> > > >
> > > > The machine option is specific to DPDK, so we can define it as we want.
> > > >
> > > >> Please check below thread and patch.
> > > >>
> > > >>
> > > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmai
> > > >> ls.dpdk.org%2Farchives%2Fdev%2F2019-
> > > January%2F122676.html&amp;data=02
> > > >> %7C01%7Cyskoh%40mellanox.com%7C96576e66c4b6434b47ad08d6764
> > 2
> > > be9a%7Ca65
> > > >>
> > >
> > 2971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636826426371146146&a
> > > mp;sdata=
> > > >>
> > >
> > MNMzpjs7e71l4vZAmoyqicpElp7UIFO48UuamggQWHQ%3D&amp;reserved=0
> > > >>
> > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> > > >>
> > >
> > tches.dpdk.org%2Fpatch%2F49477%2F&amp;data=02%7C01%7Cyskoh%40m
> > > ellanox
> > > >> .com%7C96576e66c4b6434b47ad08d67642be9a%7Ca652971c7d2e4d9
> > ba
> > > 6a4d149256
> > > >>
> > >
> > f461b%7C0%7C0%7C636826426371146146&amp;sdata=okIrz7Idc8t7nMbFkc
> > > RnjxZg
> > > >> 2wMn9ZTjqaTLlEXCnaU%3D&amp;reserved=0
> > > >>
> > > >> Debian folks are building like this for the _generic_ image.
> > > >> What ever works for every distros, I am fine with that.
> > > >>
> > > >> meson configure -Dmachine=default
> > > >> meson build
> > > >> cd build
> > > >> ninja
> > > >> ninja install
> > > >
> > > > I think we agree on the idea of having different configs for
> > > > unmodified A72 core and generic build working for all.
> > > > The remaining bits to discuss are:
> > > > 	- do we want to use the armv8 config for unmodified A72?
> > > > 	- what should be the name of the generic config?
> > > >
> > > > When digging more the config files in meson, I found this:
> > > >
> > > >
> > >
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmeso
> > > > nbuild.com%2FCross-compilation.html%23cross-file-
> > > locations&amp;data=02
> > > > %7C01%7Cyskoh%40mellanox.com%7C96576e66c4b6434b47ad08d6764
> > 2b
> > > e9a%7Ca652
> > > >
> > >
> > 971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636826426371146146&am
> > > p;sdata=J8
> > > > XiCovgwqxm8HHRCJ5bSUbx4yTHCO2YuZz2ryZJx8I%3D&amp;reserved=0
> > > > It says that distros or compilers should provide some config files.
> > > > It means we should check if some standard names are emerging and try
> > > > to follow the same naming, or even re-use existing config files.
> > >
> > > I'll come up with a new patch based on the discussion here.
> > > A few things noted,
> > > - we still want it to be 128B for generic build
> > > - we at least agreed on changing it to 64B for A72
> > How will this be done? Will you add
> > config/arm/arm64_bluefield_linuxapp_gcc?
> I asked this question as there was a proposal containing 'a72' in the file
> name. IMO, the file name should contain 'bluefield', not on a72.

Sorry for late reply. It's been buried for some reason. :-)

Nope, I won't create such a file. That's for cross-compiler AFAIK.
I'm thinking about changing meson.build. Currently, one CL size is applied to
all kinds of cores. Consequently, for armv8a, both 'default' and 'a72' have to
have the same CL size. And one more thing raised from ARM was that 'crypto' in
-march can't be a default.

> > > - As Qualcomm Centriq CPU has 128B cache line with A72, they should
> > >   create a profile in meson.build based on their impl_id.
> > Qualcomm is not A72 core. Can it not use RTE_CACHE_LINE_SIZE from
> > 'flags_generic' in config/arm/meson.build?

We can add flags_qualcomm for their processor.

Thanks,
Yongseok