RE: [PATCH v3] dmadev: add telemetry support

["Walsh, Conor" <conor.walsh@intel.com>]

> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Friday 1 April 2022 11:50
> To: Morrissey, Sean <sean.morrissey@intel.com>
> Cc: Chengwen Feng <fengchengwen@huawei.com>; Laatz, Kevin
> <kevin.laatz@intel.com>; dev@dpdk.org; Pai G, Sunil
> <sunil.pai.g@intel.com>
> Subject: Re: [PATCH v3] dmadev: add telemetry support
> 
> On Fri, Apr 01, 2022 at 10:24:02AM +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,
> > "max_vchans": 1, "max_desc": 4096, "min_desc": 32, "max_sges": 0,
> > "capabilities": {"fill": 1, "sva": 0, "silent": 0, ...}}}
> > --> /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>
> 
> Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

Hi Sean,

I'd agree with Bruce's comment below about trying to keep the names the same.
Looks good to me though and I've tested it with IOAT and dmafwd.

Thanks,
Reviewed-by: Conor Walsh <conor.walsh@intel.com>

> 
> One comment inline below, which I'd like feedback from others on.
> > ---
> > V3:
> > * update docs with correct examples
> > * code cleanup and added comments
> <snip>
> 
> > +
> > +#define ADD_CAPA(c, s) rte_tel_data_add_dict_int(dma_caps, #c,
> !!(dev_capa & RTE_DMA_CAPA_ ## s))
> > +
> > +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;
> <snip>
> > +	dma_caps = rte_tel_data_alloc();
> > +	if (!dma_caps)
> > +		return -ENOMEM;
> > +
> > +	rte_tel_data_start_dict(dma_caps);
> > +	ADD_CAPA(fill, OPS_FILL);
> > +	ADD_CAPA(sva, SVA);
> > +	ADD_CAPA(silent, SILENT);
> > +	ADD_CAPA(copy, OPS_COPY);
> > +	ADD_CAPA(mem2mem, MEM_TO_MEM);
> 
> I'm not 100% sure about this approach of having slightly different names
> compared to the flags, just to have things in lower-case. Looking to have
> some more input here - I'd tend to have the capabilities in upper case to
> avoid duplicating parameters, but I'm not massively concerned either way.
> 
> > +	ADD_CAPA(mem2dev, MEM_TO_DEV);
> > +	ADD_CAPA(dev2mem, DEV_TO_MEM);
> > +	ADD_CAPA(dev2dev, DEV_TO_DEV);
> > +	ADD_CAPA(copy_sg, OPS_COPY_SG);
> > +	ADD_CAPA(handles_errors, HANDLES_ERRORS);
> > +	rte_tel_data_add_dict_container(d, "capabilities", dma_caps, 0);
> > +
> > +	return 0;
> > +}