Re: [dpdk-dev] [PATCH 02/15] eventdev: add APIs for extended stats

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

On Mon, Jan 16, 2017 at 03:40:42PM +0000, Harry van Haaren wrote:
> From: Bruce Richardson <bruce.richardson@intel.com>
> 
> Add in APIs for extended stats so that eventdev implementations can report
> out information on their internal state. The APIs are based on, but not
> identical to, the equivalent ethdev functions.

Overall the xstat API looks good. I think, we need to extend the API to
support event port and event queue specific xstats too.

> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  lib/librte_eventdev/rte_eventdev.c           | 64 ++++++++++++++++++++++++
>  lib/librte_eventdev/rte_eventdev.h           | 75 ++++++++++++++++++++++++++++
>  lib/librte_eventdev/rte_eventdev_pmd.h       | 58 +++++++++++++++++++++
>  lib/librte_eventdev/rte_eventdev_version.map |  3 ++
>  4 files changed, 200 insertions(+)
> 
> +
> +uint64_t
> +rte_event_dev_get_xstat_by_name(uint8_t dev_id, const char *name,
> +		unsigned int *id)
> +{
> +	const struct rte_eventdev *dev = &rte_eventdevs[dev_id];
> +	unsigned int temp = -1;
> +
> +	if (id != NULL)
> +		*id = (unsigned int)-1;
> +	else
> +		id = &temp; /* ensure driver never gets a NULL value */
> +
> +	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, 0);
> +
> +	/* implemented by driver */
> +	if (dev->dev_ops->get_xstat_by_name != NULL)
> +		return (*dev->dev_ops->get_xstat_by_name)(dev, name, id);
> +	return 0;

Shouldn't we return -1 as per API specification?

> +}
> +
>  int
>  rte_event_dev_start(uint8_t dev_id)
>  {
> diff --git a/lib/librte_eventdev/rte_eventdev.h b/lib/librte_eventdev/rte_eventdev.h
> index c2f9310..681cbfa 100644
> --- a/lib/librte_eventdev/rte_eventdev.h
> +++ b/lib/librte_eventdev/rte_eventdev.h
> @@ -1401,6 +1401,81 @@ rte_event_port_links_get(uint8_t dev_id, uint8_t port_id,
>  int
>  rte_event_dev_dump(uint8_t dev_id, FILE *f);
>  
> +/** Maximum name length for extended statistics counters */
> +#define RTE_EVENT_DEV_XSTAT_NAME_SIZE 64
> +
> +/**
> + * A name-key lookup element for extended statistics.
> + *
> + * This structure is used to map between names and ID numbers
> + * for extended ethdev statistics.
> + */
> +struct rte_event_dev_xstat_name {
> +	char name[RTE_EVENT_DEV_XSTAT_NAME_SIZE];
> +};
> +
> +/**
> + * Retrieve names of extended statistics of an event device.
> + *
> + * @param dev_id
> + *   The identifier of the event device.
> + * @param xstat_names
> + *  Block of memory to insert names into. Must be at least size in capacity.
> + *  If set to NULL, function returns required capacity.
> + * @param size
> + *  Capacity of xstat_names (number of names).
> + * @return
> + *   - positive value lower or equal to size: success. The return value
> + *     is the number of entries filled in the stats table.
> + *   - positive value higher than size: error, the given statistics table
> + *     is too small. The return value corresponds to the size that should
> + *     be given to succeed. The entries in the table are not valid and
> + *     shall not be used by the caller.
> + *   - negative value on error (invalid port id)

How about updating the rte_errno also in the failure case? It may useful
for application developer to have single check for error condition

> + */
> +int
> +rte_event_dev_get_xstat_names(uint8_t dev_id,
> +		struct rte_event_dev_xstat_name *xstat_names,
> +		unsigned int size);
> +
> +/**
> + * Retrieve extended statistics of an event device.
> + *
> + * @param dev_id
> + *   The identifier of the device.
> + * @param ids
> + *   The id numbers of the stats to get. The ids can be got from the stat
> + *   position in the stat list from rte_event_dev_get_xstat_names(), or
> + *   by using rte_eventdev_get_xstat_by_name()
> + * @param values

Please add [out] to indicate it is a output parameter
example: @param[out] values

> + *   The values for each stats request by ID.
> + * @param n
> + *   The number of stats requested
> + * @return
> + *   Number of stat entries filled into the values array
> + */
> +int
> +rte_event_dev_get_xstats(uint8_t dev_id, const unsigned int ids[],
> +		uint64_t values[], unsigned int n);
> +
> +/**
> + * Retrieve the value of a single stat by requesting it by name.
> + *
> + * @param dev_id
> + *   The identifier of the device
> + * @param name
> + *   The stat name to retrieve
> + * @param id

Please add [out] to indicate it is output parameter

> + *   If non-NULL, the numerical id of the stat will be returned, so that further
> + *   requests for the stat can be got using rte_eventdev_xstats_get, which will

The function prototype is rte_event_dev_get_xstats. Change the
rte_eventdev_xstats_get to rte_event_dev_get_xstats in the above description

The rest of the file is following rte_eventdev_xxx_xxx_get syntax for
get functions. How about changing rte_eventdev_xxx_xxx_get syntax to
maintain the consistency?


> + *   be faster as it doesn't need to scan a list of names for the stat.
> + *   If the stat cannot be found, the id returned will be (unsigned)-1.
> + * @return
> + *   The stat value, or -1 if not found.
> + */
> +uint64_t
> +rte_event_dev_get_xstat_by_name(uint8_t dev_id, const char *name, unsigned int *id);