Re: [dpdk-dev] [v3,1/5] eventdev: introduce event crypto adapter

["Gujjar, Abhinandan S" <abhinandan.gujjar@intel.com>]

Hi Akhil,

Thanks for the feedback on the diagram. My thoughts are also in line with diagram.
In fact, my diagram also depicts same. The only difference is that, you have shown application in a separate block.
The "crypto stage + enqueue to cryptodev" block itself is part of an application.
So, for more clarity, I will add "application" as additional text to the block.
With the limited time, changing diagram needs change in SVG file as well.
In case, if you still feel there a change required we can take it up later.

Thanks
Abhinandan

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Monday, May 7, 2018 6:02 PM
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>; Gujjar, Abhinandan S
> <abhinandan.gujjar@intel.com>
> Cc: hemant.agrawal@nxp.com; akhil.goyal@nxp.com; dev@dpdk.org; Vangati,
> Narender <narender.vangati@intel.com>; Rao, Nikhil <nikhil.rao@intel.com>;
> Eads, Gage <gage.eads@intel.com>
> Subject: Re: [dpdk-dev] [v3,1/5] eventdev: introduce event crypto adapter
> 
> Hi Jerin, Abhinandan,
> Overall the patch looks good.
> But one comment on block diagram for OP_NEW mode functioning.
> The comment was also made on previous version but it looks the intent was
> misunderstood.
> 
> 
> On 5/7/2018 3:05 PM, Jerin Jacob wrote:
> > -----Original Message-----
> >> Date: Sun, 6 May 2018 00:17:06 +0530
> >> From: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> >> To: jerin.jacob@caviumnetworks.com, hemant.agrawal@nxp.com,
> >>  akhil.goyal@nxp.com, dev@dpdk.org
> >> CC: narender.vangati@intel.com, abhinandan.gujjar@intel.com,
> >>  nikhil.rao@intel.com, gage.eads@intel.com
> >> Subject: [v3,1/5] eventdev: introduce event crypto adapter
> >> X-Mailer: git-send-email 1.9.1
> >>
> >> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> >> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
> >> Signed-off-by: Gage Eads <gage.eads@intel.com>
> >> ---
> >>  MAINTAINERS                                    |   5 +
> >>  lib/librte_eventdev/rte_event_crypto_adapter.h | 554
> +++++++++++++++++++++++++
> >>  2 files changed, 559 insertions(+)
> >>  create mode 100644 lib/librte_eventdev/rte_event_crypto_adapter.h
> >>
> >
> > Overall it looks good.
> >
> > #1)
> >
> > Please fix the following ./devtools/checkpatches.sh warning.
> > ➜ [master]laptop [dpdk.org] $ ./devtools/checkpatches.sh
> >
> > ### eventdev: add crypto adapter implementation
> >
> > WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier
> > tag in line 1
> > #106: FILE: lib/librte_eventdev/rte_event_crypto_adapter.c:1:
> > +/* SPDX-License-Identifier: BSD-3-Clause
> >
> > ### test: add event crypto adapter auto-test
> >
> > WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier
> > tag in line 1
> > #38: FILE: test/test/test_event_crypto_adapter.c:1:
> > +/* SPDX-License-Identifier: BSD-3-Clause
> >
> > total: 0 errors, 1 warnings, 927 lines checked
> >
> > ### doc: add event crypto adapter documentation
> >
> > WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier
> > tag in line 1
> > #41: FILE: doc/guides/prog_guide/event_crypto_adapter.rst:1:
> > +..  SPDX-License-Identifier: BSD-3-Clause
> >
> >  * In the RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode, if HW supports
> >  * RTE_EVENT_CRYPTO_ADAPTER_CAP_INTERNAL_PORT_OP_FWD capability
> the
> >  * application
> >  * can directly submit the crypto operations to the cryptodev.
> >  * If not,
> >
> >
> > #2) I have added minor changes in description, Wherever it makes sense
> > to you then please pull it for next revision. Else we can discuss more.
> >
> > a) I have uploaded the diff at https://ufile.io/247t9 for
> > you convince.
> > b) Please update the similar change in programmers guide too.
> >
> >
> > diff --git a/lib/librte_eventdev/rte_event_crypto_adapter.h
> b/lib/librte_eventdev/rte_event_crypto_adapter.h
> > index 2c1f54f76..55fbdc55e 100644
> > --- a/lib/librte_eventdev/rte_event_crypto_adapter.h
> > +++ b/lib/librte_eventdev/rte_event_crypto_adapter.h
> > @@ -23,14 +23,17 @@
> >   * between the crypto device and the event device.
> >   *
> >   * The application can choose to submit a crypto operation directly to
> > - * crypto device or send it to the crypto adapter via eventdev, the crypto
> > - * adapter then submits the crypto operation to the crypto device.
> > - * The first mode is known as the event new (OP_NEW) mode and the
> > - * second as the event forward (OP_FORWARD) mode. The choice of mode
> can
> > - * be specified while creating the adapter.
> > + * crypto device or send it to the crypto adapter via eventdev based on
> > + * RTE_EVENT_CRYPTO_ADAPTER_CAP_INTERNAL_PORT_OP_FWD
> capability.
> > + * The first mode is known as the event
> new(RTE_EVENT_CRYPTO_ADAPTER_OP_NEW)
> > + * mode and the second as the event
> forward(RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD)
> > + * mode. The choice of mode can be specified while creating the adapter.
> > + * In the former mode, it is an application responsibility to enable ingress
> packet
> > + * ordering. In the latter mode, it is the adapter responsibility to enable
> > + * the ingress packet ordering.
> >   *
> >   *
> > - * Working model of OP_NEW mode:
> > + * Working model of RTE_EVENT_CRYPTO_ADAPTER_OP_NEW mode:
> >   *
> >   *                +--------------+         +--------------+
> >   *        --[1]-->|              |         | Crypto stage |
> > @@ -47,25 +50,27 @@
> >   *                |              |         |              |
> >   *                +--------------+         +--------------+
> >   *
> > - *         [1] Events from the previous stage.
> > + *         [1] Events from the previous stage and enqueue to crypto/atomic
> stage
> >   *         [2] Application in atomic stage dequeues events from eventdev.
> > - *         [3] Crypto operations are submitted to cryptodev.
> > + *         [3] Crypto operations are submitted to cryptodev by application.
> >   *         [4] Crypto adapter dequeues crypto completions from cryptodev.
> >   *         [5] Crypto adapter enqueues events to the eventdev.
> >   *         [6] Events to the next stage.
> 
> I think the sequence should be as follows:
> [1] Application dequeues from the previous stage.
> [2] Application prepare for enqueue to cryptodev
> [3] Application enqueues to cryptodev
> [4] Crypto adapter dequeues crypto completions from cryptodev.
> [5] Crypto adapter enqueues events to the eventdev.
> [6] Application dequeues from eventdev and prepare for further processing.
> 
> So the Block diagram should be something like
> 
> + *                +--------------+         +--------------+
> + *                |              |         | Crypto stage |
> + *                | Application  |---[2]-->| + enqueue to |
> + *                |              |         |   cryptodev  |
> + *                +--------------+         +--------------+
> + *                    ^   ^                       |
> + *                    |   |                      [3]
> + *                   [6] [1]                      |
> + *                    |   |                       |
> + *                +--------------+                |
> + *                |              |                |
> + *                | Event device |                |
> + *                |              |                |
> + *                +--------------+                |
> + *                       ^                        |
> + *                       |                        |
> + *                      [5]                       |
> + *                       |                        v
> + *                +--------------+         +--------------+
> + *                |              |         |              |
> + *                |Crypto adapter|<--[4]---|  Cryptodev   |
> + *                |              |         |              |
> + *                +--------------+         +--------------+
> Please let me know if my understanding is not correct.
> 
> 
> >   *
> > - * In the OP_NEW mode, application submits crypto operations directly to
> > - * crypto device. The adapter then dequeues crypto completions from crypto
> > + * In the RTE_EVENT_CRYPTO_ADAPTER_OP_NEW mode, application submits
> crypto
> > + * operations directly to crypto device.
> > + * The adapter then dequeues crypto completions from crypto
> >   * device and enqueue events to the event device.
> > - * This mode does not ensure ingress ordering. The application is expected
> > - * to be in atomic stage. Events dequeued from the adapter will be treated
> > - * as new events.
> > + * This mode does not ensure ingress ordering if the application directly
> > + * enqueues to cryptodev without going through crypto/atomic stage. i.e
> removing
> > + * item [1] and [2].
> > + * Events dequeued from the adapter will be treated as new events.
> >   * In this mode, application needs to specify event information (response
> >   * information) which is needed to enqueue an event after the crypto
> operation
> >   * is completed.
> >   *
> >   *
> > - * Working model of OP_FORWARD mode:
> > + * Working model of RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode:
> >   *
> >   *                +--------------+         +--------------+
> >   *        --[1]-->|              |---[2]-->|              |
> > @@ -93,8 +98,9 @@
> >   *         [7] Crypto adapter enqueues events to the eventdev
> >   *         [8] Events to the next stage
> >   *
> > - * In the OP_FORWARD mode, if HW supports *_OP_FORWARD capability the
> > - * application can directly submit the crypto operations to the cryptodev.
> > + * In the RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode, if HW
> supports
> > + * RTE_EVENT_CRYPTO_ADAPTER_CAP_INTERNAL_PORT_OP_FWD
> capability the application
> > + * can directly submit the crypto operations to the cryptodev.
> >   * If not, application retrieves crypto adapter's event port using
> >   * rte_event_crypto_adapter_event_port_get() API. Then, links its event
> >   * queue to this port and starts enqueuing crypto operations as events
> > @@ -121,7 +127,7 @@
> >   *  - rte_event_crypto_adapter_stop()
> >   *  - rte_event_crypto_adapter_stats_get()
> >   *  - rte_event_crypto_adapter_stats_reset()
> > -
> > + *
> >   * The application creates an instance using
> rte_event_crypto_adapter_create()
> >   * or rte_event_crypto_adapter_create_ext().
> >   *
> > @@ -173,8 +179,10 @@ enum rte_event_crypto_adapter_mode {
> >  	/**< Start the crypto adapter in event forward mode.
> >  	 * @see RTE_EVENT_OP_FORWARD.
> >  	 * Application submits crypto requests as events to the crypto
> > -	 * adapter. Adapter submits crypto requests to the cryptodev
> > -	 * and crypto completions are enqueued back to the eventdev.
> > +	 * adapter or crypto device based on
> > +	 * RTE_EVENT_CRYPTO_ADAPTER_CAP_INTERNAL_PORT_OP_FWD
> capability.
> > +	 * crypto completions are enqueued back to the eventdev by
> > +	 * crypto adapter.
> >  	 */
> >  };
> >
> > @@ -215,11 +223,12 @@ struct rte_event_crypto_request {
> >  union rte_event_crypto_metadata {
> >  	struct rte_event_crypto_request request_info;
> >  	/**< Request information to be filled in by application
> > -	 * for OP_FORWARD mode.
> > +	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW mode.
> >  	 */
> >  	struct rte_event response_info;
> >  	/**< Response information to be filled in by application
> > -	 * for OP_NEW and OP_FORWARD mode.
> > +	 * for RTE_EVENT_CRYPTO_ADAPTER_OP_NEW and
> > +	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> >  	 */
> >  };
> >
> > @@ -234,7 +243,8 @@ union rte_event_crypto_metadata {
> >  struct rte_event_crypto_adapter_conf {
> >  	uint8_t event_port_id;
> >  	/**< Event port identifier, the adapter enqueues events to this
> > -	 * port and dequeues crypto request events in OP_FORWARD mode.
> > +	 * port and dequeues crypto request events in
> > +	 * RTE_EVENT_CRYPTO_ADAPTER_OP_FORWARD mode.
> >  	 */
> >  	uint32_t max_nb;
> >  	/**< The adapter can return early if it has processed at least
> >