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

[Anoob Joseph <anoobj@marvell.com>]

Hi Akhil, Ray, Thomas,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, October 13, 2021 12:32 PM
> To: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org; Kinsella, Ray
> <mdr@ashroe.eu>; Anoob Joseph <anoobj@marvell.com>
> 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 <rnagadheeraj@marvell.com>; Ankur Dwivedi
> <adwivedi@marvell.com>; ciara.power@intel.com; Stephen Hemminger
> <stephen@networkplumber.org>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> 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 <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.
> 

[Anoob] Understood. Will do that.

@Ray, @Akhil, you are also in agreement, right?