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/
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?
>
> 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.