Hi Jerin,
> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.ja...@caviumnetworks.com]
> Sent: Monday, March 12, 2018 6:21 AM
> To: Carrillo, Erik G <erik.g.carri...@intel.com>
> Cc: pbhagavat...@caviumnetworks.com; dev@dpdk.org;
> nipun.gu...@nxp.com; hemant.agra...@nxp.com
> Subject: Re: [PATCH v7 6/7] doc: add event timer adapter section to
> programmer's guide
>
> -----Original Message-----
> > Date: Thu, 8 Mar 2018 15:54:05 -0600
> > From: Erik Gabriel Carrillo <erik.g.carri...@intel.com>
> > To: pbhagavat...@caviumnetworks.com
> > CC: dev@dpdk.org, jerin.ja...@caviumnetworks.com,
> nipun.gu...@nxp.com,
> > hemant.agra...@nxp.com
> > Subject: [PATCH v7 6/7] doc: add event timer adapter section to
> > programmer's guide
> > X-Mailer: git-send-email 1.7.10
> >
> > Signed-off-by: Erik Gabriel Carrillo <erik.g.carri...@intel.com>
> > Signed-off-by: Jerin Jacob <jerin.ja...@caviumnetworks.com>
> > Signed-off-by: Pavan Nikhilesh <pbhagavat...@caviumnetworks.com>
> > ---
<... snipped ...>
> > +
> > +Event Timer struct
> > +------------------
> > +Event timers are timers that enqueue a timer expiration event to an
> > +event device upon firing.
>
> I think, it better to change to _timer expiry_ from _firing_.
>
Sounds good.
> > +
> > +The Event Timer Adapter API represents each event timer with a
> > +generic struct, which contains an event and user metadata. The
> > +``rte_event_timer`` struct is defined in
> ``lib/librte_event/librte_event_timer_adapter.h``.
> > +
> > +.. _timer_expiry_event:
> > +
> > +Arming Event Timers
> > +~~~~~~~~~~~~~~~~~~~~~
> > +
> > +Once an event timer adapter has been started, an application can
> > +begin to manage event timers with it.
> > +
> > +The application should allocate ``struct rte_event_timer`` objects
> > +from a mempool or huge-page backed application buffers of required
> > +size. Upon successful allocation, the application should initialize
> > +the event timer, and then set any of the necessary event attributes
> > +described in the `Timer Expiry Event`_ section. In the following
> > +example, assume ``conn`` represents a TCP connection and that
> > +``event_timer_pool`` is a mempool that was created previously:
> > +
> > +.. code-block:: c
> > +
> > + rte_mempool_get(event_timer_pool, (void **)&conn->evtim);
> > + if (conn->evtim == NULL) { ... }
> > +
> > + rte_event_timer_init(&conn->evtim);
> > +
> > + /* Set up the expiry event. */
> > + conn->evtim->ev.event_ptr = conn;
>
> Not specific to this specific example, What would be the default behaviour if
> application does not set ev.event_ptr value.
>
Currently, it's undefined.
> Can we say?
> "NULL value is allowed, in which case adapter set the event_ptr to struct
> rte_event_timer *
>
I like that idea; it would be a convenient touch. I'll look at making this
update.
> > + conn->evtim->ev.queue_id = event_queue_id;
> > + ...
> > + conn->evtim->timeout_ticks = 30; //3 sec Per RFC1122(TCP returns)
> > +
> > +Note that we have saved a pointer to the ``conn`` object in the
> > +timer's event payload. This will allow us to locate the connection
> > +object again once we dequeue the timer expiry event from the event
> device later.
<... snipped ...>
>
> Overall, it looks good. With above changes
>
> Acked-by: Jerin Jacob <jerin.ja...@caviumnetworks.com>
>
>
> > --
> > 2.6.4
> >
Thanks for the suggestions,
Gabriel
