Re: [PATCH v2] dmadev: add telemetry support

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Mar 31, 2022 at 06:39:46PM +0000, Sean Morrissey wrote:
> Telemetry commands are now registered through the dmadev library
> for the gathering of DSA stats. The corresponding callback
> functions for listing dmadevs and providing info and stats for a
> specific dmadev are implemented in the dmadev library.
> 
> An example usage can be seen below:
> 
> Connecting to /var/run/dpdk/rte/dpdk_telemetry.v2
> {"version": "DPDK 22.03.0-rc2", "pid": 2956551, "max_output_len": 16384}
> Connected to application: "dpdk-dma"
> --> /
> {"/": ["/", "/dmadev/info", "/dmadev/list", "/dmadev/stats", ...]}
> --> /dmadev/list
> {"/dmadev/list": [0, 1]}
> --> /dmadev/info,0
> {"/dmadev/info": {"name": "0000:00:01.0", "nb_vchans": 1, "numa_node": 0}}

This needs an update now that more info is being output. It probably all
does not need to be displayed here, so adding "..." may be enough if you
like.

> --> /dmadev/stats,0,0
> {"/dmadev/stats": {"submitted": 0, "completed": 0, "errors": 0}}
> 
> Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
> Tested-by: Sunil Pai G <sunil.pai.g@intel.com>
> ---
> V2:
> * add device capabilities to info command
> * no requirement to pass vchan id
>   if the device only has one vchan
> * minor code cleanup
> ---
>  doc/guides/prog_guide/dmadev.rst       |  27 ++++++
>  doc/guides/rel_notes/release_22_07.rst |   4 +
>  lib/dmadev/meson.build                 |   2 +
>  lib/dmadev/rte_dmadev.c                | 127 +++++++++++++++++++++++++
>  4 files changed, 160 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/dmadev.rst b/doc/guides/prog_guide/dmadev.rst
> index 77863f8028..a2e4617091 100644
> --- a/doc/guides/prog_guide/dmadev.rst
> +++ b/doc/guides/prog_guide/dmadev.rst
> @@ -118,3 +118,30 @@ i.e. ``rte_dma_stats_get()``. The statistics returned for each device instance a
>  * ``submitted``: The number of operations submitted to the device.
>  * ``completed``: The number of operations which have completed (successful and failed).
>  * ``errors``: The number of operations that completed with error.
> +
> +The dmadev library has support for displaying DMA device information
> +through the Telemetry interface. Telemetry commands that can be used
> +are shown below.
> +
> +#. Get the list of available DMA devices by ID::
> +
> +     --> /dmadev/list
> +     {"/dmadev/list": [0, 1]}
> +
> +#. Get general information from a DMA device by passing the device id as a parameter::
> +
> +     --> /dmadev/info,0
> +     {"/dmadev/info": {"name": "0000:00:01.0", "nb_vchans": 1, "numa_node": 0}}
> +
As above

> +#. Get the statistics for a particular DMA device and virtual DMA channel by passing the device id and vchan id as parameters::
> +
> +     --> /dmadev/stats,0,0
> +     {"/dmadev/stats": {"submitted": 0, "completed": 0, "errors": 0}}
> +
> +#. If a DMA device only has one virtual DMA channel you only need to pass the device id to get its statistics::
> +
> +     --> /dmadev/stats,0
> +     {"/dmadev/stats": {"submitted": 0, "completed": 0, "errors": 0}}
> +

Since this is only a convenience, I think you can omit the example for it,
and just include the info about it in brackets at the end of the bullet
point above - it doesn't need its own bullet.

> +For more information on how to use the Telemetry interface, see
> +the :doc:`../howto/telemetry`.
> diff --git a/doc/guides/rel_notes/release_22_07.rst b/doc/guides/rel_notes/release_22_07.rst
> index 42a5f2d990..5a236b45ae 100644
> --- a/doc/guides/rel_notes/release_22_07.rst
> +++ b/doc/guides/rel_notes/release_22_07.rst
> @@ -55,6 +55,10 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =======================================================
>  
> +* **Added telemetry callbacks to dmadev library.**
> +
> +  Added telemetry callback functions which allow for a list of DMA devices,
> +  stats for a DMA device, and other DMA device information to be queried.
>  

For correct formatting of the doc, I think you need to add a new blank line
here, since sections end with two empty lines.

>  Removed Items
>  -------------
> diff --git a/lib/dmadev/meson.build b/lib/dmadev/meson.build
> index d2fc85e8c7..2f17587b75 100644
> --- a/lib/dmadev/meson.build
> +++ b/lib/dmadev/meson.build
> @@ -5,3 +5,5 @@ sources = files('rte_dmadev.c')
>  headers = files('rte_dmadev.h')
>  indirect_headers += files('rte_dmadev_core.h')
>  driver_sdk_headers += files('rte_dmadev_pmd.h')
> +
> +deps += ['telemetry']
> diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
> index d4b32b2971..6cd6a8db46 100644
> --- a/lib/dmadev/rte_dmadev.c
> +++ b/lib/dmadev/rte_dmadev.c
> @@ -11,6 +11,7 @@
>  #include <rte_malloc.h>
>  #include <rte_memzone.h>
>  #include <rte_string_fns.h>
> +#include <rte_telemetry.h>
>  
>  #include "rte_dmadev.h"
>  #include "rte_dmadev_pmd.h"
> @@ -864,3 +865,129 @@ dma_fp_object_dummy(struct rte_dma_fp_object *obj)
>  	obj->completed_status = dummy_completed_status;
>  	obj->burst_capacity   = dummy_burst_capacity;
>  }
> +
> +static int
> +dmadev_handle_dev_list(const char *cmd __rte_unused,
> +		const char *params __rte_unused,
> +		struct rte_tel_data *d)
> +{
> +	int dev_id;
> +
> +	rte_tel_data_start_array(d, RTE_TEL_INT_VAL);
> +	for (dev_id = 0; dev_id < dma_devices_max; dev_id++)
> +		if (rte_dma_is_valid(dev_id))
> +			rte_tel_data_add_array_int(d, dev_id);
> +
> +	return 0;
> +}
> +
> +static int
> +dmadev_handle_dev_info(const char *cmd __rte_unused,
> +		const char *params, struct rte_tel_data *d)
> +{
> +	struct rte_dma_info dma_info;
> +	struct rte_tel_data *dma_caps;
> +	int dev_id, ret;
> +	char *end_param;
> +
> +	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
> +		return -EINVAL;
> +
> +	dev_id = strtoul(params, &end_param, 0);
> +	if (*end_param != '\0')
> +		RTE_DMA_LOG(WARNING, "Extra parameters passed to dmadev telemetry command, ignoring");
> +
> +	ret = rte_dma_info_get(dev_id, &dma_info);

For clarity, I think it might be worthwhile adding a one-line comment here
that info_get does parameter checking of dev_id so we don't need to do so.
Same comment would also apply to stats function below.

> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	rte_tel_data_start_dict(d);
> +	rte_tel_data_add_dict_string(d, "name", dma_info.dev_name);
> +	rte_tel_data_add_dict_int(d, "nb_vchans", dma_info.nb_vchans);
> +	rte_tel_data_add_dict_int(d, "numa_node", dma_info.numa_node);
> +	rte_tel_data_add_dict_int(d, "max_vchans", dma_info.max_vchans);
> +	rte_tel_data_add_dict_int(d, "max_desc", dma_info.max_desc);
> +	rte_tel_data_add_dict_int(d, "min_desc", dma_info.min_desc);
> +	rte_tel_data_add_dict_int(d, "max_sges", dma_info.max_sges);
> +
> +	dma_caps = rte_tel_data_alloc();
> +	if (!dma_caps)
> +		return -ENOMEM;
> +
> +	rte_tel_data_start_dict(dma_caps);
> +	rte_tel_data_add_dict_int(dma_caps, "fill", !!(dma_info.dev_capa & RTE_DMA_CAPA_OPS_FILL));
> +	rte_tel_data_add_dict_int(dma_caps, "sva", !!(dma_info.dev_capa & RTE_DMA_CAPA_SVA));
> +	rte_tel_data_add_dict_int(dma_caps, "silent", !!(dma_info.dev_capa & RTE_DMA_CAPA_SILENT));
> +	rte_tel_data_add_dict_int(dma_caps, "copy", !!(dma_info.dev_capa & RTE_DMA_CAPA_OPS_COPY));
> +	rte_tel_data_add_dict_int(dma_caps, "mem2mem",
> +					!!(dma_info.dev_capa & RTE_DMA_CAPA_MEM_TO_MEM));

Two ideas here in case you wan to consider them for shortening these lines:
* assign "dma_info.dev_capa" to a local var with a short name
* define a macro for adding the capabilities based on the name, something
  like (completely untested!):

#define ADD_CAPA (td, c, name) \
	rte_tel_data_add_dict_int(td, #name, !!(RTE_DMA_CAPA_ ## name & c))

...
ADD_CAPA(dma_caps, dev_capa, MEM_TO_MEM);
ADD_CAPA(dma_caps, dev_capa, MEM_TO_DEV);

The small downside of the macro scheme is that you'd have to do extra work
to avoid having the capabilities in caps, but it does guarantee consistency
of naming.

> +	rte_tel_data_add_dict_int(dma_caps, "mem2dev",
> +					!!(dma_info.dev_capa & RTE_DMA_CAPA_MEM_TO_DEV));
> +	rte_tel_data_add_dict_int(dma_caps, "dev2mem",
> +					!!(dma_info.dev_capa & RTE_DMA_CAPA_DEV_TO_MEM));
> +	rte_tel_data_add_dict_int(dma_caps, "dev2dev",
> +					!!(dma_info.dev_capa & RTE_DMA_CAPA_DEV_TO_DEV));
> +	rte_tel_data_add_dict_int(dma_caps, "copy_sg",
> +					!!(dma_info.dev_capa & RTE_DMA_CAPA_OPS_COPY_SG));
> +	rte_tel_data_add_dict_int(dma_caps, "handles_errors",
> +					!!(dma_info.dev_capa & RTE_DMA_CAPA_HANDLES_ERRORS));
> +	rte_tel_data_add_dict_container(d, "capabilities", dma_caps, 0);
> +
> +	return 0;
> +}
> +
> +#define ADD_DICT_STAT(s) rte_tel_data_add_dict_u64(d, #s, dma_stats.s)
> +
> +static int
> +dmadev_handle_dev_stats(const char *cmd __rte_unused,
> +		const char *params,
> +		struct rte_tel_data *d)
> +{
> +	struct rte_dma_info dma_info;
> +	struct rte_dma_stats dma_stats;
> +	int dev_id, vchan_id, ret;
> +	char *end_param;
> +	const char *vchan_param;
> +
> +	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
> +		return -EINVAL;
> +
> +	dev_id = strtoul(params, &end_param, 0);
> +
> +	ret = rte_dma_info_get(dev_id, &dma_info);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	if (dma_info.nb_vchans == 1 && *end_param == '\0') {

Needs a comment explaining what is happening.
Also for single-line blocks in DPDK, we allow omitting the braces (even if
the else leg needs them).

If you want to remove this block, you can also just set vchan_id = 0 when
defining it, and then change the else to be and if with the inverse of the
condition above

> +		vchan_id = 0;
> +	} else {
> +		vchan_param = strtok(end_param, ",");
> +		if (!vchan_param || strlen(vchan_param) == 0 || !isdigit(*vchan_param))
> +			return -EINVAL;
> +
> +		vchan_id = strtoul(vchan_param, &end_param, 0);
> +	}
> +	if (*end_param != '\0')
> +		RTE_DMA_LOG(WARNING, "Extra parameters passed to dmadev telemetry command, ignoring");
> +
> +	ret = rte_dma_stats_get(dev_id, vchan_id, &dma_stats);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	rte_tel_data_start_dict(d);
> +	ADD_DICT_STAT(submitted);
> +	ADD_DICT_STAT(completed);
> +	ADD_DICT_STAT(errors);
> +
> +	return 0;
> +}
> +
> +RTE_INIT(dmadev_init_telemetry)
> +{
> +	rte_telemetry_register_cmd("/dmadev/list", dmadev_handle_dev_list,
> +			"Returns list of available dmadev devices by IDs. No parameters.");
> +	rte_telemetry_register_cmd("/dmadev/info", dmadev_handle_dev_info,
> +			"Returns information for a dmadev. Parameters: int dev_id");
> +	rte_telemetry_register_cmd("/dmadev/stats", dmadev_handle_dev_stats,
> +			"Returns the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (If only one vchannel, vchan_id param is optional)");

Can shorten text in brackets to "(Optional if only one vchannel)", since it
immediately follows the parameter.

Regards,
/Bruce