From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 47DDF42653; Wed, 27 Sep 2023 17:03:31 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C9E144029B; Wed, 27 Sep 2023 17:03:30 +0200 (CEST) Received: from mail-qk1-f174.google.com (mail-qk1-f174.google.com [209.85.222.174]) by mails.dpdk.org (Postfix) with ESMTP id 22B2240271 for ; Wed, 27 Sep 2023 17:03:29 +0200 (CEST) Received: by mail-qk1-f174.google.com with SMTP id af79cd13be357-77433e7a876so403197285a.3 for ; Wed, 27 Sep 2023 08:03:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf.com; s=google; t=1695827008; x=1696431808; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=J8xWj09/97yM/kzsIx6ypF/Or1XCd8jgK4+qpCO5Z10=; b=exbDMaBf7JSMQTi96nK2onKU9jrP0ZF88fngYBX8iFcjtrnf+sIVyKoYSLqo5NRdl3 wIqLv7f12heRRZhJU92MRz2sdnEixBZ4mzAh1Bu9oPdEkJeNZnyUegE4jFdnY8dKspXq /FTDQwwC7L7xmgIe1yQbFGnFleYu+dY0E1bTUx1k43n43NCOhgeGI/uPrOvGdru3GB34 ZZWQdjmfjSS3Xqa3ztFEsDoYZ2tEx73PiFUmSdYWrK5p7RWGFpf+mcw9d93JO+PDWH6I k6rEpp9YVgzmX7mvdHXV6ldoot7zQXryv15HWIpObrd1tIfvYf9p/yAwUxyn1zimp5R+ R5tg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695827008; x=1696431808; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=J8xWj09/97yM/kzsIx6ypF/Or1XCd8jgK4+qpCO5Z10=; b=gGE1JNIZ1t9qmhW8q3G0CaKZv4aSz8XJsPqNhlhKSBlpQK6WtVaxWvkpRIYR1THx6l 2dvqxsqfNNR/GPHfQHRlYELAUatrHLa8NLrJx0N0IOkwgue53tyRF+7QHrHWE8r7tCJy 3vei26P/Lzh/PSDItjA/DsEtfBzAF4WiF4Wg72A6+gqBZmOrMZ/Ja3Rpc3Gp+Av1WfOD frdu02EiQSWZ6VGSnoZyqU5159NH96Kg/97HZJ0D5FWbFZE7W2TFN6u/BR+G+INfTm3y 8gRikne9uFF+LNrM4Mx0Yin7NkSU10CyaCH6ODzU9T8482I68pjWUVfP6M0sF/ZT4rF4 gNUA== X-Gm-Message-State: AOJu0Yy7qAIvjs/kj77yI9uQbdLZjfqj/nQtYchSRi/9ynoaTlrQrbbw MaCrDlKBQo5/oYxcSKjgTeDbKxiRh/xi83g12k9qfg== X-Google-Smtp-Source: AGHT+IHhK1jNO4gYNPecwx+vlTPJwsOc2iu1vpEBxlIl2OF5L9pwUIBHO0o3ZuXg4L+gt7lZAL5rn2q6dqwillFp8HI= X-Received: by 2002:a05:620a:40c8:b0:76f:1272:2aa8 with SMTP id g8-20020a05620a40c800b0076f12722aa8mr2417722qko.6.1695827008387; Wed, 27 Sep 2023 08:03:28 -0700 (PDT) MIME-Version: 1.0 References: <20230802211150.939121-1-sivaprasad.tummala@amd.com> <20230811060755.481572-1-sivaprasad.tummala@amd.com> <20230811060755.481572-2-sivaprasad.tummala@amd.com> <530da851-fcf3-4dcf-8f51-e11e9784763b@amd.com> <6434ac72-0f5b-450b-900d-e34078756355@amd.com> In-Reply-To: <6434ac72-0f5b-450b-900d-e34078756355@amd.com> From: =?UTF-8?Q?Stanis=C5=82aw_Kardach?= Date: Wed, 27 Sep 2023 17:03:17 +0200 Message-ID: Subject: Re: [PATCH v2 2/2] eal: remove NUMFLAGS enumeration To: Ferruh Yigit Cc: "Tummala, Sivaprasad" , David Marchand , Ruifeng Wang , Min Zhou , David Christensen , Bruce Richardson , Konstantin Ananyev , dev , Thomas Monjalon Content-Type: multipart/alternative; boundary="0000000000006f9b710606587e6d" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --0000000000006f9b710606587e6d Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Sep 27, 2023, 16:09 Ferruh Yigit wrote: > On 9/27/2023 2:48 PM, Stanis=C5=82aw Kardach wrote: > > On Wed, Sep 27, 2023 at 1:55=E2=80=AFPM Ferruh Yigit > wrote: > >> > >> On 9/21/2023 3:49 PM, Stanis=C5=82aw Kardach wrote: > >>> On Thu, Sep 21, 2023, 15:18 Tummala, Sivaprasad > >>> > > wrote: > >>> > >>> [AMD Official Use Only - General] > >>> > >>> > -----Original Message----- > >>> > From: David Marchand >>> > > >>> > Sent: Wednesday, September 20, 2023 1:05 PM > >>> > To: Stanis=C5=82aw Kardach >>> >; Tummala, Sivaprasad > >>> > = > > >>> > Cc: Ruifeng Wang >>> >; Min Zhou >>> >; > >>> > David Christensen >>> >; Bruce Richardson > >>> > >>; > >>> Konstantin Ananyev > >>> > >>> >; dev >>> >; Yigit, Ferruh > >>> > >; Thomas > >>> Monjalon > > >>> > 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=E2=80=AFAM Stanis=C5=82aw Kardach > >>> > wrote: > >>> > > > >>> > > On Tue, Sep 19, 2023 at 4:47=E2=80=AFPM David Marchand > >>> > > > wrote: > >>> > > > >>> > > > > 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 som= e > >>> 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/ > >>> > >>> > > >>> > 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 bi= t > >>> 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: fro= m > 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/ > >>> > >>> > > 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=3D5da7c13521 > >>> > >>> > 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 th= is > >>> 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 reaso= n > >>> behind this patch was > >>> > to not break the ABI and I don't think it does that. What it do= es > >>> is enforces users to > >>> > use explicit cpu flag values which is a good thing. Though if s= o, > >>> 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.htm= l > >>> >). > >>> > >>> Because of ABI breakage, the feature was postponed to this releas= e. > >>> > https://patchwork.dpdk.org/project/dpdk/patch/20230413115334.43172-3-siva= prasad.tummala@amd.com/ > < > https://patchwork.dpdk.org/project/dpdk/patch/20230413115334.43172-3-siva= prasad.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 enu= m > >> 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 tha= t > >> this can cause an ABI breakage, because application can receive a valu= e > >> 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 bas= ed > >> on returned enum item value, if the enum extended in the new version o= f > >> the shared library, this can cause invalid memory access in applicatio= n. > > 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 caus= e > >> 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 t= o > >> 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=3Dv= 23.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 > >>> > >> > > > > > > --0000000000006f9b710606587e6d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Wed, Sep 27, 2023, 16:09 Ferruh Yigit <ferruh.yigit@amd.com> wrote:
On 9/27/2023 2:48 PM, Stanis=C5=82aw Kard= ach wrote:
> On Wed, Sep 27, 2023 at 1:55=E2=80=AFPM Ferruh Yigit <ferruh.yigi= t@amd.com> wrote:
>>
>> On 9/21/2023 3:49 PM, Stanis=C5=82aw Kardach wrote:
>>> On Thu, Sep 21, 2023, 15:18 Tummala, Sivaprasad
>>> <Sivaprasad.Tummala@amd.com <mailto:Sivaprasad.Tummala@amd.com>> wrote:
>>>
>>>=C2=A0 =C2=A0 =C2=A0[AMD Official Use Only - General]
>>>
>>>=C2=A0 =C2=A0 =C2=A0> -----Original Message-----
>>>=C2=A0 =C2=A0 =C2=A0> From: David Marchand <david.= marchand@redhat.com
>>>=C2=A0 =C2=A0 =C2=A0<mailto:david.marchand@redhat.com= >>
>>>=C2=A0 =C2=A0 =C2=A0> Sent: Wednesday, September 20, 2023 1:= 05 PM
>>>=C2=A0 =C2=A0 =C2=A0> To: Stanis=C5=82aw Kardach <kda@semih= alf.com
>>>=C2=A0 =C2=A0 =C2=A0<mailto:kda@semihalf.com>>; Tumm= ala, Sivaprasad
>>>=C2=A0 =C2=A0 =C2=A0> <Sivaprasad.Tummala@amd.com= <mailto:Sivaprasad.Tummala@amd.com>>
>>>=C2=A0 =C2=A0 =C2=A0> Cc: Ruifeng Wang <ruifeng.wang@ar= m.com
>>>=C2=A0 =C2=A0 =C2=A0<mailto:ruifeng.wang@arm.com>&g= t;; Min Zhou <zhoumin@loongson.cn
>>>=C2=A0 =C2=A0 =C2=A0<mailto:zhoumin@loongson.cn>>= ;
>>>=C2=A0 =C2=A0 =C2=A0> David Christensen <drc@linux.vn= et.ibm.com
>>>=C2=A0 =C2=A0 =C2=A0<mailto:drc@linux.vnet.ibm.com&g= t;>; Bruce Richardson
>>>=C2=A0 =C2=A0 =C2=A0> <bruce.richardson@intel.com= <mailto:bruce.richardson@intel.com>>;
>>>=C2=A0 =C2=A0 =C2=A0Konstantin Ananyev
>>>=C2=A0 =C2=A0 =C2=A0> <konstantin.v.ananyev@y= andex.ru
>>>=C2=A0 =C2=A0 =C2=A0<mailto:konstantin.v.ananyev= @yandex.ru>>; dev <dev@dpdk.org
>>>=C2=A0 =C2=A0 =C2=A0<mailto:dev@dpdk.org>>; Yigit, Ferru= h
>>>=C2=A0 =C2=A0 =C2=A0> <Ferruh.Yigit@amd.com <mai= lto:Ferruh.Yigit@amd.com>>; Thomas
>>>=C2=A0 =C2=A0 =C2=A0Monjalon <thomas@monjalon.net <m= ailto:thomas@monjalon.net>>
>>>=C2=A0 =C2=A0 =C2=A0> Subject: Re: [PATCH v2 2/2] eal: remov= e NUMFLAGS enumeration
>>>=C2=A0 =C2=A0 =C2=A0>
>>>=C2=A0 =C2=A0 =C2=A0> Caution: This message originated from = an External Source. Use
>>>=C2=A0 =C2=A0 =C2=A0proper caution
>>>=C2=A0 =C2=A0 =C2=A0> when opening attachments, clicking lin= ks, or responding.
>>>=C2=A0 =C2=A0 =C2=A0>
>>>=C2=A0 =C2=A0 =C2=A0>
>>>=C2=A0 =C2=A0 =C2=A0> On Wed, Sep 20, 2023 at 8:01=E2=80=AFA= M Stanis=C5=82aw Kardach
>>>=C2=A0 =C2=A0 =C2=A0<kda@semihalf.com <mailto:kda@semih= alf.com>> wrote:
>>>=C2=A0 =C2=A0 =C2=A0> >
>>>=C2=A0 =C2=A0 =C2=A0> > On Tue, Sep 19, 2023 at 4:47=E2= =80=AFPM David Marchand
>>>=C2=A0 =C2=A0 =C2=A0> <david.marchand@redhat.com <mailto:david.marchand@redhat.com>> wrote:
>>>=C2=A0 =C2=A0 =C2=A0> > <snip>
>>>=C2=A0 =C2=A0 =C2=A0> > > > Also I see you're s= till removing the RTE_CPUFLAG_NUMFLAGS
>>>=C2=A0 =C2=A0 =C2=A0(what I call a
>>>=C2=A0 =C2=A0 =C2=A0> last element canary). Why? If you'= re concerned with ABI, then
>>>=C2=A0 =C2=A0 =C2=A0we're talking about
>>>=C2=A0 =C2=A0 =C2=A0> an application linking dynamically wit= h DPDK or talking via some
>>>=C2=A0 =C2=A0 =C2=A0RPC channel with
>>>=C2=A0 =C2=A0 =C2=A0> another DPDK application. So clashing = with this definition does
>>>=C2=A0 =C2=A0 =C2=A0not come into
>>>=C2=A0 =C2=A0 =C2=A0> question. One should rather use rte_cp= u_get_flag_enabled().
>>>=C2=A0 =C2=A0 =C2=A0> > > > Also if you want to int= roduce new features, one would add
>>>=C2=A0 =C2=A0 =C2=A0them yo the
>>>=C2=A0 =C2=A0 =C2=A0> rte_cpuflags headers, unless you'd= like to not add those and keep an
>>>=C2=A0 =C2=A0 =C2=A0> undocumented list "above" th= e last defined element.
>>>=C2=A0 =C2=A0 =C2=A0> > > > Could you explain a bit= more Your use-case?
>>>=C2=A0 =C2=A0 =C2=A0> > >
>>>=C2=A0 =C2=A0 =C2=A0> > > Hey Stanislaw,
>>>=C2=A0 =C2=A0 =C2=A0> > >
>>>=C2=A0 =C2=A0 =C2=A0> > > Talking generically, one pro= blem with such pattern (having a LAST,
>>>=C2=A0 =C2=A0 =C2=A0> > > or MAX enum) is when an arra= y sized with such a symbol is exposed.
>>>=C2=A0 =C2=A0 =C2=A0> > > As I mentionned in the past,= this can have unwanted effects:
>>>=C2=A0 =C2=A0 =C2=A0> > >
>>>=C2=A0 =C2=A0 =C2=A0https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.32= 51493
>>>=C2=A0 =C2=A0 =C2=A0<https://patchwork.dpdk.org/project/dpdk/patch/20230919140430= .3251493>
>>>=C2=A0 =C2=A0 =C2=A0> > > -1-davi= d.marchand@redhat.com/
>>>=C2=A0 =C2=A0 =C2=A0<http://1-david.mar= chand@redhat.com/>
>>>=C2=A0 =C2=A0 =C2=A0>
>>>=C2=A0 =C2=A0 =C2=A0> Argh... who broke copy/paste in my bro= wser ?!
>>>=C2=A0 =C2=A0 =C2=A0> Wrt to MAX and arrays, I wanted to poi= nt at:
>>>=C2=A0 =C2=A0 =C2=A0>
>>>=C2=A0 =C2=A0 =C2=A0http://inbox.dpdk.org/dev/CAJFAV8xs5CVdE2xwRtaxk5vE_PiQMV5L= Y5tKStk3R1gOuR <http://inbox.dpdk.org/dev/CAJFAV8xs5CVdE2xwRtaxk5vE_PiQMV5LY5tKStk3R= 1gOuR>
>>>=C2=A0 =C2=A0 =C2=A0> TsUw@mail.gmail.com/ = <http://TsUw@mail.gmail.com/>
>>>=C2=A0 =C2=A0 =C2=A0>
>>>=C2=A0 =C2=A0 =C2=A0> > I agree, though I'd argue &qu= ot;LAST" and "MAX" semantics are a bit
>>>=C2=A0 =C2=A0 =C2=A0different. "LAST"
>>>=C2=A0 =C2=A0 =C2=A0> delimits the known enumeration territo= ry while "MAX" is more of a
>>>=C2=A0 =C2=A0 =C2=A0`constepxr`
>>>=C2=A0 =C2=A0 =C2=A0> value type.
>>>=C2=A0 =C2=A0 =C2=A0> > >
>>>=C2=A0 =C2=A0 =C2=A0> > > Another issue is when an exi= sting enum meaning changes: from the
>>>=C2=A0 =C2=A0 =C2=A0> > > application pov, the (old) M= AX value is incorrect, but for the
>>>=C2=A0 =C2=A0 =C2=A0> > > library pov, a new meaning h= as been associated.
>>>=C2=A0 =C2=A0 =C2=A0> > > This may trigger bugs in the= application when calling a function
>>>=C2=A0 =C2=A0 =C2=A0> > > that returns such an enum wh= ich never return this MAX value in
>>>=C2=A0 =C2=A0 =C2=A0the past.
>>>=C2=A0 =C2=A0 =C2=A0> > >
>>>=C2=A0 =C2=A0 =C2=A0> > > For at least those two reaso= ns, removing those canary elements is
>>>=C2=A0 =C2=A0 =C2=A0> > > being done in DPDK.
>>>=C2=A0 =C2=A0 =C2=A0> > >
>>>=C2=A0 =C2=A0 =C2=A0> > > This specific removal has be= en announced:
>>>=C2=A0 =C2=A0 =C2=A0> > >
>>>=C2=A0 =C2=A0 =C2=A0https://patchwork.dpdk.org/project/dpdk/patch/20230919140430.32= 51493
>>>=C2=A0 =C2=A0 =C2=A0<https://patchwork.dpdk.org/project/dpdk/patch/20230919140430= .3251493>
>>>=C2=A0 =C2=A0 =C2=A0> > > -1-davi= d.marchand@redhat.com/
>>>=C2=A0 =C2=A0 =C2=A0<http://1-david.mar= chand@redhat.com/>
>>>=C2=A0 =C2=A0 =C2=A0> > Thanks for pointing this out but = did you mean to link to the
>>>=C2=A0 =C2=A0 =C2=A0patch again here?
>>>=C2=A0 =C2=A0 =C2=A0>
>>>=C2=A0 =C2=A0 =C2=A0> Sorry, same here, bad copy/paste :-(.<= br> >>>=C2=A0 =C2=A0 =C2=A0>
>>>=C2=A0 =C2=A0 =C2=A0> The intended link is:
>>>=C2=A0 =C2=A0 =C2=A0https://= git.dpdk.org/dpdk/commit/?id=3D5da7c13521
>>>=C2=A0 =C2=A0 =C2=A0<http= s://git.dpdk.org/dpdk/commit/?id=3D5da7c13521>
>>>=C2=A0 =C2=A0 =C2=A0> The deprecation notice was badly formu= lated and this patch here is
>>>=C2=A0 =C2=A0 =C2=A0consistent with
>>>=C2=A0 =C2=A0 =C2=A0> it.
>>>=C2=A0 =C2=A0 =C2=A0>
>>>=C2=A0 =C2=A0 =C2=A0>
>>>=C2=A0 =C2=A0 =C2=A0> > >
>>>=C2=A0 =C2=A0 =C2=A0> > > Now, practically, when I loo= k at the cpuflags API, I don't see us
>>>=C2=A0 =C2=A0 =C2=A0> > > exposed to those two issues = wrt rte_cpu_flag_t, so maybe this
>>>=C2=A0 =C2=A0 =C2=A0change
>>>=C2=A0 =C2=A0 =C2=A0> > > is unneeded.
>>>=C2=A0 =C2=A0 =C2=A0> > > But on the other hand, is it= really an issue for an application to
>>>=C2=A0 =C2=A0 =C2=A0> > > lose this (internal) informa= tion?
>>>=C2=A0 =C2=A0 =C2=A0> > I doubt it, maybe it could be use= d as a sanity check for
>>>=C2=A0 =C2=A0 =C2=A0choosing proper functors
>>>=C2=A0 =C2=A0 =C2=A0> in the application. Though the initial= description of the reason
>>>=C2=A0 =C2=A0 =C2=A0behind this patch was
>>>=C2=A0 =C2=A0 =C2=A0> to not break the ABI and I don't t= hink it does that. What it does
>>>=C2=A0 =C2=A0 =C2=A0is enforces users to
>>>=C2=A0 =C2=A0 =C2=A0> use explicit cpu flag values which is = a good thing. Though if so,
>>>=C2=A0 =C2=A0 =C2=A0then it should be
>>>=C2=A0 =C2=A0 =C2=A0> stated in the commit description.
>>>=C2=A0 =C2=A0 =C2=A0>
>>>=C2=A0 =C2=A0 =C2=A0> I agree.
>>>=C2=A0 =C2=A0 =C2=A0> Siva, can you work on a new revision?<= br> >>>=C2=A0 =C2=A0 =C2=A0>
>>>=C2=A0 =C2=A0 =C2=A0David, Stanislaw,
>>>
>>>=C2=A0 =C2=A0 =C2=A0The original motivation of this patch was t= o avoid ABI breakage with
>>>=C2=A0 =C2=A0 =C2=A0the introduction of new CPU flag
>>>=C2=A0 =C2=A0 =C2=A0"RTE_CPUFLAG_MONITORX"
>>>=C2=A0 =C2=A0 =C2=A0(http://mails.dpdk.org/archives/test-report/2023-April/382489.html<= /a>
>>>=C2=A0 =C2=A0 =C2=A0<http://mails.dpdk.org/archives/test-report/2023-April/382489.ht= ml>).
>>>
>>>=C2=A0 =C2=A0 =C2=A0Because of ABI breakage, the feature was po= stponed to this release.
>>>=C2=A0 =C2=A0 =C2=A0https://patchwork.dpdk.org/project/d= pdk/patch/20230413115334.43172-3-sivaprasad.tummala@amd.com/ <https://patchwork.dpdk.org/project/dpdk/patch/20230413115334.43172-3-sivap= rasad.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 bet= ween
>> 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<= br> > should not be treated as a "real" value and hence my suggest= ion 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 red= uce
>> the risk.
> Agreed though as I have mentioned before, "MAX" has a differ= ent
> semantics than "NUM". Then again since we have rte_cpu_featu= re_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 element= s in an enumeration where elements describe some items and their value is j= ust 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 arr= ay index based
>> on returned enum item value, if the enum extended in the new versi= on of
>> the shared library, this can cause invalid memory access in applic= ation.
> 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 runti= me
> 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 s= ure
>> how problematic it is to be honest. Like being in the
>> 'rte_cpu_get_flag_enabled()' & 'rte_cpu_get_flag_n= ame()' 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_n= ame()' 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 l= ibrary 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_fl= ag_t'
>> cause ABI breakage and removing 'RTE_CPUFLAG_NUMFLAGS' hel= ps 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=3Dv= 23.07#n63
>>
>>
>>> Your change did not break the ABI because you have properly ad= ded the
>>> new flag at the end.
>>> So I would ask to change the commit description to mention tha= t NUMFLAGS
>>> is removed to:
>>> 1. Prevent users from treating it as a usable value or an arra= y size.
>>> 2. Prevent false-positive failures in the ABI test.
>>>
>>> Also it would be good to link to the aforementioned ABI test f= ailure to
>>> give readers some context when inspecting the git tree.
>>>
>>>
>>>
>>>=C2=A0 =C2=A0 =C2=A0Can you please add what exactly needs to be= reworked in the new version.
>>>
>>>=C2=A0 =C2=A0 =C2=A0>
>>>=C2=A0 =C2=A0 =C2=A0> Thanks.
>>>=C2=A0 =C2=A0 =C2=A0>
>>>=C2=A0 =C2=A0 =C2=A0> --
>>>=C2=A0 =C2=A0 =C2=A0> David Marchand
>>>
>>
>
>

--0000000000006f9b710606587e6d--