Re: [dpdk-dev] [PATCH v5 1/6] eal/interrupts: implement get set APIs

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

2021-10-23 02:19 (UTC+0530), Harman Kalra:
> Prototype/Implement get set APIs for interrupt handle fields.
> User wont be able to access any of the interrupt handle fields
> directly while should use these get/set APIs to access/manipulate
> them.
> 
> Internal interrupt header i.e. rte_eal_interrupt.h is rearranged,
> as APIs defined are moved to rte_interrupts.h and epoll specific
> definitions are moved to a new header rte_epoll.h.
> Later in the series rte_eal_interrupt.h will be removed.
> 
> Signed-off-by: Harman Kalra <hkalra@marvell.com>
> Acked-by: Ray Kinsella <mdr@ashroe.eu>

Hi Harman,

After fixing some comment below feel free to add:
Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

> ---
>  MAINTAINERS                            |   1 +
>  lib/eal/common/eal_common_interrupts.c | 421 ++++++++++++++++
>  lib/eal/common/meson.build             |   1 +
>  lib/eal/include/meson.build            |   1 +
>  lib/eal/include/rte_eal_interrupts.h   | 209 +-------
>  lib/eal/include/rte_epoll.h            | 118 +++++
>  lib/eal/include/rte_interrupts.h       | 648 ++++++++++++++++++++++++-
>  lib/eal/version.map                    |  46 +-
>  8 files changed, 1232 insertions(+), 213 deletions(-)
>  create mode 100644 lib/eal/common/eal_common_interrupts.c
>  create mode 100644 lib/eal/include/rte_epoll.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 04ea23a04a..d2950400d2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -211,6 +211,7 @@ F: app/test/test_memzone.c
>  
>  Interrupt Subsystem
>  M: Harman Kalra <hkalra@marvell.com>
> +F: lib/eal/include/rte_epoll.h
>  F: lib/eal/*/*interrupts.*
>  F: app/test/test_interrupts.c
>  
> diff --git a/lib/eal/common/eal_common_interrupts.c b/lib/eal/common/eal_common_interrupts.c
> [...]
> +int rte_intr_efds_index_get(const struct rte_intr_handle *intr_handle,
> +			    int index)
> +{
> +	CHECK_VALID_INTR_HANDLE(intr_handle);
> +
> +	if (index >= intr_handle->nb_intr) {
> +		RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index,

"size" -> "index"

> +			intr_handle->nb_intr);
> +		rte_errno = EINVAL;
> +		goto fail;
> +	}
> +
> +	return intr_handle->efds[index];
> +fail:
> +	return -rte_errno;
> +}

> [...]
> +int rte_intr_vec_list_alloc(struct rte_intr_handle *intr_handle,
> +				   const char *name, int size)
> +{
> +	CHECK_VALID_INTR_HANDLE(intr_handle);
> +
> +	/* Vector list already allocated */
> +	if (intr_handle->intr_vec != NULL)
> +		return 0;

What if `size > intr_handle->vec_list_size`?

> +
> +	if (size > intr_handle->nb_intr) {
> +		RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", size,
> +		       intr_handle->nb_intr);
> +		rte_errno = ERANGE;
> +		goto fail;
> +	}
> +
> +	intr_handle->intr_vec = rte_zmalloc(name, size * sizeof(int), 0);
> +	if (intr_handle->intr_vec == NULL) {
> +		RTE_LOG(ERR, EAL, "Failed to allocate %d intr_vec", size);
> +			rte_errno = ENOMEM;
> +			goto fail;
> +	}
> +
> +	intr_handle->vec_list_size = size;
> +
> +	return 0;
> +fail:
> +	return -rte_errno;
> +}

> [...]
> diff --git a/lib/eal/include/rte_interrupts.h b/lib/eal/include/rte_interrupts.h
> index cc3bf45d8c..a29232e16a 100644
> --- a/lib/eal/include/rte_interrupts.h
> +++ b/lib/eal/include/rte_interrupts.h
> @@ -5,8 +5,11 @@
>  #ifndef _RTE_INTERRUPTS_H_
>  #define _RTE_INTERRUPTS_H_
>  
> +#include <stdbool.h>
> +
>  #include <rte_common.h>
>  #include <rte_compat.h>
> +#include <rte_epoll.h>
>  
>  /**
>   * @file
> @@ -22,6 +25,16 @@ extern "C" {
>  /** Interrupt handle */
>  struct rte_intr_handle;
>  
> +/** Interrupt instance allocation flags
> + * @see rte_intr_instance_alloc
> + */
> +/** Interrupt instance would not be shared within primary secondary process. */
> +#define RTE_INTR_INSTANCE_F_UNSHARED	0x00000001
> +/** Interrupt instance could be shared within primary secondary process. */
> +#define RTE_INTR_INSTANCE_F_SHARED	0x00000002

Nits:

"would not" -> "will not"

"could" -> "will be"

"within primary secondary process" ->
"between primary and secondary processes"

You previously suggested PRIVATE instead of UNSHARED,
it sounded better, but no strog opinion.

> [...]
> +/**
> + * @internal
> + * This API is used to populate interrupt handle with src handler fields.

"handler" -> "handle"

> + *
> + * @param intr_handle
> + *  Interrupt handle pointer.
> + * @param src
> + *  Source interrupt handle to be cloned.
> + *
> + * @return
> + *   - On success, zero.
> + *   - On failure, a negative value and rte_errno is set.
> + */
> +__rte_internal
> +int
> +rte_intr_instance_copy(struct rte_intr_handle *intr_handle,
> +		       const struct rte_intr_handle *src);
> [...]

> +/**
> + * @internal
> + * This API returns the sources from where memory is allocated for interrupt
> + * instance.
> + *
> + * @param intr_handle
> + *  pointer to the interrupt handle.
> + *
> + * @return
> + *  - On success, 1 corresponds to memory allocated via DPDK allocator APIs
> + *  - On success, 0 corresponds to memory allocated from traditional heap.
> + *  - On failure, negative value.
> + */
> +__rte_internal
> +int
> +rte_intr_instance_alloc_flag_get(const struct rte_intr_handle *intr_handle);

It returns flags passed on allow,
so return value type and description is inaccurate.
Should be uint32_t and a reference to the flag constants.