[dpdk-dev] [EXT] Re: [PATCH v2 1/3] cryptodev: remove LIST_END enumerators

[Kinsella, Ray]

On 13/10/2021 08:04, Anoob Joseph wrote:
> Hi Akhil, Ray, Thomas,
> 
> Please see inline.
> 
> Thanks,
> Anoob
> 
>> -----Original Message-----
>> From: Thomas Monjalon <thomas at monjalon.net>
>> Sent: Wednesday, October 13, 2021 12:32 PM
>> To: Akhil Goyal <gakhil at marvell.com>; dev at dpdk.org; Kinsella, Ray
>> <mdr at ashroe.eu>; Anoob Joseph <anoobj at marvell.com>
>> Cc: david.marchand at redhat.com; hemant.agrawal at nxp.com;
>> pablo.de.lara.guarch at intel.com; fiona.trahe at intel.com;
>> declan.doherty at intel.com; matan at nvidia.com; g.singh at nxp.com;
>> roy.fan.zhang at intel.com; jianjay.zhou at huawei.com; asomalap at amd.com;
>> ruifeng.wang at arm.com; konstantin.ananyev at intel.com;
>> radu.nicolau at intel.com; ajit.khaparde at broadcom.com; Nagadheeraj
>> Rottela <rnagadheeraj at marvell.com>; Ankur Dwivedi
>> <adwivedi at marvell.com>; ciara.power at intel.com; Stephen Hemminger
>> <stephen at networkplumber.org>; Yigit, Ferruh <ferruh.yigit at intel.com>;
>> bruce.richardson at 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 <thomas at monjalon.net>
>>>> 12/10/2021 16:47, Kinsella, Ray:
>>>>> On 12/10/2021 15:18, Anoob Joseph wrote:
>>>>>> From: Thomas Monjalon <thomas at monjalon.net>
>>>>>>> 12/10/2021 15:38, Anoob Joseph:
>>>>>>>> From: Thomas Monjalon <thomas at monjalon.net>
>>>>>>>>> 12/10/2021 13:34, Anoob Joseph:
>>>>>>>>>> From: Kinsella, Ray <mdr at ashroe.eu>
>>>>>>>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
>>>>>>>>>>>> From: Akhil Goyal <gakhil at marvell.com>
>>>>>>>>>>>>>> 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 <gakhil at marvell.com>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>> -	} 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.