Re: [dpdk-dev] [RFC PATCH v4 2/4] eventtimer: add common code

[Pavan Nikhilesh Bhagavatula <pbhagavatula@caviumnetworks.com>]

Hi Gabriel,

Comments inline

On Tue, Nov 28, 2017 at 11:40:06AM -0600, Erik Gabriel Carrillo wrote:
<snip>
> +struct rte_event_timer_adapter *
> +rte_event_timer_adapter_create(const struct rte_event_timer_adapter_conf *conf)
> +{
> +	/* default port conf values */
> +	struct rte_event_port_conf port_conf = {
> +		.new_event_threshold = 128,
> +		.dequeue_depth = 32,
> +		.enqueue_depth = 32
> +	};
Instead of harcoding get the port conf values from
rte_event_port_default_conf_get().

> +
> +	return rte_event_timer_adapter_create_ext(conf, default_port_conf_cb,
> +						  &port_conf);
> +}
> +
> +struct rte_event_timer_adapter *
> +rte_event_timer_adapter_create_ext(
> +		const struct rte_event_timer_adapter_conf *conf,
> +		rte_event_timer_adapter_port_conf_cb_t conf_cb,
> +		void *conf_arg)
> +{
> +	uint16_t adapter_id;
> +	struct rte_event_timer_adapter *adapter;
> +	const struct rte_memzone *mz;
> +	char mz_name[DATA_MZ_NAME_MAX_LEN];
> +	int n, ret;
> +	struct rte_eventdev *dev;
> +
> +	if (conf == NULL) {
> +		rte_errno = -EINVAL;
> +		return NULL;
> +	}
> +
> +	/* Check eventdev ID */
> +	if (!rte_event_pmd_is_valid_dev(conf->event_dev_id)) {
> +		rte_errno = -EINVAL;
> +		return NULL;
> +	}
> +	dev = &rte_eventdevs[conf->event_dev_id];
> +
> +	adapter_id = conf->timer_adapter_id;
> +
> +	/* Check adapter ID not already allocated */
> +	adapter = &adapters[adapter_id];
> +	if (adapter->allocated) {
> +		rte_errno = -EEXIST;
> +		return NULL;
> +	}
> +
> +	/* Create shared data area. */
> +	n = snprintf(mz_name, sizeof(mz_name), DATA_MZ_NAME_FORMAT, adapter_id);
> +	if (n >= (int)sizeof(mz_name)) {
> +		rte_errno = -EINVAL;
> +		return NULL;
> +	}
> +	mz = rte_memzone_reserve(mz_name,
> +				 sizeof(struct rte_event_timer_adapter_data),
> +				 conf->socket_id, 0);
> +	if (mz == NULL)
> +		/* rte_errno set by rte_memzone_reserve */
> +		return NULL;
> +
> +	adapter->data = mz->addr;
> +	memset(adapter->data, 0, sizeof(struct rte_event_timer_adapter_data));
> +
> +	adapter->data->mz = mz;
> +	adapter->data->event_dev_id = conf->event_dev_id;
> +	adapter->data->id = adapter_id;
> +	adapter->data->socket_id = conf->socket_id;
> +	adapter->data->conf = *conf;  /* copy conf structure */
> +
> +	/* Query eventdev PMD for timer adapter capabilities and ops */
> +	ret = dev->dev_ops->timer_adapter_caps_get(dev,
> +						   &adapter->data->caps,
> +						   &adapter->ops);

The underlying driver needs info about the adapter flags i.e.
RTE_EVENT_TIMER_ADAPTER_F_ADJUST_RES and RTE_EVENT_TIMER_ADAPTER_F_SP_PUT so we
need to pass those too conf->flags.

> +	if (ret < 0) {
> +		rte_errno = -EINVAL;
> +		return NULL;
> +	}
> +
> +	if (!(adapter->data->caps &
> +	      RTE_EVENT_TIMER_ADAPTER_CAP_INTERNAL_PORT)) {
> +		FUNC_PTR_OR_NULL_RET_WITH_ERRNO(conf_cb, -EINVAL);
> +		ret = conf_cb(adapter->data->id, adapter->data->event_dev_id,
> +			      &adapter->data->event_port_id, conf_arg);
> +		if (ret < 0) {
> +			rte_errno = -EINVAL;
> +			return NULL;
> +		}
> +	}
> +
> +	/* Allow driver to do some setup */
> +	FUNC_PTR_OR_NULL_RET_WITH_ERRNO(adapter->ops->init, -ENOTSUP);
> +	ret = adapter->ops->init(adapter);
> +	if (ret < 0) {
> +		rte_errno = -EINVAL;
> +		return NULL;
> +	}
> +
> +	/* Set fast-path function pointers */
> +	adapter->arm_burst = adapter->ops->arm_burst;
> +	adapter->arm_tmo_tick_burst = adapter->ops->arm_tmo_tick_burst;
> +	adapter->cancel_burst = adapter->ops->cancel_burst;
> +
> +	adapter->allocated = 1;
> +
> +	return adapter;
> +}
> +
> +struct rte_event_timer_adapter *
> +rte_event_timer_adapter_lookup(uint16_t adapter_id)
> +{
> +	char name[DATA_MZ_NAME_MAX_LEN];
> +	const struct rte_memzone *mz;
> +	struct rte_event_timer_adapter_data *data;
> +	struct rte_event_timer_adapter *adapter;
> +	int ret;
> +	struct rte_eventdev *dev;
> +
> +	if (adapters[adapter_id].allocated)
> +		return &adapters[adapter_id]; /* Adapter is already loaded */
> +
> +	snprintf(name, DATA_MZ_NAME_MAX_LEN, DATA_MZ_NAME_FORMAT, adapter_id);
> +	mz = rte_memzone_lookup(name);
> +	if (mz == NULL) {
> +		rte_errno = -ENOENT;
> +		return NULL;
> +	}
> +
> +	data = mz->addr;
> +
> +	adapter = &adapters[data->id];
> +	adapter->data = data;
> +
> +	dev = &rte_eventdevs[adapter->data->event_dev_id];
> +
> +	/* Query eventdev PMD for timer adapter capabilities and ops */
> +	ret = dev->dev_ops->timer_adapter_caps_get(dev,
> +						   &adapter->data->caps,
> +						   &adapter->ops);

Same as above.

> +	if (ret < 0) {
> +		rte_errno = -EINVAL;
> +		return NULL;
> +	}
> +
> +	/* Set fast-path function pointers */
> +	adapter->arm_burst = adapter->ops->arm_burst;
> +	adapter->arm_tmo_tick_burst = adapter->ops->arm_tmo_tick_burst;
> +	adapter->cancel_burst = adapter->ops->cancel_burst;
> +
> +	adapter->allocated = 1;
> +
> +	return adapter;
> +}
> +
<snip>
> +int
> +rte_event_timer_arm_burst(const struct rte_event_timer_adapter *adapter,
> +			  struct rte_event_timer **event_timers,
> +			  uint16_t nb_event_timers)
> +{
> +	int ret;
> +
> +	ADAPTER_VALID_OR_ERR_RET(adapter, -EINVAL);
> +	FUNC_PTR_OR_ERR_RET(adapter->arm_burst, -EINVAL);
> +
> +	if (!adapter->data->started)
> +		return -EAGAIN;

These checks are datapath heavy so enable them under debug compilation.

> +
> +	ret = adapter->arm_burst(adapter, event_timers, nb_event_timers);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

when burst is called we need to return the number of timers successfully set
and free the failures. Return the result directly.

> +}
> +
> +int
> +rte_event_timer_arm_tmo_tick_burst(
> +			const struct rte_event_timer_adapter *adapter,
> +			struct rte_event_timer **event_timers,
> +			const uint64_t timeout_ticks,
> +			const uint16_t nb_event_timers)
> +{
> +	int ret;
> +
> +	ADAPTER_VALID_OR_ERR_RET(adapter, -EINVAL);
> +	FUNC_PTR_OR_ERR_RET(adapter->arm_tmo_tick_burst, -EINVAL);
> +
> +	if (!adapter->data->started)
> +		return -EAGAIN;
> +
> +	for (int i = 0; i < nb_event_timers; i++)
> +		event_timers[i]->timeout_ticks = timeout_ticks;
> +
> +	ret = adapter->arm_tmo_tick_burst(adapter, event_timers, timeout_ticks,
> +					  nb_event_timers);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

Same as above.

> +}
> +
> +int
> +rte_event_timer_cancel_burst(const struct rte_event_timer_adapter *adapter,
> +			     struct rte_event_timer **event_timers,
> +			     uint16_t nb_event_timers)
> +{
> +	int ret;
> +
> +	ADAPTER_VALID_OR_ERR_RET(adapter, -EINVAL);
> +	FUNC_PTR_OR_ERR_RET(adapter->cancel_burst, -EINVAL);
> +
> +	if (!adapter->data->started)
> +		return -EAGAIN;
> +
> +	ret = adapter->cancel_burst(adapter, event_timers, nb_event_timers);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

Same as above.

> +}
> diff --git a/lib/librte_eventdev/rte_event_timer_adapter_driver.h b/lib/librte_eventdev/rte_event_timer_adapter_driver.h

Consider naming this _pmd.h for consistency

> new file mode 100644
> index 0000000..485fad1
> --- /dev/null
> +++ b/lib/librte_eventdev/rte_event_timer_adapter_driver.h
> @@ -0,0 +1,159 @@

Regards,
Pavan.