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

From: Anoob Joseph <hidden>
Date: 2021-10-13 07:04:47

Hi Akhil, Ray, Thomas,

Please see inline.

Thanks,
Anoob

-----Original Message-----
From: Thomas Monjalon <redacted>
Sent: Wednesday, October 13, 2021 12:32 PM
To: Akhil Goyal <redacted>; dev@dpdk.org; Kinsella, Ray
[off-list ref]; Anoob Joseph [off-list ref]
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 [off-list ref]; Ankur Dwivedi
[off-list ref]; ciara.power@intel.com; Stephen Hemminger
[off-list ref]; Yigit, Ferruh [off-list ref];
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:

quoted

From: Thomas Monjalon <redacted>

quoted

12/10/2021 16:47, Kinsella, Ray:

quoted

On 12/10/2021 15:18, Anoob Joseph wrote:

quoted

From: Thomas Monjalon <redacted>

quoted

12/10/2021 15:38, Anoob Joseph:

quoted

From: Thomas Monjalon <redacted>

quoted

12/10/2021 13:34, Anoob Joseph:

quoted

From: Kinsella, Ray <redacted>

quoted

On 12/10/2021 11:50, Anoob Joseph wrote:

quoted

From: Akhil Goyal <redacted>

quoted

On 08/10/2021 21:45, Akhil Goyal wrote:

quoted

Remove *_LIST_END enumerators from asymmetric

crypto

quoted

lib to

quoted

avoid ABI breakage for every new addition in enums.

Signed-off-by: Akhil Goyal <redacted>
---
-	} else if (xform->xform_type >=

RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END

quoted

+	} else if (xform->xform_type >

RTE_CRYPTO_ASYM_XFORM_ECPM
[...]

quoted

quoted
quoted
quoted
quoted
So I am not sure that this is an improvement.

Indeed, it is not an improvement.

quoted

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?

quoted

Yes we did this same exercise for symmetric crypto enums

earlier.

quoted

Asym enums were left as it was experimental at that point.
They are still experimental, but thought of making this
uniform throughout DPDK enums.

quoted

I am not sure that swapping out _LIST_END, and then
littering the code with RTE_CRYPTO_ASYM_XFORM_ECPM

and

quoted

quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE, is an
improvement
quoted
quoted
quoted
quoted
here.
quoted
My 2c is that from an ABI PoV
RTE_CRYPTO_ASYM_OP_LIST_END is
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
not better or worse, than
RTE_CRYPTO_ASYM_OP_SHARED_SECRET_COMPUTE?
quoted
quoted
quoted
Interested to hear other thoughts.
I don’t have any better solution for avoiding ABI issues for

now.

quoted

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?

quoted

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.

quoted

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?

quoted

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.

quoted

The app is supposed to test "n" asymmetric features supported
by

DPDK.

quoted

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?

quoted

Yes

quoted

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.

quoted

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?

quoted

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)

quoted

                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)

quoted

                                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?

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help