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

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

Hello Harman,

This patch looks good to me, there are just some tiny comments inline.

2021-10-20 00:05 (UTC+0530), Harman Kalra:
> [...]
> +/* Macros to check for valid port */
> +#define CHECK_VALID_INTR_HANDLE(intr_handle) do { \
> +	if (intr_handle == NULL) { \
> +		RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); \
> +		rte_errno = EINVAL; \
> +		goto fail; \
> +	} \
> +} while (0)

In most cases "goto fail" could be "return -rte_errno".
How about this (feel free to ignore)?

#define CHECK_VALID_INTR_HANDLE_RET(intr_handle) do { \
		CHECK_VALID_INTR_HANDLE(intr_handle); \
	fail: \
		return -rte_errno; \
	} while (0)

> [...]
> +struct rte_intr_handle *rte_intr_instance_alloc(void)
> +{
> +	struct rte_intr_handle *intr_handle;
> +	bool is_rte_memory;
> +
> +	/* Detect if DPDK malloc APIs are ready to be used. */
> +	is_rte_memory = rte_malloc_is_ready();
> +	if (is_rte_memory)
> +		intr_handle = rte_zmalloc(NULL, sizeof(struct
> rte_intr_handle),
> +					  0);
> +	else
> +		intr_handle = calloc(1, sizeof(struct rte_intr_handle));

Nit: sizeof(*intr_handle).

> +	if (!intr_handle) {

intr_handle == NULL

> +		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->is_rte_memory = is_rte_memory;
> +
> +	return intr_handle;
> +}
> +

[...]
> +
> +void rte_intr_instance_free(struct rte_intr_handle *intr_handle)
> +{
> +	if (intr_handle) {

intr_handle != NULL

> +		if (intr_handle->is_rte_memory)
> +			rte_free(intr_handle);
> +		else
> +			free(intr_handle);
> +	}
> +}
[...]

> +
> +int rte_intr_elist_index_set(struct rte_intr_handle *intr_handle,
> +			     int index, struct rte_epoll_event elist)
> +{
> +	CHECK_VALID_INTR_HANDLE(intr_handle);
> +
> +	if (index >= intr_handle->nb_intr) {
> +		RTE_LOG(ERR, EAL, "Invalid size %d, max limit %d\n", index,
> +			intr_handle->nb_intr);

Which "size"?

> +		rte_errno = ERANGE;
> +		goto fail;
> +	}
> +
> +	intr_handle->elist[index] = elist;
> +
> +	return 0;
> +fail:
> +	return -rte_errno;
> +}
> +

[...]
> +int rte_intr_vec_list_index_get(const struct rte_intr_handle *intr_handle,
> +				int index)
> +{
> +	CHECK_VALID_INTR_HANDLE(intr_handle);
> +
> +	if (!intr_handle->intr_vec) {
> +		RTE_LOG(ERR, EAL, "Intr vector list not allocated\n");
> +		rte_errno = ENOTSUP;
> +		goto fail;
> +	}

Can be RTE_ASSERT(), because vec_list_size will be 0 in this case.

> +
> +	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_vec_list_index_set(struct rte_intr_handle *intr_handle,
> +				   int index, int vec)
> +{
> +	CHECK_VALID_INTR_HANDLE(intr_handle);
> +
> +	if (!intr_handle->intr_vec) {
> +		RTE_LOG(ERR, EAL, "Intr vector list not allocated\n");
> +		rte_errno = ENOTSUP;
> +		goto fail;
> +	}

Same here.

> +
> +	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_vec_list_free(struct rte_intr_handle *intr_handle)
> +{
> +	if (intr_handle) {
> +		rte_free(intr_handle->intr_vec);
> +		intr_handle->intr_vec = NULL;

intr_handle->vec_list_size = 0;

> +	}
> +}
> +

[...]
> diff --git 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 auto detects if memory
> + * for the instance should be allocated using DPDK memory management library
> + * APIs or normal heap allocation, based on if DPDK memory subsystem is
> + * initialized and ready to be used.

This is too much implementation detail and not very specific from user PoV.
Suggestion:

	After rte_eal_init() has finished, it allocates from DDPK heap,
	otherwise it allocates from normal heap. In particular,
	it allocates from the normal heap during initial bus scanning.

See also my reply to v3 regarding allocation.

> + *
> + * Default memory allocation for event fds and epoll event array is done which
> + * can be realloced later as per the requirement.

BTW, why do this?

> + *
> + * This function should be called from application or driver, before calling any
> + * of the interrupt APIs.
> + *
> + * @return
> + *  - On success, address of first interrupt handle.

Not "first".

> + *  - 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 interrupt handle resources.
> + *
> + * @param intr_handle
> + *  Base address of interrupt handle array.

Not "array".

> + *
> + */
> +__rte_experimental
> +void
> +rte_intr_instance_free(struct rte_intr_handle *intr_handle);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * This API is used to set the fd field of interrupt handle with user provided
> + * file descriptor.
> + *
> + * @param intr_handle
> + *  pointer to the interrupt handle.
> + * @param fd
> + *  file descriptor value provided by user.
> + *
> + * @return
> + *  - On success, zero.
> + *  - On failure, a negative value.

+ "and rte_errno is set" here and in other places.

[...]
> +/**
> + * @internal
> + * This API is used to populate interrupt handle, with src handler fields.

Comma is not needed.

> + *
> + * @param intr_handle
> + *  Start address of interrupt handles

It's a single handle.

> + * @param src
> + *  Source interrupt handle to be cloned.
> + *
> + * @return
> + *   - On success, zero.
> + *   - On failure, a negative value.
> + */
> +__rte_internal
> +int
> +rte_intr_instance_copy(struct rte_intr_handle *intr_handle,
> +		       const struct rte_intr_handle *src);
> +

[...]
> +/**
> + * @internal
> + * This API is used to set the event fd counter size field of interrupt handle
> + * with user provided efd counter size.
> + *
> + * @param intr_handle
> + *  pointer to the interrupt handle.
> + * @param efd_counter_size
> + *  size of efd counter, used for vdev

No need to mention vdev.

> + *
> + * @return
> + *  - On success, zero.
> + *  - On failure, a negative value.
> + */
> +__rte_internal
> +int
> +rte_intr_efd_counter_size_set(struct rte_intr_handle *intr_handle,
> +			      uint8_t efd_counter_size);
> +

[...]
> +/**
> + * @internal
> + * Freeing the memory allocated for interrupt vector list array.

"Freeing" -> "Frees"

> + *
> + * @param intr_handle
> + *  pointer to the interrupt handle.
> + *
> + * @return
> + *  - On success, zero
> + *  - On failure, a negative value.
> + */
> +__rte_internal
> +void
> +rte_intr_vec_list_free(struct rte_intr_handle *intr_handle);
> +
> +/**
> + * @internal
> + * Reallocates the size efds and elist array based on size provided by user.
> + * By default efds and elist array are allocated with default size
> + * RTE_MAX_RXTX_INTR_VEC_ID on interrupt handle array creation. Later on device
> + * probe, device may have capability of more interrupts than
> + * RTE_MAX_RXTX_INTR_VEC_ID. Hence using this API, PMDs can reallocate the

"Hence" word is unexpected, I think it should be removed.

> + * arrays as per the max interrupts capability of device.
> + *
> + * @param intr_handle
> + *  pointer to the interrupt handle.
> + * @param size
> + *  efds and elist array size.
> + *
> + * @return
> + *  - On success, zero
> + *  - On failure, a negative value.
> + */
> +__rte_internal
> +int
> +rte_intr_event_list_update(struct rte_intr_handle *intr_handle, int size);
> +
[...]