On Fri, Apr 01, 2022 at 01:58:49PM +0100, Morrissey, Sean wrote:
>
> On 01/04/2022 12:00, Walsh, Conor wrote:
> > > From: Bruce Richardson <bruce.richard...@intel.com>
> > > Sent: Friday 1 April 2022 11:50
> > > To: Morrissey, Sean <sean.morris...@intel.com>
> > > Cc: Chengwen Feng <fengcheng...@huawei.com>; Laatz, Kevin
> > > <kevin.la...@intel.com>; dev@dpdk.org; Pai G, Sunil
> > > <sunil.pa...@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.morris...@intel.com>
> > > > Tested-by: Sunil Pai G <sunil.pa...@intel.com>
> > > Reviewed-by: Bruce Richardson <bruce.richard...@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.wa...@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".
My apologies, I didn't realise that that function and list of names
existed. Can that be used instead of hard-coding the names and values here?
Can we iterate through the array of names and check if the relevant bit
position is set?
>
> 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.
>
If there are printable names elsewhere that is what we should keep
consistent with. However, we should not duplicate those in this code, but
reuse existing defined names.
/Bruce
