RE: [PATCH v2 2/2] eal: remove NUMFLAGS enumeration

["Tummala, Sivaprasad" <Sivaprasad.Tummala@amd.com>]

[AMD Official Use Only - General]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, September 20, 2023 1:05 PM
> To: Stanisław Kardach <kda@semihalf.com>; Tummala, Sivaprasad
> <Sivaprasad.Tummala@amd.com>
> Cc: Ruifeng Wang <ruifeng.wang@arm.com>; Min Zhou <zhoumin@loongson.cn>;
> David Christensen <drc@linux.vnet.ibm.com>; Bruce Richardson
> <bruce.richardson@intel.com>; Konstantin Ananyev
> <konstantin.v.ananyev@yandex.ru>; dev <dev@dpdk.org>; Yigit, Ferruh
> <Ferruh.Yigit@amd.com>; Thomas Monjalon <thomas@monjalon.net>
> Subject: Re: [PATCH v2 2/2] eal: remove NUMFLAGS enumeration
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Wed, Sep 20, 2023 at 8:01 AM Stanisław Kardach <kda@semihalf.com> wrote:
> >
> > On Tue, Sep 19, 2023 at 4:47 PM David Marchand
> <david.marchand@redhat.com> wrote:
> > <snip>
> > > > Also I see you're still removing the RTE_CPUFLAG_NUMFLAGS (what I call a
> last element canary). Why? If you're concerned with ABI, then we're talking about
> an application linking dynamically with DPDK or talking via some RPC channel with
> another DPDK application. So clashing with this definition does not come into
> question. One should rather use rte_cpu_get_flag_enabled().
> > > > Also if you want to introduce new features, one would add them yo the
> rte_cpuflags headers, unless you'd like to not add those and keep an
> undocumented list "above" the last defined element.
> > > > Could you explain a bit more Your use-case?
> > >
> > > Hey Stanislaw,
> > >
> > > Talking generically, one problem with such pattern (having a LAST,
> > > or MAX enum) is when an array sized with such a symbol is exposed.
> > > As I mentionned in the past, this can have unwanted effects:
> > > https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493
> > > -1-david.marchand@redhat.com/
>
> Argh... who broke copy/paste in my browser ?!
> Wrt to MAX and arrays, I wanted to point at:
> http://inbox.dpdk.org/dev/CAJFAV8xs5CVdE2xwRtaxk5vE_PiQMV5LY5tKStk3R1gOuR
> TsUw@mail.gmail.com/
>
> > I agree, though I'd argue "LAST" and "MAX" semantics are a bit different. "LAST"
> delimits the known enumeration territory while "MAX" is more of a `constepxr`
> value type.
> > >
> > > Another issue is when an existing enum meaning changes: from the
> > > application pov, the (old) MAX value is incorrect, but for the
> > > library pov, a new meaning has been associated.
> > > This may trigger bugs in the application when calling a function
> > > that returns such an enum which never return this MAX value in the past.
> > >
> > > For at least those two reasons, removing those canary elements is
> > > being done in DPDK.
> > >
> > > This specific removal has been announced:
> > > https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493
> > > -1-david.marchand@redhat.com/
> > Thanks for pointing this out but did you mean to link to the patch again here?
>
> Sorry, same here, bad copy/paste :-(.
>
> The intended link is: https://git.dpdk.org/dpdk/commit/?id=5da7c13521
> The deprecation notice was badly formulated and this patch here is consistent with
> it.
>
>
> > >
> > > Now, practically, when I look at the cpuflags API, I don't see us
> > > exposed to those two issues wrt rte_cpu_flag_t, so maybe this change
> > > is unneeded.
> > > But on the other hand, is it really an issue for an application to
> > > lose this (internal) information?
> > I doubt it, maybe it could be used as a sanity check for choosing proper functors
> in the application. Though the initial description of the reason behind this patch was
> to not break the ABI and I don't think it does that. What it does is enforces users to
> use explicit cpu flag values which is a good thing. Though if so, then it should be
> stated in the commit description.
>
> I agree.
> Siva, can you work on a new revision?
>
David, Stanislaw,

The original motivation of this patch was to avoid ABI breakage with the introduction of new CPU flag
"RTE_CPUFLAG_MONITORX" (http://mails.dpdk.org/archives/test-report/2023-April/382489.html).

Because of ABI breakage, the feature was postponed to this release.
https://patchwork.dpdk.org/project/dpdk/patch/20230413115334.43172-3-sivaprasad.tummala@amd.com/

Can you please add what exactly needs to be reworked in the new version.

>
> Thanks.
>
> --
> David Marchand