After quick look into it...
> -----Original Message-----
> From: [email protected] [mailto:lng-odp-
> [email protected]] On Behalf Of ext Ola Liljedahl
> Sent: Wednesday, December 17, 2014 12:46 AM
> To: [email protected]
> Subject: [lng-odp] [PATCHv2 2/3] api: odp_timer.h: updated API, lock-less
> implementation
>
> The timer API is updated. A major change is that timers are allocated and
> freed
> separately from timeouts being set and cancelled. The life-length of a
> timer
> normally corresponds to the life-length of the associated stateful flow
> while
> the life-length of a timeout corresponds to individual packets being
> transmitted and received.
> The reference timer implementation is lock-less for platforms with support
> for 128-bit (16-byte) atomic exchange and CAS operations. Otherwise a
> lock-based
> implementation (using as many locks as desired, no global lock) is used
> but
> some operations (e.g. reset re-using existing timeout buffer) may still be
> lock-less on some architectures.
> Updated the example example/timer/odp_timer_test.c.
>
> Signed-off-by: Ola Liljedahl <[email protected]>
> ---
> (This document/code contribution attached is provided under the terms of
> agreement LES-LTM-21309)
> example/timer/odp_timer_test.c | 173 +--
> platform/linux-generic/include/api/odp_timer.h | 474 +++++++--
> .../linux-generic/include/odp_timer_internal.h | 60 +-
> platform/linux-generic/odp_timer.c | 1098 ++++++++++++++-
> -----
> 4 files changed, 1324 insertions(+), 481 deletions(-)
>
> diff --git a/example/timer/odp_timer_test.c
> b/example/timer/odp_timer_test.c
> index 972bc96..33ef219 100644
> --- a/example/timer/odp_timer_test.c
> +++ b/example/timer/odp_timer_test.c
> @@ -26,7 +26,7 @@
>
>
> #define MAX_WORKERS 32 /**< Max worker threads */
> -#define MSG_POOL_SIZE (4*1024*1024) /**< Message pool size */
> +#define MSG_POOL_SIZE (4*1024) /**< Message pool size */
>
>
> /** Test arguments */
> @@ -43,67 +43,116 @@ typedef struct {
> /** @private Barrier for test synchronisation */
> static odp_barrier_t test_barrier;
>
> -/** @private Timer handle*/
> -static odp_timer_t test_timer;
> +/** @private Buffer pool handle */
> +static odp_buffer_pool_t pool;
>
> +/** @private Timer pool handle */
> +static odp_timer_pool_t tp;
> +
> +/** @private Timeout status ASCII strings */
> +static const char *const status2str[] = {
> + "fresh", "stale", "orphaned"
> +};
> +
> +/** @private Timer set status ASCII strings */
> +static const char *timerset2str(uint64_t val)
> +{
> + switch (val) {
> + case ODP_TICK_TOOEARLY:
> + return "tooearly";
> + case ODP_TICK_TOOLATE:
> + return "toolate";
> + case ODP_TICK_INVALID:
> + return "error";
> + default:
> + return "success";
> + }
> +};
> +
> +/** @private Helper struct for timers */
> +struct test_timer {
> + odp_timer_t tim;
> + odp_buffer_t buf;
> + uint64_t tick;
> +};
> +
> +/** @private Array of all timer helper structs */
> +static struct test_timer tt[256];
>
> /** @private test timeout */
> static void test_abs_timeouts(int thr, test_args_t *args)
> {
> - uint64_t tick;
> uint64_t period;
> uint64_t period_ns;
> odp_queue_t queue;
> - odp_buffer_t buf;
> - int num;
> + int remain = args->tmo_count;
> + uint64_t tick;
> + struct test_timer *ttp;
>
> EXAMPLE_DBG(" [%i] test_timeouts\n", thr);
>
> queue = odp_queue_lookup("timer_queue");
>
> period_ns = args->period_us*ODP_TIME_USEC;
> - period = odp_timer_ns_to_tick(test_timer, period_ns);
> + period = odp_timer_ns_to_tick(tp, period_ns);
>
> EXAMPLE_DBG(" [%i] period %"PRIu64" ticks, %"PRIu64" ns\n", thr,
> period, period_ns);
>
> - tick = odp_timer_current_tick(test_timer);
> + EXAMPLE_DBG(" [%i] current tick %"PRIu64"\n", thr,
> + odp_timer_current_tick(tp));
>
> - EXAMPLE_DBG(" [%i] current tick %"PRIu64"\n", thr, tick);
> -
> - tick += period;
> -
> - if (odp_timer_absolute_tmo(test_timer, tick, queue,
> ODP_BUFFER_INVALID)
> - == ODP_TIMER_TMO_INVALID){
> - EXAMPLE_DBG("Timeout request failed\n");
> + ttp = &tt[thr - 1]; /* Thread starts at 1 */
> + ttp->tim = odp_timer_alloc(tp, queue, ttp);
> + if (ttp->tim == ODP_TIMER_INVALID) {
> + EXAMPLE_ERR("Failed to allocate timer\n");
> return;
> }
> + ttp->buf = odp_buffer_alloc(pool);
> + if (ttp->buf == ODP_BUFFER_INVALID) {
> + EXAMPLE_ERR("Failed to allocate buffer\n");
> + return;
> + }
> + tick = odp_timer_current_tick(tp);
>
> - num = args->tmo_count;
> -
> - while (1) {
> - odp_timeout_t tmo;
> -
> - buf = odp_schedule_one(&queue, ODP_SCHED_WAIT);
> -
> - tmo = odp_timeout_from_buffer(buf);
> - tick = odp_timeout_tick(tmo);
> -
> - EXAMPLE_DBG(" [%i] timeout, tick %"PRIu64"\n", thr, tick);
> -
> - odp_buffer_free(buf);
> -
> - num--;
> -
> - if (num == 0)
> - break;
> + while (remain != 0) {
> + odp_buffer_t buf;
>
> tick += period;
> + ttp->tick = odp_timer_set_abs(ttp->tim, tick, &ttp->buf);
> + if (odp_unlikely(ttp->tick == ODP_TICK_TOOEARLY ||
> + ttp->tick == ODP_TICK_TOOLATE ||
> + ttp->tick == ODP_TICK_INVALID)) {
> + /* Too early or too late timeout requested */
> + EXAMPLE_ABORT("odp_timer_set_abs() failed: %s\n",
> + timerset2str(ttp->tick));
> + }
>
> - odp_timer_absolute_tmo(test_timer, tick,
> - queue, ODP_BUFFER_INVALID);
> + /* Get the next expired timeout */
> + buf = odp_schedule_one(&queue, ODP_SCHED_WAIT);
> + if (odp_timer_tmo_metadata(buf, NULL, &tick, (void **)&ttp)) {
> + ttp->buf = buf;
> + if (odp_likely(tick != ttp->tick)) {
Isn't this a potential race condition introduced by the API ? I.e. if same
operations run on multiple threads and returned tick value is used for checking
tmo validity (STALE vs FRESH).
Thread A:
// HW timer is set, timer expires,
// tmo buf is scheduled to thread B, before this function returns (due to an
interrupt, etc)
... = odp_timer_set_abs(ttp->tim, tick, &ttp->buf);
Thread B:
// receives and handles tmo
buf = odp_schedule_one(&queue, ODP_SCHED_WAIT);
if (odp_timer_tmo_metadata(buf, NULL, &tick, (void **)&ttp)) {
ttp->buf = buf;
// ttp->tick is not set yet
if (odp_likely(tick != ttp->tick)) { ...
Thread A:
// odp_timer_set_abs() returns and sets ttp->tick
ttp->tick = ...
> --- a/platform/linux-generic/include/api/odp_timer.h
> +++ b/platform/linux-generic/include/api/odp_timer.h
> @@ -8,9 +8,211 @@
> /**
> * @file
> *
> - * ODP timer
> + * ODP timer service
> */
>
> +/** Example #1 Retransmission timer (e.g. for reliable connections)
> + @code
> +
> +//Create timer pool for reliable connections
> +#define SEC 1000000000ULL //1s expressed in nanoseconds
> +odp_timer_pool_t tcp_tpid =
> + odp_timer_pool_create("TCP",
> + buffer_pool,
> + 1000000,//resolution 1ms
> + 0,//min tmo
> + 7200 * SEC,//max tmo length 2hours
> + 40000,//num_timers
> + true,//shared
> + ODP_CLOCK_CPU
> + );
> +if (tcp_tpid == ODP_TIMER_POOL_INVALID)
> +{
> + //Failed to create timer pool => fatal error
> +}
> +
It's better to remove all this pseudo example code from the API file and
add one or two as real examples that can be built, run and verified
that they actually work and are in sync with the current API version.
>
> +/**
> + * Return values of timer set calls.
> + */
> +/**
> + * Timer set operation failed, expiration too early.
> + * Either retry with a later expiration time or process the timeout
> + * immediately. */
> +#define ODP_TICK_TOOEARLY 0xFFFFFFFFFFFFFFFDULL
> +/**
> + * Timer set operation failed, expiration too late.
> + * Truncate the expiration time against the maximum timeout for the
> + * timer pool. */
> +#define ODP_TICK_TOOLATE 0xFFFFFFFFFFFFFFFEULL
> +/**
> + * Timer set operation failed because not timeout buffer present or
> specified.
> + * This value is also return from odp_timer_cancel() and
> odp_timer_free().
> + */
> +#define ODP_TICK_INVALID 0xFFFFFFFFFFFFFFFFULL
I think it's not good idea to overload tick variables with special tick values.
Yes, the type is uint64_t and it's very unlikely run into this values when tick
starts from 0 - but (on API level) it opens a door for strange behavior with if
the system tick is uninitialized or init with a large value.
>
> /**
> - * Create a timer
> + * Create a timer pool
> *
> - * Creates a new timer with requested properties.
> + * Create a new timer pool.
> *
> * @param name Name
> - * @param pool Buffer pool for allocating timeout notifications
> + * @param buf_pool Buffer pool for allocating timeouts (and only
> timeouts)
> * @param resolution Timeout resolution in nanoseconds
> - * @param min_tmo Minimum timeout duration in nanoseconds
> - * @param max_tmo Maximum timeout duration in nanoseconds
> + * @param min_tmo Minimum relative timeout in nanoseconds
> + * @param max_tmo Maximum relative timeout in nanoseconds
> + * @param num_timers Number of supported timers (minimum)
> + * @param shared Shared or private timer pool.
> + * Operations on shared timers will include the necessary
> + * mutual exclusion, operations on private timers may not
> + * (mutual exclusion is the responsibility of the caller).
> + * @param clk_src Clock source to use
> + *
> + * @return Timer pool handle if successful, otherwise
> ODP_TIMER_POOL_INVALID
> + * and errno set
> + */
> +odp_timer_pool_t
> +odp_timer_pool_create(const char *name,
> + odp_buffer_pool_t buf_pool,
> + uint64_t resolution,
> + uint64_t min_tmo,
> + uint64_t max_tmo,
> + uint32_t num_timers,
> + int shared,
> + odp_timer_clk_src_t clk_src);
Not a must change in this point, but the API would be cleaner if all these
params would be packed into a odp_timer_pool_param_t struct (all except name,
just like odp_buffer_pool_param_t).
Also I'd swap "int shared" to "int private". So that the default value (private
== 0) would select the shared timer mode (== multicore, which is the default
for everything else in ODP).
> /**
> - * Maximum timeout in timer ticks
> + * ODP timer pool information and configuration
> + */
> +
> +typedef struct odp_timer_pool_info_s {
> + uint64_t resolution;/**< Timer resolution (in ns) */
> + uint64_t min_tmo; /**< Min supported relative timeout (in ticks)*/
> + uint64_t max_tmo; /**< Max supported relative timeout (in ticks)*/
> + uint32_t num_timers;/**< Number of supported timers */
> + uint32_t cur_timers;/**< Number of currently allocated timers */
> + uint32_t hwm_timers;/**< Number of used timers high watermark */
> + int shared; /**< Shared flag */
> + char name[80]; /**< Name of timer pool */
Name can be a const pointer to the string.
Params from pool_create could be packed into the odp_timer_pool_param_t (just
like odp_buffer_pool_info_t).
> +} odp_timer_pool_info_t;
> +
> +/**
> + * Query timer pool information and configuration
> + * Timer resolution in nanoseconds
> + * Minimum and maximum (relative) timeouts in timer ticks
> + * Number of supported timers
> + * Nunber of timers in use
> + * Nunber of timers in use - high watermark
> + * Shared or private timer pool
> + * Name of timer pool.
> *
> - * @param timer Timer
> + * @param tpid Timer pool identifier
> + * @param buf Pointer to information buffer
> + * @param buf_size Size of information buffer
Could be dropped since sizeof(odp_timer_pool_info_t) defines the size (just
like odp_buffer_pool_info(
) outputs info and returns success/failure).
> *
> - * @return Maximum timeout in timer ticks
> + * @return Actual size written
> */
> -uint64_t odp_timer_maximum_tmo(odp_timer_t timer);
> +size_t odp_timer_pool_info(odp_timer_pool_t tpid,
> + odp_timer_pool_info_t *buf,
> + size_t buf_size);
>
> +
> +/**
> + * Free a timer
> + *
> + * Free (destroy) a timer, freeing associated resources.
> + * The timeout buffer for an active timer will be returned.
> + * An expired and enqueued timeout buffer will not be freed.
> + * It is the responsibility of the application to free this timeout when
> it
> + * is received.
> + *
> + * @param tim Timer handle
> + * @param tmo_buf Reference to a buffer variable which will be written
> with
> + * the buffer handle of any present timeout buffer (e.g. for an active
> timer).
> + * @return A tick value which will not match any valid expiration
> tick.
> */
> -uint64_t odp_timer_current_tick(odp_timer_t timer);
> +uint64_t odp_timer_free(odp_timer_t tim, odp_buffer_t *tmo_buf);
Why this returns tick? And what value it actually returns?
The documentation of tmo_buf output is misleading. It should say that if the
operation outputs handle if active timer was successfully canceled, if it
failed on that BUFFER_INVALID is output. I think that's the intention, so that
user do not get handle to a buffer that may be on the way on the queue and
potentially double free that.
>
> /**
> - * Request timeout with an absolute timer tick
> + * Set a timer (absolute time) with a user-provided timeout buffer
> + *
> + * Set (arm) the timer to expire at specific time. The timeout
> + * buffer will be enqueued when the timer expires.
> *
> - * When tick reaches tmo_tick, the timer enqueues the timeout
> notification into
> - * the destination queue.
> + * Note: any invalid parameters will be treated as programming errors and
> will
> + * cause the application to abort.
> *
> - * @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.
> + * @param tim Timer
> + * @param abs_tck Expiration time in absolute timer ticks
> + * @param tmo_buf Reference to a buffer variable that points to timeout
> buffer
> + * or NULL to reuse the existing timeout buffer
> *
> - * @return Timeout handle if successful, otherwise ODP_TIMER_TMO_INVALID
> + * @return The expiration tick or ODP_TICK_TOOEARLY, ODP_TICK_TOOLATE or
> + * ODP_TICK_INVALID (timer not active and no timeout buffer to reuse).
> */
> -odp_timer_tmo_t odp_timer_absolute_tmo(odp_timer_t timer, uint64_t
> tmo_tick,
> - odp_queue_t queue, odp_buffer_t buf);
> +uint64_t odp_timer_set_abs(odp_timer_t tim,
> + uint64_t abs_tck,
> + odp_buffer_t *tmo_buf);
Why this returns tick and not just a status. It promotes the race condition
highlighted earlier (my->tick vs. tmo->tick).
Can this accept also user defined buffers (not type timeout)?
>
> /**
> - * Convert buffer handle to timeout handle
> + * Cancel a timer
> + *
> + * Cancel a timer, preventing future expiration and delivery. Return any
> + * present timeout buffer.
> + *
> + * A timer that has already expired may be impossible to cancel and the
> timeout
> + * will instead be delivered to the destination queue.
> *
> - * @param buf Buffer handle
> + * Note: any invalid parameters will be treated as programming errors and
> will
> + * cause the application to abort.
> *
> - * @return Timeout buffer handle
> + * @param tim Timer
> + * @param tmo_buf Reference to a buffer variable which will be written
> with
> + * the buffer handle of any present timeout buffer (e.g. for an active
> timer).
> + * @return A tick value which will not match any valid expiration
> tick.
> */
> -odp_timeout_t odp_timeout_from_buffer(odp_buffer_t buf);
> +uint64_t odp_timer_cancel(odp_timer_t tim, odp_buffer_t *tmo_buf);
Same comments with the free call.
>
> /**
> - * Return absolute timeout tick
> + * Get metadata from system timeout buffer
> *
> - * @param tmo Timeout buffer handle
> + * @param buf A timeout buffer
> + * @param hdl NULL or a pointer where the timer handle will be written.
> + * @param exp_tck NULL or a pointer where the expiration tick will be
> written.
> + * @param user_ptr NULL or a pointer where the user pointer will be
> written.
> *
> - * @return Absolute timeout tick
> + * @return True (1) if timeout buffer is of type ODP_BUFFER_TYPE_TIMEOUT
> and
> + * metadata variables have been updated from the timeout.
> + * False (0) if timeout buffer is not of type
> ODP_BUFFER_TYPE_TIMEOUT.
> */
> -uint64_t odp_timeout_tick(odp_timeout_t tmo);
> +int odp_timer_tmo_metadata(odp_buffer_t buf,
> + odp_timer_t *hdl,
> + uint64_t *exp_tck,
> + void **user_ptr);
Normal odp_buffer_type() call should be used to check the buffer type first.
This generic metadata call should be broke into separate calls just like packet
API does.
odp_timer_t odp_timeout_timer(tmo)
uint64_t odp_timeout_tick(tmo)
void* odp_timeout_user_ptr(tmo)
-Petri
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
