Re: [dpdk-dev] [PATCH 07/22] event/dlb2: add xstats

["Eads, Gage" <gage.eads@intel.com>]

> diff --git a/drivers/event/dlb2/dlb2_xstats.c b/drivers/event/dlb2/dlb2_xstats.c
> new file mode 100644
> index 0000000..9a69d78
> --- /dev/null
> +++ b/drivers/event/dlb2/dlb2_xstats.c
> @@ -0,0 +1,1269 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2016-2020 Intel Corporation
> + */
> +
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <sys/mman.h>
> +#include <sys/fcntl.h>
> +#include <errno.h>
> +#include <assert.h>
> +#include <unistd.h>
> +#include <string.h>

+inttypes.h for the printf format specifiers (PRIu64, etc.)

> +
> +#include <rte_debug.h>
> +#include <rte_log.h>
> +#include <rte_dev.h>
> +#include <rte_mbuf.h>
> +#include <rte_ring.h>
> +#include <rte_errno.h>
> +#include <rte_kvargs.h>
> +#include <rte_malloc.h>
> +#include <rte_cycles.h>
> +#include <rte_io.h>
> +#include <rte_eventdev.h>
> +#include <rte_eventdev_pmd.h>
> +#include <rte_eventdev_pmd_vdev.h>

Besides rte_malloc.h and rte_eventdev.h, I suspect the rest are unnecessary.

[...]

> +int
> +dlb2_eventdev_xstats_get_names(const struct rte_eventdev *dev,
> +		enum rte_event_dev_xstats_mode mode, uint8_t
> queue_port_id,
> +		struct rte_event_dev_xstats_name *xstats_names,
> +		unsigned int *ids, unsigned int size)
> +{
> +	const struct dlb2_eventdev *dlb2 = dlb2_pmd_priv(dev);
> +	unsigned int i;
> +	unsigned int xidx = 0;
> +
> +	RTE_SET_USED(mode);
> +	RTE_SET_USED(queue_port_id);

Not needed, these variables are used below.

> +
> +	uint32_t xstats_mode_count = 0;
> +	uint32_t start_offset = 0;
> +
> +	switch (mode) {
> +	case RTE_EVENT_DEV_XSTATS_DEVICE:
> +		xstats_mode_count = dlb2->xstats_count_mode_dev;
> +		break;
> +	case RTE_EVENT_DEV_XSTATS_PORT:
> +		if (queue_port_id >= DLB2_MAX_NUM_PORTS)
> +			break;
> +		xstats_mode_count = dlb2-
> >xstats_count_per_port[queue_port_id];
> +		start_offset = dlb2->xstats_offset_for_port[queue_port_id];
> +		break;
> +	case RTE_EVENT_DEV_XSTATS_QUEUE:
> +#if (DLB2_MAX_NUM_QUEUES <= 255) /* max 8 bit value */

Looks like this macro is tied to the h/w definition, is it subject to grow?

> +		if (queue_port_id >= DLB2_MAX_NUM_QUEUES)
> +			break;
> +#endif
> +		xstats_mode_count = dlb2-
> >xstats_count_per_qid[queue_port_id];
> +		start_offset = dlb2->xstats_offset_for_qid[queue_port_id];
> +		break;
> +	default:
> +		return -EINVAL;
> +	};
> +
> +	if (xstats_mode_count > size || !ids || !xstats_names)
> +		return xstats_mode_count;
> +
> +	for (i = 0; i < dlb2->xstats_count && xidx < size; i++) {
> +		if (dlb2->xstats[i].mode != mode)
> +			continue;
> +
> +		if (mode != RTE_EVENT_DEV_XSTATS_DEVICE &&
> +		    queue_port_id != dlb2->xstats[i].obj_idx)
> +			continue;
> +
> +		xstats_names[xidx] = dlb2->xstats[i].name;
> +		if (ids)
> +			ids[xidx] = start_offset + xidx;
> +		xidx++;
> +	}
> +	return xidx;
> +}
> +
> +static int
> +dlb2_xstats_update(struct dlb2_eventdev *dlb2,
> +		enum rte_event_dev_xstats_mode mode,
> +		uint8_t queue_port_id, const unsigned int ids[],
> +		uint64_t values[], unsigned int n, const uint32_t reset)
> +{
> +	unsigned int i;
> +	unsigned int xidx = 0;
> +
> +	RTE_SET_USED(mode);
> +	RTE_SET_USED(queue_port_id);
> +

Not needed, these variables are used below.

> +	uint32_t xstats_mode_count = 0;
> +
> +	switch (mode) {
> +	case RTE_EVENT_DEV_XSTATS_DEVICE:
> +		xstats_mode_count = dlb2->xstats_count_mode_dev;
> +		break;
> +	case RTE_EVENT_DEV_XSTATS_PORT:
> +		if (queue_port_id >= DLB2_MAX_NUM_PORTS)
> +			goto invalid_value;
> +		xstats_mode_count = dlb2-
> >xstats_count_per_port[queue_port_id];
> +		break;
> +	case RTE_EVENT_DEV_XSTATS_QUEUE:
> +#if (DLB2_MAX_NUM_QUEUES <= 255) /* max 8 bit value */

(Same comment as above)

[...]

> +int
> +dlb2_eventdev_xstats_reset(struct rte_eventdev *dev,
> +			   enum rte_event_dev_xstats_mode mode,
> +			   int16_t queue_port_id,
> +			   const uint32_t ids[],
> +			   uint32_t nb_ids)
> +{
> +	struct dlb2_eventdev *dlb2 = dlb2_pmd_priv(dev);
> +	uint32_t i;
> +
> +	/* handle -1 for queue_port_id here, looping over all ports/queues */
> +	switch (mode) {
> +	case RTE_EVENT_DEV_XSTATS_DEVICE:
> +		if (dlb2_xstats_reset_dev(dlb2, ids, nb_ids))
> +			return -EINVAL;
> +		break;
> +	case RTE_EVENT_DEV_XSTATS_PORT:
> +		if (queue_port_id == -1) {
> +			for (i = 0; i < DLB2_MAX_NUM_PORTS; i++) {
> +				if (dlb2_xstats_reset_port(dlb2, i,
> +								ids, nb_ids))

Nit: argument alignment (occurs below as well)

> +					return -EINVAL;
> +			}
> +		} else if (queue_port_id < DLB2_MAX_NUM_PORTS) {
> +			if (dlb2_xstats_reset_port(dlb2, queue_port_id,
> +							ids, nb_ids))
> +				return -EINVAL;
> +		}
> +		break;
> +	case RTE_EVENT_DEV_XSTATS_QUEUE:
> +		if (queue_port_id == -1) {
> +			for (i = 0; i < DLB2_MAX_NUM_QUEUES; i++) {
> +				if (dlb2_xstats_reset_queue(dlb2, i,
> +								ids, nb_ids))
> +					return -EINVAL;
> +			}
> +		} else if (queue_port_id < DLB2_MAX_NUM_QUEUES) {
> +			if (dlb2_xstats_reset_queue(dlb2, queue_port_id,
> +								ids, nb_ids))
> +				return -EINVAL;
> +		}
> +		break;
> +	};
> +
> +	return 0;
> +}

Thanks,
Gage