Re: [dpdk-dev] [PATCH v3] cryptodev: add telemetry callbacks

["Power, Ciara" <ciara.power@intel.com>]

Hi Becky,

>-----Original Message-----
>From: Troy, Rebecca <rebecca.troy@intel.com>
>Sent: Thursday 7 October 2021 15:11
>To: dev@dpdk.org
>Cc: Power, Ciara <ciara.power@intel.com>; Zhang, Roy Fan
><roy.fan.zhang@intel.com>; Troy, Rebecca <rebecca.troy@intel.com>; Akhil
>Goyal <gakhil@marvell.com>; Doherty, Declan <declan.doherty@intel.com>
>Subject: [PATCH v3] cryptodev: add telemetry callbacks
>
>The cryptodev library now registers commands with telemetry, and
>implements the corresponding callback functions. These commands allow a
>list of cryptodevs to be queried, as well as info and stats for the corresponding
>cryptodev.
>
>An example usage can be seen below:
>
>Connecting to /var/run/dpdk/rte/dpdk_telemetry.v2
>{"version": "DPDK 21.11.0-rc0", "pid": 1135019, "max_output_len": 16384}
>--> /
>{"/": ["/", "/cryptodev/info", "/cryptodev/list", "/cryptodev/stats", ...]}
>--> /cryptodev/list
>{"/cryptodev/list": [0,1,2,3]}
>--> /cryptodev/info,0
>{"/cryptodev/info": {"device_name": "0000:1c:01.0_qat_sym", \
>	 "max_nb_queue_pairs": 2}}
>--> /cryptodev/stats,0
>{"/cryptodev/stats": {"enqueued_count": 0, "dequeued_count": 0, \
>	"enqueue_err_count": 0, "dequeue_err_count": 0}}
>
>Signed-off-by: Rebecca Troy <rebecca.troy@intel.com>
>
>---
>v3:
>  - Added missing version tag to patch.
>v2:
>  - Added documentation and release notes.
>  - Changed the /cryptodev/list command to list the devices as an
>	array of IDs, rather than as names and IDs.
>  - Added the /cryptodev/info command as described above.
>---
>---
> doc/guides/prog_guide/cryptodev_lib.rst | 30 ++++++++
>doc/guides/rel_notes/release_21_11.rst  |  5 ++
> lib/cryptodev/rte_cryptodev.c           | 92 +++++++++++++++++++++++++
> 3 files changed, 127 insertions(+)
>
>diff --git a/doc/guides/prog_guide/cryptodev_lib.rst
>b/doc/guides/prog_guide/cryptodev_lib.rst
>index 9b1cf8d49f..b94d3caa60 100644
>--- a/doc/guides/prog_guide/cryptodev_lib.rst
>+++ b/doc/guides/prog_guide/cryptodev_lib.rst
>@@ -1282,3 +1282,33 @@ Asymmetric Crypto Device API
>
> The cryptodev Library API is described in the  `DPDK API Reference
><https://doc.dpdk.org/api/>`_
>+
>+
>+Device Statistics
>+~~~~~~~~~~~~~~~~~

This header underline (~~~) means this section will be a subsection of "Asymmetric crypto Sample code", which isn't correct.
I suggest changing the underline to "----" to move it up a level to be its own section.

>+
>+The Cryptodev library has support for displaying cryptodev information
>+through the Telemetry interface. Telemetry commands that can be used
>+are shown below.
>+
>+#. Get the list of available Crypto devices by ID::
>+
>+     --> /cryptodev/list
>+     {"/cryptodev/list": [0, 1, 2, 3]}
>+
>+#. Get general information from a Crypto device::
>+
>+     --> /cryptodev/info,0
>+     {"/cryptodev/info": {"device_name": "0000:1c:01.0_qat_sym",
>+     "max_nb_queue_pairs": 2}}
>+
>+#. Get the statistics for a particular Crypto device::
>+
>+     --> /cryptodev/stats,0
>+     {"/cryptodev/stats": {"enqueued_count": 0,
>+     "dequeued_count": 0, "enqueue_err_count": 0,
>+     "dequeue_err_count": 0}}
>+
>+
>+For more information on how to use the Telemetry interface, see the
>+:doc:../howto/telemetry.

There are ` symbols missing here, so this link doesn't work.
Needs to be:
     :doc:`../howto/telemetry`

<snip>

>+static int
>+cryptodev_handle_dev_info(const char *cmd __rte_unused,
>+		const char *params, struct rte_tel_data *d) {
>+	struct rte_cryptodev_info cryptodev_info;
>+	int dev_id;
>+	char *end_param;
>+
>+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
>+		return -EINVAL;
>+
>+	dev_id = strtoul(params, &end_param, 0);
>+	if (*end_param != '\0')
>+		CDEV_LOG_ERR("Extra parameters passed to command,
>ignoring");
>+	if (!rte_cryptodev_is_valid_dev(dev_id))
>+		return -1;

Maybe -EINVAL return value would be better here, same comment for handle_dev_stats function.

<snip>

Thanks,
Ciara