Re: [dpdk-dev] [PATCH v2 2/3] config: add arm neoverse N1 SDP configuration

[Gavin Hu (Arm Technology China)]

Hi Honnappa,

> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> Sent: Tuesday, October 29, 2019 1:47 PM
> To: tho...@monjalon.net
> Cc: Jerin Jacob <jerinjac...@gmail.com>; Ola Liljedahl
> <ola.liljed...@arm.com>; Gavin Hu (Arm Technology China)
> <gavin...@arm.com>; dev@dpdk.org; pbhagavat...@marvell.com; nd
> <n...@arm.com>; jer...@marvell.com; hemant.agra...@nxp.com;
> bruce.richard...@intel.com; Honnappa Nagarahalli
> <honnappa.nagaraha...@arm.com>; nd <n...@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v2 2/3] config: add arm neoverse N1 SDP
> configuration
> 
> <snip>
> 
> >
> > 28/10/2019 04:24, Honnappa Nagarahalli:
> > > > 23/10/2019 07:03, Jerin Jacob:
> > > > > On Wed, Oct 23, 2019 at 2:37 AM Honnappa Nagarahalli
> > > > > <honnappa.nagaraha...@arm.com> wrote:
> > > > > > > > > On Thu, 2019-08-01 at 07:48 +0800, Gavin Hu wrote:
> > > > > > > > > > Arm N1 SDP is an infrastructure segment development
> > > > > > > > > > platform based on armv8.2-a Neoverse N1 CPU. For more
> > > > > > > > > > information, refer
> > > > to:
> > > > > > > > > > https://community.arm.com/developer/tools-software/oss-
> p
> > > > > > > > > > latf
> > > > > > > > > > orms/w
> > > > > > > > > > /
> > > > > > > > > > docs/440/neoverse-n1-sdp
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Gavin Hu <gavin...@arm.com>
> > > > > > > > > > Reviewed-by: Honnappa Nagarahalli
> > > > > > > > > > <honnappa.nagaraha...@arm.com>
> > > > > > > > > > Reviewed-by: Steve Capper <steve.cap...@arm.com>
> > > > > > > > > > ---
> > > > > > > > > > +CONFIG_RTE_MACHINE="neoversen1"
> > > > > > > > > This should probably be "n1sdp" as this is the name of the
> > > > > > > > > platform that matches the below configuration.
> > > > > > > > A clear definition of RTE_MACHINE is required. Jerin?
> > > > > > >
> > > > > > > I think, In the existing scheme of things, RTE_MACHINE
> > > > > > > defines, where to take the MACHINE_CFLAGS
> > > > > > > mk/machine/xxxx/rte.vars.mk
> > > > > > Ok, thank you
> > > > > >
> > > > > > >
> > > > > > > Considering the fact that there will be a lot of reusable
> > > > > > > IPs(for
> > > > > > > CPU) from ARM for  armv8, I think, it would make sense to
> > > > > > > introduce  RTE_MICRO_ARCH to avoid a lot of code duplications
> > > > > > > and
> > > > confusion.
> > > > > > >
> > > > > > > RTE_ARCH                             example: "x86" or "arm64"
> I see that there are already RTE_ARCH_X86, RTE_ARCH_ARM,
> RTE_ARCH_ARM64, RTE_ARCH_PPC_64 etc. I believe they should be
> sufficient.
> 
> > > > > > > RTE_MICRO_ARCH               example: "a72" or "thunderx3" -
> defines
> > > > > > > mcpu and armv8 verion arch etc
> Are you suggesting this just for Arm platforms?
> My understanding is your intention was to clean up the
> config/arm/meson.build file.
> 
> > > > > > > RTE_MACHINE                       example: "bluefield" or 
> > > > > > > "thunderx3"
> > > > > > > - defines, number of cores, NUMA or not? etc
> > > > > > Looking at mk/machine/ directory, looks like RTE_MACHINE seems
> > > > > > to be
> > > > defining micro-architecture for Intel. For ex: hsw, nhm, wsm. I see
> > > > the same for Arm as well.
> > > > > > Are you suggesting that we use RTE_MICRO_ARCH to pick
> mk/micro-
> > > > arch/xxxx/rte.vars.mk? and RTE_MACHINE would pick
> > > > mk/machine/xxxx/rte.vars.mk, but contain NUMA, #of cores etc?
> > > > >
> > > > > Yes for Make build. I think, it is deprecated soon, so we need a
> > > > > similar solution for meson.
> > > >
> > > > Yes I would prefer we clean the mess in Meson, instead of talking
> > > > about the makefile system.
> > > > And honestly, N1 is not needed in the legacy makefile system.
> > > Unfortunately, most of the guys I talk to are still on makefile.
> >
> > You need to help them to switch.
> > Adding new targets in meson-only can be a good motivation :)
> >
> > > When is makefile system getting removed?
> >
> > It must be clearly decided and announced.
> > The previous techboard discussions were about making makefile hardly
> > usable during 2020, i.e. very soon.
> >
> > > > So focusing on config/arm/meson.build, I think RTE_MACHINE is
> > > > defined only for API compatibility with makefile.
> > > > However, I doubt this value is used by any application.
> > > > I think we can try to remove RTE_MACHINE from meson builds in
> DPDK
> > > > 19.11, or use RTE_MACHINE as micro-arch (my preference).
> > > 'MACHINE' means different things to different people, which is the root
> > cause of this discussion.
> > > 'MICRO-ARCH' has a very clear meaning. Do you see any problem going
> > with MICRO-ARCH instead?
> >
> > Some applications may use RTE_MACHINE for this purpose.
> > It is part of the API since the befinning of DPDK.
> > I don't see a real motivation to break this API now.
> The suggestions are not clear to me. The original suggestion was to
> introduce RTE_MICRO_ARCH and contain all the micro-architecture related
> compiler flags in that.
> Now, the suggestion is to use RTE_MACHINE to contain micro-architecture
> related compiler flags. Will it contain NUMA, number of cores as well (as
> suggested earlier)? If yes, I do not see it changing anything.
> 
> I am not a meson expert. However, I looked at various meson.build files. I
> have few questions/concerns.
> 1) Are these suggestions are for all the platforms? IMO, these need to be the
> same across all the architectures.
> 2) I am looking at config/meson.build. Here RTE_MACHINE is defined to
> indicate Architecture for Arm (armv7-a, aarchxx) and micro-architecture for
> x86 (corei7?). Is the understanding correct? IMO, this should this have a
> common meaning across all the platforms?
> 3) If the changes are for all the platforms, is the risk high for 19.11 
> release?
> 4) The N1 config patch as such conforms to the current conventions. What is
> being asked here is an enhancement, is the understanding correct?
> 
We consider it is an enhancement to introduce the RTE_MICRO_ARCH and I 
understand it is a high risk and we did not agree here yet.
I will submit v3 to change the configuration name from neoverse-n1 to n1sdp, to 
address Honnappa's comment.
/Gavin