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

[Akhil Goyal <akhil.goyal@nxp.com>]

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
>