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 6FADFA0C4D; Wed, 13 Oct 2021 09:02:26 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0200C40150; Wed, 13 Oct 2021 09:02:26 +0200 (CEST) Received: from new4-smtp.messagingengine.com (new4-smtp.messagingengine.com [66.111.4.230]) by mails.dpdk.org (Postfix) with ESMTP id 2D72840142 for ; Wed, 13 Oct 2021 09:02:24 +0200 (CEST) Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailnew.nyi.internal (Postfix) with ESMTP id 4D39758029D; Wed, 13 Oct 2021 03:02:21 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Wed, 13 Oct 2021 03:02:21 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=fm2; bh= kQBWxMwf0nxw4/e25vDQGATBJoAc9gRoz0LHE3JxOew=; b=D3hL8xQxCEbF2y+j W1R8/HndOHJmeepk8Cmt20CGMyR1G05QY2Rt7fsfYWVvGZ/kWmGpJpi6FYhPhxQB nhZsURDBnd481RX+vH4sI4CAZDPDvEgYfzfp1wFfhWQk/lKgNo6S8bYEtHAfYKKb KuMlpmF9y7yOrXj1Za+YwiXMMeSd81SkCosztJPfWwEnq61/NKaW3e4c2gRJqp2V ufA0nBwHiZrvaR9F5Tj7uhf5HggIdER+qhNfKHsofq1BWz1aQEI270uzSA314VmH tpp7P7mabXrQJTgpXgYoc0tk0TT9+psZXkWf9Z8+qTCVM2b8C5J1XgGKnJSf1YHp f1DCmw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=kQBWxMwf0nxw4/e25vDQGATBJoAc9gRoz0LHE3JxO ew=; b=RkquUDgO6GRky3idkoIqyduR+9oZ5E8r6isBmBw/g9rUJwcWUcgE0r+0X aFLTAOQ/Vzbipv7DvE4c3vMppjDFheBP4mPxIES/YudWFAM8zvgwOSaGbXpFmS0J edIWFya2RhwYzSq1rAVDV9vQ66CfNgcyd1YsSRVrgsp/NHSTkHy3BmuSFHqKkDLq mCD3ai6PVbHMIMbkOS62J7ks/6j+/1oNw0w17pkrXAqbzHh4F4yysq588VTyO4C7 +qpzEK/ECtNHJE2R7zUIkV5LZLRLyppCSMsO2I+mGSXDfCoPtnH2dsaUv+bYcQ+t Oj2vbjANty4EQEYy0E0PUGNGWvZ0w== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrvddtledguddutdcutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefhvffufffkjghfggfgtgesthhqredttddtjeenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpeekteehtdeivefhieegjeelgedufeejheekkeetueevieeuvdev uedtjeevheevteenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfh hrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 13 Oct 2021 03:02:15 -0400 (EDT) From: Thomas Monjalon To: Akhil Goyal , "dev@dpdk.org" , "Kinsella, Ray" , Anoob Joseph Cc: "david.marchand@redhat.com" , "hemant.agrawal@nxp.com" , "pablo.de.lara.guarch@intel.com" , "fiona.trahe@intel.com" , "declan.doherty@intel.com" , "matan@nvidia.com" , "g.singh@nxp.com" , "roy.fan.zhang@intel.com" , "jianjay.zhou@huawei.com" , "asomalap@amd.com" , "ruifeng.wang@arm.com" , "konstantin.ananyev@intel.com" , "radu.nicolau@intel.com" , "ajit.khaparde@broadcom.com" , Nagadheeraj Rottela , Ankur Dwivedi , "ciara.power@intel.com" , Stephen Hemminger , "Yigit, Ferruh" , "bruce.richardson@intel.com" Date: Wed, 13 Oct 2021 09:02:12 +0200 Message-ID: <1842444.xqXZe6RN4B@thomas> In-Reply-To: References: <20210731181327.660296-1-gakhil@marvell.com> <1716871.KAiom3yBSL@thomas> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators 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 Sender: "dev" 13/10/2021 07:36, Anoob Joseph: > From: Thomas Monjalon > > 12/10/2021 16:47, Kinsella, Ray: > > > On 12/10/2021 15:18, Anoob Joseph wrote: > > > > From: Thomas Monjalon > > > >> 12/10/2021 15:38, Anoob Joseph: > > > >>> From: Thomas Monjalon > > > >>>> 12/10/2021 13:34, Anoob Joseph: > > > >>>>> From: Kinsella, Ray > > > >>>>>> On 12/10/2021 11:50, Anoob Joseph wrote: > > > >>>>>>> From: Akhil Goyal > > > >>>>>>>>> On 08/10/2021 21:45, Akhil Goyal wrote: > > > >>>>>>>>>> Remove *_LIST_END enumerators from asymmetric crypto > > lib to > > > >>>>>>>>>> avoid ABI breakage for every new addition in enums. > > > >>>>>>>>>> > > > >>>>>>>>>> Signed-off-by: Akhil Goyal > > > >>>>>>>>>> --- > > > >>>>>>>>>> - } else if (xform->xform_type >=3D > > > >>>>>>>>> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END > > > >>>>>>>>>> + } else if (xform->xform_type > > > > >>>> RTE_CRYPTO_ASYM_XFORM_ECPM > > > >>>> [...] > > > >>>>>>>>> > > > >>>>>>>>> So I am not sure that this is an improvement. > > > >>>> > > > >>>> Indeed, it is not an improvement. > > > >>>> > > > >>>>>>>>> The cryptodev issue we had, was that _LIST_END was being > > > >>>>>>>>> used to size arrays. > > > >>>>>>>>> And that broke when new algorithms got added. Is that an > > > >>>>>>>>> issue, in this > > > >>>>>> case? > > > >>>>>>>> > > > >>>>>>>> Yes we did this same exercise for symmetric crypto enums > > earlier. > > > >>>>>>>> Asym enums were left as it was experimental at that point. > > > >>>>>>>> They are still experimental, but thought of making this > > > >>>>>>>> uniform throughout DPDK enums. > > > >>>>>>>> > > > >>>>>>>>> > > > >>>>>>>>> I am not sure that swapping out _LIST_END, and then > > > >>>>>>>>> littering the code with RTE_CRYPTO_ASYM_XFORM_ECPM and > > > >>>>>>>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an > > > >>>> improvement > > > >>>>>>>> here. > > > >>>>>>>>> > > > >>>>>>>>> My 2c is that from an ABI PoV > > RTE_CRYPTO_ASYM_OP_LIST_END is > > > >>>>>>>>> not better or worse, than > > > >>>>>> RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE? > > > >>>>>>>>> > > > >>>>>>>>> Interested to hear other thoughts. > > > >>>>>>>> > > > >>>>>>>> I don=E2=80=99t have any better solution for avoiding ABI is= sues for now. > > > >>>>>>>> The change is for avoiding ABI breakage. But we can drop this > > > >>>>>>>> patch For now as asym is still experimental. > > > >>>>>>> > > > >>>>>>> [Anoob] Having LIST_END would preclude new additions to > > > >>>>>>> asymmetric > > > >>>> algos? > > > >>>>>> If yes, then I would suggest we address it now. > > > >>>>>> > > > >>>>>> Not at all - but it can be problematic, if two versions of DPDK > > > >>>>>> disagree with the value of LIST_END. > > > >>>>>> > > > >>>>>>> Looking at the "problematic changes", we only have 2-3 > > > >>>>>>> application & PMD changes. For unit test application, we could > > > >>>>>>> may be do something like, > > > >>>>>> > > > >>>>>> The essental functionality not that different, I am just not > > > >>>>>> sure that the verbosity below is helping. > > > >>>>>> What you are really trying to guard against is people using > > > >>>>>> LIST_END to size arrays. > > > >>>>> > > > >>>>> [Anoob] Our problem is application using LIST_END (which comes > > > >>>>> from library) > > > >>>> to determine the number of iterations for the loop. My suggestion > > > >>>> is to modify the UT such that, we could use RTE_DIM(types) (which > > > >>>> comes from application) to determine iterations of loop. This > > > >>>> would solve the > > > >> problem, right? > > > >>>> > > > >>>> The problem is not the application. > > > >>>> Are you asking the app to define DPDK types? > > > >>> > > > >>> [Anoob] I didn't understand how you concluded that. > > > >> > > > >> Because you define a specific array in the test app. > > > >> > > > >>> The app is supposed to test "n" asymmetric features supported by > > DPDK. > > > >> Currently, it does that by looping from 0 to LIST_END which happens > > > >> to give you the first n features. Now, if we add any new asymmetric > > > >> feature, LIST_END value would change. Isn't that the very reason > > > >> why we removed LIST_END from symmetric library and applications? > > > >> > > > >> Yes > > > >> > > > >>> Now coming to what I proposed, the app is supposed to test "n" > > > >>> asymmetric > > > >> features. LIST_END helps in doing the loops. If we remove LIST_END, > > > >> then application will not be in a position to do a loop. My > > > >> suggestion is, we list the types that are supposed to be tested by > > > >> the app, and let that array be used as feature list. > > > >>> > > > >>> PS: Just to reiterate, my proposal is just a local array which > > > >>> would hold DPDK > > > >> defined RTE enum values for the features that would be tested by > > > >> this app/function. > > > >> > > > >> I am more concerned by the general case than the test app. > > > >> I think a function returning a number is more app-friendly. > > > > > > > > [Anoob] Indeed. But there are 3 LIST_ENDs removed with this patch. = Do > > you propose 3 new APIs to just get max number? > > > > > > 1 API returning a single "info" structure perhaps - as being the most > > extensible? > >=20 > > Or 3 iterators (foreach construct). > > Instead of just returning a size, we can have an iterator for each enum= which > > needs to be iterated. >=20 > [Anoob] Something like this? >=20 > diff --git a/app/test/test_cryptodev_asym.c b/app/test/test_cryptodev_asy= m.c > index 847b074a4f..68a6197851 100644 > --- a/app/test/test_cryptodev_asym.c > +++ b/app/test/test_cryptodev_asym.c > @@ -542,7 +542,7 @@ test_one_case(const void *test_case, int sessionless) > printf(" %u) TestCase %s %s\n", test_index++, > tc.modex.description, test_msg); > } else { > - for (i =3D 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) { > + RTE_CRYPTO_ASYM_FOREACH_OP_TYPE(i) { > if (tc.modex.xform_type =3D=3D RTE_CRYPTO_ASYM_XF= ORM_RSA) { > if (tc.rsa_data.op_type_flags & (1 << i))= { > if (tc.rsa_data.key_exp) { > diff --git a/lib/cryptodev/rte_crypto_asym.h b/lib/cryptodev/rte_crypto_a= sym.h > index 9c866f553f..5627dcaff1 100644 > --- a/lib/cryptodev/rte_crypto_asym.h > +++ b/lib/cryptodev/rte_crypto_asym.h > @@ -119,6 +119,11 @@ enum rte_crypto_asym_op_type { > RTE_CRYPTO_ASYM_OP_LIST_END > }; >=20 > +#define RTE_CRYPTO_ASYM_FOREACH_OP_TYPE(i) \ > + for (i =3D RTE_CRYPTO_ASYM_OP_ENCRYPT; \ > + i <=3D RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE; \ > + i++) You must not use enum values in the .h, otherwise ABI compatibility is not = ensured. Yes you can do a macro, but it must call functions, not using direct values= =2E