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

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

Hi Arek,

> >
> > 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>

We first need to decide where to put this application. 
Thomas, does not like the idea to have it in app/
Please analyze if it can be combined with l2fwd-crypto.

Regards,
Akhil

> > > ---
> > >  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