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

[Carrillo, Erik G]

> -----Original Message-----
> From: Pavan Nikhilesh Bhagavatula
> [mailto:pbhagavatula at caviumnetworks.com]
> Sent: Thursday, November 30, 2017 11:13 PM
> To: Carrillo, Erik G <erik.g.carrillo at intel.com>
> Cc: dev at dpdk.org
> Subject: Re: [RFC PATCH v4 2/4] eventtimer: add common code
> 
> 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

Yes, I see your point now.  I agree that it would be useful to allow different ops structures to be selected based on the flags that are set in addition to being able to inspect the flags within the ops functions themselves.  I have made the change in the follow-up patch series.

Thanks,
Gabriel

> 
> > 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 ...>