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