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 0C2C342651; Wed, 27 Sep 2023 15:49:10 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E3501402AA; Wed, 27 Sep 2023 15:49:09 +0200 (CEST) Received: from mail-qk1-f172.google.com (mail-qk1-f172.google.com [209.85.222.172]) by mails.dpdk.org (Postfix) with ESMTP id ED4BC4028C for ; Wed, 27 Sep 2023 15:49:08 +0200 (CEST) Received: by mail-qk1-f172.google.com with SMTP id af79cd13be357-7740cd2e393so700932585a.1 for ; Wed, 27 Sep 2023 06:49:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf.com; s=google; t=1695822548; x=1696427348; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=ND0rv3aUo6wDDaNQCP4+eu2ME/ZE090PFxnygwrLEko=; b=KpK4LmzsAeS88+FAt+BD5B774kNBQNpl5DCFw1TXTH66QCt+YA5qW3U1zWQa6JEn7k h+92WVfM9UilkkX7+oC0gljTDhZE0VMP8aOF1yU9mc+hOarUoMMoFqfxK/AppiMan7E5 ZGJxAwMWaiMPJvDEvjr6BPwfCNuhsHOtmrZ3McRQVRFMc8XRp4YH9J0xTNsv+EB1jj28 g6e+6DIbUG9ZhX6BKnEyhShYpHUxf2AEBcttnMLPRvqUF7yEpfcw61icHlcMTo6LVOGl SMo98ez/0mV3Jvt68shwD4lZFPrmDOOV/g+OJMwRIHKq9G76GIzXMG/7Cr2lUPhvQlye 0iTQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695822548; x=1696427348; h=content-transfer-encoding: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=ND0rv3aUo6wDDaNQCP4+eu2ME/ZE090PFxnygwrLEko=; b=RanDkT3bMUsiMpV4pf8rILKcIbi+d66yCf1fLOi0R33RhfPwvrzswyRoNczZRxSoKj oQkPbmYUPdmrULdyAtKIspUXrJhWADA9ArZDYBnwoh8aZW7R2U0ggUlVRTldjrTjb09f +Bap/orD+GeKRAxRBv33Uc2H47Ebd/Ft9ak/kjlUbTBeMUIG+JMIzgLl+dU5IlSfdtq4 E2NgK7CtvfffYmvLLu2bIyhKvf5C+MMjDVDTc6FtR8GciaOffTwzijTsS6cACcyBvWck F663aVPv0Muy9Aj25i6D588QZsRsf4JIlJ2gGakPTdnQb9mCZjT0uozrMrHKT77ouAiA 3Azw== X-Gm-Message-State: AOJu0Yw1Jx4hNzMGEZItQcUB8r7tfbBNvj5uAG8jT4d0CEcEhr7ki5d2 ELGmONn72WLck0BzYW/XUWNNEX2kCNwJo3cDnPYGc29kdxcYGAJxKik= X-Google-Smtp-Source: AGHT+IEKanlHwvy190YvwpFpXhB0mcGVBrRVgJXjyn7v60zqBwog5vx6VxZJ8u4IouCYzQijFI7XMeDNzE4xWqpdOWQ= X-Received: by 2002:a0c:db94:0:b0:636:e4f:6b9a with SMTP id m20-20020a0cdb94000000b006360e4f6b9amr2142512qvk.17.1695822527762; Wed, 27 Sep 2023 06:48:47 -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> In-Reply-To: <530da851-fcf3-4dcf-8f51-e11e9784763b@amd.com> From: =?UTF-8?Q?Stanis=C5=82aw_Kardach?= Date: Wed, 27 Sep 2023 15:48:11 +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: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 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 > > > > wr= ote: > > > > > > > > > > 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 exp= osed. > > > > > 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_PiQMV5LY5tKStk3= R1gOuR > > > 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 th= e > > > > > library pov, a new meaning has been associated. > > > > > This may trigger bugs in the application when calling a funct= ion > > > > > that returns such an enum which never return this MAX value i= n > > the past. > > > > > > > > > > For at least those two reasons, removing those canary element= s 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 i= s > > consistent with > > > it. > > > > > > > > > > > > > > > > Now, practically, when I look at the cpuflags API, I don't se= e 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 applicati= on 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 wit= h > > 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. > > > > 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 behavi= or. > 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=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 NUMFLAG= S > > 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 ver= sion. > > > > > > > > Thanks. > > > > > > -- > > > David Marchand > > > --=20 Best Regards, Stanis=C5=82aw Kardach