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

[Thomas Monjalon]

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.

Feel free to consider the alternative which fits the best in cryptodev.