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

[Harman Kalra <hkalra@marvell.com>]

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.