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

[David Marchand <david.marchand@redhat.com>]

Hello,

On Tue, Aug 15, 2023 at 8:10 AM Stanisław Kardach <kda@semihalf.com> wrote:
> On Fri, Aug 11, 2023, 08:08 Sivaprasad Tummala <sivaprasad.tummala@amd.com> wrote:
>> diff --git a/lib/eal/arm/include/rte_cpuflags_32.h b/lib/eal/arm/include/rte_cpuflags_32.h
>> index 4e254428a2..41ab0d5f21 100644
>> --- a/lib/eal/arm/include/rte_cpuflags_32.h
>> +++ b/lib/eal/arm/include/rte_cpuflags_32.h
>> @@ -43,7 +43,6 @@ enum rte_cpu_flag_t {
>>         RTE_CPUFLAG_V7L,
>>         RTE_CPUFLAG_V8L,
>>         /* The last item */
>> -       RTE_CPUFLAG_NUMFLAGS,/**< This should always be the last! */
>>  };
>
> Since there is no description of V1 to V2 changes, could you point me to what has changed?

Siva? I did not see a reply on this question.


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

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/

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?


-- 
David Marchand