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

[Anoob Joseph <anoobj@marvell.com>]

Hi Thomas, Ray,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, October 12, 2021 8:37 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal
> <gakhil@marvell.com>; dev@dpdk.org; Kinsella, Ray <mdr@ashroe.eu>
> 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
> 
> 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++)
+
 /**
  * Padding types for RSA signature.
  */