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 E8A1A425F4; Thu, 21 Sep 2023 16:50:09 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BC4D1402C9; Thu, 21 Sep 2023 16:50:09 +0200 (CEST) Received: from mail-qv1-f42.google.com (mail-qv1-f42.google.com [209.85.219.42]) by mails.dpdk.org (Postfix) with ESMTP id 3A2C1402B3 for ; Thu, 21 Sep 2023 16:50:08 +0200 (CEST) Received: by mail-qv1-f42.google.com with SMTP id 6a1803df08f44-6586739cfeeso4957686d6.1 for ; Thu, 21 Sep 2023 07:50:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf.com; s=google; t=1695307807; x=1695912607; 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=m5N4KG4QOWKsCJucfN+LdC3XTdHTQXRpGzjYdIXT98I=; b=oWY3vQtVPAe/gX2BsEtcUyMscaihiN0BdwWm5VBYBWHr4mfvfcB0k/377hlgFsELzH NfNYYV3ZedHud1K+va2N3e7Y+8bUxyzos5lqID4KfmtngWk1YSLe6wNQVAPXS3NOYlcR EAkC+Bq7z6UiIyOI2/KIXacyvUL/yawW0Y0uojO7HrgPTWLIx1zC5aHarm0LDjdp6jr1 fhwMCYS/PiUYW1624xp2gglpjzsRruZfhIJVDxzK2O2HFFs09Vtz/auF8Iq8X9UHU7Em rawzkmSIy4T1w6dZ/VjudHGAPUh9BDlRD2JqEHva/SUUwB6xcaLtjkXuupsLCI0qbd87 7NJg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695307807; x=1695912607; 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=m5N4KG4QOWKsCJucfN+LdC3XTdHTQXRpGzjYdIXT98I=; b=hYN4t/R54epPnVsokDWmeTjqOREZ/Pvqkz6CPXAZ8+XfD+Dp4NfjrmeUTaztNdtsZX rM8s22O+FobYWnlijCnjWA30k31//AqkhO/SA7NPijQygsw9KUxxyVa0pNi03WwbIwVD hd6XQ2qNDxuByL2CksDMsXr5apDQ2QbLhC6+Ek/EnGobCecgyCqZet3Is2VNOk32Wpik QvdZ1Kk7yxVcqDlCegeAKB1HRTjrDnwcP+Iklp/0fpUhD+l9Ju+Yy7C4n6HK4hDbLgHo Z1apugqGvGremKn38rviQm4mj+NplT11uRtPiPdlvQjKojnF4/5aKCvyOSgysQl4sHfJ mp0g== X-Gm-Message-State: AOJu0YyZOfRH+iLVpRiAg88pVas+aN9/cj/48wFA709o/Xcfl7NLTlTi L4eJeTG7alZ8itNQdvtY2vJpWgf91t+At+k8GQRatQ== X-Google-Smtp-Source: AGHT+IHdcwinFBYtMmbdSxndmQkVSEAlqLbsszZd7IMHpFesbkOgEOYli0Zjq741kMjPSbpxOIRDBqsshjcg0AJDJtI= X-Received: by 2002:a0c:de10:0:b0:65a:7b9:e214 with SMTP id t16-20020a0cde10000000b0065a07b9e214mr684690qvk.65.1695307807490; Thu, 21 Sep 2023 07:50:07 -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> In-Reply-To: From: =?UTF-8?Q?Stanis=C5=82aw_Kardach?= Date: Thu, 21 Sep 2023 16:49:55 +0200 Message-ID: Subject: Re: [PATCH v2 2/2] eal: remove NUMFLAGS enumeration To: "Tummala, Sivaprasad" Cc: David Marchand , Ruifeng Wang , Min Zhou , David Christensen , Bruce Richardson , Konstantin Ananyev , dev , "Yigit, Ferruh" , Thomas Monjalon Content-Type: multipart/alternative; boundary="000000000000a6a2820605df9bbb" 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 --000000000000a6a2820605df9bbb Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 some RPC > channel with > > another DPDK application. So clashing with this definition does not com= e > 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.325149= 3 > > > > -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.325149= 3 > > > > -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 this chang= e > > > > 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-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. 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 > > --000000000000a6a2820605df9bbb Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Thu, Sep 21, 2023, 15:18 Tummala, Sivaprasad <Sivaprasad.Tummala@amd.com> wrot= e:
[AMD Official Use Only - General= ]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, September 20, 2023 1:05 PM
> To: Stanis=C5=82aw Kardach <kda@semihalf.com>; Tummala, Sivapr= asad
> <Sivaprasad.Tummala@amd.com>
> Cc: Ruifeng Wang <ruifeng.wang@arm.com>; Min Zhou <zho= umin@loongson.cn>;
> David Christensen <drc@linux.vnet.ibm.com>; Bruce Richar= dson
> <bruce.richardson@intel.com>; Konstantin Ananyev
> <konstantin.v.ananyev@yandex.ru>; dev <dev@dpdk.org= >; Yigit, Ferruh
> <Ferruh.Yigit@amd.com>; Thomas Monjalon <thomas@monjal= on.net>
> Subject: Re: [PATCH v2 2/2] eal: remove NUMFLAGS enumeration
>
> Caution: This message originated from an External Source. Use proper c= aution
> when opening attachments, clicking links, or responding.
>
>
> On Wed, Sep 20, 2023 at 8:01=E2=80=AFAM Stanis=C5=82aw Kardach <kda@s= emihalf.com> wrote:
> >
> > On Tue, Sep 19, 2023 at 4:47=E2=80=AFPM David Marchand
> <david.marchand@redhat.com> wrote:
> > <snip>
> > > > Also I see you're still removing the RTE_CPUFLAG_NU= MFLAGS (what I call a
> last element canary). Why? If you're concerned with ABI, then we&#= 39;re talking about
> an application linking dynamically with DPDK or talking via some RPC c= hannel with
> another DPDK application. So clashing with this definition does not co= me into
> question. One should rather use rte_cpu_get_flag_enabled().
> > > > Also if you want to introduce new features, one would a= dd 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 ex= posed.
> > > 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://inbo= x.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 t= he
> > > library pov, a new meaning has been associated.
> > > This may trigger bugs in the application when calling a func= tion
> > > that returns such an enum which never return this MAX value = in the past.
> > >
> > > For at least those two reasons, removing those canary elemen= ts 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 patc= h 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 con= sistent 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 thi= s change
> > > is unneeded.
> > > But on the other hand, is it really an issue for an applicat= ion 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 behin= d 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 in= troduction of new CPU flag
"RTE_CPUFLAG_MONITORX" (http://mails.dpdk.org/archives/test-report/2023-April/382489.ht= ml).

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 a= s a flag value and instead as a canary but this test is not taking into acc= ount.
Your change did not break the ABI because you = have properly added the new flag at the end.
So I wo= uld ask to change the commit description to mention that NUMFLAGS is remove= d to:
1. Prevent users from treating it as a usable = value or an array size.
2. Prevent false-positive fa= ilures in the ABI test.

= Also it would be good to link to the aforementioned ABI test failure to giv= e 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

--000000000000a6a2820605df9bbb--