Re: [dpdk-dev] [PATCH v3 5/5] doc: add documentation for multi process crypto app

["Kusztal, ArkadiuszX" <arkadiuszx.kusztal@intel.com>]

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Wednesday, July 15, 2020 8:22 PM
> To: Kusztal, ArkadiuszX <arkadiuszx.kusztal@intel.com>; dev@dpdk.org
> Cc: Trahe, Fiona <fiona.trahe@intel.com>; thomas@monjalon.net
> Subject: RE: [PATCH v3 5/5] doc: add documentation for multi process crypto
> app
> 
> Hi Arek,
> 
> > Subject: [PATCH v3 5/5] doc: add documentation for multi process
> > crypto app
> 
> Please do not make separate patches for documentation.
> Your first patch description says more info can be found in mp_crypto.rst.
> But it is not there in that patch.
> 
> You should add the relevant documentation in the first patch so that people
> can review and understand the patchset before actually reviewing the patch.
> 
> You should split the documentation patch and add in the patches where they
> are relevant.
[AK] - sure, I will.
> 
> MAINTAINERS file update is also missing for this app.
> 
> >
> 
> > This commit adds documentation for multi process crypto test
> > application.
> >
> > Signed-off-by: Arek Kusztal <arkadiuszx.kusztal@intel.com>
> > ---
> >  doc/guides/tools/index.rst     |   1 +
> >  doc/guides/tools/mp_crypto.rst | 151
> > +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 152 insertions(+)
> >  create mode 100644 doc/guides/tools/mp_crypto.rst
> >
> > diff --git a/doc/guides/tools/index.rst b/doc/guides/tools/index.rst
> > index 4840cf4..a360307 100644
> > --- a/doc/guides/tools/index.rst
> > +++ b/doc/guides/tools/index.rst
> > @@ -17,3 +17,4 @@ DPDK Tools User Guides
> >      cryptoperf
> >      comp_perf
> >      testeventdev
> > +    mp-crypto
> > diff --git a/doc/guides/tools/mp_crypto.rst
> > b/doc/guides/tools/mp_crypto.rst new file mode 100644 index
> > 0000000..201834f
> > --- /dev/null
> > +++ b/doc/guides/tools/mp_crypto.rst
> > @@ -0,0 +1,151 @@
> > +..  SPDX-License-Identifier: BSD-3-Clause
> > +    Copyright(c) 2020 Intel Corporation.
> > +
> > +dpdk-test-mp-crypto Application
> > +===============================
> > +
> > +The Multi-process Crypto application is a simple application that
> > +allows to run crypto related operations in a multiple process
> > +environment. It builds on the EAL primary/secondary process
> infrastructure.
> > +
> > +The application allows a user to configure devices, setup
> > +queue-pairs, create and init sessions and specify data-path flow
> > +(enqueue/dequeue) in different processes. The app can help to check
> > +if the PMD behaves correctly in scenarios like the following:
> > +
> > +* device is configured in primary process, queue-pairs are setup in
> > +secondary
> > process
> > +
> > +* queue pair is shared across processes, i.e. enqueue in one process
> > +and
> > dequeue in another
> > +
> > +
> > +Compiling the Application
> > +-------------------------
> > +
> > +To compile the sample application see :doc:`compiling`.
> > +
> > +The application is located in the ``app/test-mp_crypto`` directory.
> > +
> > +Running the Application
> > +-----------------------
> > +
> > +App binary: dpdk-test-mp-crypto (in build/app)
> > +
> > +For running PRIMARY or SECONDARY process standard EAL options apply:
> > +
> > +.. code-block:: console
> > +
> > +    ./dpdk-test-mp-crypto --proc-type primary
> > +
> > +    ./dpdk-test-mp-crypto --proc-type secondary
> > +
> > +.. Note::
> > +
> > +	The same set of BDFs must be passed to all processes.
> 
> BDFs ?? Statement should be generic. Not all devices are PCI based.
[Arek] Sure, will change.
> 
> > +
> > +.. Note::
> > +	The same crypto devices must be created in all processes, e.g. in qat
> > +	case if asym and sym devices are enabled in the primary process,
> they
> > +	must be enabled in all secondary processes.
> In all secondary processes as well.
> 
> > +
> > +General help can by checked by running:
> > +
> > +. code-block:: console
> > +
> > +   ./dpdk-test-mp-crypto -- -h
> > +
> > +The application has a number of command line options:
> > +
> > +.. code-block:: console
> > +
> > +    ./dpdk-test-mp-crypto -- --devtype [dev-name]
> > +
> > +This option specifies which driver to use by its name (for example
> "crypto_qat").
> > +The same name must be passed to all processes.
> > +
> > +.. code-block:: console
> > +
> > +    ./dpdk-test-mp-crypto -- --config_dev [devA, devB,]
> > +
> > +This option specifies the list of devices that should be configured
> > +by this
> > process,
> > +this results in a call to the ``rte_cryptodev_configure`` API. devX
> > +is a positive integer (including zero), the value is according to
> > +probe order (from the
> > smallest
> > +BDF number), not necessarily the cmdline order.
> 
> Isn't it better to have a cryptodev_mask instead of config_dev? Same as we
> do in Ipsec-secgw and l2fwd-crypto.
> Mask will specify the devices which will be configured in the current process.
> 
[AK] - actually we initially have done that, in code actually list is translated to mask because of this. We though later list can be easier to use.
> 
> > +
> > +Example command:
> > +
> > +.. code-block:: console
> > +
> > +    ./dpdk-test-mp-crypto -w 03:01.2 -w 03:01.1 -w 03:01.3
> > + --config-dev 0,2
> > +
> > ++will configure devices 03:01.1 and 03:01.3.
> > +
> > +.. code-block:: console
> > +
> > +    ./dpdk-test-mp-crypto -- --qp-config=[devA]:[qp_A,
> > + qp_B,];[devB]:[qp_A,
> > qp_C];
> > +
> > +devX - positive integer (including zero), as in config_dev command
> > +
> > ++qp_X - positive integer (including zero), specifies which queue pair
> > ++should be
> > setup
> > +
> > +This command specifies which queue pairs should be setup, resulting
> > +in a call to ``rte_cryptodev_queue_pair_setup`` API.
> > +
> > +.. code-block:: console
> > +
> > +    ./dpdk-test-mp-crypto -w 03:01.2 -w 03:01.1 -w 03:01.3 --qp-
> > config="0:0,1;1:1;2:0,1;"
> > +
> > +This command will configure queue pairs 0 and 1 on device 0
> > +(03:01.1), queue
> > pair 1
> > +on device 1 (03:01.2), queue pairs 0 and 1 on device 2 (03:01.3). The
> > +device in
> > question
> > +should be configured before that, though not necessarily by the same
> process.
> > +
> > +.. code-block:: console
> > +
> > +    ./dpdk-test-mp-crypto -- --enq=[devX]:[qpX]:[ops]:[vector_id]
> > +    ./dpdk-test-mp-crypto -- --deq=[devX]:[qpX]:[ops]:[vector_id]
> > +
> > +devX - positive integer (including zero), as in config_dev command
> > +
> > +qp_X - positive integer (including zero), as in qp-config command
> 
> qpX
[Arek] Ok.
> 
> > +
> > +ops - when positive integer - number of operations to
> > +enqueue/dequeue, when
> > 0 infinite loop
> 
> This should be an optional parameter and should not be part of enq/deq
> configs And default value should be 0
[Arek] So something like (if vector number removed):
--enq=0:0, -> dev 0, qp 0 , infinite loop
--enq=0:0:5000, -> dev 0, qp 0, 5000 packets
?
> 
> > +
> > +vector_id - positive integer (including zero), vector_id used by this
> > +process
> 
> What is vector id? I do not see it changing in any of your examples. Do we
> really need it?
[Arek] - with current implementation it could be dropped, but initially it was created for multiple vectors. Eventually may be bring back in future.
> 
> It looks that the dev id, qp_id are redundant in all three. We can probably
> squeeze it in a single config
> --enq=(devA:qpX,qpY),(devB:qpX)
> --deq=(devC:qpX,qpY),(devB:qpY,qpZ)
[Arek] - comma separates processes here? So 3 processes for enqueue, 4 for dequeue?
> 
> And remove config-dev and qp-config
[Arek] - not sure about that, initially first use case we were targeting was to do all config in one process and enq/deq in other process which can be quite popular use case.
> 
> Cumulative sum of all the devices(dev - A,B,C)/queues(A->X,Y; B->X,Y,Z;C-
> >X,Y) in enq and deq can be configured in that process.
> This will be simple to configure from user perspective and will reduce the risk
> of setting mismatch configuration.
> 
> And if the enq and deq are same, we should be able to skip either of the
> configs and assume that it is same as the other one.
> 
> What say?
> 
> > +
> > +This commands will enqueue/dequeue "ops" number of packets to qp_X
> on
> > devX.
> > +Example usage:
> > +
> > +.. code-block:: console
> > +
> > +    ./dpdk-test-mp-crypto -- --enq=2:0:0:0, --deq=2:0:0:0,
> > +
> > +Note. ',' comma character is necessary at the end due to some parser
> > shortcomings.
> > +
> > +To close the application when running in an infinite loop a signal
> > +handler is registered to catch interrupt signals i.e. ``ctrl-c``
> > +should be used. When used in primary process other processes will be
> > +notified about exiting intention and will close after collecting remaining
> packets (if dequeuing).
> > +
> > +Example commands
> > +----------------
> > +
> > +Use two different devices on 3 separate queues:
> > +
> > +.. code-block:: console
> > +
> > +    ./dpdk-test-mp-crypto --proc-type primary -c 1 -w 03:01.1 -w
> > + 03:01.2 -- --
> > devtype "crypto_qat" --config-dev 0,1   --qp-config="0:0,1;1:0,1;" --session-
> > mask=0x3  --enq=0:0:0:0, --deq=0:0:0:0,  --print-stats
> 
> You did not explain session-mask above.
> 
> > +    ./dpdk-test-mp-crypto --proc-type secondary -c 2 -w 03:01.1 -w
> > + 03:01.2 -- --
> > devtype "crypto_qat"  --enq=0:1:0:0, --deq=0:1:0:0,  --print-stats
> > +    ./dpdk-test-mp-crypto --proc-type secondary -c 4 -w 03:01.1 -w
> > + 03:01.2 -- --
> > devtype "crypto_qat"  --enq=1:0:0:0, --deq=1:0:0:0,  --print-stats
> > +
> > +Use different processes to enqueue and dequeue to one queue pair:
> > +
> > +.. code-block:: console
> > +
> > +    ./dpdk-test-mp-crypto --proc-type primary -c 1 -w 03:01.1 --
> > + --devtype
> > "crypto_qat" --config-dev 0    --session-mask=0x3 --qp-config="0:1;"   --
> > enq=0:1:0:0,   --print-stats
> > +    ./dpdk-test-mp-crypto --proc-type secondary -c 2 -w 03:01.1 --
> > + --devtype
> > "crypto_qat"  --deq=0:1:0:0,   --print-stats
> 
> We can probably add a sample print-stats output here
[Arek] - Sure.
> 
> > +
> > +Limitations
> > +-----------
> > +
> > +Only one crypto vector and session type is possible to chose right
> > +now and it is
> > AES-GCM test case.
> > +
> > +Number of descriptors if set by default to 4096
> 
> Number of descriptors is set by default to 4096
> 
> > --
> > 2.1.0