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_piqmv5ly5tkstk3r1gourt...@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? Thanks. -- David Marchand
