Re: [dpdk-dev] [PATCH v3 1/1] eventdev: add new software event timer adapter

["Carrillo, Erik G" <erik.g.carrillo@intel.com>]

> >   lib/librte_eventdev/rte_event_timer_adapter.c | 688 +++++++++++------
> ---------
> >   1 file changed, 276 insertions(+), 412 deletions(-)
> >
> > diff --git a/lib/librte_eventdev/rte_event_timer_adapter.c
> > b/lib/librte_eventdev/rte_event_timer_adapter.c
> > index 79070d4..029a45a 100644
> > --- a/lib/librte_eventdev/rte_event_timer_adapter.c
> > +++ b/lib/librte_eventdev/rte_event_timer_adapter.c
> > @@ -19,6 +19,7 @@
> >   #include <rte_timer.h>
> >   #include <rte_service_component.h>
> >   #include <rte_cycles.h>
> > +#include <rte_random.h>
> 
> You aren't using anything from rte_random.h.
> 
> /../
> 

Removed in next version.

<...snipped...>

> > -		if (check_destination_event_queue(evtims[i], adapter) < 0) {
> > +		if (unlikely(check_destination_event_queue(evtims[i],
> > +							   adapter) < 0)) {
> >   			evtims[i]->state = RTE_EVENT_TIMER_ERROR;
> >   			rte_errno = EINVAL;
> >   			break;
> >   		}
> >
> > -		/* Checks passed, set up a message to enqueue */
> > -		msgs[i]->type = MSG_TYPE_ARM;
> > -		msgs[i]->evtim = evtims[i];
> > +		tim = tims[i];
> > +		rte_timer_init(tim);
> >
> > -		/* Set the payload pointer if not set. */
> > -		if (evtims[i]->ev.event_ptr == NULL)
> > -			evtims[i]->ev.event_ptr = evtims[i];
> > +		evtims[i]->impl_opaque[0] = (uintptr_t)tim;
> > +		evtims[i]->impl_opaque[1] = (uintptr_t)adapter;
> >
> > -		/* msg objects that get enqueued successfully will be freed
> > -		 * either by a future cancel operation or by the timer
> > -		 * expiration callback.
> > -		 */
> > -		if (rte_ring_enqueue(sw_data->msg_ring, msgs[i]) < 0) {
> > -			rte_errno = ENOSPC;
> > +		cycles = get_timeout_cycles(evtims[i], adapter);
> > +		ret = rte_timer_alt_reset(sw->timer_data_id, tim, cycles,
> > +					  SINGLE, lcore_id, NULL, evtims[i]);
> > +		if (ret < 0) {
> > +			/* tim was in RUNNING or CONFIG state */
> > +			evtims[i]->state = RTE_EVENT_TIMER_ERROR;
> >   			break;
> >   		}
> >
> > -		EVTIM_LOG_DBG("enqueued ARM message to ring");
> > -
> > +		rte_smp_wmb();
> > +		EVTIM_LOG_DBG("armed an event timer");
> >   		evtims[i]->state = RTE_EVENT_TIMER_ARMED;
> 
> This looks like you want a reader to see the impl_opaque[] stores, before the
> state store, which sounds like a good idea.
> 
> However, I fail to find the corresponding read barriers on the reader side.
> Shouldn't swtim_cancel_burst() have such? It's loading state, and loading
> impl_opaque[], but there's no guarantee those memory loads happens in
> program order.

Thanks again for the good catch.  Updating this in the next version of the patch.