Re: [PATCH v3] dmadev: add telemetry support

["Morrissey, Sean" <sean.morrissey@intel.com>]

On 01/04/2022 12:00, Walsh, Conor wrote:
>> 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.

Hi all,

If that is the preferred approach then I will send another version. I 
got the lower case

names from the capa_names struct in the dma_capability_name() function 
and these

naming conventions are also used in the logs i.e. "Device %d don't 
support mem2mem transfer".

For this reason, I thought this was the preferred approach to naming the 
capabilities, however

I will keep the names consistent with the flags as suggested.

>>> +	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;
>>> +}