[AMD Official Use Only - General] > -----Original Message----- > From: David Marchand <david.march...@redhat.com> > Sent: Wednesday, September 20, 2023 1:05 PM > To: Stanisław Kardach <k...@semihalf.com>; Tummala, Sivaprasad > <sivaprasad.tumm...@amd.com> > Cc: Ruifeng Wang <ruifeng.w...@arm.com>; Min Zhou <zhou...@loongson.cn>; > David Christensen <d...@linux.vnet.ibm.com>; Bruce Richardson > <bruce.richard...@intel.com>; Konstantin Ananyev > <konstantin.v.anan...@yandex.ru>; dev <dev@dpdk.org>; Yigit, Ferruh > <ferruh.yi...@amd.com>; Thomas Monjalon <tho...@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 <k...@semihalf.com> wrote: > > > > On Tue, Sep 19, 2023 at 4:47 PM David Marchand > <david.march...@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.march...@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 > t...@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.march...@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.tumm...@amd.com/ Can you please add what exactly needs to be reworked in the new version. > > Thanks. > > -- > David Marchand
