[dpdk-dev] [PATCH v3 1/1] eventdev: add new software event timer adapter
Carrillo, Erik G
erik.g.carrillo at intel.com
Sat Dec 15 00:04:23 CET 2018
> > 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.
More information about the dev
mailing list