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

[Thomas Monjalon <thomas@monjalon.net>]

13/10/2021 07:36, Anoob Joseph:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 12/10/2021 16:47, Kinsella, Ray:
> > > On 12/10/2021 15:18, Anoob Joseph wrote:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > >> 12/10/2021 15:38, Anoob Joseph:
> > > >>> From: Thomas Monjalon <thomas@monjalon.net>
> > > >>>> 12/10/2021 13:34, Anoob Joseph:
> > > >>>>> From: Kinsella, Ray <mdr@ashroe.eu>
> > > >>>>>> On 12/10/2021 11:50, Anoob Joseph wrote:
> > > >>>>>>> From: Akhil Goyal <gakhil@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@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.