Re: [dpdk-dev] [PATCH v6 05/23] eventtimer: add adapter allocation definitions

[Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>]

On Wed, Jan 10, 2018 at 06:20:56PM -0600, Erik Gabriel Carrillo wrote:
> Add definitions for the functions that allocate and deallocate adapter
> instances in the default software implementation.
>
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---
>  lib/Makefile                                  |   2 +-
>  lib/librte_eventdev/Makefile                  |   2 +-
>  lib/librte_eventdev/rte_event_timer_adapter.c | 138 +++++++++++++++++++++++++-
>  lib/librte_eventdev/rte_event_timer_adapter.h |   2 +-
>  4 files changed, 139 insertions(+), 5 deletions(-)
>
> diff --git a/lib/Makefile b/lib/Makefile
> index 4202702..4c53f8c 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -29,7 +29,7 @@ DEPDIRS-librte_security := librte_eal librte_mempool librte_ring librte_mbuf
>  DEPDIRS-librte_security += librte_ether
>  DEPDIRS-librte_security += librte_cryptodev
>  DIRS-$(CONFIG_RTE_LIBRTE_EVENTDEV) += librte_eventdev
> -DEPDIRS-librte_eventdev := librte_eal librte_ring librte_ether librte_hash
> +DEPDIRS-librte_eventdev := librte_eal librte_ring librte_ether librte_hash librte_mempool
>  DIRS-$(CONFIG_RTE_LIBRTE_VHOST) += librte_vhost
>  DEPDIRS-librte_vhost := librte_eal librte_mempool librte_mbuf librte_ether
>  DIRS-$(CONFIG_RTE_LIBRTE_HASH) += librte_hash
> diff --git a/lib/librte_eventdev/Makefile b/lib/librte_eventdev/Makefile
> index 8f11a79..e68f888 100644
> --- a/lib/librte_eventdev/Makefile
> +++ b/lib/librte_eventdev/Makefile
> @@ -13,7 +13,7 @@ LIBABIVER := 3
>  # build flags
>  CFLAGS += -O3
>  CFLAGS += $(WERROR_FLAGS)
> -LDLIBS += -lrte_eal -lrte_ring -lrte_ethdev -lrte_hash
> +LDLIBS += -lrte_eal -lrte_ring -lrte_ethdev -lrte_hash -lrte_mempool
>
>  # library source files
>  SRCS-y += rte_eventdev.c
> diff --git a/lib/librte_eventdev/rte_event_timer_adapter.c b/lib/librte_eventdev/rte_event_timer_adapter.c
> index 540be95..9c4ba1c 100644
> --- a/lib/librte_eventdev/rte_event_timer_adapter.c
> +++ b/lib/librte_eventdev/rte_event_timer_adapter.c
> @@ -31,11 +31,17 @@
>   */
>
>  #include <string.h>
> +#include <stdbool.h>
>
>  #include <rte_memzone.h>
>  #include <rte_memory.h>
>  #include <rte_dev.h>
>  #include <rte_errno.h>
> +#include <rte_malloc.h>
> +#include <rte_ring.h>
> +#include <rte_mempool.h>
> +#include <rte_timer.h>
> +#include <rte_service_component.h>
>
>  #include "rte_eventdev.h"
>  #include "rte_eventdev_pmd.h"
> @@ -163,6 +169,11 @@ rte_event_timer_adapter_create_ext(
>
>  	adapter_id = conf->timer_adapter_id;
>
> +	if (adapter_id >= RTE_EVENT_TIMER_ADAPTER_NUM_MAX) {
> +		rte_errno = -EINVAL;
> +		return NULL;
> +	}
> +
>  	/* Check adapter ID not already allocated */
>  	adapter = &adapters[adapter_id];
>  	if (adapter->allocated) {
> @@ -412,18 +423,141 @@ rte_event_timer_cancel_burst(const struct rte_event_timer_adapter *adapter,
>   * Software event timer adapter ops definitions
>   */
>
> +struct rte_event_timer_adapter_sw_data {
> +	/* Number of outstanding timers managed by event adapter. */
> +	int nb_armed_evtims;
> +	/* Identifier of service executing timer management logic. */
> +	uint32_t service_id;
> +	/* Ring containing messages to arm or cancel event timers */
> +	struct rte_ring *msg_ring;
> +	/* Mempool containing msg objects */
> +	struct rte_mempool *msg_pool;
> +	/* Mempool containing timer objects */
> +	struct rte_mempool *tim_pool;
> +};

Don't use rte_ prefix for internal data.

> +
> +enum msg_type {MSG_TYPE_ARM, MSG_TYPE_CANCEL};
> +
> +struct msg {
> +	enum msg_type type;
> +	struct rte_event_timer *evtim;
> +};
> +
> +static int
> +sw_event_timer_adapter_service_func(void *arg)
> +{
> +	RTE_SET_USED(arg);
> +	return 0;
> +}
> +
>  static int
>  sw_event_timer_adapter_init(struct rte_event_timer_adapter *adapter)
>  {
> -	RTE_SET_USED(adapter);
> +	int ret;
> +	struct rte_event_timer_adapter_sw_data *sw_data;
> +	uint64_t nb_timers;
<snip>
> +
> +	char pool_name[RTE_RING_NAMESIZE];
> +	snprintf(pool_name, RTE_RING_NAMESIZE, "sw_evtim_adap_msg_pool_%"PRIu8,
> +		 adapter->data->id);
> +	sw_data->msg_pool = rte_mempool_create(pool_name, nb_timers,
> +					       sizeof(struct msg), 32, 0, NULL,
> +					       NULL, NULL, NULL,
> +					       adapter->data->socket_id, 0);
> +	if (sw_data->msg_pool == NULL) {
> +		rte_errno = ENOMEM;
> +		return -1;
> +	}
> +
> +	snprintf(pool_name, RTE_RING_NAMESIZE, "sw_evtim_adap_tim_pool_%"PRIu8,
> +		 adapter->data->id);
> +	sw_data->tim_pool = rte_mempool_create(pool_name, nb_timers,
> +					       sizeof(struct rte_timer), 32, 0,
> +					       NULL, NULL, NULL, NULL,
> +					       adapter->data->socket_id, 0);
> +	if (sw_data->tim_pool == NULL) {
> +		printf("Could not allocate tim mempool\n");
> +		return -1;
> +	}

Any specific reason for having seperate mempool for msg and timers?
You could have a internal structure as a wrapper for both.

> +
> +	/* Register a service component */
> +	memset(&service, 0, sizeof(service));
> +	snprintf(service.name, RTE_SERVICE_NAME_MAX,
> +		 "sw_evimer_adap_svc_%"PRIu8, adapter->data->id);
> +	service.socket_id = adapter->data->socket_id;
> +	service.callback = sw_event_timer_adapter_service_func;
> +	service.callback_userdata = adapter;
> +	ret = rte_service_component_register(&service, &sw_data->service_id);
> +	if (ret < 0)
> +		return ret;
>
>  	return 0;
>  }
>
> -#define RTE_EVENT_TIMER_ADAPTER_NUM_MAX 64
> +#define RTE_EVENT_TIMER_ADAPTER_NUM_MAX 32

Any specific reason for this change?
maybe make it compile-time configurable

>
>  /**
>   * @warning
> --
> 2.6.4
>