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

["Stanisław Kardach" <kda@semihalf.com>]

[-- Attachment #1: Type: text/plain, Size: 5540 bytes --]

On Thu, Sep 21, 2023, 15:18 Tummala, Sivaprasad <Sivaprasad.Tummala@amd.com>
wrote:

> [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/

This test is flawed, reason being that the NUMFLAGS should not be treated
as a flag value and instead as a canary but this test is not taking into
account.
Your change did not break the ABI because you have properly added the new
flag at the end.
So I would ask to change the commit description to mention that NUMFLAGS is
removed to:
1. Prevent users from treating it as a usable value or an array size.
2. Prevent false-positive failures in the ABI test.

Also it would be good to link to the aforementioned ABI test failure to
give readers some context when inspecting the git tree.

>
>
> Can you please add what exactly needs to be reworked in the new version.
>
> >
> > Thanks.
> >
> > --
> > David Marchand
>
>

[-- Attachment #2: Type: text/html, Size: 8942 bytes --]