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

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

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.

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.

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


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

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

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

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)

And remove config-dev and qp-config

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

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