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

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Mon, 7 May 2018 18:02:04 +0530
> From: Akhil Goyal <akhil.goyal@nxp.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Abhinandan Gujjar
>  <abhinandan.gujjar@intel.com>
> CC: hemant.agrawal@nxp.com, akhil.goyal@nxp.com, dev@dpdk.org,
>  narender.vangati@intel.com, nikhil.rao@intel.com, gage.eads@intel.com
> Subject: Re: [dpdk-dev] [v3,1/5] eventdev: introduce event crypto adapter
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
>  Thunderbird/45.8.0
> 
> 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.

Looks good to me.

/Jerin