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

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

> diff --git a/drivers/event/dlb/dlb_xstats.c b/drivers/event/dlb/dlb_xstats.c
> new file mode 100644
> index 0000000..4d01cc0
> --- /dev/null
> +++ b/drivers/event/dlb/dlb_xstats.c
> @@ -0,0 +1,1252 @@
> +/* 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

> +#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>

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

[...]

> +int
> +dlb_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 dlb_eventdev *dlb = dlb_pmd_priv(dev);
> +	unsigned int i;
> +	unsigned int xidx = 0;
> +
> +	RTE_SET_USED(mode);
> +	RTE_SET_USED(queue_port_id);

RTE_SET_USED not needed for mode and queue_port_id

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

If this macro is tied to the hardware definition, will it change?

> +		if (queue_port_id >= DLB_MAX_NUM_QUEUES)
> +			break;
> +#endif
> +		xstats_mode_count = dlb-
> >xstats_count_per_qid[queue_port_id];
> +		start_offset = dlb->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 < dlb->xstats_count && xidx < size; i++) {
> +		if (dlb->xstats[i].mode != mode)
> +			continue;
> +
> +		if (mode != RTE_EVENT_DEV_XSTATS_DEVICE &&
> +		    queue_port_id != dlb->xstats[i].obj_idx)
> +			continue;
> +
> +		xstats_names[xidx] = dlb->xstats[i].name;
> +		if (ids)
> +			ids[xidx] = start_offset + xidx;
> +		xidx++;
> +	}
> +	return xidx;
> +}
> +
> +static int
> +dlb_xstats_update(struct dlb_eventdev *dlb,
> +		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);

RTE_SET_USED not needed for mode and queue_port_id

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

(See comment above)

[...]

> +void
> +dlb_eventdev_dump(struct rte_eventdev *dev, FILE *f)
> +{
> +	struct dlb_eventdev *dlb;
> +	struct dlb_hw_dev *handle;
> +	int i;
> +
> +	if (!f) {

Like Mike pointed out the dlb2 review, avoid !pointer checks.

Thanks,
Gage