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.morris...@intel.com>
> Tested-by: Sunil Pai G <sunil.pa...@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
