From mboxrd@z Thu Jan 1 00:00:00 1970 From: Akhil Goyal Subject: Re: [v3,1/5] eventdev: introduce event crypto adapter Date: Mon, 7 May 2018 18:02:04 +0530 Message-ID: References: <1525546030-11204-1-git-send-email-abhinandan.gujjar@intel.com> <1525546030-11204-2-git-send-email-abhinandan.gujjar@intel.com> <20180507093516.GA8052@jerin> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: hemant.agrawal@nxp.com, akhil.goyal@nxp.com, dev@dpdk.org, narender.vangati@intel.com, nikhil.rao@intel.com, gage.eads@intel.com To: Jerin Jacob , Abhinandan Gujjar Return-path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0078.outbound.protection.outlook.com [104.47.2.78]) by dpdk.org (Postfix) with ESMTP id CB7792BAE for ; Mon, 7 May 2018 14:32:41 +0200 (CEST) In-Reply-To: <20180507093516.GA8052@jerin> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >> 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 >> Signed-off-by: Nikhil Rao >> Signed-off-by: Gage Eads >> --- >> 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 >