Hi Dmitry,
Thanks for reviewing the series.
Please find my comments inline.
> -----Original Message-----
> From: Dmitry Kozlyuk <dmitry.kozl...@gmail.com>
> Sent: Sunday, October 3, 2021 11:35 PM
> To: Harman Kalra <hka...@marvell.com>
> Cc: dev@dpdk.org; Ray Kinsella <m...@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.
