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

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

On Thu, Nov 30, 2017 at 08:59:19PM +0000, Carrillo, Erik G wrote:
> Hi Pavan,
>
> Thanks for the review;  I'm working on addressing the comments and have the following question (inline):
>
> <... snipped ...>
>
> > > +	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.
>
> By "underlying driver", are you referring to the eventdev PMD, or the event timer adapter "driver" (i.e., the set of ops functions)?
>
> If the latter, the adapter "driver" will have a chance to inspect the flags when adapter->ops->init() is called below, since it can look at the flags through the adapter arg.
>

I was refering to the timer driver, the presence of flag RTE_EVENT_TIMER_ADAPTER_F_SP_PUT
would suggest the driver to use a multi thread unsafe arm/cancel data path API
and it would set a different function pointers to adapter->arm_burst etc.

I dont think in the current scheme this is possible. Currently, if we see mempool it inspects
flags before setting ops.

Hope this clears things up.

-Pavan

> If the former, will the eventdev PMD consider the flags when deciding whether or not to provide an ops definition in the timer_adapter_caps_get() call?
>
> >
> > > +	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;
> > > +}
>
> <... snipped ...>