[v2,2/2] eal: remove NUMFLAGS enumeration
Checks
Commit Message
This patch removes RTE_CPUFLAG_NUMFLAGS to allow new CPU
features without breaking ABI each time.
Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
---
lib/eal/arm/include/rte_cpuflags_32.h | 1 -
lib/eal/arm/include/rte_cpuflags_64.h | 1 -
lib/eal/arm/rte_cpuflags.c | 7 +++++--
lib/eal/loongarch/include/rte_cpuflags.h | 1 -
lib/eal/loongarch/rte_cpuflags.c | 7 +++++--
lib/eal/ppc/include/rte_cpuflags.h | 1 -
lib/eal/ppc/rte_cpuflags.c | 7 +++++--
lib/eal/riscv/include/rte_cpuflags.h | 1 -
lib/eal/riscv/rte_cpuflags.c | 7 +++++--
lib/eal/x86/include/rte_cpuflags.h | 1 -
lib/eal/x86/rte_cpuflags.c | 7 +++++--
11 files changed, 25 insertions(+), 16 deletions(-)
Comments
On Fri, Aug 11, 2023, 08:08 Sivaprasad Tummala <sivaprasad.tummala@amd.com>
wrote:
> This patch removes RTE_CPUFLAG_NUMFLAGS to allow new CPU
> features without breaking ABI each time.
>
> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
> ---
> lib/eal/arm/include/rte_cpuflags_32.h | 1 -
> lib/eal/arm/include/rte_cpuflags_64.h | 1 -
> lib/eal/arm/rte_cpuflags.c | 7 +++++--
> lib/eal/loongarch/include/rte_cpuflags.h | 1 -
> lib/eal/loongarch/rte_cpuflags.c | 7 +++++--
> lib/eal/ppc/include/rte_cpuflags.h | 1 -
> lib/eal/ppc/rte_cpuflags.c | 7 +++++--
> lib/eal/riscv/include/rte_cpuflags.h | 1 -
> lib/eal/riscv/rte_cpuflags.c | 7 +++++--
> lib/eal/x86/include/rte_cpuflags.h | 1 -
> lib/eal/x86/rte_cpuflags.c | 7 +++++--
> 11 files changed, 25 insertions(+), 16 deletions(-)
>
> 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?
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?
>
> #include "generic/rte_cpuflags.h"
> diff --git a/lib/eal/arm/include/rte_cpuflags_64.h
> b/lib/eal/arm/include/rte_cpuflags_64.h
> index aa7a56d491..ea5193e510 100644
> --- a/lib/eal/arm/include/rte_cpuflags_64.h
> +++ b/lib/eal/arm/include/rte_cpuflags_64.h
> @@ -37,7 +37,6 @@ enum rte_cpu_flag_t {
> RTE_CPUFLAG_SVEBF16,
> RTE_CPUFLAG_AARCH64,
> /* The last item */
> - RTE_CPUFLAG_NUMFLAGS,/**< This should always be the last! */
> };
>
> #include "generic/rte_cpuflags.h"
> diff --git a/lib/eal/arm/rte_cpuflags.c b/lib/eal/arm/rte_cpuflags.c
> index 56e7b2e689..f33fee242b 100644
> --- a/lib/eal/arm/rte_cpuflags.c
> +++ b/lib/eal/arm/rte_cpuflags.c
> @@ -139,8 +139,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
> {
> const struct feature_entry *feat;
> hwcap_registers_t regs = {0};
> + unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
>
> - if (feature >= RTE_CPUFLAG_NUMFLAGS)
> + if ((unsigned int)feature >= num_flags)
> return -ENOENT;
>
> feat = &rte_cpu_feature_table[feature];
> @@ -154,7 +155,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
> const char *
> rte_cpu_get_flag_name(enum rte_cpu_flag_t feature)
> {
> - if (feature >= RTE_CPUFLAG_NUMFLAGS)
> + unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
> +
> + if ((unsigned int)feature >= num_flags)
> return NULL;
> return rte_cpu_feature_table[feature].name;
> }
> diff --git a/lib/eal/loongarch/include/rte_cpuflags.h
> b/lib/eal/loongarch/include/rte_cpuflags.h
> index 1c80779262..9ff8baaa3c 100644
> --- a/lib/eal/loongarch/include/rte_cpuflags.h
> +++ b/lib/eal/loongarch/include/rte_cpuflags.h
> @@ -27,7 +27,6 @@ enum rte_cpu_flag_t {
> RTE_CPUFLAG_LBT_ARM,
> RTE_CPUFLAG_LBT_MIPS,
> /* The last item */
> - RTE_CPUFLAG_NUMFLAGS /**< This should always be the last! */
> };
>
> #include "generic/rte_cpuflags.h"
> diff --git a/lib/eal/loongarch/rte_cpuflags.c
> b/lib/eal/loongarch/rte_cpuflags.c
> index 0a75ca58d4..73b53b8a3a 100644
> --- a/lib/eal/loongarch/rte_cpuflags.c
> +++ b/lib/eal/loongarch/rte_cpuflags.c
> @@ -66,8 +66,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
> {
> const struct feature_entry *feat;
> hwcap_registers_t regs = {0};
> + unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
>
> - if (feature >= RTE_CPUFLAG_NUMFLAGS)
> + if ((unsigned int)feature >= num_flags)
> return -ENOENT;
>
> feat = &rte_cpu_feature_table[feature];
> @@ -81,7 +82,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
> const char *
> rte_cpu_get_flag_name(enum rte_cpu_flag_t feature)
> {
> - if (feature >= RTE_CPUFLAG_NUMFLAGS)
> + unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
> +
> + if ((unsigned int)feature >= num_flags)
> return NULL;
> return rte_cpu_feature_table[feature].name;
> }
> diff --git a/lib/eal/ppc/include/rte_cpuflags.h
> b/lib/eal/ppc/include/rte_cpuflags.h
> index a88355d170..b74e7a73ee 100644
> --- a/lib/eal/ppc/include/rte_cpuflags.h
> +++ b/lib/eal/ppc/include/rte_cpuflags.h
> @@ -49,7 +49,6 @@ enum rte_cpu_flag_t {
> RTE_CPUFLAG_HTM,
> RTE_CPUFLAG_ARCH_2_07,
> /* The last item */
> - RTE_CPUFLAG_NUMFLAGS,/**< This should always be the last! */
> };
>
> #include "generic/rte_cpuflags.h"
> diff --git a/lib/eal/ppc/rte_cpuflags.c b/lib/eal/ppc/rte_cpuflags.c
> index 61db5c216d..a173c62631 100644
> --- a/lib/eal/ppc/rte_cpuflags.c
> +++ b/lib/eal/ppc/rte_cpuflags.c
> @@ -90,8 +90,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
> {
> const struct feature_entry *feat;
> hwcap_registers_t regs = {0};
> + unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
>
> - if (feature >= RTE_CPUFLAG_NUMFLAGS)
> + if ((unsigned int)feature >= num_flags)
> return -ENOENT;
>
> feat = &rte_cpu_feature_table[feature];
> @@ -105,7 +106,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
> const char *
> rte_cpu_get_flag_name(enum rte_cpu_flag_t feature)
> {
> - if (feature >= RTE_CPUFLAG_NUMFLAGS)
> + unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
> +
> + if ((unsigned int)feature >= num_flags)
> return NULL;
> return rte_cpu_feature_table[feature].name;
> }
> diff --git a/lib/eal/riscv/include/rte_cpuflags.h
> b/lib/eal/riscv/include/rte_cpuflags.h
> index 66e787f898..803c3655ae 100644
> --- a/lib/eal/riscv/include/rte_cpuflags.h
> +++ b/lib/eal/riscv/include/rte_cpuflags.h
> @@ -43,7 +43,6 @@ enum rte_cpu_flag_t {
> RTE_CPUFLAG_RISCV_ISA_Y, /* Reserved */
> RTE_CPUFLAG_RISCV_ISA_Z, /* Reserved */
> /* The last item */
> - RTE_CPUFLAG_NUMFLAGS,/**< This should always be the last! */
> };
>
> #include "generic/rte_cpuflags.h"
> diff --git a/lib/eal/riscv/rte_cpuflags.c b/lib/eal/riscv/rte_cpuflags.c
> index 4f6d29b947..6d3f8f16cc 100644
> --- a/lib/eal/riscv/rte_cpuflags.c
> +++ b/lib/eal/riscv/rte_cpuflags.c
> @@ -95,8 +95,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
> {
> const struct feature_entry *feat;
> hwcap_registers_t regs = {0};
> + unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
>
> - if (feature >= RTE_CPUFLAG_NUMFLAGS)
> + if ((unsigned int)feature >= num_flags)
> return -ENOENT;
>
> feat = &rte_cpu_feature_table[feature];
> @@ -110,7 +111,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
> const char *
> rte_cpu_get_flag_name(enum rte_cpu_flag_t feature)
> {
> - if (feature >= RTE_CPUFLAG_NUMFLAGS)
> + unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
> +
> + if ((unsigned int)feature >= num_flags)
> return NULL;
> return rte_cpu_feature_table[feature].name;
> }
> diff --git a/lib/eal/x86/include/rte_cpuflags.h
> b/lib/eal/x86/include/rte_cpuflags.h
> index 92e90fb6e0..7fc6117243 100644
> --- a/lib/eal/x86/include/rte_cpuflags.h
> +++ b/lib/eal/x86/include/rte_cpuflags.h
> @@ -135,7 +135,6 @@ enum rte_cpu_flag_t {
> RTE_CPUFLAG_WAITPKG, /**< UMONITOR/UMWAIT/TPAUSE */
>
> /* The last item */
> - RTE_CPUFLAG_NUMFLAGS, /**< This should always be the
> last! */
> };
>
> #include "generic/rte_cpuflags.h"
> diff --git a/lib/eal/x86/rte_cpuflags.c b/lib/eal/x86/rte_cpuflags.c
> index d6b518251b..22061cb6d3 100644
> --- a/lib/eal/x86/rte_cpuflags.c
> +++ b/lib/eal/x86/rte_cpuflags.c
> @@ -149,8 +149,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
> const struct feature_entry *feat;
> cpuid_registers_t regs;
> unsigned int maxleaf;
> + unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
>
> - if (feature >= RTE_CPUFLAG_NUMFLAGS)
> + if ((unsigned int)feature >= num_flags)
> /* Flag does not match anything in the feature tables */
> return -ENOENT;
>
> @@ -176,7 +177,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
> const char *
> rte_cpu_get_flag_name(enum rte_cpu_flag_t feature)
> {
> - if (feature >= RTE_CPUFLAG_NUMFLAGS)
> + unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
> +
> + if ((unsigned int)feature >= num_flags)
> return NULL;
> return rte_cpu_feature_table[feature].name;
> }
> --
> 2.34.1
>
>
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?
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.
>
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_PiQMV5LY5tKStk3R1gOuRTsUw@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?
Thanks.
[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/
Can you please add what exactly needs to be reworked in the new version.
>
> Thanks.
>
> --
> David Marchand
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
>
>
On 9/21/2023 3:49 PM, Stanisław Kardach wrote:
> On Thu, Sep 21, 2023, 15:18 Tummala, Sivaprasad
> <Sivaprasad.Tummala@amd.com <mailto:Sivaprasad.Tummala@amd.com>> wrote:
>
> [AMD Official Use Only - General]
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com
> <mailto:david.marchand@redhat.com>>
> > Sent: Wednesday, September 20, 2023 1:05 PM
> > To: Stanisław Kardach <kda@semihalf.com
> <mailto:kda@semihalf.com>>; Tummala, Sivaprasad
> > <Sivaprasad.Tummala@amd.com <mailto:Sivaprasad.Tummala@amd.com>>
> > Cc: Ruifeng Wang <ruifeng.wang@arm.com
> <mailto:ruifeng.wang@arm.com>>; Min Zhou <zhoumin@loongson.cn
> <mailto:zhoumin@loongson.cn>>;
> > David Christensen <drc@linux.vnet.ibm.com
> <mailto:drc@linux.vnet.ibm.com>>; Bruce Richardson
> > <bruce.richardson@intel.com <mailto:bruce.richardson@intel.com>>;
> Konstantin Ananyev
> > <konstantin.v.ananyev@yandex.ru
> <mailto:konstantin.v.ananyev@yandex.ru>>; dev <dev@dpdk.org
> <mailto:dev@dpdk.org>>; Yigit, Ferruh
> > <Ferruh.Yigit@amd.com <mailto:Ferruh.Yigit@amd.com>>; Thomas
> Monjalon <thomas@monjalon.net <mailto: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 <mailto:kda@semihalf.com>> wrote:
> > >
> > > On Tue, Sep 19, 2023 at 4:47 PM David Marchand
> > <david.marchand@redhat.com <mailto: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
> <https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493>
> > > > -1-david.marchand@redhat.com/
> <http://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 <http://inbox.dpdk.org/dev/CAJFAV8xs5CVdE2xwRtaxk5vE_PiQMV5LY5tKStk3R1gOuR>
> > TsUw@mail.gmail.com/ <http://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
> <https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493>
> > > > -1-david.marchand@redhat.com/
> <http://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
> <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
> <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/ <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.
>
Hi Stanislaw,
Why test is flawed?
The enum in in the public header, so the 'RTE_CPUFLAG_NUMFLAGS' enum
item, and there are APIs using the enum, so the enum exchanged between
shared library and the application.
Similar thing discussed before and when enum exchanged between
application and shared library, there is an ABI breakage risk when enum
extended and general tendency is to eliminate the MAX value to reduce
the risk.
When enum value sent from library to application, it is more clear that
this can cause an ABI breakage, because application can receive a value
that it is not aware in the build time, which can cause unexpected behavior.
Simply think about a case application allocated array in
'RTE_CPUFLAG_NUMFLAGS' size and directly accessing the array index based
on returned enum item value, if the enum extended in the new version of
the shared library, this can cause invalid memory access in application.
When enum value sent from application to library, I am not quite sure
how problematic it is to be honest. Like being in the
'rte_cpu_get_flag_enabled()' & 'rte_cpu_get_flag_name()' in question.
Only when application sends 'RTE_CPUFLAG_NUMFLAGS' to
'rte_cpu_get_flag_name()', it expects a NULL returned, but this won't
happen in new version of the shared library, not sure if this can cause
any problem for the application.
But as I mentioned, general guidance is to eliminate this kind of MAX
enum value usage.
And for this specific issue, although usage of the enum in
'rte_cpu_get_flag_enabled()' & 'rte_cpu_get_flag_name()' APIs is not
clear if it cause ABI breakage,
enum being embedded into the 'struct rte_bbdev_driver_info' struct
doesn't leave a question, since this struct is returned from library to
the application and change in the enum causes an ABI breakage.
Briefly, I think even appending to the end of 'enum rte_cpu_flag_t'
cause ABI breakage and removing 'RTE_CPUFLAG_NUMFLAGS' helps to extend
this enum in the future.
And an outstanding deprecation notice already exists for this:
https://git.dpdk.org/dpdk/tree/doc/guides/rel_notes/deprecation.rst?h=v23.07#n63
> 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
>
On 8/11/2023 7:07 AM, Sivaprasad Tummala wrote:
> This patch removes RTE_CPUFLAG_NUMFLAGS to allow new CPU
> features without breaking ABI each time.
>
> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
> ---
> lib/eal/arm/include/rte_cpuflags_32.h | 1 -
> lib/eal/arm/include/rte_cpuflags_64.h | 1 -
> lib/eal/arm/rte_cpuflags.c | 7 +++++--
> lib/eal/loongarch/include/rte_cpuflags.h | 1 -
> lib/eal/loongarch/rte_cpuflags.c | 7 +++++--
> lib/eal/ppc/include/rte_cpuflags.h | 1 -
> lib/eal/ppc/rte_cpuflags.c | 7 +++++--
> lib/eal/riscv/include/rte_cpuflags.h | 1 -
> lib/eal/riscv/rte_cpuflags.c | 7 +++++--
> lib/eal/x86/include/rte_cpuflags.h | 1 -
> lib/eal/x86/rte_cpuflags.c | 7 +++++--
> 11 files changed, 25 insertions(+), 16 deletions(-)
>
Hi Sivaprasad,
Is the relevant deprecation notice removed somewhere, if not can you
please remove it in this set?
Thanks,
ferruh
On Wed, Sep 27, 2023 at 1:55 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 9/21/2023 3:49 PM, Stanisław Kardach wrote:
> > On Thu, Sep 21, 2023, 15:18 Tummala, Sivaprasad
> > <Sivaprasad.Tummala@amd.com <mailto:Sivaprasad.Tummala@amd.com>> wrote:
> >
> > [AMD Official Use Only - General]
> >
> > > -----Original Message-----
> > > From: David Marchand <david.marchand@redhat.com
> > <mailto:david.marchand@redhat.com>>
> > > Sent: Wednesday, September 20, 2023 1:05 PM
> > > To: Stanisław Kardach <kda@semihalf.com
> > <mailto:kda@semihalf.com>>; Tummala, Sivaprasad
> > > <Sivaprasad.Tummala@amd.com <mailto:Sivaprasad.Tummala@amd.com>>
> > > Cc: Ruifeng Wang <ruifeng.wang@arm.com
> > <mailto:ruifeng.wang@arm.com>>; Min Zhou <zhoumin@loongson.cn
> > <mailto:zhoumin@loongson.cn>>;
> > > David Christensen <drc@linux.vnet.ibm.com
> > <mailto:drc@linux.vnet.ibm.com>>; Bruce Richardson
> > > <bruce.richardson@intel.com <mailto:bruce.richardson@intel.com>>;
> > Konstantin Ananyev
> > > <konstantin.v.ananyev@yandex.ru
> > <mailto:konstantin.v.ananyev@yandex.ru>>; dev <dev@dpdk.org
> > <mailto:dev@dpdk.org>>; Yigit, Ferruh
> > > <Ferruh.Yigit@amd.com <mailto:Ferruh.Yigit@amd.com>>; Thomas
> > Monjalon <thomas@monjalon.net <mailto: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 <mailto:kda@semihalf.com>> wrote:
> > > >
> > > > On Tue, Sep 19, 2023 at 4:47 PM David Marchand
> > > <david.marchand@redhat.com <mailto: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
> > <https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493>
> > > > > -1-david.marchand@redhat.com/
> > <http://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 <http://inbox.dpdk.org/dev/CAJFAV8xs5CVdE2xwRtaxk5vE_PiQMV5LY5tKStk3R1gOuR>
> > > TsUw@mail.gmail.com/ <http://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
> > <https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493>
> > > > > -1-david.marchand@redhat.com/
> > <http://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
> > <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
> > <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/ <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.
> >
>
> Hi Stanislaw,
>
> Why test is flawed?
>
> The enum in in the public header, so the 'RTE_CPUFLAG_NUMFLAGS' enum
> item, and there are APIs using the enum, so the enum exchanged between
> shared library and the application.
In a similar way lots of Linux uapi headers contain bits that should
not be used directly, even though they are defined there. The reason
for that is the C language syntax, not necessarily the intent of a
developer.
Since NUMFLAGS was a canary to make the flag handling code easier, it
should not be treated as a "real" value and hence my suggestion of a
flawed test. That said, NUMFLAGS does not bring enough value to not
remove it. :)
>
> Similar thing discussed before and when enum exchanged between
> application and shared library, there is an ABI breakage risk when enum
> extended and general tendency is to eliminate the MAX value to reduce
> the risk.
Agreed though as I have mentioned before, "MAX" has a different
semantics than "NUM". Then again since we have rte_cpu_feature_table,
we can RTE_DIM to check the user input.
>
>
> When enum value sent from library to application, it is more clear that
> this can cause an ABI breakage, because application can receive a value
> that it is not aware in the build time, which can cause unexpected behavior.
> Simply think about a case application allocated array in
> 'RTE_CPUFLAG_NUMFLAGS' size and directly accessing the array index based
> on returned enum item value, if the enum extended in the new version of
> the shared library, this can cause invalid memory access in application.
Using the NUM enum element (which serves as a last item canary) to
size an array is not a good idea unless it's returned from a runtime
call. Otherwise one hits issues that you've described.
>
> When enum value sent from application to library, I am not quite sure
> how problematic it is to be honest. Like being in the
> 'rte_cpu_get_flag_enabled()' & 'rte_cpu_get_flag_name()' in question.
> Only when application sends 'RTE_CPUFLAG_NUMFLAGS' to
> 'rte_cpu_get_flag_name()', it expects a NULL returned, but this won't
> happen in new version of the shared library, not sure if this can cause
> any problem for the application.
> But as I mentioned, general guidance is to eliminate this kind of MAX
> enum value usage.
>
>
> And for this specific issue, although usage of the enum in
> 'rte_cpu_get_flag_enabled()' & 'rte_cpu_get_flag_name()' APIs is not
> clear if it cause ABI breakage,
> enum being embedded into the 'struct rte_bbdev_driver_info' struct
> doesn't leave a question, since this struct is returned from library to
> the application and change in the enum causes an ABI breakage.
Enum size does not change irrespective of changing its values. So
size-wise it's not an ABI breakage. Re-ordering values is an ABI
breakage.
>
>
> Briefly, I think even appending to the end of 'enum rte_cpu_flag_t'
> cause ABI breakage and removing 'RTE_CPUFLAG_NUMFLAGS' helps to extend
> this enum in the future.
> And an outstanding deprecation notice already exists for this:
> https://git.dpdk.org/dpdk/tree/doc/guides/rel_notes/deprecation.rst?h=v23.07#n63
>
>
> > 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
> >
>
On 9/27/2023 2:48 PM, Stanisław Kardach wrote:
> On Wed, Sep 27, 2023 at 1:55 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>
>> On 9/21/2023 3:49 PM, Stanisław Kardach wrote:
>>> On Thu, Sep 21, 2023, 15:18 Tummala, Sivaprasad
>>> <Sivaprasad.Tummala@amd.com <mailto:Sivaprasad.Tummala@amd.com>> wrote:
>>>
>>> [AMD Official Use Only - General]
>>>
>>> > -----Original Message-----
>>> > From: David Marchand <david.marchand@redhat.com
>>> <mailto:david.marchand@redhat.com>>
>>> > Sent: Wednesday, September 20, 2023 1:05 PM
>>> > To: Stanisław Kardach <kda@semihalf.com
>>> <mailto:kda@semihalf.com>>; Tummala, Sivaprasad
>>> > <Sivaprasad.Tummala@amd.com <mailto:Sivaprasad.Tummala@amd.com>>
>>> > Cc: Ruifeng Wang <ruifeng.wang@arm.com
>>> <mailto:ruifeng.wang@arm.com>>; Min Zhou <zhoumin@loongson.cn
>>> <mailto:zhoumin@loongson.cn>>;
>>> > David Christensen <drc@linux.vnet.ibm.com
>>> <mailto:drc@linux.vnet.ibm.com>>; Bruce Richardson
>>> > <bruce.richardson@intel.com <mailto:bruce.richardson@intel.com>>;
>>> Konstantin Ananyev
>>> > <konstantin.v.ananyev@yandex.ru
>>> <mailto:konstantin.v.ananyev@yandex.ru>>; dev <dev@dpdk.org
>>> <mailto:dev@dpdk.org>>; Yigit, Ferruh
>>> > <Ferruh.Yigit@amd.com <mailto:Ferruh.Yigit@amd.com>>; Thomas
>>> Monjalon <thomas@monjalon.net <mailto: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 <mailto:kda@semihalf.com>> wrote:
>>> > >
>>> > > On Tue, Sep 19, 2023 at 4:47 PM David Marchand
>>> > <david.marchand@redhat.com <mailto: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
>>> <https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493>
>>> > > > -1-david.marchand@redhat.com/
>>> <http://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 <http://inbox.dpdk.org/dev/CAJFAV8xs5CVdE2xwRtaxk5vE_PiQMV5LY5tKStk3R1gOuR>
>>> > TsUw@mail.gmail.com/ <http://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
>>> <https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493>
>>> > > > -1-david.marchand@redhat.com/
>>> <http://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
>>> <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
>>> <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/ <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.
>>>
>>
>> Hi Stanislaw,
>>
>> Why test is flawed?
>>
>> The enum in in the public header, so the 'RTE_CPUFLAG_NUMFLAGS' enum
>> item, and there are APIs using the enum, so the enum exchanged between
>> shared library and the application.
> In a similar way lots of Linux uapi headers contain bits that should
> not be used directly, even though they are defined there. The reason
> for that is the C language syntax, not necessarily the intent of a
> developer.
> Since NUMFLAGS was a canary to make the flag handling code easier, it
> should not be treated as a "real" value and hence my suggestion of a
> flawed test. That said, NUMFLAGS does not bring enough value to not
> remove it. :)
>
Both it doesn't enough value to hang on, and we don't have control on
how it is used by the application once it is exposed by the library.
>>
>> Similar thing discussed before and when enum exchanged between
>> application and shared library, there is an ABI breakage risk when enum
>> extended and general tendency is to eliminate the MAX value to reduce
>> the risk.
> Agreed though as I have mentioned before, "MAX" has a different
> semantics than "NUM". Then again since we have rte_cpu_feature_table,
> we can RTE_DIM to check the user input.
>
Their usage and intention on having them is same I think, can you please
elaborate what is the difference between MAX and NUM enum items that is
added as last item in an enum?
>>
>>
>> When enum value sent from library to application, it is more clear that
>> this can cause an ABI breakage, because application can receive a value
>> that it is not aware in the build time, which can cause unexpected behavior.
>> Simply think about a case application allocated array in
>> 'RTE_CPUFLAG_NUMFLAGS' size and directly accessing the array index based
>> on returned enum item value, if the enum extended in the new version of
>> the shared library, this can cause invalid memory access in application.
> Using the NUM enum element (which serves as a last item canary) to
> size an array is not a good idea unless it's returned from a runtime
> call. Otherwise one hits issues that you've described.
>
I agree :), but that is a way to describe how it can be a problem.
Also last time I argued similar to what you said, that application
should check against MAX value before using it but I have been told
not to assume what application does. My take from it is, expect worst
from application as a library side developer.
>>
>> When enum value sent from application to library, I am not quite sure
>> how problematic it is to be honest. Like being in the
>> 'rte_cpu_get_flag_enabled()' & 'rte_cpu_get_flag_name()' in question.
>> Only when application sends 'RTE_CPUFLAG_NUMFLAGS' to
>> 'rte_cpu_get_flag_name()', it expects a NULL returned, but this won't
>> happen in new version of the shared library, not sure if this can cause
>> any problem for the application.
>> But as I mentioned, general guidance is to eliminate this kind of MAX
>> enum value usage.
>>
>>
>> And for this specific issue, although usage of the enum in
>> 'rte_cpu_get_flag_enabled()' & 'rte_cpu_get_flag_name()' APIs is not
>> clear if it cause ABI breakage,
>> enum being embedded into the 'struct rte_bbdev_driver_info' struct
>> doesn't leave a question, since this struct is returned from library to
>> the application and change in the enum causes an ABI breakage.
> Enum size does not change irrespective of changing its values. So
> size-wise it's not an ABI breakage. Re-ordering values is an ABI
> breakage.>
Agree it is not size-wise issue. But still an issue.
>>
>>
>> Briefly, I think even appending to the end of 'enum rte_cpu_flag_t'
>> cause ABI breakage and removing 'RTE_CPUFLAG_NUMFLAGS' helps to extend
>> this enum in the future.
>> And an outstanding deprecation notice already exists for this:
>> https://git.dpdk.org/dpdk/tree/doc/guides/rel_notes/deprecation.rst?h=v23.07#n63
>>
>>
>>> 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
>>>
>>
>
>
On Wed, Sep 27, 2023, 16:09 Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> On 9/27/2023 2:48 PM, Stanisław Kardach wrote:
> > On Wed, Sep 27, 2023 at 1:55 PM Ferruh Yigit <ferruh.yigit@amd.com>
> wrote:
> >>
> >> On 9/21/2023 3:49 PM, Stanisław Kardach wrote:
> >>> On Thu, Sep 21, 2023, 15:18 Tummala, Sivaprasad
> >>> <Sivaprasad.Tummala@amd.com <mailto:Sivaprasad.Tummala@amd.com>>
> wrote:
> >>>
> >>> [AMD Official Use Only - General]
> >>>
> >>> > -----Original Message-----
> >>> > From: David Marchand <david.marchand@redhat.com
> >>> <mailto:david.marchand@redhat.com>>
> >>> > Sent: Wednesday, September 20, 2023 1:05 PM
> >>> > To: Stanisław Kardach <kda@semihalf.com
> >>> <mailto:kda@semihalf.com>>; Tummala, Sivaprasad
> >>> > <Sivaprasad.Tummala@amd.com <mailto:Sivaprasad.Tummala@amd.com>>
> >>> > Cc: Ruifeng Wang <ruifeng.wang@arm.com
> >>> <mailto:ruifeng.wang@arm.com>>; Min Zhou <zhoumin@loongson.cn
> >>> <mailto:zhoumin@loongson.cn>>;
> >>> > David Christensen <drc@linux.vnet.ibm.com
> >>> <mailto:drc@linux.vnet.ibm.com>>; Bruce Richardson
> >>> > <bruce.richardson@intel.com <mailto:bruce.richardson@intel.com
> >>;
> >>> Konstantin Ananyev
> >>> > <konstantin.v.ananyev@yandex.ru
> >>> <mailto:konstantin.v.ananyev@yandex.ru>>; dev <dev@dpdk.org
> >>> <mailto:dev@dpdk.org>>; Yigit, Ferruh
> >>> > <Ferruh.Yigit@amd.com <mailto:Ferruh.Yigit@amd.com>>; Thomas
> >>> Monjalon <thomas@monjalon.net <mailto: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 <mailto:kda@semihalf.com>> wrote:
> >>> > >
> >>> > > On Tue, Sep 19, 2023 at 4:47 PM David Marchand
> >>> > <david.marchand@redhat.com <mailto: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
> >>> <
> https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493>
> >>> > > > -1-david.marchand@redhat.com/
> >>> <http://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
> <http://inbox.dpdk.org/dev/CAJFAV8xs5CVdE2xwRtaxk5vE_PiQMV5LY5tKStk3R1gOuR
> >
> >>> > TsUw@mail.gmail.com/ <http://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
> >>> <
> https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493>
> >>> > > > -1-david.marchand@redhat.com/
> >>> <http://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
> >>> <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
> >>> <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/
> <
> 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.
> >>>
> >>
> >> Hi Stanislaw,
> >>
> >> Why test is flawed?
> >>
> >> The enum in in the public header, so the 'RTE_CPUFLAG_NUMFLAGS' enum
> >> item, and there are APIs using the enum, so the enum exchanged between
> >> shared library and the application.
> > In a similar way lots of Linux uapi headers contain bits that should
> > not be used directly, even though they are defined there. The reason
> > for that is the C language syntax, not necessarily the intent of a
> > developer.
> > Since NUMFLAGS was a canary to make the flag handling code easier, it
> > should not be treated as a "real" value and hence my suggestion of a
> > flawed test. That said, NUMFLAGS does not bring enough value to not
> > remove it. :)
> >
>
> Both it doesn't enough value to hang on, and we don't have control on
> how it is used by the application once it is exposed by the library.
>
>
> >>
> >> Similar thing discussed before and when enum exchanged between
> >> application and shared library, there is an ABI breakage risk when enum
> >> extended and general tendency is to eliminate the MAX value to reduce
> >> the risk.
> > Agreed though as I have mentioned before, "MAX" has a different
> > semantics than "NUM". Then again since we have rte_cpu_feature_table,
> > we can RTE_DIM to check the user input.
> >
>
> Their usage and intention on having them is same I think, can you please
> elaborate what is the difference between MAX and NUM enum items that is
> added as last item in an enum?
>
MAX specifies a semantic numerical value, such as MTU. NUM counts elements
in an enumeration where elements describe some items and their value is
just an implementation detail.
>
>
> >>
> >>
> >> When enum value sent from library to application, it is more clear that
> >> this can cause an ABI breakage, because application can receive a value
> >> that it is not aware in the build time, which can cause unexpected
> behavior.
> >> Simply think about a case application allocated array in
> >> 'RTE_CPUFLAG_NUMFLAGS' size and directly accessing the array index based
> >> on returned enum item value, if the enum extended in the new version of
> >> the shared library, this can cause invalid memory access in application.
> > Using the NUM enum element (which serves as a last item canary) to
> > size an array is not a good idea unless it's returned from a runtime
> > call. Otherwise one hits issues that you've described.
> >
>
> I agree :), but that is a way to describe how it can be a problem.
> Also last time I argued similar to what you said, that application
> should check against MAX value before using it but I have been told
> not to assume what application does. My take from it is, expect worst
> from application as a library side developer.
>
>
> >>
> >> When enum value sent from application to library, I am not quite sure
> >> how problematic it is to be honest. Like being in the
> >> 'rte_cpu_get_flag_enabled()' & 'rte_cpu_get_flag_name()' in question.
> >> Only when application sends 'RTE_CPUFLAG_NUMFLAGS' to
> >> 'rte_cpu_get_flag_name()', it expects a NULL returned, but this won't
> >> happen in new version of the shared library, not sure if this can cause
> >> any problem for the application.
> >> But as I mentioned, general guidance is to eliminate this kind of MAX
> >> enum value usage.
> >>
> >>
> >> And for this specific issue, although usage of the enum in
> >> 'rte_cpu_get_flag_enabled()' & 'rte_cpu_get_flag_name()' APIs is not
> >> clear if it cause ABI breakage,
> >> enum being embedded into the 'struct rte_bbdev_driver_info' struct
> >> doesn't leave a question, since this struct is returned from library to
> >> the application and change in the enum causes an ABI breakage.
> > Enum size does not change irrespective of changing its values. So
> > size-wise it's not an ABI breakage. Re-ordering values is an ABI
> > breakage.>
>
> Agree it is not size-wise issue. But still an issue.
>
>
> >>
> >>
> >> Briefly, I think even appending to the end of 'enum rte_cpu_flag_t'
> >> cause ABI breakage and removing 'RTE_CPUFLAG_NUMFLAGS' helps to extend
> >> this enum in the future.
> >> And an outstanding deprecation notice already exists for this:
> >>
> https://git.dpdk.org/dpdk/tree/doc/guides/rel_notes/deprecation.rst?h=v23.07#n63
> >>
> >>
> >>> 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
> >>>
> >>
> >
> >
>
>
On 9/27/2023 4:03 PM, Stanisław Kardach wrote:
>
>
> On Wed, Sep 27, 2023, 16:09 Ferruh Yigit <ferruh.yigit@amd.com
> <mailto:ferruh.yigit@amd.com>> wrote:
>
> On 9/27/2023 2:48 PM, Stanisław Kardach wrote:
> > On Wed, Sep 27, 2023 at 1:55 PM Ferruh Yigit <ferruh.yigit@amd.com
> <mailto:ferruh.yigit@amd.com>> wrote:
> >>
> >> On 9/21/2023 3:49 PM, Stanisław Kardach wrote:
> >>> On Thu, Sep 21, 2023, 15:18 Tummala, Sivaprasad
> >>> <Sivaprasad.Tummala@amd.com <mailto:Sivaprasad.Tummala@amd.com>
> <mailto:Sivaprasad.Tummala@amd.com
> <mailto:Sivaprasad.Tummala@amd.com>>> wrote:
> >>>
> >>> [AMD Official Use Only - General]
> >>>
> >>> > -----Original Message-----
> >>> > From: David Marchand <david.marchand@redhat.com
> <mailto:david.marchand@redhat.com>
> >>> <mailto:david.marchand@redhat.com
> <mailto:david.marchand@redhat.com>>>
> >>> > Sent: Wednesday, September 20, 2023 1:05 PM
> >>> > To: Stanisław Kardach <kda@semihalf.com
> <mailto:kda@semihalf.com>
> >>> <mailto:kda@semihalf.com <mailto:kda@semihalf.com>>>;
> Tummala, Sivaprasad
> >>> > <Sivaprasad.Tummala@amd.com
> <mailto:Sivaprasad.Tummala@amd.com>
> <mailto:Sivaprasad.Tummala@amd.com <mailto:Sivaprasad.Tummala@amd.com>>>
> >>> > Cc: Ruifeng Wang <ruifeng.wang@arm.com
> <mailto:ruifeng.wang@arm.com>
> >>> <mailto:ruifeng.wang@arm.com
> <mailto:ruifeng.wang@arm.com>>>; Min Zhou <zhoumin@loongson.cn
> <mailto:zhoumin@loongson.cn>
> >>> <mailto:zhoumin@loongson.cn <mailto:zhoumin@loongson.cn>>>;
> >>> > David Christensen <drc@linux.vnet.ibm.com
> <mailto:drc@linux.vnet.ibm.com>
> >>> <mailto:drc@linux.vnet.ibm.com
> <mailto:drc@linux.vnet.ibm.com>>>; Bruce Richardson
> >>> > <bruce.richardson@intel.com
> <mailto:bruce.richardson@intel.com>
> <mailto:bruce.richardson@intel.com
> <mailto:bruce.richardson@intel.com>>>;
> >>> Konstantin Ananyev
> >>> > <konstantin.v.ananyev@yandex.ru
> <mailto:konstantin.v.ananyev@yandex.ru>
> >>> <mailto:konstantin.v.ananyev@yandex.ru
> <mailto:konstantin.v.ananyev@yandex.ru>>>; dev <dev@dpdk.org
> <mailto:dev@dpdk.org>
> >>> <mailto:dev@dpdk.org <mailto:dev@dpdk.org>>>; Yigit, Ferruh
> >>> > <Ferruh.Yigit@amd.com <mailto:Ferruh.Yigit@amd.com>
> <mailto:Ferruh.Yigit@amd.com <mailto:Ferruh.Yigit@amd.com>>>; Thomas
> >>> Monjalon <thomas@monjalon.net <mailto:thomas@monjalon.net>
> <mailto:thomas@monjalon.net <mailto: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 <mailto:kda@semihalf.com>
> <mailto:kda@semihalf.com <mailto:kda@semihalf.com>>> wrote:
> >>> > >
> >>> > > On Tue, Sep 19, 2023 at 4:47 PM David Marchand
> >>> > <david.marchand@redhat.com
> <mailto:david.marchand@redhat.com> <mailto:david.marchand@redhat.com
> <mailto: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 <https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493>
> >>>
> <https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493 <https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493>>
> >>> > > > -1-david.marchand@redhat.com/
> <http://1-david.marchand@redhat.com/>
> >>> <http://1-david.marchand@redhat.com/
> <http://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 <http://inbox.dpdk.org/dev/CAJFAV8xs5CVdE2xwRtaxk5vE_PiQMV5LY5tKStk3R1gOuR> <http://inbox.dpdk.org/dev/CAJFAV8xs5CVdE2xwRtaxk5vE_PiQMV5LY5tKStk3R1gOuR <http://inbox.dpdk.org/dev/CAJFAV8xs5CVdE2xwRtaxk5vE_PiQMV5LY5tKStk3R1gOuR>>
> >>> > TsUw@mail.gmail.com/ <http://TsUw@mail.gmail.com/>
> <http://TsUw@mail.gmail.com/ <http://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 <https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493>
> >>>
> <https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493 <https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.3251493>>
> >>> > > > -1-david.marchand@redhat.com/
> <http://1-david.marchand@redhat.com/>
> >>> <http://1-david.marchand@redhat.com/
> <http://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
> <https://git.dpdk.org/dpdk/commit/?id=5da7c13521>
> >>> <https://git.dpdk.org/dpdk/commit/?id=5da7c13521
> <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
> <http://mails.dpdk.org/archives/test-report/2023-April/382489.html>
> >>>
> <http://mails.dpdk.org/archives/test-report/2023-April/382489.html
> <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/ <https://patchwork.dpdk.org/project/dpdk/patch/20230413115334.43172-3-sivaprasad.tummala@amd.com/> <https://patchwork.dpdk.org/project/dpdk/patch/20230413115334.43172-3-sivaprasad.tummala@amd.com/ <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.
> >>>
> >>
> >> Hi Stanislaw,
> >>
> >> Why test is flawed?
> >>
> >> The enum in in the public header, so the 'RTE_CPUFLAG_NUMFLAGS' enum
> >> item, and there are APIs using the enum, so the enum exchanged
> between
> >> shared library and the application.
> > In a similar way lots of Linux uapi headers contain bits that should
> > not be used directly, even though they are defined there. The reason
> > for that is the C language syntax, not necessarily the intent of a
> > developer.
> > Since NUMFLAGS was a canary to make the flag handling code easier, it
> > should not be treated as a "real" value and hence my suggestion of a
> > flawed test. That said, NUMFLAGS does not bring enough value to not
> > remove it. :)
> >
>
> Both it doesn't enough value to hang on, and we don't have control on
> how it is used by the application once it is exposed by the library.
>
>
> >>
> >> Similar thing discussed before and when enum exchanged between
> >> application and shared library, there is an ABI breakage risk
> when enum
> >> extended and general tendency is to eliminate the MAX value to reduce
> >> the risk.
> > Agreed though as I have mentioned before, "MAX" has a different
> > semantics than "NUM". Then again since we have rte_cpu_feature_table,
> > we can RTE_DIM to check the user input.
> >
>
> Their usage and intention on having them is same I think, can you please
> elaborate what is the difference between MAX and NUM enum items that is
> added as last item in an enum?
>
> MAX specifies a semantic numerical value, such as MTU. NUM counts
> elements in an enumeration where elements describe some items and their
> value is just an implementation detail.
>
>
Ahh, I see your point now.
Most of the times, in enums, last item with name MAX is indeed used as
count of elements more than maximum semantic value, and sometimes LAST
is used for the same. So we are using them interchangeably with NUM.
I agree, for a MAX value as you are referring semantics is different,
but the one mentioned in this context is not an instance for that case.
>
> >>
> >>
> >> When enum value sent from library to application, it is more
> clear that
> >> this can cause an ABI breakage, because application can receive a
> value
> >> that it is not aware in the build time, which can cause
> unexpected behavior.
> >> Simply think about a case application allocated array in
> >> 'RTE_CPUFLAG_NUMFLAGS' size and directly accessing the array
> index based
> >> on returned enum item value, if the enum extended in the new
> version of
> >> the shared library, this can cause invalid memory access in
> application.
> > Using the NUM enum element (which serves as a last item canary) to
> > size an array is not a good idea unless it's returned from a runtime
> > call. Otherwise one hits issues that you've described.
> >
>
> I agree :), but that is a way to describe how it can be a problem.
> Also last time I argued similar to what you said, that application
> should check against MAX value before using it but I have been told
> not to assume what application does. My take from it is, expect worst
> from application as a library side developer.
>
>
> >>
> >> When enum value sent from application to library, I am not quite sure
> >> how problematic it is to be honest. Like being in the
> >> 'rte_cpu_get_flag_enabled()' & 'rte_cpu_get_flag_name()' in question.
> >> Only when application sends 'RTE_CPUFLAG_NUMFLAGS' to
> >> 'rte_cpu_get_flag_name()', it expects a NULL returned, but this won't
> >> happen in new version of the shared library, not sure if this can
> cause
> >> any problem for the application.
> >> But as I mentioned, general guidance is to eliminate this kind of MAX
> >> enum value usage.
> >>
> >>
> >> And for this specific issue, although usage of the enum in
> >> 'rte_cpu_get_flag_enabled()' & 'rte_cpu_get_flag_name()' APIs is not
> >> clear if it cause ABI breakage,
> >> enum being embedded into the 'struct rte_bbdev_driver_info' struct
> >> doesn't leave a question, since this struct is returned from
> library to
> >> the application and change in the enum causes an ABI breakage.
> > Enum size does not change irrespective of changing its values. So
> > size-wise it's not an ABI breakage. Re-ordering values is an ABI
> > breakage.>
>
> Agree it is not size-wise issue. But still an issue.
>
>
> >>
> >>
> >> Briefly, I think even appending to the end of 'enum rte_cpu_flag_t'
> >> cause ABI breakage and removing 'RTE_CPUFLAG_NUMFLAGS' helps to
> extend
> >> this enum in the future.
> >> And an outstanding deprecation notice already exists for this:
> >>
> https://git.dpdk.org/dpdk/tree/doc/guides/rel_notes/deprecation.rst?h=v23.07#n63 <https://git.dpdk.org/dpdk/tree/doc/guides/rel_notes/deprecation.rst?h=v23.07#n63>
> >>
> >>
> >>> 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
> >>>
> >>
> >
> >
>
[AMD Official Use Only - General]
> -----Original Message-----
> From: Yigit, Ferruh <Ferruh.Yigit@amd.com>
> Sent: Wednesday, September 27, 2023 6:44 PM
> To: Tummala, Sivaprasad <Sivaprasad.Tummala@amd.com>;
> ruifeng.wang@arm.com; zhoumin@loongson.cn; drc@linux.vnet.ibm.com;
> kda@semihalf.com; bruce.richardson@intel.com; konstantin.v.ananyev@yandex.ru
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2 2/2] eal: remove NUMFLAGS enumeration
>
> On 8/11/2023 7:07 AM, Sivaprasad Tummala wrote:
> > This patch removes RTE_CPUFLAG_NUMFLAGS to allow new CPU features
> > without breaking ABI each time.
> >
> > Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
> > ---
> > lib/eal/arm/include/rte_cpuflags_32.h | 1 -
> > lib/eal/arm/include/rte_cpuflags_64.h | 1 -
> > lib/eal/arm/rte_cpuflags.c | 7 +++++--
> > lib/eal/loongarch/include/rte_cpuflags.h | 1 -
> > lib/eal/loongarch/rte_cpuflags.c | 7 +++++--
> > lib/eal/ppc/include/rte_cpuflags.h | 1 -
> > lib/eal/ppc/rte_cpuflags.c | 7 +++++--
> > lib/eal/riscv/include/rte_cpuflags.h | 1 -
> > lib/eal/riscv/rte_cpuflags.c | 7 +++++--
> > lib/eal/x86/include/rte_cpuflags.h | 1 -
> > lib/eal/x86/rte_cpuflags.c | 7 +++++--
> > 11 files changed, 25 insertions(+), 16 deletions(-)
> >
>
> Hi Sivaprasad,
>
> Is the relevant deprecation notice removed somewhere, if not can you please
> remove it in this set?
>
> Thanks,
> Ferruh
Sure, will do the needful
On Fri, Aug 11, 2023 at 8:08 AM Sivaprasad Tummala
<sivaprasad.tummala@amd.com> wrote:
>
> This patch removes RTE_CPUFLAG_NUMFLAGS to allow new CPU
> features without breaking ABI each time.
>
> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
I relooked at the API and I see one case (that I had missed in my
previous reply) where an ABI issue may be present so I updated the
commitlog in this regard.
And then I merged this patch, with a few modifications.
- removed the deprecation notice and updated RN,
- removed leftover "Last item" comments in arch cpuflag headers,
I hope everyone is happy with this.
Thanks.
On 10/6/2023 9:27 AM, David Marchand wrote:
> On Fri, Aug 11, 2023 at 8:08 AM Sivaprasad Tummala
> <sivaprasad.tummala@amd.com> wrote:
>>
>> This patch removes RTE_CPUFLAG_NUMFLAGS to allow new CPU
>> features without breaking ABI each time.
>>
>> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
>
> I relooked at the API and I see one case (that I had missed in my
> previous reply) where an ABI issue may be present so I updated the
> commitlog in this regard.
>
> And then I merged this patch, with a few modifications.
> - removed the deprecation notice and updated RN,
> - removed leftover "Last item" comments in arch cpuflag headers,
>
> I hope everyone is happy with this.
>
>
Looks good to me, thanks David.
@@ -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! */
};
#include "generic/rte_cpuflags.h"
@@ -37,7 +37,6 @@ enum rte_cpu_flag_t {
RTE_CPUFLAG_SVEBF16,
RTE_CPUFLAG_AARCH64,
/* The last item */
- RTE_CPUFLAG_NUMFLAGS,/**< This should always be the last! */
};
#include "generic/rte_cpuflags.h"
@@ -139,8 +139,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
{
const struct feature_entry *feat;
hwcap_registers_t regs = {0};
+ unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
- if (feature >= RTE_CPUFLAG_NUMFLAGS)
+ if ((unsigned int)feature >= num_flags)
return -ENOENT;
feat = &rte_cpu_feature_table[feature];
@@ -154,7 +155,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
const char *
rte_cpu_get_flag_name(enum rte_cpu_flag_t feature)
{
- if (feature >= RTE_CPUFLAG_NUMFLAGS)
+ unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
+
+ if ((unsigned int)feature >= num_flags)
return NULL;
return rte_cpu_feature_table[feature].name;
}
@@ -27,7 +27,6 @@ enum rte_cpu_flag_t {
RTE_CPUFLAG_LBT_ARM,
RTE_CPUFLAG_LBT_MIPS,
/* The last item */
- RTE_CPUFLAG_NUMFLAGS /**< This should always be the last! */
};
#include "generic/rte_cpuflags.h"
@@ -66,8 +66,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
{
const struct feature_entry *feat;
hwcap_registers_t regs = {0};
+ unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
- if (feature >= RTE_CPUFLAG_NUMFLAGS)
+ if ((unsigned int)feature >= num_flags)
return -ENOENT;
feat = &rte_cpu_feature_table[feature];
@@ -81,7 +82,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
const char *
rte_cpu_get_flag_name(enum rte_cpu_flag_t feature)
{
- if (feature >= RTE_CPUFLAG_NUMFLAGS)
+ unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
+
+ if ((unsigned int)feature >= num_flags)
return NULL;
return rte_cpu_feature_table[feature].name;
}
@@ -49,7 +49,6 @@ enum rte_cpu_flag_t {
RTE_CPUFLAG_HTM,
RTE_CPUFLAG_ARCH_2_07,
/* The last item */
- RTE_CPUFLAG_NUMFLAGS,/**< This should always be the last! */
};
#include "generic/rte_cpuflags.h"
@@ -90,8 +90,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
{
const struct feature_entry *feat;
hwcap_registers_t regs = {0};
+ unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
- if (feature >= RTE_CPUFLAG_NUMFLAGS)
+ if ((unsigned int)feature >= num_flags)
return -ENOENT;
feat = &rte_cpu_feature_table[feature];
@@ -105,7 +106,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
const char *
rte_cpu_get_flag_name(enum rte_cpu_flag_t feature)
{
- if (feature >= RTE_CPUFLAG_NUMFLAGS)
+ unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
+
+ if ((unsigned int)feature >= num_flags)
return NULL;
return rte_cpu_feature_table[feature].name;
}
@@ -43,7 +43,6 @@ enum rte_cpu_flag_t {
RTE_CPUFLAG_RISCV_ISA_Y, /* Reserved */
RTE_CPUFLAG_RISCV_ISA_Z, /* Reserved */
/* The last item */
- RTE_CPUFLAG_NUMFLAGS,/**< This should always be the last! */
};
#include "generic/rte_cpuflags.h"
@@ -95,8 +95,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
{
const struct feature_entry *feat;
hwcap_registers_t regs = {0};
+ unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
- if (feature >= RTE_CPUFLAG_NUMFLAGS)
+ if ((unsigned int)feature >= num_flags)
return -ENOENT;
feat = &rte_cpu_feature_table[feature];
@@ -110,7 +111,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
const char *
rte_cpu_get_flag_name(enum rte_cpu_flag_t feature)
{
- if (feature >= RTE_CPUFLAG_NUMFLAGS)
+ unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
+
+ if ((unsigned int)feature >= num_flags)
return NULL;
return rte_cpu_feature_table[feature].name;
}
@@ -135,7 +135,6 @@ enum rte_cpu_flag_t {
RTE_CPUFLAG_WAITPKG, /**< UMONITOR/UMWAIT/TPAUSE */
/* The last item */
- RTE_CPUFLAG_NUMFLAGS, /**< This should always be the last! */
};
#include "generic/rte_cpuflags.h"
@@ -149,8 +149,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
const struct feature_entry *feat;
cpuid_registers_t regs;
unsigned int maxleaf;
+ unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
- if (feature >= RTE_CPUFLAG_NUMFLAGS)
+ if ((unsigned int)feature >= num_flags)
/* Flag does not match anything in the feature tables */
return -ENOENT;
@@ -176,7 +177,9 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature)
const char *
rte_cpu_get_flag_name(enum rte_cpu_flag_t feature)
{
- if (feature >= RTE_CPUFLAG_NUMFLAGS)
+ unsigned int num_flags = RTE_DIM(rte_cpu_feature_table);
+
+ if ((unsigned int)feature >= num_flags)
return NULL;
return rte_cpu_feature_table[feature].name;
}