From: "Stanisław Kardach" <kda@semihalf.com>
To: Ferruh Yigit <ferruh.yigit@amd.com>
Cc: "Tummala, Sivaprasad" <Sivaprasad.Tummala@amd.com>,
David Marchand <david.marchand@redhat.com>,
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>, Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [PATCH v2 2/2] eal: remove NUMFLAGS enumeration
Date: Wed, 27 Sep 2023 17:03:17 +0200 [thread overview]
Message-ID: <CALVGJWL1oepVLPj88vnhwBZ+F8qn58RrwvsBAZW9BNzafBh_KQ@mail.gmail.com> (raw)
In-Reply-To: <6434ac72-0f5b-450b-900d-e34078756355@amd.com>
[-- Attachment #1: Type: text/plain, Size: 12535 bytes --]
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
> >>>
> >>
> >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 21051 bytes --]
next prev parent reply other threads:[~2023-09-27 15:03 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-02 21:11 [PATCH 1/2] eal: remove RTE_CPUFLAG_NUMFLAGS Sivaprasad Tummala
2023-08-02 21:11 ` [PATCH 2/2] test/cpuflags: " Sivaprasad Tummala
2023-08-02 23:50 ` [PATCH 1/2] eal: " Stanisław Kardach
2023-08-11 4:02 ` Tummala, Sivaprasad
2023-08-11 6:07 ` [PATCH v2 1/2] test/cpuflags: removed test for NUMFLAGS Sivaprasad Tummala
2023-08-11 6:07 ` [PATCH v2 2/2] eal: remove NUMFLAGS enumeration Sivaprasad Tummala
2023-08-15 6:10 ` Stanisław Kardach
2023-09-19 14:47 ` David Marchand
2023-09-20 6:00 ` Stanisław Kardach
2023-09-20 7:34 ` David Marchand
2023-09-21 13:18 ` Tummala, Sivaprasad
2023-09-21 14:49 ` Stanisław Kardach
2023-09-27 11:55 ` Ferruh Yigit
2023-09-27 13:48 ` Stanisław Kardach
2023-09-27 14:09 ` Ferruh Yigit
2023-09-27 15:03 ` Stanisław Kardach [this message]
2023-09-27 16:13 ` Ferruh Yigit
2023-09-27 13:13 ` Ferruh Yigit
2023-09-28 9:31 ` Tummala, Sivaprasad
2023-10-06 8:27 ` David Marchand
2023-10-06 11:19 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CALVGJWL1oepVLPj88vnhwBZ+F8qn58RrwvsBAZW9BNzafBh_KQ@mail.gmail.com \
--to=kda@semihalf.com \
--cc=Sivaprasad.Tummala@amd.com \
--cc=bruce.richardson@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=drc@linux.vnet.ibm.com \
--cc=ferruh.yigit@amd.com \
--cc=konstantin.v.ananyev@yandex.ru \
--cc=ruifeng.wang@arm.com \
--cc=thomas@monjalon.net \
--cc=zhoumin@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).