On 12 September 2014 11:30, Savolainen, Petri (NSN - FI/Espoo) <
[email protected]> wrote:
> > + /* Get the next ready buffer/timeout */
> > + tmo = odp_schedule_one(&queue, ODP_SCHED_WAIT);
>
> This is not portable. Scheduler returns odp_buffer_t. odp_timer_tmo_t type
> is not always odp_buffer_t.
>
The implementation has to define odp_timer_tmo_t as a subtype of
odp_buffer_t. This is equivalent to how
odp_packet_t is defined and handled by linux-generic. Why is this portable
for packets but not for timeouts?
The example could be more realistic, using an odp_buffer_t variable for the
return of odp_schedule_one() and
then check if the buffer is actually a timeout and not some other type of
buffer (e.g. packet).
>
> > + switch (odp_timer_tmo_status(tmo)) {
> > + case ODP_TMO_FRESH:
> > + break;
> > + case ODP_TMO_STALE:
> > + ODP_DBG("[%i] Stale timeout received\n", thr);
> > + break;
> > + case ODP_TMO_ORPHAN:
> > + ODP_DBG("[%i] Orphaned timeout received\n",
> > + thr);
> > + odp_buffer_free(tmo);
>
> odp_timer_tmo_t != odp_buffer_t
>
odp_timer_tmo_t is a subtype of odp_buffer_t by definition.
odp_buffer_alloc and odp_buffer_free are used,
just like for packets.
>From odp_buffer.h:
#define ODP_BUFFER_TYPE_INVALID (-1) /**< Buffer type invalid */
#define ODP_BUFFER_TYPE_ANY 0 /**< Buffer that can hold any other
buffer type */
#define ODP_BUFFER_TYPE_RAW 1 /**< Raw buffer, no additional
metadata */
#define ODP_BUFFER_TYPE_PACKET 2 /**< Packet buffer */
#define ODP_BUFFER_TYPE_TIMEOUT 3 /**< Timeout buffer */
> > + continue;
> > + }
>
> Could we optimize for the common case. Minimize C lines/cycles for
> freshness check and ack.
>
> if (odp_unlikely(odp_timer_tmo_check(tmo))) {
> /* handle errors */
> status = odp_timer_tmo_status(tmo);
> ...
> }
>
> OK. This was just an example.
> +/**
> > +* ODP timer pool handle (platform dependent)
> > +*/
> > +struct odp_timer_pool;
>
>
> struct odp_timer_pool_t (or struct odp_timer_pool_s or struct
> odp_timer_pool_)
>
OK if this is the coding standard.
Struct names are in their own name space and always preceded by the
"struct" keyword so I cannot
really see how there could be confusion to either the compiler or the user.
> -/** Invalid timer */
> > -#define ODP_TIMER_INVALID 0
> > +typedef enum odp_timer_pool_clock_source_e {
> > + ODP_CLOCK_DEFAULT = 0,
> > + /* Platform dependent which clock sources exist beyond
> > + ODP_CLOCK_DEFAULT */
>
>
> Maybe this comment can be removed. We may be able standardize some. E.g.
> _CPU_CLOCK (chip internal) and _TIMER_INPUT (chip external, connected to
> timer input pin)
>
I am open to changes here but think the above is good for linux-generic and
I can't see how linux-generic
should handle _CPU_CLOCK and _TIMER_INPUT in any other way than just
ignoring them and do it the
current way anyway. Perhaps this is what we should do?
What do the other platform developers think about Petri's suggestion? Can
we standardize on that (with some
liberal interpretation if needed by different platforms)?
>
> > + ODP_CLOCK_NONE = 1
>
> When none is used?
>
Sort of a left over from early testing without any HW or OS timer providing
a tick. Instead the
application was providing the tick (a la DPDK) via the
odp_timer_pool_expire() function.
If we don't think this situation makes sense, I can remove the support.
> > +} odp_timer_pool_clock_source_t;
>
> Pretty long type name, could we use odp_timer_clock_source_t instead?
>
OK. We can even abbreviate some of the words, e.g. odp_timer_clk_src_t. OK?
>
> >
> > +/**
> > +* ODP timer handle (platform dependent)
> > +*/
> > +struct odp_timer;
> > +typedef struct odp_timer *odp_timer_t;
>
>
> struct odp_timer_t
>
OK.
> + * ODP timeout status
> > */
> > -typedef odp_buffer_t odp_timeout_t;
> > +typedef enum odp_timer_tmo_status_e {
> > + ODP_TMO_FRESH, /* Timeout is fresh, process it */
> > + ODP_TMO_STALE, /* Timer reset or cancelled, do nothing */
>
>
> Do nothing? Did the status function free the buffer? The buffer has to be
> freed or reused, otherwise it's a memory leak.
>
If a timer is reset or cancelled, the implementation is reusing the timeout
buffer automatically.
> > + ODP_TMO_ORPHAN,/* Timer deleted, free timeout */
> > +} odp_timer_tmo_status_t;
> > +
> > +/**
> > +* ODP tick value
> > +*/
> > +typedef uint64_t odp_timer_tick_t;
>
>
> Why tick could not be fixed to uint64_t? It's simpler for the user to
> perform (portable) tick calculations if the type is fixed. Now it could be
> signed vs. unsigned, 16/32/64 bits...
>
I expect timer_tick to be fixed to uint64_t (although one can imagine
architectures where uint32_t would make sense). I just want an explicit
type that denotes timer_ticks. When everything is declared as uint64_t, I
become confused about what those variables and function parameters and
return values actually mean. This is for clarity and much more important
that some "_s" suffix on a struct type.
Perhaps we need a way to limit what ODP platforms redefine types etc? The
ODP validation suite could verify that odp_timer_tick_t is a 64-bit
unsigned scalar type.
>
> >
> >
> > /**
> > - * Create a timer
> > + * Create a timer pool
> > *
> > - * Creates a new timer with requested properties.
> > + * Create a new timer pool.
> > + * odp_timer_pool_create() is typically called once or a couple of times
> > during
> > + * application initialisation.
>
>
> I think the speculation above is not needed. There's also timer pool
> destroy
> function, and user may destroy/create pools also after init.
>
OK.
>
> > *
> > * @param name Name
> > - * @param pool Buffer pool for allocating timeout notifications
> > + * @param buf_pool Buffer pool for allocating timers
>
>
> odp_timer_t or odp_timer_tmo_t or both? I'm thinking timeouts.
>
You are correct, timeouts it is.
>
> > * @param resolution Timeout resolution in nanoseconds
> > - * @param min_tmo Minimum timeout duration in nanoseconds
>
>
> Min_tmo could be useful still. Some implementations may appreciate to have
> more information about the intended usage, e.g. what kind of (cascading)
> timer wheels to create, etc. Zero could be the default (== resolution).
>
If the application specifies a minimum timeout duration, what should happen
if it attempts to request timeouts smaller than that threshold? Should the
implementation silently increase the period to the min duration? Or throw an
error? If we introduce this parameter, we need to define how it affects
user-visible behavior.
What is interesting is whether we should give the implementation some hints
about allowed jitter for a timer. Even with a user-defined resolution, the
application
might be able to tolerate some additional latency which can help the
imlementation
to coalesce timers etc.
>
> > + * Destroy a timer pool
> > *
> > - * @return Timer handle if successful, otherwise ODP_TIMER_INVALID
> > + * Destroy a timer pool, freeing all resources.
> > + * All timers must have been freed.
>
>
> When user have called odp_timer_free() for all timers, there may be still
> timeouts in timer HW and in queues. Is it OK to delete the timer pool
> if all remaining timeouts have not been received yet? E.g. user could not
> delete the "timer buffer pool" after this call returns, since some timeouts
> may be still on the way and need to be freed. How user knows when the
> buffer
> pool can be reused or freed?
>
This is a tricky question. And related to buffer pool semantics I would
think.
I assume that when freeing a timer, if the timeout is still in the HW, it
can be
reclaimed as well (but it doesn't have to be done). Timeouts can
definitively
still be on queues and will eventually be received by some agent.
What happens if you attempt to destroy a buffer pool which has buffers
still in
use? (Well currently there is no such call implemented but it is in the
buffer
management design doc). This is what the linux-generic implementation will
boil down to. If the buffer pool API would provide some API to help with
this
situation, it could also be used by the timer API.
Maybe we need to add a sentence that all timeouts also must have been
received and freed. But it is unknown how the timer implementation currently
could check or enforce this condition.
The timer API does handle the situation where a timeout is received after
the
corresponding timer has been freed. Such timeouts will have status orphan
and the application should just free them.
>
>
> > +uintptr_t odp_timer_pool_query_conf(odp_timer_pool_t tpid,
> > + odp_timer_pool_conf_t item);
>
>
> It would be simpler to just query all information at once.
>
> typedef struct odp_timer_pool_info_s {
> const char *name;
> uint64_t resolution;
> uint64_t max_tmo;
> uint32_t num_timers;
> bool shared
> } odp_timer_pool_info_t;
>
> int odp_timer_pool_info(odp_timer_pool_t tpid, odp_timer_pool_info_t
> *info);
>
> Yes but I expect that there could be future additions and thus I wanted a
more
extensible API. If you think the all info in a struct still is better, I
will change to that.
> > +/**
> > + * Free a timer
> > + *
> > + * Free (destroy) a timer, freeing all associated resources (e.g.
> default
> > + * timeout event). An expired and enqueued timeout event will not be
> > freed.
>
>
> User should see from a return code if free was successful or not. Now user
> does not know if it has to wait for timeout or not.
>
OK, I think I can add such a return code. It is not an error, the free timer
call won't fail.
> >
> > /**
> > - * Request timeout with an absolute timer tick
> > + * Set a timer (absolute time) with a user-defined timeout buffer
> > *
> > - * When tick reaches tmo_tick, the timer enqueues the timeout
> > notification into
> > - * the destination queue.
> > + * Set (arm) the timer to expire at specific time. The user-defined
> > + * buffer will be enqueued when the timer expires.
> > + * Arming may fail (if the timer is in state EXPIRED), an earlier
> timeout
> > + * will then be received. odp_timer_tmo_status() must be used to check
> if
> > + * the received timeout is valid.
> > *
> > - * @param timer Timer
> > - * @param tmo_tick Absolute timer tick value which triggers the timeout
> > - * @param queue Destination queue for the timeout notification
> > - * @param buf User defined timeout notification buffer. When
> > - * ODP_BUFFER_INVALID, default timeout notification is
> > used.
> > + * Note: any invalid parameters will be treated as programming errors
> and
> > will
> > + * cause the application to abort.
> > + * Note: a timeout too near in time may be delivered immediately.
> > + * Note: a timeout too far away in time (beyond max_timeout) might be
> > + * delivered early.
>
>
> In the two cases above, application must be in control what happens next.
> ODP does not know what
> application was trying to do with the timeout (tmo priority or severity of
> the failure) or
> what is the right corrective action (if any is needed). E.g. in "too near"
> case it may be better
> for application to skip that tmo and just continue from the next cycle,
> etc. Speculation on
> "too far" is also bad. For example, if user was supposed to ask a 1 sec,
> but miss
> calculated and asked a 500 year timeout, now ODP implementation would
> silently round
> that to the max e.g. 3 days. How do you debug it after 3 days? With a "too
> far" return code
> application could log/report error and take the corrective action where
> the bug
> was noticed the first time.
>
Possibly the application would like to handle too near/too far situations
more explicitly, I just
want to avoid the application to have to handle "error" situations when a
flow/connection is
set up. No one else that read and commented on the Timer Use Cases and API
design doc
thought we needed to report such situations here. I think we need a
discussion on this subject
with some more stake holders.
> > +void odp_timer_set_rel(odp_timer_t tim, odp_timer_tick_t rel_tck);
>
>
> odp_timer_set_rel_w_buf() missing?
>
Yes. Do you really want it?
> >
> > /**
> > - * Return absolute timeout tick
> > + * Cancel a timer
> > + *
> > + * Cancel a timer, preventing future expiration and delivery.
> > + *
> > + * A timer that has already expired and been enqueued for delivery may
> be
> > + * impossible to cancel and will instead be delivered to the destination
> > queue.
>
>
> How user knows if it has to wait for a pending timeout still, or can it
> e.g.
> remove the destination queue? Function should return a success code.
>
The destination queue should be removed after the timer has been freed, not
after a timer/timeout has been cancelled. We agree that odp_timer_free()
should
return a value that indicates whether timeouts are still pending.
> > + * Use odp_timer_tmo_status() the check whether a received timeout is
> > fresh or
> > + * stale (cancelled). Stale timeouts will automatically be recycled.
> > *
> > - * @param tmo Timeout buffer handle
> > + * Note: any invalid parameters will be treated as programming errors
> and
> > will
> > + * cause the application to abort.
> > *
> > - * @return Absolute timeout tick
> > + * @param tim Timer handle
> > */
> > -uint64_t odp_timeout_tick(odp_timeout_t tmo);
> > +void odp_timer_cancel(odp_timer_t tim);
> > +
> > +/**
> > + * Return fresh/stale/orphan status of timeout.
> > + *
> > + * Check a received timeout for orphaness (i.e. parent timer freed) and
> > + * staleness (i.e. parent timer has been reset or cancelled after
> timeout
> > + * was enqueued).
>
>
> It's not very well documented how a timer is reset. E.g. another
> odp_timer_set_abs_w_buf() call on an already active timer?
>
Yes, just (re-) set or cancel the timer.
I will see how I can improve the documentation to make this clearer.
>
> Can user mix timer set calls when resetting e.g.
> odp_timer_set_abs_w_buf(timer, tick, buf) -> odp_timer_set_abs(timer, tick)?
>
> What happens if timer is reset too late? Does user receive the original,
> the new or a stale tmo?
>
Does this have to be specified?
The linux-generic implementation of course will behave in some consistent
way but I don't think this has to be
normative as I expect timer HW implementations may differ here.
Do applications have any specific interest in one behavior or the other?
I don't think it is a great idea to mix used-defined and system-defined
timeout buffers for the same timer.
The linux-generic implementation handles it but possibly this behavior
could be implementation specific.
Maybe we should add a restriction that once the application has switched to
using user-defined timeout buffers,
it should not change behavior. Maybe this is another parameter to
odp_timer_alloc()?
>
> > + * Get timer handle
> > + *
> > + * Return Handle of parent timer.
> > + *
> > + * @param tmo Timeout
> > + *
> > + * @return Timer handle or ODP_TIMER_INVALID for orphaned timeouts
> > + */
> > +odp_timer_t odp_timer_get_handle(odp_timer_tmo_t tmo);
>
>
> Should we drop _get_ to be consistent with other ODP APIs.
>
> > +odp_timer_tick_t odp_timer_get_expiry(odp_timer_tmo_t tmo);
>
>
> Should we drop _get_ to be consistent with other ODP APIs.
>
> > +void *odp_timer_get_userptr(odp_timer_tmo_t tmo);
>
>
> Should we drop _get_ to be consistent with other ODP APIs.
>
Three OK's.
>
> > +
> > +/* Helper functions */
> > +unsigned odp_timer_pool_expire(odp_timer_pool_t tpid, odp_timer_tick_t
> > tick);
>
> Is this function part of the API?
>
It depends whether we want to support ODP_CLOCK_NONE.
We could remove it, maybe this is best in order to minimize time spent on
less important issues.
In all a good review, caught some mistakes and suggested some improvements.
>
> -Petri
>
>
>
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
