Re: [PATCH v6 5/7] eal: provide option to use compiler memcpy instead of RTE

[Thomas Monjalon <thomas@monjalon.net>]

04/10/2024 11:54, David Marchand:
> On Fri, Oct 4, 2024 at 11:21 AM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> > > - Now, looking at what was available for arches so far in DPDK:
> > > * ARM was relying by default on compiler implementation, with specific
> > > implementations for ARM32 and ARM64 available (see for more details
> > > below) => possible values (default first) RTE_USE_CC_MEMCPY = true /
> > > false
> > > * loongarch was relying on compiler implementation, with no specific
> > > implementations, => RTE_USE_CC_MEMCPY = true
> > > * ppc was relying on arch specific implementation, => RTE_USE_CC_MEMCPY = false
> > > * risc was relying on compiler implementation, with no specific
> > > implementations, => RTE_USE_CC_MEMCPY = true
> > > * x86 was relying on arch specific implementation, => RTE_USE_CC_MEMCPY = false
> > >
> > > We can't get a unified default value for a meson option and keep
> > > compat for all arches (except maybe introduce a "auto" value).
> > >
> > > Plus, disabling RTE_USE_CC_MEMCPY on loongarch and risc makes no
> > > sense, as there was never a specific implementation.
> > >
> > > My suggestion is to drop the meson option and instead just set
> > > RTE_USE_CC_MEMCPY in config/$arch/meson.build.
> > > Testers / interested users may edit config/$arch/meson.build on their own.
> > >
> >
> > So we've gone from...
> >
> > "Eliminate DPDK custom per-arch memcpy altogether"
> > to
> > "Keep custom memcpy, but make cc memcpy the default"
> > to
> > "Keep custom memcpy as the default, but make cc memcpy a build option"
> > to
> > "Keep custom memcpy as the default, and have the user modify some
> > obscure build file to use cc memcpy"
> >
> > I seems like the natural next step is just
> >
> > "Keep the custom memcpy. Period."
> 
> Well, the current implementation has holes, that I tried to list so we
> can move forward.
> 
> About adding a meson option, we try to have as less of them as
> possible to reduce complexity.
> And this is why an obscure option is probably the best so that
> performance tests can be run with the compiler, without breaking
> existing users.
> 
> 
> If what I replied is irrelevant to others, well, I will let others
> review _*in* *depth*_ and Thomas can merge the series.

No I won't merge it as is.

Mattias, please take a breath and think again about the comments David did.

We support the move to CC memcpy.
The CC memcpy was already used for some arches.
We agree having it as an option for all arches is a good step forward.
Having a compilation define to do some benchmark on any arch is good.
When it will become good enough, we will enable it on more arches by default.

I think it looks very reasonable, we just ask you to prepare the future
without breaking anything for now.
Then it will be very simple patches to decide enabling it more.
And because we want CC memcpy to become the only option,
it does not make sense to introduce a new option for the user.