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

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

2021-10-19 01:07 (UTC+0530), Harman Kalra:
[...]
> +struct rte_intr_handle *rte_intr_instance_alloc(void)
> +{
> +	struct rte_intr_handle *intr_handle;
> +	bool mem_allocator;

This name is not very descriptive; what would "mem_allocator is false" mean?
How about "is_rte_memory"?

> +
> +	/* Detect if DPDK malloc APIs are ready to be used. */
> +	mem_allocator = rte_malloc_is_ready();
> +	if (mem_allocator)
> +		intr_handle = rte_zmalloc(NULL, sizeof(struct rte_intr_handle),
> +					  0);
> +	else
> +		intr_handle = calloc(1, sizeof(struct rte_intr_handle));
> +	if (!intr_handle) {
> +		RTE_LOG(ERR, EAL, "Fail to allocate intr_handle\n");
> +		rte_errno = ENOMEM;
> +		return NULL;
> +	}
> +
> +	intr_handle->nb_intr = RTE_MAX_RXTX_INTR_VEC_ID;
> +	intr_handle->mem_allocator = mem_allocator;
> +
> +	return intr_handle;
> +}
> +
> +int rte_intr_instance_copy(struct rte_intr_handle *intr_handle,
> +			   const struct rte_intr_handle *src)
> +{
> +	uint16_t nb_intr;
> +
> +	CHECK_VALID_INTR_HANDLE(intr_handle);
> +
> +	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;
> +
> +	nb_intr = RTE_MIN(src->nb_intr, intr_handle->nb_intr);

Truncating copy is error-prone.
It should be either a reallocation (in the future) or an error (now).

> +	memcpy(intr_handle->efds, src->efds, nb_intr);
> +	memcpy(intr_handle->elist, src->elist, nb_intr);
> +
> +	return 0;
> +fail:
> +	return -rte_errno;
> +}
> +
> +void rte_intr_instance_free(struct rte_intr_handle *intr_handle)
> +{
> +	if (intr_handle->mem_allocator)

This function should accept NULL and be a no-op in such case.

> +		rte_free(intr_handle);
> +	else
> +		free(intr_handle);
> +}

[...]
> +void *rte_intr_instance_windows_handle_get(struct rte_intr_handle *intr_handle)
> +{
> +	CHECK_VALID_INTR_HANDLE(intr_handle);
> +
> +	return intr_handle->windows_handle;
> +fail:
> +	return NULL;
> +}
> +
> +int rte_intr_instance_windows_handle_set(struct rte_intr_handle *intr_handle,
> +					 void *windows_handle)
> +{
> +	CHECK_VALID_INTR_HANDLE(intr_handle);
> +
> +	if (!windows_handle) {
> +		RTE_LOG(ERR, EAL, "Windows handle should not be NULL\n");
> +		rte_errno = EINVAL;
> +		goto fail;
> +	}

Thanks for adding this API, but please remove the check.
It is possible that the API user will pass NULL to reset the state
(also NULL is not the only invalid value for a Windows handle).
There is no check for Unix FD, neither should be here.

> +
> +	intr_handle->windows_handle = windows_handle;
> +
> +	return 0;
> +fail:
> +	return -rte_errno;
> +}

[...]
> @@ -79,191 +53,20 @@ struct rte_intr_handle {
>  			};
>  			int fd;	/**< interrupt event file descriptor */
>  		};
> -		void *handle; /**< device driver handle (Windows) */
> +		void *windows_handle; /**< device driver handle (Windows) */

I guess Windows can be dropped from the comment since it's now in the name.

>  	};
> +	bool mem_allocator;
>  	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/include/rte_interrupts.h b/lib/eal/include/rte_interrupts.h
> index cc3bf45d8c..98edf774af 100644
> --- a/lib/eal/include/rte_interrupts.h
> +++ b/lib/eal/include/rte_interrupts.h
[...]
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * It allocates memory for interrupt instance. API takes flag as an argument

Not anymore. Please update the description.

> + * which define from where memory should be allocated i.e. using DPDK memory
> + * management library APIs or normal heap allocation.
> + * Default memory allocation for event fds and event list array is done which
> + * can be realloced later as per the requirement.
> + *
> + * This function should be called from application or driver, before calling any
> + * of the interrupt APIs.
> + *
> + * @param flags
> + *  Memory allocation from DPDK allocator or normal allocation
> + *
> + * @return
> + *  - On success, address of first interrupt handle.
> + *  - On failure, NULL.
> + */
> +__rte_experimental
> +struct rte_intr_handle *
> +rte_intr_instance_alloc(void);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * This API is used to free the memory allocated for event fds. event lists
> + * and interrupt handle array.

It's simpler and more future-proof to just say "interrupt handle resources"
instead of enumerating them.

> + *
> + * @param intr_handle
> + *  Base address of interrupt handle array.

It's not an array anymore.

[...]
> +/**
> + * @internal
> + * This API is used to set the event list array index with the given elist

"Event list array" sound like an array of lists,
while it is really an array of scalar elements.
"Event data array"? TBH, I don't know how it's usually named in Unices.

> + * instance.
> + *
> + * @param intr_handle
> + *  pointer to the interrupt handle.
> + * @param index
> + *  elist array index to be set
> + * @param elist
> + *  event list instance of struct rte_epoll_event
> + *
> + * @return
> + *  - On success, zero.
> + *  - On failure, a negative value.
> + */
> +__rte_internal
> +int
> +rte_intr_elist_index_set(struct rte_intr_handle *intr_handle, int index,
> +			 struct rte_epoll_event elist);
> +
> +/**
> + * @internal
> + * Returns the address of elist instance of event list array at a given index.
> + *
> + * @param intr_handle
> + *  pointer to the interrupt handle.
> + * @param index
> + *  elist array index to be returned
> + *
> + * @return
> + *  - On success, elist
> + *  - On failure, a negative value.
> + */
> +__rte_internal
> +struct rte_epoll_event *
> +rte_intr_elist_index_get(struct rte_intr_handle *intr_handle, int index);
> +
> +/**
> + * @internal
> + * Allocates the memory of interrupt vector list array, with size defining the
> + * no of elements required in the array.

Typo: "no" -> "number".

[...]
> +
> +/**
> + * @internal
> + * This API returns the windows handle of the given interrupt instance.

Typo: "windows" -> "Windows" here and below.

> + *
> + * @param intr_handle
> + *  pointer to the interrupt handle.
> + *
> + * @return
> + *  - On success, windows handle.
> + *  - On failure, NULL.
> + */
> +__rte_internal
> +void *
> +rte_intr_instance_windows_handle_get(struct rte_intr_handle *intr_handle);
> +
> +/**
> + * @internal
> + * This API set the windows handle for the given interrupt instance.
> + *
> + * @param intr_handle
> + *  pointer to the interrupt handle.
> + * @param windows_handle
> + *  windows handle to be set.
> + *
> + * @return
> + *  - On success, zero
> + *  - On failure, a negative value.
> + */
> +__rte_internal
> +int
> +rte_intr_instance_windows_handle_set(struct rte_intr_handle *intr_handle,
> +				     void *windows_handle);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/eal/version.map b/lib/eal/version.map
> index 38f7de83e1..0ef77c3b40 100644
> --- a/lib/eal/version.map
> +++ b/lib/eal/version.map
> @@ -109,18 +109,10 @@ DPDK_22 {
>  	rte_hexdump;
>  	rte_hypervisor_get;
>  	rte_hypervisor_get_name; # WINDOWS_NO_EXPORT
> -	rte_intr_allow_others;
>  	rte_intr_callback_register;
>  	rte_intr_callback_unregister;
> -	rte_intr_cap_multiple;
> -	rte_intr_disable;
> -	rte_intr_dp_is_en;
> -	rte_intr_efd_disable;
> -	rte_intr_efd_enable;
>  	rte_intr_enable;
> -	rte_intr_free_epoll_fd;
> -	rte_intr_rx_ctl;
> -	rte_intr_tls_epfd;
> +	rte_intr_disable;
>  	rte_keepalive_create; # WINDOWS_NO_EXPORT
>  	rte_keepalive_dispatch_pings; # WINDOWS_NO_EXPORT
>  	rte_keepalive_mark_alive; # WINDOWS_NO_EXPORT
> @@ -420,6 +412,14 @@ EXPERIMENTAL {
>  
>  	# added in 21.08
>  	rte_power_monitor_multi; # WINDOWS_NO_EXPORT
> +
> +	# added in 21.11
> +	rte_intr_fd_set; # WINDOWS_NO_EXPORT
> +	rte_intr_fd_get; # WINDOWS_NO_EXPORT

OK, these are not feasible on Windows.

> +	rte_intr_type_set; # WINDOWS_NO_EXPORT
> +	rte_intr_type_get; # WINDOWS_NO_EXPORT
> +	rte_intr_instance_alloc; # WINDOWS_NO_EXPORT
> +	rte_intr_instance_free; # WINDOWS_NO_EXPORT

No, these *are* needed on Windows.

>  };
>  
>  INTERNAL {
> @@ -430,4 +430,33 @@ INTERNAL {
>  	rte_mem_map;
>  	rte_mem_page_size;
>  	rte_mem_unmap;
> +	rte_intr_cap_multiple;
> +	rte_intr_dp_is_en;
> +	rte_intr_efd_disable;
> +	rte_intr_efd_enable;
> +	rte_intr_free_epoll_fd;
> +	rte_intr_rx_ctl;
> +	rte_intr_allow_others;
> +	rte_intr_tls_epfd;
> +	rte_intr_dev_fd_set; # WINDOWS_NO_EXPORT
> +	rte_intr_dev_fd_get; # WINDOWS_NO_EXPORT

OK.

> +	rte_intr_instance_copy; # WINDOWS_NO_EXPORT
> +	rte_intr_event_list_update; # WINDOWS_NO_EXPORT
> +	rte_intr_max_intr_set; # WINDOWS_NO_EXPORT
> +	rte_intr_max_intr_get; # WINDOWS_NO_EXPORT

These are needed on Windows.

> +	rte_intr_nb_efd_set; # WINDOWS_NO_EXPORT
> +	rte_intr_nb_efd_get; # WINDOWS_NO_EXPORT
> +	rte_intr_nb_intr_get; # WINDOWS_NO_EXPORT
> +	rte_intr_efds_index_set; # WINDOWS_NO_EXPORT
> +	rte_intr_efds_index_get; # WINDOWS_NO_EXPORT

OK.

> +	rte_intr_elist_index_set; # WINDOWS_NO_EXPORT
> +	rte_intr_elist_index_get; # WINDOWS_NO_EXPORT

These are needed on Windows.

> +	rte_intr_efd_counter_size_set; # WINDOWS_NO_EXPORT
> +	rte_intr_efd_counter_size_get; # WINDOWS_NO_EXPORT

OK.

> +	rte_intr_vec_list_alloc; # WINDOWS_NO_EXPORT
> +	rte_intr_vec_list_index_set; # WINDOWS_NO_EXPORT
> +	rte_intr_vec_list_index_get; # WINDOWS_NO_EXPORT
> +	rte_intr_vec_list_free; # WINDOWS_NO_EXPORT

These are needed on Windows.

> +	rte_intr_instance_windows_handle_get;
> +	rte_intr_instance_windows_handle_set;
>  };