Re: [dpdk-dev] [dpdk-dev v4 1/4] cryptodev: add symmetric crypto data-path APIs

["Zhang, Roy Fan" <roy.fan.zhang@intel.com>]

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Monday, July 6, 2020 1:13 PM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org;
> anoobj@marvell.com; asomalap@amd.com; ruifeng.wang@arm.com;
> Nagadheeraj Rottela <rnagadheeraj@marvell.com>; Michael Shamis
> <michaelsh@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>; Jay
> Zhou <jianjay.zhou@huawei.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Cc: Trahe, Fiona <fiona.trahe@intel.com>; Bronowski, PiotrX
> <piotrx.bronowski@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Subject: RE: [dpdk-dev v4 1/4] cryptodev: add symmetric crypto data-path
> APIs
> 
...
> >
> > As you may have seen the structure definition is hW centric with the
> > IOVA addresses all over. Also as you will from the patch series the
> operation is
> > Per operation basis instead of operating in a burst. The external application
> > may sooner know when a specific enqueue is failed.
> 
> You may also need to save a virtual address as well. As some hardware are
> able to
> Convert virtual to physical addresses on it's own giving a performance
> improvement.
> 
> I do not see an issue in using enqueue burst with burst size=1 , but since you
> are doing
> Optimizations, none of the hardware can perform well with burst = 1, I think
> it is always
> Greater than 1.

Shall I update the rte_crypto_sym_vec as the following - so the 2 problems can be
resolved?

struct rte_crypto_sym_vec {
	/** array of SGL vectors */
	struct rte_crypto_sgl *sgl;
	union {
		/* Supposed to be used with CPU crypto API call. */
		struct {
			/** array of pointers to IV */
			void **iv;
			/** array of pointers to AAD */
			void **aad;
			/** array of pointers to digest */
			void **digest;
			/**
			 * array of statuses for each operation:
			 *  - 0 on success
			 *  - errno on error
			 */
			int32_t *status;
		};

		/* Supposed to be used with HW crypto API call. */
		struct {
			/** array of pointers to IV */
			struct rte_crypto_vec *iv_hw;
			/** array of pointers to AAD */
			struct rte_crypto_vec *aad_hw;
			/** array of pointers to Digest */
			struct rte_crypto_vec *digest_hw;
		};

	};
	/** number of operations to perform */
	uint32_t num;
};

> >
> > > Now for this patchset, the requirement is
> > > - raw buffers
> > > - asynchronous APIs
> > >
> > > The data structure for raw buffers and crypto related offsets are already
> > > defined
> > > So they should be reused.
> > > And I believe with some changes in rte_crypto_op  and
> rte_crypto_sym_op,
> > > We can support raw buffers with the same APIs.
> > > Instead of m_src and m_dst, raw buffer data structures can be combined
> in a
> > > Union and some of the fields in the rte_crypto_op can be left NULL in
> case of
> > > raw buffers.
> > >
> >
> > This is a good point but we still face too many unnecessary fields to be
> NULL,
> > such as
> > digest pointers, I have given a lot thought to this structure. Hopefully it
> covers
> > all vendor's HW symmetric crypto needs and in the same time it well
> squeeze
> > the required HW addresses into 1 cacheline, instead of rte_crypto_op +
> > rte_crypto_sym_op 3 cacheline footprint. Another purpose of the
> structure
> > design
> > is the structure buffer can be taken from stack and can be used to fill all
> > jobs to the PMD HW.
> 
> Which fields you think are not useful and should be set as NULL?
> Digest pointers you are anyways setting in the new structure.
> Your new struct does not support session less as well as security sessions.
> It does not take care of asymmetric crypto.
> So whenever, a vendor need to support all these, we would end up getting
> the rte_crypto_op structure.
> IMO, you only need to make m_src and m_dst as union to a raw
> input/output
> buffers. Everything else will be relevant.
> 

Rte_crypto_op is designed to be allocated from mempool with HW address
info contained so it is possible to deduct IV and AAD physical address from
it. More importantly rte_crypto_op is designed to be taken from heap and 
being freed after dequeue. So they cannot be allocated from stack - for this
reason I think rte_crypot_sym_vec is a better fit for the patch, do you agree? 
(the Proposed change is at above).

> Have you done some profiling with using rte_crypto_op instead of this new
> struct?
> 
Yes, the code are actually upstreamed in VPP
https://gerrit.fd.io/r/c/vpp/+/18036, please try out. If you have a look at the
enqueue/dequeue functions you should see the struggle we had to translate
ops, and creating a second software ring to make sure we only dequeue a 
frame of data. Lucky VPP has space to store mbufs otherwise the perf will
be even worse.

> >
> > >
> > > > +/* Struct for user to perform HW specific enqueue/dequeue function
> calls
> > > */
> > > > +struct rte_crypto_hw_ops {
> > > > +	/* Driver written queue pair data pointer, should NOT be alterred by
> > > > +	 * the user.
> > > > +	 */
> > > > +	void *qp;
> > > > +	/* Function handler to enqueue AEAD job */
> > > > +	rte_crypto_hw_enq_cb_fn enqueue_aead;
> > > > +	/* Function handler to enqueue cipher only job */
> > > > +	rte_crypto_hw_enq_cb_fn enqueue_cipher;
> > > > +	/* Function handler to enqueue auth only job */
> > > > +	rte_crypto_hw_enq_cb_fn enqueue_auth;
> > > > +	/* Function handler to enqueue cipher + hash chaining job */
> > > > +	rte_crypto_hw_enq_cb_fn enqueue_chain;
> > > > +	/* Function handler to query processed jobs */
> > > > +	rte_crypto_hw_query_processed query_processed;
> > > > +	/* Function handler to dequeue one job and return opaque data
> > > stored
> > > > */
> > > > +	rte_crypto_hw_deq_one_cb_fn dequeue_one;
> > > > +	/* Function handler to dequeue many jobs */
> > > > +	rte_crypto_hw_deq_many_cb_fn dequeue_many;
> > > > +	/* Reserved */
> > > > +	void *reserved[8];
> > > > +};
> > >
> > > Why do we need such callbacks in the library?
> > > These should be inside the drivers, or else we do the same for
> > > Legacy case as well. The pain of finding the correct enq function for
> > > Appropriate crypto operation is already handled by all the drivers
> > > And we can reuse that or else we modify it there as well.
> > >
> >
> > Providing different types of enqueue functions for specific operation type
> > could save a lot of branches for the driver to handle. As mentioned this
> > data-path API is intended to be used as an advanced feature to provide
> > close-to-native perf to external library/applications that are not mbuf
> > centric. And I don't agree classifying choosing 1 enqueue function from
> > 4 candidates as "pain".
> 
> My point is why don't we have it in the Legacy code path as well?
> I think it is useful in both the paths. Branching is a pain for the driver.
> 

That's a good point :-) we definitely can do something about it in future releases.

> >
> > > We should not add a lot of data paths for the user, otherwise the
> > > APIs will become centric to a particular vendor and it will be very difficult
> > > For the user to migrate from one vendor to another and would defeat
> the
> > > Purpose of DPDK which provide uniform abstraction layer for all the
> > > hardware
> > > Vendors.
> > >
> >
> > The purpose of adding data-path for the user is performance for non-mbuf
> > data-path centric applications/libraries, in the meantime not creating
> > confusion. In this version we aim to provide a more friendly data-path for
> 
> I do not see the new path as friendly.
> Adding a parallel new datapath with create more confusion for the
> application
> developer. It would be convenient, if we can use the same path with minimal
> changes so that people can migrate easily.
> 

We are working on next version that is based on rte_crypto_sym_vec and single
Enqueue-then-dequeue API. To be honest it won't be that of friendly to application
developer that the applications are mbuf-based or already built on top of cryptodev,
however for the applications that are not mbuf based and having their own data-path
structures it will be surely friendlier than existing enqueue and dequeue APIs. No
dependency to mbuf, mbuf and crypto op pool, and no need to consider how to adapt
different working methods.

> > them, and aims to be useful to all vendor's PMDs. If there is any place in
> > the API that blocks a PMD please let me know.
> 
> As commented above, sessionless, rte_security sessions, asymmetric crypto
> Not supported.
> 
You are right - 
Sessionless support aims the usability and is not intended to be used in high-throughput
Application.
Rte_security is built on top of mbuf and ethdev and is not intended to "mbuf-independent"
applications either.

> 
> >
> > > Adding other vendors to comment.
> > >
> > > Regards,
> > > Akhil

Regards,
Fan