Hi Fan, > > Hi Akhil, > > Thanks for the review and comments! > Knowing you are extremely busy. Here is my point in brief: > I think placing the CPU synchronous crypto in the rte_security make sense, as > > 1. rte_security contains inline crypto and lookaside crypto action type > already, > adding cpu_crypto action type is reasonable.
The argument here is not about cpu-crypto, any SW PMD is nothing but a cpu-crypto. Hence cryptodev already support that. Here we are concerned only with synchronous processing for crypto workloads. > 2. rte_security contains the security features may not supported by all > devices, > such as crypto, ipsec, and PDCP. cpu_crypto follow this category, again > crypto. I do not get the intent of this comment. Looking at your patchset, what I get is, You need a synchronous API for crypto workloads. If sync processing is required for security payloads, we can add a sync API there as well. I have made that comment before also. We can have sync API in both security and cryptodev. > 3. placing CPU synchronous crypto API in rte_security is natural - as inline > mode > works synchronously, too. However cryptodev doesn't. It is a valid use case for all the cryptodev SW PMDs that there should be a synchronous API for crypto processing and that is what your usecase need. > 4. placing CPU synchronous crypto API in rte_security helps boosting SW crypto > performance, I have already provided a simple perf test inside the unit test > in the > patchset for the user to try out - just comparing its output against DPDK > crypto > perf app output. I don't expect any performance difference while moving this from security to cryptodev. Have you done any profiling? > 5. placing CPU synchronous crypto API in cryptodev will never serve HW > lookaside crypto PMDs, as making them to work synchronously have huge > performance penalty. However Cryptodev framework's existing design is > providing APIs that will work in all crypto PMDs (rte_cryptodev_enqueue_burst > / > dequeue_burst for example), this does not fit in cryptodev's principle. Agreed that it is not for HW PMDs, however the op may be null in those cases. Why it is against the cryptodev principle? There are some ops which PMDs may or may not support. > 6. placing CPU synchronous crypto API in cryptodev confuses the user, as: > - the session created for async mode may not work in sync mode Why? The whole idea for my conversations on this patchset talks about that. Same session should work for both sync and async processing. > - both enqueue/dequeue and cpu_crypto_process does the same crypto > processing, but one PMD may support only one API (set), the other may support > another, and the third PMD supports both. We have to provide another API to > let the user query which one to support which. This should be based on a Feature flag. It would be upto the application developer To decide which (sync/async) processing is required for which type of flows that it is configuring. > - two completely different code paths for async/sync mode. > 7. You said in the end of the email - placing CPU synchronous crypto API into > rte_security is not acceptable as it does not do any rte_security stuff - > crypto > isn't? You may call this a quibble, but in my idea, in the patchset both PMDs' > implementations did offload the work to the CPU's special circuit designed > dedicated to accelerate the crypto processing. This is specific to Intel SW PMDs only. IMO, if you are talking about SW PMDs, openssl can also benefit from this. > > To me cryptodev is the one CPU synchronous crypto API should not go into, > rte_security is. > > Regards, > Fan > Regards, Akhil
