RE: [PATCH v14] app/procinfo: display eventdev xstats

["Pattan, Reshma" <reshma.pattan@intel.com>]

> -----Original Message-----
> From: Sevincer, Abdullah <abdullah.sevincer@intel.com>

The code looks better now, however if you can make below changes that would be great.

> +
> +struct eventdev_params {
> +	uint32_t show_eventdev;

Can't we get rid of the "show_eventdev",  and just rely below flags 

<snip>
> +	uint8_t num_queues;
> +	uint8_t num_ports;
> +	uint32_t shw_all_queues;
> +	uint32_t shw_all_ports;
> +	uint32_t dump_xstats;
> +	uint32_t reset_xstats;
> +	uint32_t shw_device_xstats;


> +parse_eventdev_dump_xstats_params(char *list) {
<snip>
> +
> +	if (!eventdev_var[evdev_id].show_eventdev)
> +		eventdev_var[evdev_id].show_eventdev = 1;

You can set the " eventdev_var[evdev_id].show_eventdev = 1;" without  the if check.  Here and in other places.
However , you don't need to have  show_eventdev=1. You can  think of getting rid of the "show_eventdev" altogether. Here and other places.


<snip>

> +parse_eventdev_port_xstats_params(char *list) {
> +	uint8_t port_id;
> +	uint8_t evdev_id;
> +	if (sscanf(list, "*:%hhu", &evdev_id) == 1) {
> +		if (evdev_id >= RTE_EVENT_MAX_DEVS) {
> +			printf("Invalid eventdev id, id should be between 0 -
> %d\n",

Do you need to validate the port id and queue ids here and in below functions.

<snip>

> +xstats_display(uint8_t dev_id,
> +	  enum rte_event_dev_xstats_mode mode,
> +	  uint8_t queue_port_id)
> +{
> +	int ret;
> +	struct rte_event_dev_xstats_name *xstats_names;
> +	uint64_t *ids;
> +	uint64_t *values;
> +	unsigned int size;
> +	int i;
> +
> +	size = xstats_get_names_and_ids_size(dev_id, mode, queue_port_id);


> +
> +	if (size == 0) {
> +		printf(
> +		"No stats available for this item, mode=%d,
> queue_port_id=%d\n",
> +			mode, queue_port_id);
> +		return;
> +	}
> +
> +	/* Get memory to hold stat names, IDs, and values */
> +	xstats_names = malloc(sizeof(struct rte_event_dev_xstats_name) *
> size);
> +	ids = malloc(sizeof(unsigned int) * size);
> +
> +	if (!xstats_names || !ids)
> +		rte_panic("unable to alloc memory for stats retrieval\n");
> +
> +	ret = rte_event_dev_xstats_names_get(dev_id, mode, queue_port_id,
> +					     xstats_names, ids,
> +					     size);

You need size in unsigned int form  only to pass to the function rte_event_dev_xstats_names_get() , so why not use unsigned int(size) here  and remove 
the cast of size in function xstats_get_names_and_ids_size(). 

 I also feel you don't need xstats_get_names_and_ids_size(), just call rte_event_dev_xstats_names_get() directly in this function , to get the size in int, use int size everywhere else, 
and use it with cast  while passing to rte_event_dev_xstats_names_get() again. It makes easy to follow the code .

This way You no need to do (int)size in below code too.


> +	if (ret != (int)size)
> +		rte_panic("rte_event_dev_xstats_names_get err %d\n", ret);


<snip>


> +
> +A typical command line usage for eventdev stats:
> +
> +    .. code-block:: console
> +
> +       ./dpdk-proc-info -- --show-edev-port-xstats=1:0

If you want to provide the command examples,  you can give example under each command section, else you can remove this as well.