Re: [dpdk-dev] [PATCH v1 2/7] eal/interrupts: implement get set APIs

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

2021-09-03 18:10 (UTC+0530), Harman Kalra:
> [...]
> diff --git a/lib/eal/common/eal_common_interrupts.c b/lib/eal/common/eal_common_interrupts.c
> new file mode 100644
> index 0000000000..2e4fed96f0
> --- /dev/null
> +++ b/lib/eal/common/eal_common_interrupts.c
> @@ -0,0 +1,506 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2021 Marvell.
> + */
> +
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include <rte_errno.h>
> +#include <rte_log.h>
> +#include <rte_malloc.h>
> +
> +#include <rte_interrupts.h>
> +
> +
> +struct rte_intr_handle *rte_intr_handle_instance_alloc(int size,
> +						       bool from_hugepage)

Since the purpose of the series is to reduce future ABI breakages,
how about making the second parameter "flags" to have some spare bits?
(If not removing it completely per David's suggestion.)

> +{
> +	struct rte_intr_handle *intr_handle;
> +	int i;
> +
> +	if (from_hugepage)
> +		intr_handle = rte_zmalloc(NULL,
> +					  size * sizeof(struct rte_intr_handle),
> +					  0);
> +	else
> +		intr_handle = calloc(1, size * sizeof(struct rte_intr_handle));
> +	if (!intr_handle) {
> +		RTE_LOG(ERR, EAL, "Fail to allocate intr_handle\n");
> +		rte_errno = ENOMEM;
> +		return NULL;
> +	}
> +
> +	for (i = 0; i < size; i++) {
> +		intr_handle[i].nb_intr = RTE_MAX_RXTX_INTR_VEC_ID;
> +		intr_handle[i].alloc_from_hugepage = from_hugepage;
> +	}
> +
> +	return intr_handle;
> +}
> +
> +struct rte_intr_handle *rte_intr_handle_instance_index_get(
> +				struct rte_intr_handle *intr_handle, int index)

If rte_intr_handle_instance_alloc() returns a pointer to an array,
this function is useless since the user can simply manipulate a pointer.
If we want to make a distinction between a single struct rte_intr_handle and a
commonly allocated bunch of such (but why?), then they should be represented
by distinct types.

> +{
> +	if (intr_handle == NULL) {
> +		RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> +		rte_errno = ENOMEM;

Why it's sometimes ENOMEM and sometimes ENOTSUP when the handle is not
allocated?

> +		return NULL;
> +	}
> +
> +	return &intr_handle[index];
> +}
> +
> +int rte_intr_handle_instance_index_set(struct rte_intr_handle *intr_handle,
> +				       const struct rte_intr_handle *src,
> +				       int index)

See above regarding the "index" parameter. If it can be removed, a better name
for this function would be rte_intr_handle_copy().

> +{
> +	if (intr_handle == NULL) {
> +		RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> +		rte_errno = ENOTSUP;
> +		goto fail;
> +	}
> +
> +	if (src == NULL) {
> +		RTE_LOG(ERR, EAL, "Source interrupt instance unallocated\n");
> +		rte_errno = EINVAL;
> +		goto fail;
> +	}
> +
> +	if (index < 0) {
> +		RTE_LOG(ERR, EAL, "Index cany be negative");
> +		rte_errno = EINVAL;
> +		goto fail;
> +	}

How about making this parameter "size_t"?

> +
> +	intr_handle[index].fd = src->fd;
> +	intr_handle[index].vfio_dev_fd = src->vfio_dev_fd;
> +	intr_handle[index].type = src->type;
> +	intr_handle[index].max_intr = src->max_intr;
> +	intr_handle[index].nb_efd = src->nb_efd;
> +	intr_handle[index].efd_counter_size = src->efd_counter_size;
> +
> +	memcpy(intr_handle[index].efds, src->efds, src->nb_intr);
> +	memcpy(intr_handle[index].elist, src->elist, src->nb_intr);
> +
> +	return 0;
> +fail:
> +	return rte_errno;

Should be (-rte_errno) per documentation.
Please check all functions in this file that return an "int" status.

> [...]
> +int rte_intr_handle_dev_fd_get(const struct rte_intr_handle *intr_handle)
> +{
> +	if (intr_handle == NULL) {
> +		RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> +		rte_errno = ENOTSUP;
> +		goto fail;
> +	}
> +
> +	return intr_handle->vfio_dev_fd;
> +fail:
> +	return rte_errno;
> +}

Returning a errno value instead of an FD is very error-prone.
Probably returning (-1) is both safe and convenient?

> +
> +int rte_intr_handle_max_intr_set(struct rte_intr_handle *intr_handle,
> +				 int max_intr)
> +{
> +	if (intr_handle == NULL) {
> +		RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> +		rte_errno = ENOTSUP;
> +		goto fail;
> +	}
> +
> +	if (max_intr > intr_handle->nb_intr) {
> +		RTE_LOG(ERR, EAL, "Max_intr=%d greater than PLT_MAX_RXTX_INTR_VEC_ID=%d",

Seems like this common/cnxk name leaked here by mistake?

> +			max_intr, intr_handle->nb_intr);
> +		rte_errno = ERANGE;
> +		goto fail;
> +	}
> +
> +	intr_handle->max_intr = max_intr;
> +
> +	return 0;
> +fail:
> +	return rte_errno;
> +}
> +
> +int rte_intr_handle_max_intr_get(const struct rte_intr_handle *intr_handle)
> +{
> +	if (intr_handle == NULL) {
> +		RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> +		rte_errno = ENOTSUP;
> +		goto fail;
> +	}
> +
> +	return intr_handle->max_intr;
> +fail:
> +	return rte_errno;
> +}

Should be negative per documentation and to avoid returning a positive value
that cannot be distinguished from a successful return.
Please also check other functions in this file returning an "int" result
(not status).