Re: [dpdk-dev] [EXT] [v9 1/4] common/cnxk: add telemetry endpoints to npa

[Harman Kalra <hkalra@marvell.com>]

Hi Gowrishankar,

Some comments inline.


> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Gowrishankar
> Muthukrishnan
> Sent: Wednesday, September 29, 2021 12:25 PM
> To: dev@dpdk.org
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Kiran Kumar
> Kokkilagadda <kirankumark@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>; Sunil Kumar Kori <skori@marvell.com>; Satha
> Koteswara Rao Kottidi <skoteshwar@marvell.com>; Ashwin Sekhar
> Thalakalath Kottilveetil <asekhar@marvell.com>; Pavan Nikhilesh
> Bhagavatula <pbhagavatula@marvell.com>; Gowrishankar Muthukrishnan
> <gmuthukrishn@marvell.com>
> Subject: [EXT] [dpdk-dev] [v9 1/4] common/cnxk: add telemetry endpoints to
> npa
> 
> External Email
> 
> ----------------------------------------------------------------------
> Add telemetry endpoints to npa.
> 
> Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
> ---
>  drivers/common/cnxk/cnxk_telemetry.h     |  26 +++
>  drivers/common/cnxk/cnxk_telemetry_npa.c | 224
> +++++++++++++++++++++++
>  drivers/common/cnxk/meson.build          |   6 +-
>  drivers/common/cnxk/roc_platform.h       |  12 ++
>  4 files changed, 266 insertions(+), 2 deletions(-)  create mode 100644
> drivers/common/cnxk/cnxk_telemetry.h
>  create mode 100644 drivers/common/cnxk/cnxk_telemetry_npa.c
> 

[...]

> +static int
> +cnxk_tel_npa(struct plt_tel_data *d)
> +{
> +	struct npa_lf *lf;
> +	int aura_cnt = 0;
> +	uint32_t i;
> +
> +	lf = idev_npa_obj_get();
> +	if (lf == NULL)
> +		return NPA_ERR_DEVICE_NOT_BOUNDED;
> +
> +	for (i = 0; i < lf->nr_pools; i++) {
> +		if (plt_bitmap_get(lf->npa_bmp, i))
> +			continue;
> +		aura_cnt++;
> +	}
> +
> +	plt_tel_data_add_dict_ptr(d, "npa", lf);
> +	plt_tel_data_add_dict_int(d, "pf", dev_get_pf(lf->pf_func));
> +	plt_tel_data_add_dict_int(d, "vf", dev_get_vf(lf->pf_func));
> +	plt_tel_data_add_dict_int(d, "aura_cnt", aura_cnt);
> +
> +	CNXK_TEL_DICT_PTR(d, lf, pci_dev);

Can we print PCI BDF string in place of its handle.

> +	CNXK_TEL_DICT_PTR(d, lf, npa_bmp);
> +	CNXK_TEL_DICT_PTR(d, lf, npa_bmp_mem);
> +	CNXK_TEL_DICT_PTR(d, lf, npa_qint_mem);
> +	CNXK_TEL_DICT_PTR(d, lf, intr_handle);

Handles can be avoided, just a suggestion no strong objection against it.

> +	CNXK_TEL_DICT_PTR(d, lf, mbox);
> +	CNXK_TEL_DICT_PTR(d, lf, base);
> +	CNXK_TEL_DICT_INT(d, lf, stack_pg_ptrs);
> +	CNXK_TEL_DICT_INT(d, lf, stack_pg_bytes);
> +	CNXK_TEL_DICT_INT(d, lf, npa_msixoff);
> +	CNXK_TEL_DICT_INT(d, lf, nr_pools);
> +	CNXK_TEL_DICT_INT(d, lf, pf_func);
> +	CNXK_TEL_DICT_INT(d, lf, aura_sz);
> +	CNXK_TEL_DICT_INT(d, lf, qints);
> +
> +	return 0;
> +

[...]

> +static int
> +cnxk_npa_tel_handle_info(const char *cmd __plt_unused,
> +			 const char *params __plt_unused,
> +			 struct plt_tel_data *d)
> +{
> +	plt_tel_data_start_dict(d);
> +	return cnxk_tel_npa(d);
> +}
> +
> +static int
> +cnxk_npa_tel_handle_info_x(const char *cmd, const char *params,
> +			   struct plt_tel_data *d)
> +{
> +	int id, rc;
> +
> +	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
> +		return -1;
> +
> +	id = atoi(params);

Please avoid using atoi as it obsolete. atoi() function is subsumed by strtol(), problem with atoi is
it does not perform any error checking.


> +	plt_tel_data_start_dict(d);
> +
> +	if (strstr(cmd, "aura/info"))
> +		rc = cnxk_tel_npa_aura(id, d);
> +	else
> +		rc = cnxk_tel_npa_pool(id, d);
> +
> +	return rc;
> +}
> +
> +PLT_INIT(cnxk_telemetry_npa_init)
> +{
> +	plt_telemetry_register_cmd(
> +		"/cnxk/npa/info", cnxk_npa_tel_handle_info,
> +		"Returns npa information. Takes no parameters");
> +
> +	plt_telemetry_register_cmd(
> +		"/cnxk/npa/aura/info", cnxk_npa_tel_handle_info_x,
> +		"Returns npa aura information. Parameters: aura_id");

Can we also add a list command "/cnxk/npa/aura/list" something similar to
"/ethdev/list" so its easy to know for what all aura ids  "/cnxk/npa/aura/info"
can be used.


> +
> +	plt_telemetry_register_cmd(
> +		"/cnxk/npa/pool/info", cnxk_npa_tel_handle_info_x,

Same, pool/list if possible.


> +		"Returns npa pool information. Parameters: pool_id"); }
> diff --git a/drivers/common/cnxk/meson.build


Thanks
Harman