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 B1FBEA0C4D; Wed, 13 Oct 2021 10:39:33 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 840DD4116C; Wed, 13 Oct 2021 10:39:33 +0200 (CEST) Received: from mail-108-mta57.mxroute.com (mail-108-mta57.mxroute.com [136.175.108.57]) by mails.dpdk.org (Postfix) with ESMTP id 2BAFB41162 for ; Wed, 13 Oct 2021 10:39:32 +0200 (CEST) Received: from filter004.mxroute.com ([149.28.56.236] filter004.mxroute.com) (Authenticated sender: mN4UYu2MZsgR) by mail-108-mta57.mxroute.com (ZoneMTA) with ESMTPSA id 17c78ce71870000b55.004 for (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES128-GCM-SHA256); Wed, 13 Oct 2021 08:39:27 +0000 X-Zone-Loop: e6c217e9e1ba2190fbcda699308c6d797a8793b71730 X-Originating-IP: [149.28.56.236] DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=ashroe.eu; s=x; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:MIME-Version:Date: Message-ID:From:References:Cc:To:Subject:Sender:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=/f3Akf3qIjBdTcuYec7emB/HXMgM7QJoTXRy+wudZ30=; b=YtG9iXZtNHBsvXEEp6zbK1x3lU ouRsUlT3KyEP3yiHzxs2EmV5SoCyde37+F62SZ9RjaySGKmvsqT5hAPpsTXxGlidCYuNQazEnentn obq7dxLBzLKV0BOW7pJTD+DYUFn8vykwplYOact+QU2WRvm4BXIbhhCvFYOVMXZoYNwvkoPYYscmw PZiwj03t4cOAORqire1XUGpXrNOLKJQrJMPkMcmYTIWtsQ7GzRUkUXfv2310hTVS6lM0WkniqOOM7 xKApYL4zhVAb6b/eTB9A2lw2A0223PqGEIydwMPU6k1mioO1SsQm5OPY0M7G45WJ7UIDKUS/QrAIW EiPEoSqA==; To: Anoob Joseph , Thomas Monjalon , Akhil Goyal , "dev@dpdk.org" 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" References: <20210731181327.660296-1-gakhil@marvell.com> <1716871.KAiom3yBSL@thomas> <1842444.xqXZe6RN4B@thomas> From: "Kinsella, Ray" Message-ID: Date: Wed, 13 Oct 2021 09:39:21 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-AuthUser: mdr@ashroe.eu X-Zone-Spam-Resolution: no action X-Zone-Spam-Status: No, score=-0.1, required=15, tests=[ARC_NA=0, TO_DN_EQ_ADDR_SOME=0, RCPT_COUNT_TWELVE=0, FROM_HAS_DN=0, TO_DN_SOME=0, MIME_GOOD=-0.1, FROM_EQ_ENVFROM=0, MIME_TRACE=0, RCVD_COUNT_ZERO=0, NEURAL_SPAM=0, MID_RHS_MATCH_FROM=0] 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" On 13/10/2021 08:04, Anoob Joseph wrote: > Hi Akhil, Ray, Thomas, > > Please see inline. > > Thanks, > Anoob > >> -----Original Message----- >> From: Thomas Monjalon >> Sent: Wednesday, October 13, 2021 12:32 PM >> 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 >> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v2 1/3] cryptodev: remove >> LIST_END enumerators >> >> 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 >= >>>>>>>>>>>>>> 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’t have any better solution for avoiding ABI issues 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? >>>> >>>> Or 3 iterators (foreach construct). >>>> Instead of just returning a size, we can have an iterator for each >>>> enum which needs to be iterated. >>> >>> [Anoob] Something like this? >>> >>> diff --git a/app/test/test_cryptodev_asym.c >>> b/app/test/test_cryptodev_asym.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 = 0; i < RTE_CRYPTO_ASYM_OP_LIST_END; i++) { >>> + RTE_CRYPTO_ASYM_FOREACH_OP_TYPE(i) { >>> if (tc.modex.xform_type == RTE_CRYPTO_ASYM_XFORM_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_asym.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 >>> }; >>> >>> +#define RTE_CRYPTO_ASYM_FOREACH_OP_TYPE(i) \ >>> + for (i = RTE_CRYPTO_ASYM_OP_ENCRYPT; \ >>> + i <= 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. >> > > [Anoob] Understood. Will do that. > > @Ray, @Akhil, you are also in agreement, right? > Yes - whether you use the MACRO or not less important. In order to maintain the ABI ... you need to learn the array size through an API.