[v1,2/7] eal/interrupts: implement get set APIs
Checks
Commit Message
Implementing get set APIs for interrupt handle fields.
To make any change to the interrupt handle fields, one
should make use of these APIs.
Signed-off-by: Harman Kalra <hkalra@marvell.com>
Acked-by: Ray Kinsella <mdr@ashroe.eu>
---
lib/eal/common/eal_common_interrupts.c | 506 +++++++++++++++++++++++++
lib/eal/common/meson.build | 2 +
lib/eal/include/rte_eal_interrupts.h | 6 +-
lib/eal/version.map | 30 ++
4 files changed, 543 insertions(+), 1 deletion(-)
create mode 100644 lib/eal/common/eal_common_interrupts.c
Comments
On Fri, Sep 3, 2021 at 2:42 PM Harman Kalra <hkalra@marvell.com> wrote:
>
> Implementing get set APIs for interrupt handle fields.
> To make any change to the interrupt handle fields, one
> should make use of these APIs.
Some global comments.
- Please merge API prototype (from patch 1) and actual implementation
in a single patch.
- rte_intr_handle_ seems a rather long prefix, does it really matter
to have the _handle part?
- what part of this API needs to be exported to applications? Let's
hide as much as we can with __rte_internal.
>
> Signed-off-by: Harman Kalra <hkalra@marvell.com>
> Acked-by: Ray Kinsella <mdr@ashroe.eu>
> ---
> lib/eal/common/eal_common_interrupts.c | 506 +++++++++++++++++++++++++
> lib/eal/common/meson.build | 2 +
> lib/eal/include/rte_eal_interrupts.h | 6 +-
> lib/eal/version.map | 30 ++
> 4 files changed, 543 insertions(+), 1 deletion(-)
> create mode 100644 lib/eal/common/eal_common_interrupts.c
>
> 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)
> +{
> + 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));
We can call DPDK allocator in all cases.
That would avoid headaches on why multiprocess does not work in some
rarely tested cases.
Wdyt?
Plus "from_hugepage" is misleading, you could be in --no-huge mode,
rte_zmalloc still works.
> + 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 (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOMEM;
> + 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)
> +{
> + 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;
> + }
> +
> + 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;
> +}
> +
> +void rte_intr_handle_instance_free(struct rte_intr_handle *intr_handle)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + }
> +
> + if (intr_handle->alloc_from_hugepage)
> + rte_free(intr_handle);
> + else
> + free(intr_handle);
> +}
> +
> +int rte_intr_handle_fd_set(struct rte_intr_handle *intr_handle, int fd)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + goto fail;
> + }
> +
> + intr_handle->fd = fd;
> +
> + return 0;
> +fail:
> + return rte_errno;
> +}
> +
> +int rte_intr_handle_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->fd;
> +fail:
> + return rte_errno;
> +}
> +
> +int rte_intr_handle_type_set(struct rte_intr_handle *intr_handle,
> + enum rte_intr_handle_type type)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + goto fail;
> + }
> +
> + intr_handle->type = type;
> +
> + return 0;
> +fail:
> + return rte_errno;
> +}
> +
> +enum rte_intr_handle_type rte_intr_handle_type_get(
> + const struct rte_intr_handle *intr_handle)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + return RTE_INTR_HANDLE_UNKNOWN;
> + }
> +
> + return intr_handle->type;
> +}
> +
> +int rte_intr_handle_dev_fd_set(struct rte_intr_handle *intr_handle, int fd)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + goto fail;
> + }
> +
> + intr_handle->vfio_dev_fd = fd;
> +
> + return 0;
> +fail:
> + return rte_errno;
> +}
> +
> +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;
> +}
> +
> +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",
> + 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;
> +}
> +
> +int rte_intr_handle_nb_efd_set(struct rte_intr_handle *intr_handle,
> + int nb_efd)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + goto fail;
> + }
> +
> + intr_handle->nb_efd = nb_efd;
> +
> + return 0;
> +fail:
> + return rte_errno;
> +}
> +
> +int rte_intr_handle_nb_efd_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->nb_efd;
> +fail:
> + return rte_errno;
> +}
> +
> +int rte_intr_handle_nb_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->nb_intr;
> +fail:
> + return rte_errno;
> +}
> +
> +int rte_intr_handle_efd_counter_size_set(struct rte_intr_handle *intr_handle,
> + uint8_t efd_counter_size)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + goto fail;
> + }
> +
> + intr_handle->efd_counter_size = efd_counter_size;
> +
> + return 0;
> +fail:
> + return rte_errno;
> +}
> +
> +int rte_intr_handle_efd_counter_size_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->efd_counter_size;
> +fail:
> + return rte_errno;
> +}
> +
> +int *rte_intr_handle_efds_base(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->efds;
> +fail:
> + return NULL;
> +}
We don't need this new accessor.
It leaks the internal representation to the API caller.
If the internal representation is later changed, we would have to
maintain this array thing.
The only user is drivers/raw/ifpga/ifpga_rawdev.c.
This driver can build an array itself, and call
rte_intr_handle_efds_index_get() as much as needed.
> +
> +int rte_intr_handle_efds_index_get(const struct rte_intr_handle *intr_handle,
> + int index)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + goto fail;
> + }
> +
> + if (index >= intr_handle->nb_intr) {
> + RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index,
> + intr_handle->nb_intr);
> + rte_errno = EINVAL;
> + goto fail;
> + }
> +
> + return intr_handle->efds[index];
> +fail:
> + return rte_errno;
> +}
> +
> +int rte_intr_handle_efds_index_set(struct rte_intr_handle *intr_handle,
> + int index, int fd)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + goto fail;
> + }
> +
> + if (index >= intr_handle->nb_intr) {
> + RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index,
> + intr_handle->nb_intr);
> + rte_errno = ERANGE;
> + goto fail;
> + }
> +
> + intr_handle->efds[index] = fd;
> +
> + return 0;
> +fail:
> + return rte_errno;
> +}
> +
> +struct rte_epoll_event *rte_intr_handle_elist_index_get(
> + struct rte_intr_handle *intr_handle, int index)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + goto fail;
> + }
> +
> + if (index >= intr_handle->nb_intr) {
> + RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index,
> + intr_handle->nb_intr);
> + rte_errno = ERANGE;
> + goto fail;
> + }
> +
> + return &intr_handle->elist[index];
> +fail:
> + return NULL;
> +}
> +
> +int rte_intr_handle_elist_index_set(struct rte_intr_handle *intr_handle,
> + int index, struct rte_epoll_event elist)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + goto fail;
> + }
> +
> + if (index >= intr_handle->nb_intr) {
> + RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index,
> + intr_handle->nb_intr);
> + rte_errno = ERANGE;
> + goto fail;
> + }
> +
> + intr_handle->elist[index] = elist;
> +
> + return 0;
> +fail:
> + return rte_errno;
> +}
> +
> +int *rte_intr_handle_vec_list_base(const struct rte_intr_handle *intr_handle)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + return NULL;
> + }
> +
> + return intr_handle->intr_vec;
> +}
rte_intr_handle_vec_list_base leaks an internal representation too.
Afaics with the whole series applied, it is always paired with a
rte_intr_handle_vec_list_alloc or rte_intr_handle_vec_list_free.
rte_intr_handle_vec_list_alloc could do this check itself.
And rte_intr_handle_vec_list_free should already be fine, since it
sets intr_vec to NULL.
> +
> +int rte_intr_handle_vec_list_alloc(struct rte_intr_handle *intr_handle,
> + const char *name, int size)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + goto fail;
> + }
> +
> + /* Vector list already allocated */
> + if (intr_handle->intr_vec)
> + return 0;
> +
> + 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) {
> + 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;
> +}
> +
> +int rte_intr_handle_vec_list_index_get(
> + const struct rte_intr_handle *intr_handle, int index)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + goto fail;
> + }
> +
> + if (!intr_handle->intr_vec) {
> + RTE_LOG(ERR, EAL, "Intr vector list not allocated\n");
> + rte_errno = ENOTSUP;
> + goto fail;
> + }
> +
> + if (index > intr_handle->vec_list_size) {
> + RTE_LOG(ERR, EAL, "Index %d greater than vec list size %d\n",
> + index, intr_handle->vec_list_size);
> + rte_errno = ERANGE;
> + goto fail;
> + }
> +
> + return intr_handle->intr_vec[index];
> +fail:
> + return rte_errno;
> +}
> +
> +int rte_intr_handle_vec_list_index_set(struct rte_intr_handle *intr_handle,
> + int index, int vec)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + goto fail;
> + }
> +
> + if (!intr_handle->intr_vec) {
> + RTE_LOG(ERR, EAL, "Intr vector list not allocated\n");
> + rte_errno = ENOTSUP;
> + goto fail;
> + }
> +
> + if (index > intr_handle->vec_list_size) {
> + RTE_LOG(ERR, EAL, "Index %d greater than vec list size %d\n",
> + index, intr_handle->vec_list_size);
> + rte_errno = ERANGE;
> + goto fail;
> + }
> +
> + intr_handle->intr_vec[index] = vec;
> +
> + return 0;
> +fail:
> + return rte_errno;
> +}
> +
> +void rte_intr_handle_vec_list_free(struct rte_intr_handle *intr_handle)
> +{
> + if (intr_handle == NULL) {
> + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> + rte_errno = ENOTSUP;
> + }
> +
> + rte_free(intr_handle->intr_vec);
> + intr_handle->intr_vec = NULL;
> +}
> diff --git a/lib/eal/common/meson.build b/lib/eal/common/meson.build
> index edfca77779..47f2977539 100644
> --- a/lib/eal/common/meson.build
> +++ b/lib/eal/common/meson.build
> @@ -17,6 +17,7 @@ if is_windows
> 'eal_common_errno.c',
> 'eal_common_fbarray.c',
> 'eal_common_hexdump.c',
> + 'eal_common_interrupts.c',
> 'eal_common_launch.c',
> 'eal_common_lcore.c',
> 'eal_common_log.c',
> @@ -53,6 +54,7 @@ sources += files(
> 'eal_common_fbarray.c',
> 'eal_common_hexdump.c',
> 'eal_common_hypervisor.c',
> + 'eal_common_interrupts.c',
> 'eal_common_launch.c',
> 'eal_common_lcore.c',
> 'eal_common_log.c',
> diff --git a/lib/eal/include/rte_eal_interrupts.h b/lib/eal/include/rte_eal_interrupts.h
> index 68ca3a042d..216aece61b 100644
> --- a/lib/eal/include/rte_eal_interrupts.h
> +++ b/lib/eal/include/rte_eal_interrupts.h
> @@ -55,13 +55,17 @@ struct rte_intr_handle {
> };
> void *handle; /**< device driver handle (Windows) */
> };
> + bool alloc_from_hugepage;
> enum rte_intr_handle_type type; /**< handle type */
> uint32_t max_intr; /**< max interrupt requested */
> uint32_t nb_efd; /**< number of available efd(event fd) */
> uint8_t efd_counter_size; /**< size of efd counter, used for vdev */
> + uint16_t nb_intr;
> + /**< Max vector count, default RTE_MAX_RXTX_INTR_VEC_ID */
> int efds[RTE_MAX_RXTX_INTR_VEC_ID]; /**< intr vectors/efds mapping */
> struct rte_epoll_event elist[RTE_MAX_RXTX_INTR_VEC_ID];
> - /**< intr vector epoll event */
> + /**< intr vector epoll event */
> + uint16_t vec_list_size;
> int *intr_vec; /**< intr vector number array */
> };
>
> diff --git a/lib/eal/version.map b/lib/eal/version.map
> index beeb986adc..56108d0998 100644
> --- a/lib/eal/version.map
> +++ b/lib/eal/version.map
> @@ -426,6 +426,36 @@ EXPERIMENTAL {
>
> # added in 21.08
> rte_power_monitor_multi; # WINDOWS_NO_EXPORT
> +
> + # added in 21.11
> + rte_intr_handle_fd_set;
> + rte_intr_handle_fd_get;
> + rte_intr_handle_dev_fd_set;
> + rte_intr_handle_dev_fd_get;
> + rte_intr_handle_type_set;
> + rte_intr_handle_type_get;
> + rte_intr_handle_instance_alloc;
> + rte_intr_handle_instance_index_get;
> + rte_intr_handle_instance_free;
> + rte_intr_handle_instance_index_set;
> + rte_intr_handle_event_list_update;
> + rte_intr_handle_max_intr_set;
> + rte_intr_handle_max_intr_get;
> + rte_intr_handle_nb_efd_set;
> + rte_intr_handle_nb_efd_get;
> + rte_intr_handle_nb_intr_get;
> + rte_intr_handle_efds_index_set;
> + rte_intr_handle_efds_index_get;
> + rte_intr_handle_efds_base;
> + rte_intr_handle_elist_index_set;
> + rte_intr_handle_elist_index_get;
> + rte_intr_handle_efd_counter_size_set;
> + rte_intr_handle_efd_counter_size_get;
> + rte_intr_handle_vec_list_alloc;
> + rte_intr_handle_vec_list_index_set;
> + rte_intr_handle_vec_list_index_get;
> + rte_intr_handle_vec_list_free;
> + rte_intr_handle_vec_list_base;
> };
>
> INTERNAL {
> --
> 2.18.0
>
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).
Hi David,
Thanks for the review.
Please see my comments inline.
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, September 28, 2021 9:17 PM
> To: Harman Kalra <hkalra@marvell.com>
> Cc: dev <dev@dpdk.org>; Ray Kinsella <mdr@ashroe.eu>
> Subject: [EXT] Re: [dpdk-dev] [PATCH v1 2/7] eal/interrupts: implement get
> set APIs
>
> External Email
>
> ----------------------------------------------------------------------
> On Fri, Sep 3, 2021 at 2:42 PM Harman Kalra <hkalra@marvell.com> wrote:
> >
> > Implementing get set APIs for interrupt handle fields.
> > To make any change to the interrupt handle fields, one should make use
> > of these APIs.
>
> Some global comments.
>
> - Please merge API prototype (from patch 1) and actual implementation in a
> single patch.
<HK> Sure, will do.
> - rte_intr_handle_ seems a rather long prefix, does it really matter to have
> the _handle part?
<HK> Will fix the API names.
> - what part of this API needs to be exported to applications? Let's hide as
> much as we can with __rte_internal.
<HK> I will make all the APIs (new and some old) not used in test suite and example app as __rte_internal.
>
>
> >
> > Signed-off-by: Harman Kalra <hkalra@marvell.com>
> > Acked-by: Ray Kinsella <mdr@ashroe.eu>
> > ---
> > lib/eal/common/eal_common_interrupts.c | 506
> +++++++++++++++++++++++++
> > lib/eal/common/meson.build | 2 +
> > lib/eal/include/rte_eal_interrupts.h | 6 +-
> > lib/eal/version.map | 30 ++
> > 4 files changed, 543 insertions(+), 1 deletion(-) create mode 100644
> > lib/eal/common/eal_common_interrupts.c
> >
> > 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) {
> > + 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));
>
> We can call DPDK allocator in all cases.
> That would avoid headaches on why multiprocess does not work in some
> rarely tested cases.
> Wdyt?
>
> Plus "from_hugepage" is misleading, you could be in --no-huge mode,
> rte_zmalloc still works.
<HK> In mellanox 5 driver interrupt handle instance is freed in destructor
" mlx5_pmd_interrupt_handler_uninstall()" while DPDK memory allocators
are already cleaned up in "rte_eal_cleanup". Hence I allocated interrupt
instances for such cases from normal heap. There could be other such cases
so I think its ok to keep this support.
Regarding name, I will change " from_hugepage" to "dpdk_allocator".
As per suggestion from Dmitry, I will replace bool arg with a flag variable, to
support more such configurations in future.
>
>
> > + 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 (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOMEM;
> > + 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) {
> > + 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;
> > + }
> > +
> > + 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;
> > +}
> > +
> > +void rte_intr_handle_instance_free(struct rte_intr_handle
> > +*intr_handle) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + }
> > +
> > + if (intr_handle->alloc_from_hugepage)
> > + rte_free(intr_handle);
> > + else
> > + free(intr_handle);
> > +}
> > +
> > +int rte_intr_handle_fd_set(struct rte_intr_handle *intr_handle, int
> > +fd) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + intr_handle->fd = fd;
> > +
> > + return 0;
> > +fail:
> > + return rte_errno;
> > +}
> > +
> > +int rte_intr_handle_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->fd;
> > +fail:
> > + return rte_errno;
> > +}
> > +
> > +int rte_intr_handle_type_set(struct rte_intr_handle *intr_handle,
> > + enum rte_intr_handle_type type) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + intr_handle->type = type;
> > +
> > + return 0;
> > +fail:
> > + return rte_errno;
> > +}
> > +
> > +enum rte_intr_handle_type rte_intr_handle_type_get(
> > + const struct rte_intr_handle
> > +*intr_handle) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + return RTE_INTR_HANDLE_UNKNOWN;
> > + }
> > +
> > + return intr_handle->type;
> > +}
> > +
> > +int rte_intr_handle_dev_fd_set(struct rte_intr_handle *intr_handle,
> > +int fd) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + intr_handle->vfio_dev_fd = fd;
> > +
> > + return 0;
> > +fail:
> > + return rte_errno;
> > +}
> > +
> > +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;
> > +}
> > +
> > +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",
> > + 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;
> > +}
> > +
> > +int rte_intr_handle_nb_efd_set(struct rte_intr_handle *intr_handle,
> > + int nb_efd) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + intr_handle->nb_efd = nb_efd;
> > +
> > + return 0;
> > +fail:
> > + return rte_errno;
> > +}
> > +
> > +int rte_intr_handle_nb_efd_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->nb_efd;
> > +fail:
> > + return rte_errno;
> > +}
> > +
> > +int rte_intr_handle_nb_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->nb_intr;
> > +fail:
> > + return rte_errno;
> > +}
> > +
> > +int rte_intr_handle_efd_counter_size_set(struct rte_intr_handle
> *intr_handle,
> > + uint8_t efd_counter_size) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + intr_handle->efd_counter_size = efd_counter_size;
> > +
> > + return 0;
> > +fail:
> > + return rte_errno;
> > +}
> > +
> > +int rte_intr_handle_efd_counter_size_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->efd_counter_size;
> > +fail:
> > + return rte_errno;
> > +}
> > +
> > +int *rte_intr_handle_efds_base(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->efds;
> > +fail:
> > + return NULL;
> > +}
>
> We don't need this new accessor.
> It leaks the internal representation to the API caller.
> If the internal representation is later changed, we would have to maintain
> this array thing.
>
> The only user is drivers/raw/ifpga/ifpga_rawdev.c.
> This driver can build an array itself, and call
> rte_intr_handle_efds_index_get() as much as needed.
<HK> Yes, it’s a leak I will remove these base APIs and fix the ifpga_rawdev.c driver.
>
>
>
> > +
> > +int rte_intr_handle_efds_index_get(const struct rte_intr_handle
> *intr_handle,
> > + int index) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + if (index >= intr_handle->nb_intr) {
> > + RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index,
> > + intr_handle->nb_intr);
> > + rte_errno = EINVAL;
> > + goto fail;
> > + }
> > +
> > + return intr_handle->efds[index];
> > +fail:
> > + return rte_errno;
> > +}
> > +
> > +int rte_intr_handle_efds_index_set(struct rte_intr_handle *intr_handle,
> > + int index, int fd) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + if (index >= intr_handle->nb_intr) {
> > + RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index,
> > + intr_handle->nb_intr);
> > + rte_errno = ERANGE;
> > + goto fail;
> > + }
> > +
> > + intr_handle->efds[index] = fd;
> > +
> > + return 0;
> > +fail:
> > + return rte_errno;
> > +}
> > +
> > +struct rte_epoll_event *rte_intr_handle_elist_index_get(
> > + struct rte_intr_handle *intr_handle,
> > +int index) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + if (index >= intr_handle->nb_intr) {
> > + RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index,
> > + intr_handle->nb_intr);
> > + rte_errno = ERANGE;
> > + goto fail;
> > + }
> > +
> > + return &intr_handle->elist[index];
> > +fail:
> > + return NULL;
> > +}
> > +
> > +int rte_intr_handle_elist_index_set(struct rte_intr_handle *intr_handle,
> > + int index, struct rte_epoll_event
> > +elist) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + if (index >= intr_handle->nb_intr) {
> > + RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index,
> > + intr_handle->nb_intr);
> > + rte_errno = ERANGE;
> > + goto fail;
> > + }
> > +
> > + intr_handle->elist[index] = elist;
> > +
> > + return 0;
> > +fail:
> > + return rte_errno;
> > +}
> > +
> > +int *rte_intr_handle_vec_list_base(const struct rte_intr_handle
> > +*intr_handle) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + return NULL;
> > + }
> > +
> > + return intr_handle->intr_vec;
> > +}
>
>
> rte_intr_handle_vec_list_base leaks an internal representation too.
>
> Afaics with the whole series applied, it is always paired with a
> rte_intr_handle_vec_list_alloc or rte_intr_handle_vec_list_free.
> rte_intr_handle_vec_list_alloc could do this check itself.
> And rte_intr_handle_vec_list_free should already be fine, since it sets
> intr_vec to NULL.
<HK> Yes, base API not required.
Thanks
Harman
>
>
>
>
>
> > +
> > +int rte_intr_handle_vec_list_alloc(struct rte_intr_handle *intr_handle,
> > + const char *name, int size) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + /* Vector list already allocated */
> > + if (intr_handle->intr_vec)
> > + return 0;
> > +
> > + 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) {
> > + 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;
> > +}
> > +
> > +int rte_intr_handle_vec_list_index_get(
> > + const struct rte_intr_handle *intr_handle, int
> > +index) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + if (!intr_handle->intr_vec) {
> > + RTE_LOG(ERR, EAL, "Intr vector list not allocated\n");
> > + rte_errno = ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + if (index > intr_handle->vec_list_size) {
> > + RTE_LOG(ERR, EAL, "Index %d greater than vec list size %d\n",
> > + index, intr_handle->vec_list_size);
> > + rte_errno = ERANGE;
> > + goto fail;
> > + }
> > +
> > + return intr_handle->intr_vec[index];
> > +fail:
> > + return rte_errno;
> > +}
> > +
> > +int rte_intr_handle_vec_list_index_set(struct rte_intr_handle
> *intr_handle,
> > + int index, int vec) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + if (!intr_handle->intr_vec) {
> > + RTE_LOG(ERR, EAL, "Intr vector list not allocated\n");
> > + rte_errno = ENOTSUP;
> > + goto fail;
> > + }
> > +
> > + if (index > intr_handle->vec_list_size) {
> > + RTE_LOG(ERR, EAL, "Index %d greater than vec list size %d\n",
> > + index, intr_handle->vec_list_size);
> > + rte_errno = ERANGE;
> > + goto fail;
> > + }
> > +
> > + intr_handle->intr_vec[index] = vec;
> > +
> > + return 0;
> > +fail:
> > + return rte_errno;
> > +}
> > +
> > +void rte_intr_handle_vec_list_free(struct rte_intr_handle
> > +*intr_handle) {
> > + if (intr_handle == NULL) {
> > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> > + rte_errno = ENOTSUP;
> > + }
> > +
> > + rte_free(intr_handle->intr_vec);
> > + intr_handle->intr_vec = NULL;
> > +}
> > diff --git a/lib/eal/common/meson.build b/lib/eal/common/meson.build
> > index edfca77779..47f2977539 100644
> > --- a/lib/eal/common/meson.build
> > +++ b/lib/eal/common/meson.build
> > @@ -17,6 +17,7 @@ if is_windows
> > 'eal_common_errno.c',
> > 'eal_common_fbarray.c',
> > 'eal_common_hexdump.c',
> > + 'eal_common_interrupts.c',
> > 'eal_common_launch.c',
> > 'eal_common_lcore.c',
> > 'eal_common_log.c',
> > @@ -53,6 +54,7 @@ sources += files(
> > 'eal_common_fbarray.c',
> > 'eal_common_hexdump.c',
> > 'eal_common_hypervisor.c',
> > + 'eal_common_interrupts.c',
> > 'eal_common_launch.c',
> > 'eal_common_lcore.c',
> > 'eal_common_log.c',
> > diff --git a/lib/eal/include/rte_eal_interrupts.h
> > b/lib/eal/include/rte_eal_interrupts.h
> > index 68ca3a042d..216aece61b 100644
> > --- a/lib/eal/include/rte_eal_interrupts.h
> > +++ b/lib/eal/include/rte_eal_interrupts.h
> > @@ -55,13 +55,17 @@ struct rte_intr_handle {
> > };
> > void *handle; /**< device driver handle (Windows) */
> > };
> > + bool alloc_from_hugepage;
> > enum rte_intr_handle_type type; /**< handle type */
> > uint32_t max_intr; /**< max interrupt requested */
> > uint32_t nb_efd; /**< number of available efd(event fd) */
> > uint8_t efd_counter_size; /**< size of efd counter, used for vdev */
> > + uint16_t nb_intr;
> > + /**< Max vector count, default
> > + RTE_MAX_RXTX_INTR_VEC_ID */
> > int efds[RTE_MAX_RXTX_INTR_VEC_ID]; /**< intr vectors/efds
> mapping */
> > struct rte_epoll_event elist[RTE_MAX_RXTX_INTR_VEC_ID];
> > - /**< intr vector epoll event */
> > + /**< intr vector epoll event */
> > + uint16_t vec_list_size;
> > int *intr_vec; /**< intr vector number array */
> > };
> >
> > diff --git a/lib/eal/version.map b/lib/eal/version.map index
> > beeb986adc..56108d0998 100644
> > --- a/lib/eal/version.map
> > +++ b/lib/eal/version.map
> > @@ -426,6 +426,36 @@ EXPERIMENTAL {
> >
> > # added in 21.08
> > rte_power_monitor_multi; # WINDOWS_NO_EXPORT
> > +
> > + # added in 21.11
> > + rte_intr_handle_fd_set;
> > + rte_intr_handle_fd_get;
> > + rte_intr_handle_dev_fd_set;
> > + rte_intr_handle_dev_fd_get;
> > + rte_intr_handle_type_set;
> > + rte_intr_handle_type_get;
> > + rte_intr_handle_instance_alloc;
> > + rte_intr_handle_instance_index_get;
> > + rte_intr_handle_instance_free;
> > + rte_intr_handle_instance_index_set;
> > + rte_intr_handle_event_list_update;
> > + rte_intr_handle_max_intr_set;
> > + rte_intr_handle_max_intr_get;
> > + rte_intr_handle_nb_efd_set;
> > + rte_intr_handle_nb_efd_get;
> > + rte_intr_handle_nb_intr_get;
> > + rte_intr_handle_efds_index_set;
> > + rte_intr_handle_efds_index_get;
> > + rte_intr_handle_efds_base;
> > + rte_intr_handle_elist_index_set;
> > + rte_intr_handle_elist_index_get;
> > + rte_intr_handle_efd_counter_size_set;
> > + rte_intr_handle_efd_counter_size_get;
> > + rte_intr_handle_vec_list_alloc;
> > + rte_intr_handle_vec_list_index_set;
> > + rte_intr_handle_vec_list_index_get;
> > + rte_intr_handle_vec_list_free;
> > + rte_intr_handle_vec_list_base;
> > };
> >
> > INTERNAL {
> > --
> > 2.18.0
> >
>
>
> --
> David Marchand
On Mon, Oct 4, 2021 at 10:51 AM Harman Kalra <hkalra@marvell.com> wrote:
> > > +struct rte_intr_handle *rte_intr_handle_instance_alloc(int size,
> > > + bool
> > > +from_hugepage) {
> > > + 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));
> >
> > We can call DPDK allocator in all cases.
> > That would avoid headaches on why multiprocess does not work in some
> > rarely tested cases.
> > Wdyt?
> >
> > Plus "from_hugepage" is misleading, you could be in --no-huge mode,
> > rte_zmalloc still works.
>
> <HK> In mellanox 5 driver interrupt handle instance is freed in destructor
> " mlx5_pmd_interrupt_handler_uninstall()" while DPDK memory allocators
> are already cleaned up in "rte_eal_cleanup". Hence I allocated interrupt
> instances for such cases from normal heap. There could be other such cases
> so I think its ok to keep this support.
This is surprising.
Why would the mlx5 driver wait to release in a destructor?
It should be done once no interrupt handler is necessary (like when
stopping all ports), and that would be before rte_eal_cleanup().
Hi Dmitry,
Thanks for reviewing the series.
Please find my comments inline.
> -----Original Message-----
> From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> Sent: Sunday, October 3, 2021 11:35 PM
> To: Harman Kalra <hkalra@marvell.com>
> Cc: dev@dpdk.org; Ray Kinsella <mdr@ashroe.eu>
> Subject: [EXT] Re: [dpdk-dev] [PATCH v1 2/7] eal/interrupts: implement get
> set APIs
>
> External Email
>
> ----------------------------------------------------------------------
> 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.)
>
<HK> Having second parameter "flags" is a good suggestion, I will include it.
> > +{
> > + 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.
<HK> User wont be able to manipulate the pointer as he is not aware of size of struct rte_intr_handle.
He will observe "dereferencing pointer to incomplete type" compilation error.
> 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.
<HK> Do you mean, we should have separate APIs for single allocation and batch allocation? As get API
will be useful only in case of batch allocation. Currently interrupt autotests and ifpga_rawdev driver makes
batch allocation.
I think common API for single and batch is fine, get API is required for returning a particular intr_handle instance.
But one problem I see in current implementation is there should be upper limit check for index in get/set
API, which I will fix.
>
> > +{
> > + 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?
<HK> I will fix and make it symmetrical across.
>
> > + 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().
<HK> I think get API is required.
>
> > +{
> > + 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"?
<HK> You mean index ? It can be 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.
<HK> Sure will fix it across APIs.
>
> > [...]
> > +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?
<HK> Ack
>
> > +
> > +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?
<HK> Thanks for catching this.
>
> > + 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).
<HK> Will fix it.
2021-10-04 10:37 (UTC+0000), Harman Kalra:
> [...]
> > > +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.
>
> <HK> User wont be able to manipulate the pointer as he is not aware of size of struct rte_intr_handle.
> He will observe "dereferencing pointer to incomplete type" compilation error.
Sorry, my bad.
> > 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.
>
> <HK> Do you mean, we should have separate APIs for single allocation and batch allocation? As get API
> will be useful only in case of batch allocation. Currently interrupt autotests and ifpga_rawdev driver makes
> batch allocation.
> I think common API for single and batch is fine, get API is required for returning a particular intr_handle instance.
> But one problem I see in current implementation is there should be upper limit check for index in get/set
> API, which I will fix.
I don't think we need different APIs, I was asking if it was your intention.
Now I understand it and agree with you.
> > > +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().
>
> <HK> I think get API is required.
Maybe index is still not needed: "intr_handle" can just be a pointer to the
right item obtained with rte_intr_handle_instance_index_get(). This way you
also don't need to duplicate the index-checking logic.
> -----Original Message-----
> From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> Sent: Monday, October 4, 2021 4:48 PM
> To: Harman Kalra <hkalra@marvell.com>
> Cc: dev@dpdk.org; Ray Kinsella <mdr@ashroe.eu>; David Marchand
> <david.marchand@redhat.com>
> Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v1 2/7] eal/interrupts: implement
> get set APIs
>
> 2021-10-04 10:37 (UTC+0000), Harman Kalra:
> > [...]
> > > > +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.
> >
> > <HK> User wont be able to manipulate the pointer as he is not aware of
> size of struct rte_intr_handle.
> > He will observe "dereferencing pointer to incomplete type" compilation
> error.
>
> Sorry, my bad.
>
> > > 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.
> >
> > <HK> Do you mean, we should have separate APIs for single allocation
> > and batch allocation? As get API will be useful only in case of batch
> > allocation. Currently interrupt autotests and ifpga_rawdev driver makes
> batch allocation.
> > I think common API for single and batch is fine, get API is required for
> returning a particular intr_handle instance.
> > But one problem I see in current implementation is there should be
> > upper limit check for index in get/set API, which I will fix.
>
> I don't think we need different APIs, I was asking if it was your intention.
> Now I understand it and agree with you.
>
> > > > +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().
> >
> > <HK> I think get API is required.
>
> Maybe index is still not needed: "intr_handle" can just be a pointer to the
> right item obtained with rte_intr_handle_instance_index_get(). This way you
> also don't need to duplicate the index-checking logic.
In the current implementation, batch allocation of interrupt handle may lead to mem leak while
freeing efds and elist array. I am only freeing efds/elist for intr_handle[0] in rte_intr_handle_instance_free().
To free efds/elist of all the intr_handles[], either I should cache the size parameter passed during alloc. But
where should I store it? In first instance of struct rte_intr_handle. I don't think it will be a good idea.
Since batch allocation is only done in test suite and ifpga_rawdev.c, to keep things simpler let's restrict
rte_intr_handle_instance_alloc() to single instance allocation and user can call this API in a loop and
maintain array of handles locally.
With this approach get_index API is not required and set_index API can be renamed to rte_intr_handle_copy()
Thoughts?
Thanks
Harman
04/10/2021 11:57, David Marchand:
> On Mon, Oct 4, 2021 at 10:51 AM Harman Kalra <hkalra@marvell.com> wrote:
> > > > +struct rte_intr_handle *rte_intr_handle_instance_alloc(int size,
> > > > + bool
> > > > +from_hugepage) {
> > > > + 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));
> > >
> > > We can call DPDK allocator in all cases.
> > > That would avoid headaches on why multiprocess does not work in some
> > > rarely tested cases.
> > > Wdyt?
> > >
> > > Plus "from_hugepage" is misleading, you could be in --no-huge mode,
> > > rte_zmalloc still works.
> >
> > <HK> In mellanox 5 driver interrupt handle instance is freed in destructor
> > " mlx5_pmd_interrupt_handler_uninstall()" while DPDK memory allocators
> > are already cleaned up in "rte_eal_cleanup". Hence I allocated interrupt
> > instances for such cases from normal heap. There could be other such cases
> > so I think its ok to keep this support.
>
> This is surprising.
> Why would the mlx5 driver wait to release in a destructor?
> It should be done once no interrupt handler is necessary (like when
> stopping all ports), and that would be before rte_eal_cleanup().
I agree with David.
I prefer a simpler API which always use rte_malloc,
and make sure interrupts are always handled between
rte_eal_init and rte_eal_cleanup.
The mlx5 PMD could be reworked to match this requirement.
In any case we should not any memory management in constructors/destructors.
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, October 12, 2021 8:53 PM
> To: Harman Kalra <hkalra@marvell.com>
> Cc: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org; Ray Kinsella
> <mdr@ashroe.eu>; Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>; David
> Marchand <david.marchand@redhat.com>; viacheslavo@nvidia.com;
> matan@nvidia.com
> Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v1 2/7] eal/interrupts: implement
> get set APIs
>
> 04/10/2021 11:57, David Marchand:
> > On Mon, Oct 4, 2021 at 10:51 AM Harman Kalra <hkalra@marvell.com>
> wrote:
> > > > > +struct rte_intr_handle *rte_intr_handle_instance_alloc(int size,
> > > > > + bool
> > > > > +from_hugepage) {
> > > > > + 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));
> > > >
> > > > We can call DPDK allocator in all cases.
> > > > That would avoid headaches on why multiprocess does not work in
> > > > some rarely tested cases.
> > > > Wdyt?
> > > >
> > > > Plus "from_hugepage" is misleading, you could be in --no-huge
> > > > mode, rte_zmalloc still works.
> > >
> > > <HK> In mellanox 5 driver interrupt handle instance is freed in
> > > destructor " mlx5_pmd_interrupt_handler_uninstall()" while DPDK
> > > memory allocators are already cleaned up in "rte_eal_cleanup". Hence
> > > I allocated interrupt instances for such cases from normal heap.
> > > There could be other such cases so I think its ok to keep this support.
> >
> > This is surprising.
> > Why would the mlx5 driver wait to release in a destructor?
> > It should be done once no interrupt handler is necessary (like when
> > stopping all ports), and that would be before rte_eal_cleanup().
>
> I agree with David.
> I prefer a simpler API which always use rte_malloc, and make sure interrupts
> are always handled between rte_eal_init and rte_eal_cleanup.
> The mlx5 PMD could be reworked to match this requirement.
> In any case we should not any memory management in
> constructors/destructors.
Hi Thomas, David
There are couple of more dependencies on glibc heap APIs:
1. "rte_eal_alarm_init()" allocates an interrupt instance which is used for timerfd,
is called before "rte_eal_memory_init()" which does the memseg init.
Not sure what all challenges we may face in moving alarm_init after memory_init
as it might break some subsystem inits.
Other option could be to allocate interrupt instance for timerfd on first alarm_setup call.
2. Currently interrupt handle field inside struct rte_pci_device is static which is changed
to a pointer in this series(as struct rte_intr_handle is hidden inside a c file and size is unknown outside).
I am allocating the memory for this interrupt instance inside "rte_pci_probe_one_driver()" just before
"pci_vfio_map_resource()" which sets up vfio resources and calls " pci_vfio_setup_interrupts()" which setups
the interrupt support. Here challenge is "rte_bus_probe()" also gets called before "rte_eal_memory_init()".
There are many other drivers which statically declares the interrupt handles inside their respective private
structures and memory for those structure was allocated from heap. For such drivers I allocated interrupt instances
also using glibc heap APIs.
Thanks
Harman
>
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Harman Kalra
> Sent: Wednesday, October 13, 2021 11:24 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org; Ray Kinsella
> <mdr@ashroe.eu>; Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>; David
> Marchand <david.marchand@redhat.com>; viacheslavo@nvidia.com;
> matan@nvidia.com
> Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v1 2/7] eal/interrupts: implement
> get set APIs
>
>
>
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Tuesday, October 12, 2021 8:53 PM
> > To: Harman Kalra <hkalra@marvell.com>
> > Cc: Raslan Darawsheh <rasland@nvidia.com>; dev@dpdk.org; Ray Kinsella
> > <mdr@ashroe.eu>; Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>; David
> > Marchand <david.marchand@redhat.com>; viacheslavo@nvidia.com;
> > matan@nvidia.com
> > Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v1 2/7] eal/interrupts:
> > implement get set APIs
> >
> > 04/10/2021 11:57, David Marchand:
> > > On Mon, Oct 4, 2021 at 10:51 AM Harman Kalra <hkalra@marvell.com>
> > wrote:
> > > > > > +struct rte_intr_handle *rte_intr_handle_instance_alloc(int size,
> > > > > > + bool
> > > > > > +from_hugepage) {
> > > > > > + 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));
> > > > >
> > > > > We can call DPDK allocator in all cases.
> > > > > That would avoid headaches on why multiprocess does not work in
> > > > > some rarely tested cases.
> > > > > Wdyt?
> > > > >
> > > > > Plus "from_hugepage" is misleading, you could be in --no-huge
> > > > > mode, rte_zmalloc still works.
> > > >
> > > > <HK> In mellanox 5 driver interrupt handle instance is freed in
> > > > destructor " mlx5_pmd_interrupt_handler_uninstall()" while DPDK
> > > > memory allocators are already cleaned up in "rte_eal_cleanup".
> > > > Hence I allocated interrupt instances for such cases from normal heap.
> > > > There could be other such cases so I think its ok to keep this support.
> > >
> > > This is surprising.
> > > Why would the mlx5 driver wait to release in a destructor?
> > > It should be done once no interrupt handler is necessary (like when
> > > stopping all ports), and that would be before rte_eal_cleanup().
> >
> > I agree with David.
> > I prefer a simpler API which always use rte_malloc, and make sure
> > interrupts are always handled between rte_eal_init and rte_eal_cleanup.
> > The mlx5 PMD could be reworked to match this requirement.
> > In any case we should not any memory management in
> > constructors/destructors.
>
>
> Hi Thomas, David
>
> There are couple of more dependencies on glibc heap APIs:
> 1. "rte_eal_alarm_init()" allocates an interrupt instance which is used for
> timerfd, is called before "rte_eal_memory_init()" which does the memseg
> init.
> Not sure what all challenges we may face in moving alarm_init after
> memory_init as it might break some subsystem inits.
> Other option could be to allocate interrupt instance for timerfd on first
> alarm_setup call.
>
> 2. Currently interrupt handle field inside struct rte_pci_device is static which
> is changed to a pointer in this series(as struct rte_intr_handle is hidden inside
> a c file and size is unknown outside).
> I am allocating the memory for this interrupt instance inside
> "rte_pci_probe_one_driver()" just before "pci_vfio_map_resource()" which
> sets up vfio resources and calls " pci_vfio_setup_interrupts()" which setups
> the interrupt support. Here challenge is "rte_bus_probe()" also gets called
> before "rte_eal_memory_init()".
Sorry my bad, bus probing happens after "rte_eal_memory_init()", so second point
Is invalid.
>
> There are many other drivers which statically declares the interrupt handles
> inside their respective private structures and memory for those structure
> was allocated from heap. For such drivers I allocated interrupt instances also
> using glibc heap APIs.
>
> Thanks
> Harman
>
>
>
> >
13/10/2021 19:57, Harman Kalra:
> From: dev <dev-bounces@dpdk.org> On Behalf Of Harman Kalra
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 04/10/2021 11:57, David Marchand:
> > > > On Mon, Oct 4, 2021 at 10:51 AM Harman Kalra <hkalra@marvell.com>
> > > wrote:
> > > > > > > +struct rte_intr_handle *rte_intr_handle_instance_alloc(int size,
> > > > > > > + bool
> > > > > > > +from_hugepage) {
> > > > > > > + 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));
> > > > > >
> > > > > > We can call DPDK allocator in all cases.
> > > > > > That would avoid headaches on why multiprocess does not work in
> > > > > > some rarely tested cases.
> > > > > > Wdyt?
> > > > > >
> > > > > > Plus "from_hugepage" is misleading, you could be in --no-huge
> > > > > > mode, rte_zmalloc still works.
> > > > >
> > > > > <HK> In mellanox 5 driver interrupt handle instance is freed in
> > > > > destructor " mlx5_pmd_interrupt_handler_uninstall()" while DPDK
> > > > > memory allocators are already cleaned up in "rte_eal_cleanup".
> > > > > Hence I allocated interrupt instances for such cases from normal heap.
> > > > > There could be other such cases so I think its ok to keep this support.
> > > >
> > > > This is surprising.
> > > > Why would the mlx5 driver wait to release in a destructor?
> > > > It should be done once no interrupt handler is necessary (like when
> > > > stopping all ports), and that would be before rte_eal_cleanup().
> > >
> > > I agree with David.
> > > I prefer a simpler API which always use rte_malloc, and make sure
> > > interrupts are always handled between rte_eal_init and rte_eal_cleanup.
> > > The mlx5 PMD could be reworked to match this requirement.
> > > In any case we should not any memory management in
> > > constructors/destructors.
For info, Dmitry is going to send a fix for mlx5.
> > Hi Thomas, David
> >
> > There are couple of more dependencies on glibc heap APIs:
> > 1. "rte_eal_alarm_init()" allocates an interrupt instance which is used for
> > timerfd, is called before "rte_eal_memory_init()" which does the memseg
> > init.
> > Not sure what all challenges we may face in moving alarm_init after
> > memory_init as it might break some subsystem inits.
> > Other option could be to allocate interrupt instance for timerfd on first
> > alarm_setup call.
Indeed it is an issue.
[...]
> > There are many other drivers which statically declares the interrupt handles
> > inside their respective private structures and memory for those structure
> > was allocated from heap. For such drivers I allocated interrupt instances also
> > using glibc heap APIs.
Could you use rte_malloc in these drivers?
13/10/2021 20:52, Thomas Monjalon:
> 13/10/2021 19:57, Harman Kalra:
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Harman Kalra
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 04/10/2021 11:57, David Marchand:
> > > > > On Mon, Oct 4, 2021 at 10:51 AM Harman Kalra <hkalra@marvell.com>
> > > > wrote:
> > > > > > > > +struct rte_intr_handle *rte_intr_handle_instance_alloc(int size,
> > > > > > > > + bool
> > > > > > > > +from_hugepage) {
> > > > > > > > + 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));
> > > > > > >
> > > > > > > We can call DPDK allocator in all cases.
> > > > > > > That would avoid headaches on why multiprocess does not work in
> > > > > > > some rarely tested cases.
[...]
> > > > I agree with David.
> > > > I prefer a simpler API which always use rte_malloc, and make sure
> > > > interrupts are always handled between rte_eal_init and rte_eal_cleanup.
[...]
> > > There are couple of more dependencies on glibc heap APIs:
> > > 1. "rte_eal_alarm_init()" allocates an interrupt instance which is used for
> > > timerfd, is called before "rte_eal_memory_init()" which does the memseg
> > > init.
> > > Not sure what all challenges we may face in moving alarm_init after
> > > memory_init as it might break some subsystem inits.
> > > Other option could be to allocate interrupt instance for timerfd on first
> > > alarm_setup call.
>
> Indeed it is an issue.
>
> [...]
>
> > > There are many other drivers which statically declares the interrupt handles
> > > inside their respective private structures and memory for those structure
> > > was allocated from heap. For such drivers I allocated interrupt instances also
> > > using glibc heap APIs.
>
> Could you use rte_malloc in these drivers?
If we take the direction of 2 different allocations mode for the interrupts,
I suggest we make it automatic without any API parameter.
We don't have any function to check rte_malloc readiness I think.
But we can detect whether shared memory is ready with this check:
rte_eal_get_configuration()->mem_config->magic == RTE_MAGIC
This check is true at the end of rte_eal_init, so it is false during probing.
Would it be enough? Or should we implement rte_malloc_is_ready()?
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, October 14, 2021 1:53 PM
> To: Harman Kalra <hkalra@marvell.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Ray Kinsella
> <mdr@ashroe.eu>; Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>; David
> Marchand <david.marchand@redhat.com>; viacheslavo@nvidia.com;
> matan@nvidia.com
> Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v1 2/7] eal/interrupts: implement
> get set APIs
>
> 13/10/2021 20:52, Thomas Monjalon:
> > 13/10/2021 19:57, Harman Kalra:
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Harman Kalra
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 04/10/2021 11:57, David Marchand:
> > > > > > On Mon, Oct 4, 2021 at 10:51 AM Harman Kalra
> > > > > > <hkalra@marvell.com>
> > > > > wrote:
> > > > > > > > > +struct rte_intr_handle *rte_intr_handle_instance_alloc(int
> size,
> > > > > > > > > +
> > > > > > > > > +bool
> > > > > > > > > +from_hugepage) {
> > > > > > > > > + 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));
> > > > > > > >
> > > > > > > > We can call DPDK allocator in all cases.
> > > > > > > > That would avoid headaches on why multiprocess does not
> > > > > > > > work in some rarely tested cases.
> [...]
> > > > > I agree with David.
> > > > > I prefer a simpler API which always use rte_malloc, and make
> > > > > sure interrupts are always handled between rte_eal_init and
> rte_eal_cleanup.
> [...]
> > > > There are couple of more dependencies on glibc heap APIs:
> > > > 1. "rte_eal_alarm_init()" allocates an interrupt instance which is
> > > > used for timerfd, is called before "rte_eal_memory_init()" which
> > > > does the memseg init.
> > > > Not sure what all challenges we may face in moving alarm_init
> > > > after memory_init as it might break some subsystem inits.
> > > > Other option could be to allocate interrupt instance for timerfd
> > > > on first alarm_setup call.
> >
> > Indeed it is an issue.
> >
> > [...]
> >
> > > > There are many other drivers which statically declares the
> > > > interrupt handles inside their respective private structures and
> > > > memory for those structure was allocated from heap. For such
> > > > drivers I allocated interrupt instances also using glibc heap APIs.
> >
> > Could you use rte_malloc in these drivers?
>
> If we take the direction of 2 different allocations mode for the interrupts, I
> suggest we make it automatic without any API parameter.
> We don't have any function to check rte_malloc readiness I think.
> But we can detect whether shared memory is ready with this check:
> rte_eal_get_configuration()->mem_config->magic == RTE_MAGIC This check
> is true at the end of rte_eal_init, so it is false during probing.
> Would it be enough? Or should we implement rte_malloc_is_ready()?
Hi Thomas,
It's a very good suggestion. Let's implement "rte_malloc_is_ready()" which could be as
simple as " rte_eal_get_configuration()->mem_config->magic == RTE_MAGIC" check.
There may be more consumers for this API in future.
If we are making it automatic detection, shall we now even have argument to this alloc API?
I added a flags argument (32 bit) in latest series where each bit of this flag can be an allocation capability.
I used two bits for discriminating between glibc malloc and rte_malloc. Shall we keep it or drop it?
David, Dmitry please share your thoughts.
Thanks
Harman
>
On Thu, Oct 14, 2021 at 11:31 AM Harman Kalra <hkalra@marvell.com> wrote:
> If we are making it automatic detection, shall we now even have argument to this alloc API?
> I added a flags argument (32 bit) in latest series where each bit of this flag can be an allocation capability.
> I used two bits for discriminating between glibc malloc and rte_malloc. Shall we keep it or drop it?
>
> David, Dmitry please share your thoughts.
I don't have ideas of how we would extend allocations of such object,
so I am unsure.
In doubt, I would keep this flags field, and validate it's always 0
(as mentioned in my reply on ABI).
14/10/2021 11:31, Harman Kalra:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 13/10/2021 20:52, Thomas Monjalon:
> > > 13/10/2021 19:57, Harman Kalra:
> > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Harman Kalra
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > 04/10/2021 11:57, David Marchand:
> > > > > > > On Mon, Oct 4, 2021 at 10:51 AM Harman Kalra
> > > > > > > <hkalra@marvell.com>
> > > > > > wrote:
> > > > > > > > > > +struct rte_intr_handle *rte_intr_handle_instance_alloc(int
> > size,
> > > > > > > > > > +
> > > > > > > > > > +bool
> > > > > > > > > > +from_hugepage) {
> > > > > > > > > > + 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));
> > > > > > > > >
> > > > > > > > > We can call DPDK allocator in all cases.
> > > > > > > > > That would avoid headaches on why multiprocess does not
> > > > > > > > > work in some rarely tested cases.
> > [...]
> > > > > > I agree with David.
> > > > > > I prefer a simpler API which always use rte_malloc, and make
> > > > > > sure interrupts are always handled between rte_eal_init and
> > rte_eal_cleanup.
> > [...]
> > > > > There are couple of more dependencies on glibc heap APIs:
> > > > > 1. "rte_eal_alarm_init()" allocates an interrupt instance which is
> > > > > used for timerfd, is called before "rte_eal_memory_init()" which
> > > > > does the memseg init.
> > > > > Not sure what all challenges we may face in moving alarm_init
> > > > > after memory_init as it might break some subsystem inits.
> > > > > Other option could be to allocate interrupt instance for timerfd
> > > > > on first alarm_setup call.
> > >
> > > Indeed it is an issue.
> > >
> > > [...]
> > >
> > > > > There are many other drivers which statically declares the
> > > > > interrupt handles inside their respective private structures and
> > > > > memory for those structure was allocated from heap. For such
> > > > > drivers I allocated interrupt instances also using glibc heap APIs.
> > >
> > > Could you use rte_malloc in these drivers?
> >
> > If we take the direction of 2 different allocations mode for the interrupts, I
> > suggest we make it automatic without any API parameter.
> > We don't have any function to check rte_malloc readiness I think.
> > But we can detect whether shared memory is ready with this check:
> > rte_eal_get_configuration()->mem_config->magic == RTE_MAGIC This check
> > is true at the end of rte_eal_init, so it is false during probing.
> > Would it be enough? Or should we implement rte_malloc_is_ready()?
>
> Hi Thomas,
>
> It's a very good suggestion. Let's implement "rte_malloc_is_ready()" which could be as
> simple as " rte_eal_get_configuration()->mem_config->magic == RTE_MAGIC" check.
> There may be more consumers for this API in future.
You cannot rely on the magic because it is set only after probing.
For such API you need to have another internal flag to check that malloc is setup.
2021-10-14 09:31 (UTC+0000), Harman Kalra:
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Thursday, October 14, 2021 1:53 PM
> > To: Harman Kalra <hkalra@marvell.com>
> > Cc: dev@dpdk.org; Raslan Darawsheh <rasland@nvidia.com>; Ray Kinsella
> > <mdr@ashroe.eu>; Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>; David
> > Marchand <david.marchand@redhat.com>; viacheslavo@nvidia.com;
> > matan@nvidia.com
> > Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v1 2/7] eal/interrupts: implement
> > get set APIs
> >
> > 13/10/2021 20:52, Thomas Monjalon:
> > > 13/10/2021 19:57, Harman Kalra:
> > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Harman Kalra
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > 04/10/2021 11:57, David Marchand:
> > > > > > > On Mon, Oct 4, 2021 at 10:51 AM Harman Kalra
> > > > > > > <hkalra@marvell.com>
> > > > > > wrote:
> > > > > > > > > > +struct rte_intr_handle *rte_intr_handle_instance_alloc(int
> > size,
> > > > > > > > > > +
> > > > > > > > > > +bool
> > > > > > > > > > +from_hugepage) {
> > > > > > > > > > + 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));
> > > > > > > > >
> > > > > > > > > We can call DPDK allocator in all cases.
> > > > > > > > > That would avoid headaches on why multiprocess does not
> > > > > > > > > work in some rarely tested cases.
> > [...]
> > > > > > I agree with David.
> > > > > > I prefer a simpler API which always use rte_malloc, and make
> > > > > > sure interrupts are always handled between rte_eal_init and
> > rte_eal_cleanup.
> > [...]
> > > > > There are couple of more dependencies on glibc heap APIs:
> > > > > 1. "rte_eal_alarm_init()" allocates an interrupt instance which is
> > > > > used for timerfd, is called before "rte_eal_memory_init()" which
> > > > > does the memseg init.
> > > > > Not sure what all challenges we may face in moving alarm_init
> > > > > after memory_init as it might break some subsystem inits.
> > > > > Other option could be to allocate interrupt instance for timerfd
> > > > > on first alarm_setup call.
> > >
> > > Indeed it is an issue.
> > >
> > > [...]
> > >
> > > > > There are many other drivers which statically declares the
> > > > > interrupt handles inside their respective private structures and
> > > > > memory for those structure was allocated from heap. For such
> > > > > drivers I allocated interrupt instances also using glibc heap APIs.
> > >
> > > Could you use rte_malloc in these drivers?
> >
> > If we take the direction of 2 different allocations mode for the interrupts, I
> > suggest we make it automatic without any API parameter.
> > We don't have any function to check rte_malloc readiness I think.
> > But we can detect whether shared memory is ready with this check:
> > rte_eal_get_configuration()->mem_config->magic == RTE_MAGIC This check
> > is true at the end of rte_eal_init, so it is false during probing.
> > Would it be enough? Or should we implement rte_malloc_is_ready()?
>
> Hi Thomas,
>
> It's a very good suggestion. Let's implement "rte_malloc_is_ready()" which could be as
> simple as " rte_eal_get_configuration()->mem_config->magic == RTE_MAGIC" check.
> There may be more consumers for this API in future.
I doubt it should be public. How it is supposed to be used?
Any application code for DPDK necessarily calls rte_eal_init() first,
after that this function would always return true.
>
> If we are making it automatic detection, shall we now even have argument to this alloc API?
> I added a flags argument (32 bit) in latest series where each bit of this flag can be an allocation capability.
> I used two bits for discriminating between glibc malloc and rte_malloc. Shall we keep it or drop it?
>
> David, Dmitry please share your thoughts.
I'd drop it, but no strong opinion.
Since allocation type is automatic and all other properties can be set later,
there are no use cases for any options here.
And if they appear, flags may be insufficient as well.
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, October 14, 2021 3:11 PM
> To: Harman Kalra <hkalra@marvell.com>
> Cc: David Marchand <david.marchand@redhat.com>; dev@dpdk.org; Raslan
> Darawsheh <rasland@nvidia.com>; Ray Kinsella <mdr@ashroe.eu>; Dmitry
> Kozlyuk <dmitry.kozliuk@gmail.com>; viacheslavo@nvidia.com;
> matan@nvidia.com
> Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v1 2/7] eal/interrupts: implement
> get set APIs
>
> 14/10/2021 11:31, Harman Kalra:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 13/10/2021 20:52, Thomas Monjalon:
> > > > 13/10/2021 19:57, Harman Kalra:
> > > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Harman Kalra
> > > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > 04/10/2021 11:57, David Marchand:
> > > > > > > > On Mon, Oct 4, 2021 at 10:51 AM Harman Kalra
> > > > > > > > <hkalra@marvell.com>
> > > > > > > wrote:
> > > > > > > > > > > +struct rte_intr_handle
> > > > > > > > > > > +*rte_intr_handle_instance_alloc(int
> > > size,
> > > > > > > > > > > +
> > > > > > > > > > > +bool
> > > > > > > > > > > +from_hugepage) {
> > > > > > > > > > > + 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));
> > > > > > > > > >
> > > > > > > > > > We can call DPDK allocator in all cases.
> > > > > > > > > > That would avoid headaches on why multiprocess does
> > > > > > > > > > not work in some rarely tested cases.
> > > [...]
> > > > > > > I agree with David.
> > > > > > > I prefer a simpler API which always use rte_malloc, and make
> > > > > > > sure interrupts are always handled between rte_eal_init and
> > > rte_eal_cleanup.
> > > [...]
> > > > > > There are couple of more dependencies on glibc heap APIs:
> > > > > > 1. "rte_eal_alarm_init()" allocates an interrupt instance
> > > > > > which is used for timerfd, is called before
> > > > > > "rte_eal_memory_init()" which does the memseg init.
> > > > > > Not sure what all challenges we may face in moving alarm_init
> > > > > > after memory_init as it might break some subsystem inits.
> > > > > > Other option could be to allocate interrupt instance for
> > > > > > timerfd on first alarm_setup call.
> > > >
> > > > Indeed it is an issue.
> > > >
> > > > [...]
> > > >
> > > > > > There are many other drivers which statically declares the
> > > > > > interrupt handles inside their respective private structures
> > > > > > and memory for those structure was allocated from heap. For
> > > > > > such drivers I allocated interrupt instances also using glibc heap
> APIs.
> > > >
> > > > Could you use rte_malloc in these drivers?
> > >
> > > If we take the direction of 2 different allocations mode for the
> > > interrupts, I suggest we make it automatic without any API parameter.
> > > We don't have any function to check rte_malloc readiness I think.
> > > But we can detect whether shared memory is ready with this check:
> > > rte_eal_get_configuration()->mem_config->magic == RTE_MAGIC This
> > > check is true at the end of rte_eal_init, so it is false during probing.
> > > Would it be enough? Or should we implement rte_malloc_is_ready()?
> >
> > Hi Thomas,
> >
> > It's a very good suggestion. Let's implement "rte_malloc_is_ready()"
> > which could be as simple as " rte_eal_get_configuration()->mem_config-
> >magic == RTE_MAGIC" check.
> > There may be more consumers for this API in future.
>
> You cannot rely on the magic because it is set only after probing.
> For such API you need to have another internal flag to check that malloc is
> setup.
Yeah, got that. You mean in case of bus probing although rte_malloc is setup
but eal_mcfg_complete() is calledt done yet. So we should set another malloc
specific flag at the end of rte_eal_memory_init(). Correct?
But just for understanding, as David suggested that we preserve keep this flag
then why not use it, have rte_malloc and malloc bits and make a decision.
Let driver has the flexibility to choose. Do you see any harm in this?
Thanks
Harman
>
14/10/2021 12:31, Harman Kalra:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 14/10/2021 11:31, Harman Kalra:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 13/10/2021 20:52, Thomas Monjalon:
> > > > > 13/10/2021 19:57, Harman Kalra:
> > > > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Harman Kalra
> > > > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > > 04/10/2021 11:57, David Marchand:
> > > > > > > > > On Mon, Oct 4, 2021 at 10:51 AM Harman Kalra
> > > > > > > > > <hkalra@marvell.com>
> > > > > > > > wrote:
> > > > > > > > > > > > +struct rte_intr_handle
> > > > > > > > > > > > +*rte_intr_handle_instance_alloc(int
> > > > size,
> > > > > > > > > > > > +
> > > > > > > > > > > > +bool
> > > > > > > > > > > > +from_hugepage) {
> > > > > > > > > > > > + 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));
> > > > > > > > > > >
> > > > > > > > > > > We can call DPDK allocator in all cases.
> > > > > > > > > > > That would avoid headaches on why multiprocess does
> > > > > > > > > > > not work in some rarely tested cases.
> > > > [...]
> > > > > > > > I agree with David.
> > > > > > > > I prefer a simpler API which always use rte_malloc, and make
> > > > > > > > sure interrupts are always handled between rte_eal_init and
> > > > rte_eal_cleanup.
> > > > [...]
> > > > > > > There are couple of more dependencies on glibc heap APIs:
> > > > > > > 1. "rte_eal_alarm_init()" allocates an interrupt instance
> > > > > > > which is used for timerfd, is called before
> > > > > > > "rte_eal_memory_init()" which does the memseg init.
> > > > > > > Not sure what all challenges we may face in moving alarm_init
> > > > > > > after memory_init as it might break some subsystem inits.
> > > > > > > Other option could be to allocate interrupt instance for
> > > > > > > timerfd on first alarm_setup call.
> > > > >
> > > > > Indeed it is an issue.
> > > > >
> > > > > [...]
> > > > >
> > > > > > > There are many other drivers which statically declares the
> > > > > > > interrupt handles inside their respective private structures
> > > > > > > and memory for those structure was allocated from heap. For
> > > > > > > such drivers I allocated interrupt instances also using glibc heap
> > APIs.
> > > > >
> > > > > Could you use rte_malloc in these drivers?
> > > >
> > > > If we take the direction of 2 different allocations mode for the
> > > > interrupts, I suggest we make it automatic without any API parameter.
> > > > We don't have any function to check rte_malloc readiness I think.
> > > > But we can detect whether shared memory is ready with this check:
> > > > rte_eal_get_configuration()->mem_config->magic == RTE_MAGIC This
> > > > check is true at the end of rte_eal_init, so it is false during probing.
> > > > Would it be enough? Or should we implement rte_malloc_is_ready()?
> > >
> > > Hi Thomas,
> > >
> > > It's a very good suggestion. Let's implement "rte_malloc_is_ready()"
> > > which could be as simple as " rte_eal_get_configuration()->mem_config-
> > >magic == RTE_MAGIC" check.
> > > There may be more consumers for this API in future.
> >
> > You cannot rely on the magic because it is set only after probing.
> > For such API you need to have another internal flag to check that malloc is
> > setup.
>
> Yeah, got that. You mean in case of bus probing although rte_malloc is setup
> but eal_mcfg_complete() is calledt done yet. So we should set another malloc
> specific flag at the end of rte_eal_memory_init(). Correct?
I think the new internal flag should be at the end of rte_eal_malloc_heap_init().
Then a rte_internal function rte_malloc_is_ready() should check this flag.
> But just for understanding, as David suggested that we preserve keep this flag
> then why not use it, have rte_malloc and malloc bits and make a decision.
> Let driver has the flexibility to choose. Do you see any harm in this?
Which flag?
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, October 14, 2021 4:06 PM
> To: Harman Kalra <hkalra@marvell.com>
> Cc: David Marchand <david.marchand@redhat.com>; dev@dpdk.org; Raslan
> Darawsheh <rasland@nvidia.com>; Ray Kinsella <mdr@ashroe.eu>; Dmitry
> Kozlyuk <dmitry.kozliuk@gmail.com>; viacheslavo@nvidia.com;
> matan@nvidia.com
> Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v1 2/7] eal/interrupts: implement
> get set APIs
>
> 14/10/2021 12:31, Harman Kalra:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 14/10/2021 11:31, Harman Kalra:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 13/10/2021 20:52, Thomas Monjalon:
> > > > > > 13/10/2021 19:57, Harman Kalra:
> > > > > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Harman Kalra
> > > > > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > > > 04/10/2021 11:57, David Marchand:
> > > > > > > > > > On Mon, Oct 4, 2021 at 10:51 AM Harman Kalra
> > > > > > > > > > <hkalra@marvell.com>
> > > > > > > > > wrote:
> > > > > > > > > > > > > +struct rte_intr_handle
> > > > > > > > > > > > > +*rte_intr_handle_instance_alloc(int
> > > > > size,
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +bool
> > > > > > > > > > > > > +from_hugepage) {
> > > > > > > > > > > > > + 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));
> > > > > > > > > > > >
> > > > > > > > > > > > We can call DPDK allocator in all cases.
> > > > > > > > > > > > That would avoid headaches on why multiprocess
> > > > > > > > > > > > does not work in some rarely tested cases.
> > > > > [...]
> > > > > > > > > I agree with David.
> > > > > > > > > I prefer a simpler API which always use rte_malloc, and
> > > > > > > > > make sure interrupts are always handled between
> > > > > > > > > rte_eal_init and
> > > > > rte_eal_cleanup.
> > > > > [...]
> > > > > > > > There are couple of more dependencies on glibc heap APIs:
> > > > > > > > 1. "rte_eal_alarm_init()" allocates an interrupt instance
> > > > > > > > which is used for timerfd, is called before
> > > > > > > > "rte_eal_memory_init()" which does the memseg init.
> > > > > > > > Not sure what all challenges we may face in moving
> > > > > > > > alarm_init after memory_init as it might break some subsystem
> inits.
> > > > > > > > Other option could be to allocate interrupt instance for
> > > > > > > > timerfd on first alarm_setup call.
> > > > > >
> > > > > > Indeed it is an issue.
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > There are many other drivers which statically declares the
> > > > > > > > interrupt handles inside their respective private
> > > > > > > > structures and memory for those structure was allocated
> > > > > > > > from heap. For such drivers I allocated interrupt
> > > > > > > > instances also using glibc heap
> > > APIs.
> > > > > >
> > > > > > Could you use rte_malloc in these drivers?
> > > > >
> > > > > If we take the direction of 2 different allocations mode for the
> > > > > interrupts, I suggest we make it automatic without any API parameter.
> > > > > We don't have any function to check rte_malloc readiness I think.
> > > > > But we can detect whether shared memory is ready with this check:
> > > > > rte_eal_get_configuration()->mem_config->magic == RTE_MAGIC This
> > > > > check is true at the end of rte_eal_init, so it is false during probing.
> > > > > Would it be enough? Or should we implement rte_malloc_is_ready()?
> > > >
> > > > Hi Thomas,
> > > >
> > > > It's a very good suggestion. Let's implement "rte_malloc_is_ready()"
> > > > which could be as simple as "
> > > >rte_eal_get_configuration()->mem_config-
> > > >magic == RTE_MAGIC" check.
> > > > There may be more consumers for this API in future.
> > >
> > > You cannot rely on the magic because it is set only after probing.
> > > For such API you need to have another internal flag to check that
> > > malloc is setup.
> >
> > Yeah, got that. You mean in case of bus probing although rte_malloc is
> > setup but eal_mcfg_complete() is calledt done yet. So we should set
> > another malloc specific flag at the end of rte_eal_memory_init(). Correct?
>
> I think the new internal flag should be at the end of
> rte_eal_malloc_heap_init().
> Then a rte_internal function rte_malloc_is_ready() should check this flag.
Sure.
>
> > But just for understanding, as David suggested that we preserve keep
> > this flag then why not use it, have rte_malloc and malloc bits and make a
> decision.
> > Let driver has the flexibility to choose. Do you see any harm in this?
>
> Which flag?
In V2, I have replaced the bool arg with an 32bit flag in alloc api:
struct rte_intr_handle *rte_intr_instance_alloc(uint32_t flags);
Declared some flags which can be passed by the consumer
/** Interrupt instance allocation flags
* @see rte_intr_instance_alloc
*/
/** Allocate interrupt instance from traditional heap */
#define RTE_INTR_ALLOC_TRAD_HEAP 0x00000000
/** Allocate interrupt instance using DPDK memory management APIs */
#define RTE_INTR_ALLOC_DPDK_ALLOCATOR 0x00000001
As a future enhancement, if more options to the allocation is required by user,
new flags can be added.
Thanks
Harman
>
>
14/10/2021 12:44, Harman Kalra:
> > > But just for understanding, as David suggested that we preserve keep
> > > this flag then why not use it, have rte_malloc and malloc bits and make a
> > decision.
> > > Let driver has the flexibility to choose. Do you see any harm in this?
> >
> > Which flag?
>
> In V2, I have replaced the bool arg with an 32bit flag in alloc api:
> struct rte_intr_handle *rte_intr_instance_alloc(uint32_t flags);
>
> Declared some flags which can be passed by the consumer
> /** Interrupt instance allocation flags
> * @see rte_intr_instance_alloc
> */
> /** Allocate interrupt instance from traditional heap */
> #define RTE_INTR_ALLOC_TRAD_HEAP 0x00000000
> /** Allocate interrupt instance using DPDK memory management APIs */
> #define RTE_INTR_ALLOC_DPDK_ALLOCATOR 0x00000001
>
> As a future enhancement, if more options to the allocation is required by user,
> new flags can be added.
I am not sure we need such flag.
new file mode 100644
@@ -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)
+{
+ 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 (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOMEM;
+ 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)
+{
+ 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;
+ }
+
+ 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;
+}
+
+void rte_intr_handle_instance_free(struct rte_intr_handle *intr_handle)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ }
+
+ if (intr_handle->alloc_from_hugepage)
+ rte_free(intr_handle);
+ else
+ free(intr_handle);
+}
+
+int rte_intr_handle_fd_set(struct rte_intr_handle *intr_handle, int fd)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ goto fail;
+ }
+
+ intr_handle->fd = fd;
+
+ return 0;
+fail:
+ return rte_errno;
+}
+
+int rte_intr_handle_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->fd;
+fail:
+ return rte_errno;
+}
+
+int rte_intr_handle_type_set(struct rte_intr_handle *intr_handle,
+ enum rte_intr_handle_type type)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ goto fail;
+ }
+
+ intr_handle->type = type;
+
+ return 0;
+fail:
+ return rte_errno;
+}
+
+enum rte_intr_handle_type rte_intr_handle_type_get(
+ const struct rte_intr_handle *intr_handle)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ return RTE_INTR_HANDLE_UNKNOWN;
+ }
+
+ return intr_handle->type;
+}
+
+int rte_intr_handle_dev_fd_set(struct rte_intr_handle *intr_handle, int fd)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ goto fail;
+ }
+
+ intr_handle->vfio_dev_fd = fd;
+
+ return 0;
+fail:
+ return rte_errno;
+}
+
+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;
+}
+
+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",
+ 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;
+}
+
+int rte_intr_handle_nb_efd_set(struct rte_intr_handle *intr_handle,
+ int nb_efd)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ goto fail;
+ }
+
+ intr_handle->nb_efd = nb_efd;
+
+ return 0;
+fail:
+ return rte_errno;
+}
+
+int rte_intr_handle_nb_efd_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->nb_efd;
+fail:
+ return rte_errno;
+}
+
+int rte_intr_handle_nb_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->nb_intr;
+fail:
+ return rte_errno;
+}
+
+int rte_intr_handle_efd_counter_size_set(struct rte_intr_handle *intr_handle,
+ uint8_t efd_counter_size)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ goto fail;
+ }
+
+ intr_handle->efd_counter_size = efd_counter_size;
+
+ return 0;
+fail:
+ return rte_errno;
+}
+
+int rte_intr_handle_efd_counter_size_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->efd_counter_size;
+fail:
+ return rte_errno;
+}
+
+int *rte_intr_handle_efds_base(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->efds;
+fail:
+ return NULL;
+}
+
+int rte_intr_handle_efds_index_get(const struct rte_intr_handle *intr_handle,
+ int index)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ goto fail;
+ }
+
+ if (index >= intr_handle->nb_intr) {
+ RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index,
+ intr_handle->nb_intr);
+ rte_errno = EINVAL;
+ goto fail;
+ }
+
+ return intr_handle->efds[index];
+fail:
+ return rte_errno;
+}
+
+int rte_intr_handle_efds_index_set(struct rte_intr_handle *intr_handle,
+ int index, int fd)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ goto fail;
+ }
+
+ if (index >= intr_handle->nb_intr) {
+ RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index,
+ intr_handle->nb_intr);
+ rte_errno = ERANGE;
+ goto fail;
+ }
+
+ intr_handle->efds[index] = fd;
+
+ return 0;
+fail:
+ return rte_errno;
+}
+
+struct rte_epoll_event *rte_intr_handle_elist_index_get(
+ struct rte_intr_handle *intr_handle, int index)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ goto fail;
+ }
+
+ if (index >= intr_handle->nb_intr) {
+ RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index,
+ intr_handle->nb_intr);
+ rte_errno = ERANGE;
+ goto fail;
+ }
+
+ return &intr_handle->elist[index];
+fail:
+ return NULL;
+}
+
+int rte_intr_handle_elist_index_set(struct rte_intr_handle *intr_handle,
+ int index, struct rte_epoll_event elist)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ goto fail;
+ }
+
+ if (index >= intr_handle->nb_intr) {
+ RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index,
+ intr_handle->nb_intr);
+ rte_errno = ERANGE;
+ goto fail;
+ }
+
+ intr_handle->elist[index] = elist;
+
+ return 0;
+fail:
+ return rte_errno;
+}
+
+int *rte_intr_handle_vec_list_base(const struct rte_intr_handle *intr_handle)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ return NULL;
+ }
+
+ return intr_handle->intr_vec;
+}
+
+int rte_intr_handle_vec_list_alloc(struct rte_intr_handle *intr_handle,
+ const char *name, int size)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ goto fail;
+ }
+
+ /* Vector list already allocated */
+ if (intr_handle->intr_vec)
+ return 0;
+
+ 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) {
+ 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;
+}
+
+int rte_intr_handle_vec_list_index_get(
+ const struct rte_intr_handle *intr_handle, int index)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ goto fail;
+ }
+
+ if (!intr_handle->intr_vec) {
+ RTE_LOG(ERR, EAL, "Intr vector list not allocated\n");
+ rte_errno = ENOTSUP;
+ goto fail;
+ }
+
+ if (index > intr_handle->vec_list_size) {
+ RTE_LOG(ERR, EAL, "Index %d greater than vec list size %d\n",
+ index, intr_handle->vec_list_size);
+ rte_errno = ERANGE;
+ goto fail;
+ }
+
+ return intr_handle->intr_vec[index];
+fail:
+ return rte_errno;
+}
+
+int rte_intr_handle_vec_list_index_set(struct rte_intr_handle *intr_handle,
+ int index, int vec)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ goto fail;
+ }
+
+ if (!intr_handle->intr_vec) {
+ RTE_LOG(ERR, EAL, "Intr vector list not allocated\n");
+ rte_errno = ENOTSUP;
+ goto fail;
+ }
+
+ if (index > intr_handle->vec_list_size) {
+ RTE_LOG(ERR, EAL, "Index %d greater than vec list size %d\n",
+ index, intr_handle->vec_list_size);
+ rte_errno = ERANGE;
+ goto fail;
+ }
+
+ intr_handle->intr_vec[index] = vec;
+
+ return 0;
+fail:
+ return rte_errno;
+}
+
+void rte_intr_handle_vec_list_free(struct rte_intr_handle *intr_handle)
+{
+ if (intr_handle == NULL) {
+ RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
+ rte_errno = ENOTSUP;
+ }
+
+ rte_free(intr_handle->intr_vec);
+ intr_handle->intr_vec = NULL;
+}
@@ -17,6 +17,7 @@ if is_windows
'eal_common_errno.c',
'eal_common_fbarray.c',
'eal_common_hexdump.c',
+ 'eal_common_interrupts.c',
'eal_common_launch.c',
'eal_common_lcore.c',
'eal_common_log.c',
@@ -53,6 +54,7 @@ sources += files(
'eal_common_fbarray.c',
'eal_common_hexdump.c',
'eal_common_hypervisor.c',
+ 'eal_common_interrupts.c',
'eal_common_launch.c',
'eal_common_lcore.c',
'eal_common_log.c',
@@ -55,13 +55,17 @@ struct rte_intr_handle {
};
void *handle; /**< device driver handle (Windows) */
};
+ bool alloc_from_hugepage;
enum rte_intr_handle_type type; /**< handle type */
uint32_t max_intr; /**< max interrupt requested */
uint32_t nb_efd; /**< number of available efd(event fd) */
uint8_t efd_counter_size; /**< size of efd counter, used for vdev */
+ uint16_t nb_intr;
+ /**< Max vector count, default RTE_MAX_RXTX_INTR_VEC_ID */
int efds[RTE_MAX_RXTX_INTR_VEC_ID]; /**< intr vectors/efds mapping */
struct rte_epoll_event elist[RTE_MAX_RXTX_INTR_VEC_ID];
- /**< intr vector epoll event */
+ /**< intr vector epoll event */
+ uint16_t vec_list_size;
int *intr_vec; /**< intr vector number array */
};
@@ -426,6 +426,36 @@ EXPERIMENTAL {
# added in 21.08
rte_power_monitor_multi; # WINDOWS_NO_EXPORT
+
+ # added in 21.11
+ rte_intr_handle_fd_set;
+ rte_intr_handle_fd_get;
+ rte_intr_handle_dev_fd_set;
+ rte_intr_handle_dev_fd_get;
+ rte_intr_handle_type_set;
+ rte_intr_handle_type_get;
+ rte_intr_handle_instance_alloc;
+ rte_intr_handle_instance_index_get;
+ rte_intr_handle_instance_free;
+ rte_intr_handle_instance_index_set;
+ rte_intr_handle_event_list_update;
+ rte_intr_handle_max_intr_set;
+ rte_intr_handle_max_intr_get;
+ rte_intr_handle_nb_efd_set;
+ rte_intr_handle_nb_efd_get;
+ rte_intr_handle_nb_intr_get;
+ rte_intr_handle_efds_index_set;
+ rte_intr_handle_efds_index_get;
+ rte_intr_handle_efds_base;
+ rte_intr_handle_elist_index_set;
+ rte_intr_handle_elist_index_get;
+ rte_intr_handle_efd_counter_size_set;
+ rte_intr_handle_efd_counter_size_get;
+ rte_intr_handle_vec_list_alloc;
+ rte_intr_handle_vec_list_index_set;
+ rte_intr_handle_vec_list_index_get;
+ rte_intr_handle_vec_list_free;
+ rte_intr_handle_vec_list_base;
};
INTERNAL {