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

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

2021-10-05 17:44 (UTC+0530), Harman Kalra:
> [...]
> +int rte_intr_instance_copy(struct rte_intr_handle *intr_handle,
> +			   const struct rte_intr_handle *src)
> +{
> +	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;
> +	}
> +
> +	intr_handle->fd = src->fd;
> +	intr_handle->vfio_dev_fd = src->vfio_dev_fd;
> +	intr_handle->type = src->type;
> +	intr_handle->max_intr = src->max_intr;
> +	intr_handle->nb_efd = src->nb_efd;
> +	intr_handle->efd_counter_size = src->efd_counter_size;
> +
> +	memcpy(intr_handle->efds, src->efds, src->nb_intr);
> +	memcpy(intr_handle->elist, src->elist, src->nb_intr);

Buffer overrun if "intr_handle->nb_intr < src->nb_intr"?

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

ENOTSUP usually means the operation is valid from API standpoint
but not supported by the implementation. EINVAL/EFAULT suits better.

> +	}
> +
> +	return intr_handle->mem_allocator;
> +}

What do you think about having an API to retrieve the entire flags instead?

> +
> +void rte_intr_instance_free(struct rte_intr_handle *intr_handle)
> +{
> +	if (intr_handle == NULL) {
> +		RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> +		rte_errno = ENOTSUP;
> +	}

API are neater when free(NULL) is a no-op.

> +
> +	if (intr_handle->mem_allocator)
> +		rte_free(intr_handle);
> +	else
> +		free(intr_handle);
> +}
> +
> +int rte_intr_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;
> +	}

This piece repeats over and over, how about making it a function or a macro,
like in ethdev?

> +
> +	intr_handle->fd = fd;
> +
> +	return 0;
> +fail:
> +	return -rte_errno;
> +}
> +
> +int rte_intr_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 -1;
> +}

Please add a similar pair of experimental API for the "handle" member,
it is needed for Windows interrupt support I'm working on top of these series
(IIUC, API changes should be closed by RC1.)
If you will be doing this and don't like "handle" name, it might be like
"dev_handle" or "windows_device".

> [...]
> +int rte_intr_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 RTE_MAX_RXTX_INTR_VEC_ID=%d",

The macros is not used in the comparison, neither should the log mention it.

> [...]
> @@ -420,6 +412,14 @@ EXPERIMENTAL {
>  
>  	# added in 21.08
>  	rte_power_monitor_multi; # WINDOWS_NO_EXPORT
> +
> +	# added in 21.11
> +	rte_intr_fd_set;
> +	rte_intr_fd_get;

WINDOWS_NO_EXPORT

> +	rte_intr_type_set;
> +	rte_intr_type_get;
> +	rte_intr_instance_alloc;
> +	rte_intr_instance_free;
>  };

Do I understand correctly that these exports are needed
to allow an application to use DPDK callback facilities
for its own interrupt sources?
If so, I'd suggest that instead we export a simpler set of functions:
1. Create/free a handle instance with automatic fixed type selection.
2. Trigger an interrupt on the specified handle instance.
The flow would be that the application listens on whatever it wants,
probably with OS-specific mechanisms, and just notifies the interrupt thread
about events to trigger callbacks.
Because these APIs are experimental we don't need to change it now,
just my thoughts for the future.