Re: [PATCH v1] dmadev: add telemetry support

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

On Wed, Mar 23, 2022 at 04:49:07PM +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}}
> --> /dmadev/stats,0,0
> {"/dmadev/stats": {"submitted": 0, "completed": 0, "errors": 0}}

I am wondering if for simplicity we should allow the vchan id to be omitted
if only one vchan is present. For automated use by tools it probably won't
matter, but for interactive use it might be nice.

> 
> Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
> ---
>  doc/guides/prog_guide/dmadev.rst       |  24 ++++++
>  doc/guides/rel_notes/release_22_07.rst |   4 +
>  lib/dmadev/meson.build                 |   2 +
>  lib/dmadev/rte_dmadev.c                | 105 +++++++++++++++++++++++++
>  4 files changed, 135 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/dmadev.rst b/doc/guides/prog_guide/dmadev.rst
> index 77863f8028..1ff66dfe2a 100644
> --- a/doc/guides/prog_guide/dmadev.rst
> +++ b/doc/guides/prog_guide/dmadev.rst
> @@ -118,3 +118,27 @@ 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::
> +

Explain here that the device id is passed as parameter.

> +     --> /dmadev/info,0
> +     {"/dmadev/info": {"name": "0000:00:01.0", "nb_vchans": 1,
> +     "numa_node": 0}}
> +
> +#. Get the statistics for a particular DMA device and virtual DMA channel::
> +
> +     --> /dmadev/stats,0,0

Again, explain the parameters.

> +     {"/dmadev/stats": {"submitted": 0, "completed": 0,
> +     "errors": 0}}
> +
> +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.
>  
>  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']

This is actually unnecessary as EAL depends on telemetry so it's an
implicit dependency of every other lib. While functionally it doesn't hurt
to list it, I'd tend towards not doing so, as in the past I've seen meson
configuration runs slow down if we have unnecessary dependencies called out
that then need to be deduplicated in python.

> diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
> index d4b32b2971..2f068cb9d8 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,107 @@ 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;
> +
> +	if (rte_dma_count_avail() == 0)
> +		return -EINVAL;
> +
Rather than error, we should just return an empty list. Just removing this
check should do the trick.

> +	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;
> +	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(ERR,
> +		    "Extra parameters passed to dmadev telemetry command, ignoring");
> +
> +	if (!rte_dma_is_valid(dev_id))
> +		return -EINVAL;
> +
> +	ret = rte_dma_info_get(dev_id, &dma_info);
> +	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);
> +
> +	return 0;
> +}

There is more information returned by info_get function than called out
here, so I think the function should be expanded. For the capabilities, for
example, I think each one should be called out by name with a 0 or 1 value,
so things are nice and clear.

> +
> +#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_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);
> +	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(ERR,
> +		    "Extra parameters passed to dmadev telemetry command, ignoring");

You don't need to split these lines like this. Lines up to 100 chars are
allowed, and any strings are allowed to continue on past that, so it's only
after a long string you need to split the line. Also, according to our
coding guidelines, line continuations should be double-indented or
alternatively aligned to the opening brace on the previous line. Check what
is done already the file and keep it consistent with that.

> +
> +	if (!rte_dma_is_valid(dev_id))
> +		return -EINVAL;
> +
> +	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.");

This can be shrunk to 2 lines I think.

> +	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. Parameters: int dev_id, vchan_id");
> +}
> -- 
> 2.25.1
>