Timer API first draft

Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
---
 include/odp_timer.h                           | 139 ++++++++++++++++++++++++++
 platform/linux-generic/Makefile               |   1 +
 platform/linux-generic/include/odp_internal.h |   3 +
 platform/linux-generic/source/odp_init.c      |   5 +
 platform/linux-generic/source/odp_timer.c     |  14 +++
 5 files changed, 162 insertions(+)
 create mode 100644 include/odp_timer.h
 create mode 100644 platform/linux-generic/source/odp_timer.c

On Mon, Mar 10, 2014 at 01:35:37PM +0000, Petri Savolainen wrote:

This needs a commit log.

It also could do with some wordage to give context to the patch. What
are the differences between this and the proposal that we already have,
and why those differences are needed?

--
Stuart.

> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
> ---
>  include/odp_timer.h                           | 139 ++++++++++++++++++++++++++
>  platform/linux-generic/Makefile               |   1 +
>  platform/linux-generic/include/odp_internal.h |   3 +
>  platform/linux-generic/source/odp_init.c      |   5 +
>  platform/linux-generic/source/odp_timer.c     |  14 +++
>  5 files changed, 162 insertions(+)
>  create mode 100644 include/odp_timer.h
>  create mode 100644 platform/linux-generic/source/odp_timer.c
> 
> diff --git a/include/odp_timer.h b/include/odp_timer.h
> new file mode 100644
> index 0000000..ff54b8e
> --- /dev/null
> +++ b/include/odp_timer.h
> @@ -0,0 +1,139 @@
> +/* Copyright (c) 2013, Linaro Limited
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier:     BSD-3-Clause
> + */
> +
> +
> +/**
> + * @file
> + *
> + * ODP timer
> + */
> +
> +#ifndef ODP_TIMER_H_
> +#define ODP_TIMER_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <odp_std_types.h>
> +#include <odp_buffer.h>
> +#include <odp_buffer_pool.h>
> +#include <odp_queue.h>
> +
> +
> +/**
> +* ODP timer handle
> +*/
> +typedef uint32_t odp_timer_t;
> +
> +#define ODP_TIMER_INVALID 0
> +
> +
> +/**
> +* ODP timeout handle
> +*/
> +typedef uint32_t odp_timer_tmo_t;
> +
> +#define ODP_TIMER_TMO_INVALID 0
> +
> +
> +/**
> + * Create a timer
> + *
> + * Creates a new timer with requested properties.
> + *
> + * @param name       Name
> + * @param pool       Buffer pool for allocating timeout notifications
> + * @param resolution Timeout resolution in nanoseconds
> + * @param min_tmo    Minimum timeout duration in nanoseconds
> + * @param max_tmo    Maximum timeout duration in nanoseconds
> + *
> + * @return Timer handle if successful, otherwise ODP_TIMER_INVALID
> + */
> +odp_timer_t odp_timer_create(const char *name, odp_buffer_pool_t pool,
> +			     uint64_t resolution, uint64_t min_tmo,
> +			     uint64_t max_tmo);
> +
> +/**
> + * Convert timer ticks to nanoseconds
> + *
> + * @param timer Timer
> + * @param ticks Timer ticks
> + *
> + * @return Nanoseconds
> + */
> +uint64_t odp_timer_tick_to_ns(odp_timer_t timer, uint64_t ticks);
> +
> +/**
> + * Convert nanoseconds to timer ticks
> + *
> + * @param timer Timer
> + * @param ns    Nanoseconds
> + *
> + * @return Timer ticks
> + */
> +uint64_t odp_timer_ns_to_tick(odp_timer_t timer, uint64_t ns);
> +
> +/**
> + * Timer resolution in nanoseconds
> + *
> + * @param timer Timer
> + *
> + * @return Resolution in nanoseconds
> + */
> +uint64_t odp_timer_resolution(odp_timer_t timer);
> +
> +/**
> + * Maximum timeout in timer ticks
> + *
> + * @param timer Timer
> + *
> + * @return Maximum timeout in timer ticks
> + */
> +uint64_t odp_timer_maximum_tmo(odp_timer_t timer);
> +
> +/**
> + * Current timer tick
> + *
> + * @param timer Timer
> + *
> + * @return Current time in timer ticks
> + */
> +uint64_t odp_timer_current_tick(odp_timer_t timer);
> +
> +/**
> + * Request timeout with an absolute timer tick
> + *
> + * When tick reaches tmo_tick, the timer enqueues the timeout notification into
> + * the destination queue.
> + *
> + * @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.
> + *
> + * @return Timeout handle if successful, otherwise ODP_TIMER_TMO_INVALID
> + */
> +odp_timer_tmo_t odp_timer_absolute_tmo(odp_timer_t timer, uint64_t tmo_tick,
> +				       odp_queue_t queue, odp_buffer_t buf);
> +
> +/**
> + * Cancel a timeout
> + *
> + * @param timer Timer
> + * @param tmo   Timeout to cancel
> + *
> + * @return 0 if successful
> + */
> +int odp_timer_cancel_tmo(odp_timer_t timer, odp_timer_tmo_t tmo);
> +
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> diff --git a/platform/linux-generic/Makefile b/platform/linux-generic/Makefile
> index c35eb07..9f3e3db 100644
> --- a/platform/linux-generic/Makefile
> +++ b/platform/linux-generic/Makefile
> @@ -65,6 +65,7 @@ OBJS    += $(OBJ_DIR)/odp_system_info.o
>  OBJS    += $(OBJ_DIR)/odp_thread.o
>  OBJS    += $(OBJ_DIR)/odp_ticketlock.o
>  OBJS    += $(OBJ_DIR)/odp_time.o
> +OBJS    += $(OBJ_DIR)/odp_timer.o
>  OBJS    += $(OBJ_DIR)/odp_ring.o
>  OBJS    += $(OBJ_DIR)/odp_rwlock.o
>  ifeq ($(ODP_HAVE_NETMAP),yes)
> diff --git a/platform/linux-generic/include/odp_internal.h b/platform/linux-generic/include/odp_internal.h
> index 1f42d2a..ff34b5e 100644
> --- a/platform/linux-generic/include/odp_internal.h
> +++ b/platform/linux-generic/include/odp_internal.h
> @@ -43,6 +43,9 @@ int odp_schedule_init_global(void);
>  int odp_schedule_init_local(void);
>  
>  
> +int odp_timer_init_global(void);
> +
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/platform/linux-generic/source/odp_init.c b/platform/linux-generic/source/odp_init.c
> index 1d9cccd..d4c2eb8 100644
> --- a/platform/linux-generic/source/odp_init.c
> +++ b/platform/linux-generic/source/odp_init.c
> @@ -40,6 +40,11 @@ int odp_init_global(void)
>  		return -1;
>  	}
>  
> +	if (odp_timer_init_global()) {
> +		ODP_ERR("ODP timer init failed.\n");
> +		return -1;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/platform/linux-generic/source/odp_timer.c b/platform/linux-generic/source/odp_timer.c
> new file mode 100644
> index 0000000..b090257
> --- /dev/null
> +++ b/platform/linux-generic/source/odp_timer.c
> @@ -0,0 +1,14 @@
> +/* Copyright (c) 2013, Linaro Limited
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier:     BSD-3-Clause
> + */
> +
> +#include <odp_timer.h>
> +#include <odp_internal.h>
> +
> +
> +int odp_timer_init_global(void)
> +{
> +	return 0;
> +}
> -- 
> 1.8.5.3
>

Hi,

This is the minimum requirement for timer API from my point of view. I did 
go it through with Santosh in Macau (pity that Leo wasn't there also). The 
main difference to Leo's proposal is timers vs timeouts. E.g. an 
application could use two timers - one for high and one for low resolution 
(but long duration) timeouts. This way timer implementation can be 
optimized (configured in creation) towards applications intended usage. 
Timers are few (timer == a HW timer) and timeouts are many (e.g. a timeout 
per flow (upto millions)).

Error codes, etc may be added on the way. This definition would be a 
starting point for Santosh and others implementing it.

-Petri

On 10 March 2014 19:05, Petri Savolainen <petri.savolainen@linaro.org> wrote:
> Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>
> ---
>  include/odp_timer.h                           | 139 ++++++++++++++++++++++++++
>  platform/linux-generic/Makefile               |   1 +
>  platform/linux-generic/include/odp_internal.h |   3 +
>  platform/linux-generic/source/odp_init.c      |   5 +
>  platform/linux-generic/source/odp_timer.c     |  14 +++
>  5 files changed, 162 insertions(+)
>  create mode 100644 include/odp_timer.h
>  create mode 100644 platform/linux-generic/source/odp_timer.c
>
> diff --git a/include/odp_timer.h b/include/odp_timer.h
> new file mode 100644
> index 0000000..ff54b8e
> --- /dev/null
> +++ b/include/odp_timer.h
> @@ -0,0 +1,139 @@
> +/* Copyright (c) 2013, Linaro Limited
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier:     BSD-3-Clause
> + */
> +
> +
> +/**
> + * @file
> + *
> + * ODP timer
> + */
> +
> +#ifndef ODP_TIMER_H_
> +#define ODP_TIMER_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <odp_std_types.h>
> +#include <odp_buffer.h>
> +#include <odp_buffer_pool.h>
> +#include <odp_queue.h>
> +
> +
> +/**
> +* ODP timer handle
> +*/
> +typedef uint32_t odp_timer_t;
> +
> +#define ODP_TIMER_INVALID 0
> +
> +
> +/**
> +* ODP timeout handle
> +*/
> +typedef uint32_t odp_timer_tmo_t;
> +
> +#define ODP_TIMER_TMO_INVALID 0
> +
> +
> +/**
> + * Create a timer
> + *
> + * Creates a new timer with requested properties.
> + *
> + * @param name       Name
> + * @param pool       Buffer pool for allocating timeout notifications

Make sense to me.

> + * @param resolution Timeout resolution in nanoseconds

Is @param resolution == timer arming?? if so, better we keep them as a
separate API something like odp_timer_arm, with extra helper api
odp_timer_is_arm()..

Or I completely misunderstood. Please explain.

> + * @param min_tmo    Minimum timeout duration in nanoseconds
> + * @param max_tmo    Maximum timeout duration in nanoseconds
> + *
Isn't it something similar to toofar and toonear concept proposed by
Leo.. whats a difference? if not then whats use-case?

> + * @return Timer handle if successful, otherwise ODP_TIMER_INVALID
> + */
> +odp_timer_t odp_timer_create(const char *name, odp_buffer_pool_t pool,
> +                            uint64_t resolution, uint64_t min_tmo,
> +                            uint64_t max_tmo);
> +
> +/**
> + * Convert timer ticks to nanoseconds
> + *
> + * @param timer Timer
> + * @param ticks Timer ticks
> + *
> + * @return Nanoseconds
> + */
> +uint64_t odp_timer_tick_to_ns(odp_timer_t timer, uint64_t ticks);
> +
> +/**
> + * Convert nanoseconds to timer ticks
> + *
> + * @param timer Timer
> + * @param ns    Nanoseconds
> + *
> + * @return Timer ticks
> + */
> +uint64_t odp_timer_ns_to_tick(odp_timer_t timer, uint64_t ns);
> +
> +/**
> + * Timer resolution in nanoseconds
> + *
> + * @param timer Timer
> + *
> + * @return Resolution in nanoseconds
> + */
> +uint64_t odp_timer_resolution(odp_timer_t timer);
> +
> +/**
> + * Maximum timeout in timer ticks
> + *
> + * @param timer Timer
> + *
> + * @return Maximum timeout in timer ticks
> + */
> +uint64_t odp_timer_maximum_tmo(odp_timer_t timer);
> +
is it a helper api or related to specific use case?

> +/**
> + * Current timer tick
> + *
> + * @param timer Timer
> + *
> + * @return Current time in timer ticks
> + */
> +uint64_t odp_timer_current_tick(odp_timer_t timer);
> +
> +/**
> + * Request timeout with an absolute timer tick
> + *
> + * When tick reaches tmo_tick, the timer enqueues the timeout notification into
> + * the destination queue.
> + *
> + * @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.
> + *
> + * @return Timeout handle if successful, otherwise ODP_TIMER_TMO_INVALID
> + */
> +odp_timer_tmo_t odp_timer_absolute_tmo(odp_timer_t timer, uint64_t tmo_tick,
> +                                      odp_queue_t queue, odp_buffer_t buf);
> +
Make sense to me per odp api model though need some clarification that
would help me in implementation -

Is it a timeout handler, if so then why we need explicit param
"tmo_tick", timer armed tick value good enough to signal timer
expiration event.. right..

Or, You are considering something like time_arm loaded for pretty high
value and with this api use can generate many timer expiration event
based on tmo_tick.. where tmo_tick < timer_arm_value.. am I correct?

> +/**
> + * Cancel a timeout
> + *
> + * @param timer Timer
> + * @param tmo   Timeout to cancel
> + *
> + * @return 0 if successful
> + */
> +int odp_timer_cancel_tmo(odp_timer_t timer, odp_timer_tmo_t tmo);
> +
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> diff --git a/platform/linux-generic/Makefile b/platform/linux-generic/Makefile
> index c35eb07..9f3e3db 100644
> --- a/platform/linux-generic/Makefile
> +++ b/platform/linux-generic/Makefile
> @@ -65,6 +65,7 @@ OBJS    += $(OBJ_DIR)/odp_system_info.o
>  OBJS    += $(OBJ_DIR)/odp_thread.o
>  OBJS    += $(OBJ_DIR)/odp_ticketlock.o
>  OBJS    += $(OBJ_DIR)/odp_time.o
> +OBJS    += $(OBJ_DIR)/odp_timer.o
>  OBJS    += $(OBJ_DIR)/odp_ring.o
>  OBJS    += $(OBJ_DIR)/odp_rwlock.o
>  ifeq ($(ODP_HAVE_NETMAP),yes)
> diff --git a/platform/linux-generic/include/odp_internal.h b/platform/linux-generic/include/odp_internal.h
> index 1f42d2a..ff34b5e 100644
> --- a/platform/linux-generic/include/odp_internal.h
> +++ b/platform/linux-generic/include/odp_internal.h
> @@ -43,6 +43,9 @@ int odp_schedule_init_global(void);
>  int odp_schedule_init_local(void);
>
>
> +int odp_timer_init_global(void);
> +
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/platform/linux-generic/source/odp_init.c b/platform/linux-generic/source/odp_init.c
> index 1d9cccd..d4c2eb8 100644
> --- a/platform/linux-generic/source/odp_init.c
> +++ b/platform/linux-generic/source/odp_init.c
> @@ -40,6 +40,11 @@ int odp_init_global(void)
>                 return -1;
>         }
>
> +       if (odp_timer_init_global()) {
> +               ODP_ERR("ODP timer init failed.\n");
> +               return -1;
> +       }
> +
>         return 0;
>  }
>
> diff --git a/platform/linux-generic/source/odp_timer.c b/platform/linux-generic/source/odp_timer.c
> new file mode 100644
> index 0000000..b090257
> --- /dev/null
> +++ b/platform/linux-generic/source/odp_timer.c
> @@ -0,0 +1,14 @@
> +/* Copyright (c) 2013, Linaro Limited
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier:     BSD-3-Clause
> + */
> +
> +#include <odp_timer.h>
> +#include <odp_internal.h>
> +
> +
> +int odp_timer_init_global(void)
> +{
> +       return 0;
> +}
> --
> 1.8.5.3
>
> --
> You received this message because you are subscribed to the Google Groups "LNG ODP Sub-team - lng-odp@linaro.org" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to lng-odp+unsubscribe@linaro.org.
> To post to this group, send email to lng-odp@linaro.org.
> Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/.
> To view this discussion on the web visit https://groups.google.com/a/linaro.org/d/msgid/lng-odp/1394458537-7771-1-git-send-email-petri.savolainen%40linaro.org.
> For more options, visit https://groups.google.com/a/linaro.org/d/optout.

Petri,

I agree with the new top-level abstraction which separates a timer (which could be mapped to a hardware resource), and a timeout, which is an individual request of service. I think it makes perfect sense.

I do have some reservation with regards to some of the details:

+/**
+ * Create a timer
+ *
+ * Creates a new timer with requested properties.
+ *
+ * @param name       Name
+ * @param pool       Buffer pool for allocating timeout notifications
+ * @param resolution Timeout resolution in nanoseconds
+ * @param min_tmo    Minimum timeout duration in nanoseconds
+ * @param max_tmo    Maximum timeout duration in nanoseconds
+ *
+ * @return Timer handle if successful, otherwise ODP_TIMER_INVALID
+ */
+odp_timer_t odp_timer_create(const char *name, odp_buffer_pool_t pool,
+                            uint64_t resolution, uint64_t min_tmo,
+                            uint64_t max_tmo);

I think this collection of requirements of a timer implementation is casting a net that is too wide, or in other words tries to pull the rope from both ends. Specifically, if a hardware timer implements N bits in a register, the resolution would be the number of nanoseconds that the timer increments by 1 (which is here termed as "timer tick"). This particular timer would wrap-around every 2^N nanoseconds, which would be the maximum interval. Oh the other end, the function prototype seems to be suggesting that the application may request an arbitrary maximum interval, possibly larger than the wrap-around interval, which will be impossible to satisfy without a significant software portion in the implementation.

In other words, of the three parameters: 'resolution', 'min_tmo', 'max_tmo', I think you need to chose _one_ to remain part of the timer spec, and leave the other two for the implementation do derive and report via the 'get' functions.


+/**
+ * Current timer tick
+ *
+ * @param timer Timer
+ *
+ * @return Current time in timer ticks
+ */
+uint64_t odp_timer_current_tick(odp_timer_t timer);
+

I assume that the intention here is to have the timer report a tick counter that only wraps around every N^64 ticks.

This function requires the timer implementation to report current time (in timer resolution ticks) as a 64-bit counter. If the hardware implements only N<64 bits, this again means that the implementation must add a non-trivial software layer, which will be invoked at least once every 2^N ticks, which assumes there is a way to generate an interrupt from the timer that can be associated with the timer implementation internal code, and is not visible to the application.  But if the timer does not generate an interrupt, and instead en-queues an event buffer to a hardware queue, which is mapped so that the entire 'timeout' delivery is implemented in hardware, the timer implementation software would not have a means to insert itself into the timer wrap-around occurrence.

In my original proposal, the interface supports timers with any number of bits, and allows it to wrap-around without affecting the application. Under my proposal, if the application needs to use the timer to track absolute time, all it needs to do is create a single timeout with an interval close to the available maximum, and when that timeout expires, increment a software "time base" variable with the number of ticks that the long timeout actually took to expire, and use that 'time base' to keep track of absolute time with any number of bits it requires.

+/**
+ * Request timeout with an absolute timer tick
+ *
+ * When tick reaches tmo_tick, the timer enqueues the timeout notification into
+ * the destination queue.
+ *
+ * @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.
+ *
+ * @return Timeout handle if successful, otherwise ODP_TIMER_TMO_INVALID
+ */
+odp_timer_tmo_t odp_timer_absolute_tmo(odp_timer_t timer, uint64_t tmo_tick,
+                                      odp_queue_t queue, odp_buffer_t buf);
+

This function's absolute timeout interval raises similar concerns as the prior function, as it requires the timer to keep track of absolute time, which may require a non-trivial software addition to the timer implementation.

- Leo

Hi,

On Monday, 10 March 2014 21:17:49 UTC+2, Rosenboim, Leonid wrote:

> Petri,
>
> I agree with the new top-level abstraction which separates a timer (which 
> could be mapped to a hardware resource), and a timeout, which is an 
> individual request of service. I think it makes perfect sense.
>
> I do have some reservation with regards to some of the details:
>
> +/**
> + * Create a timer
> + *
> + * Creates a new timer with requested properties.
> + *
> + * @param name       Name
> + * @param pool       Buffer pool for allocating timeout notifications
> + * @param resolution Timeout resolution in nanoseconds
> + * @param min_tmo    Minimum timeout duration in nanoseconds
> + * @param max_tmo    Maximum timeout duration in nanoseconds
> + *
> + * @return Timer handle if successful, otherwise ODP_TIMER_INVALID
> + */
> +odp_timer_t odp_timer_create(const char *name, odp_buffer_pool_t pool,
> +                            uint64_t resolution, uint64_t min_tmo,
> +                            uint64_t max_tmo);
>
> I think this collection of requirements of a timer implementation is 
> casting a net that is too wide, or in other words tries to pull the rope 
> from both ends. Specifically, if a hardware timer implements N bits in a 
> register, the resolution would be the number of nanoseconds that the timer 
> increments by 1 (which is here termed as "timer tick").
>
Resolution defines the maximum interval between two ticks, ticks can also 
have shorter interval (better resolution). From the three, resolution is 
more important to application than max_tmo, because it can cascade timeouts 
(instead of a single long timeout) but it cannot do anything to improve 
resolution. 
 

> This particular timer would wrap-around every 2^N nanoseconds, which would 
> be the maximum interval. Oh the other end, the function prototype seems to 
> be suggesting that the application may request an arbitrary maximum 
> interval, possibly larger than the wrap-around interval, which will be 
> impossible to satisfy without a significant software portion in the 
> implementation. 
>
In other words, of the three parameters: 'resolution', 'min_tmo', 
> 'max_tmo', I think you need to chose _one_ to remain part of the timer 
> spec, and leave the other two for the implementation do derive and report 
> via the 'get' functions.
>
>
> Max_tmo is resolution*max_ticks. Typically max_ticks depends on timer 
memory usage (more ticks => larger rings etc => more memory). Max tmo can 
be used to optimize timer memory usage. You are correct max_tmo should be 
hint for timer implementation, not a strict requirement. Application should 
check what's the max tmo after timer has been created. Min is also a hint 
to optimize (e.g. application may need micro sec level resolution, but ask 
millisecond level timeouts). 


 

> +/**
> + * Current timer tick
> + *
> + * @param timer Timer
> + *
> + * @return Current time in timer ticks
> + */
> +uint64_t odp_timer_current_tick(odp_timer_t timer);
> +
>
> I assume that the intention here is to have the timer report a tick 
> counter that only wraps around every N^64 ticks.
>
> This function requires the timer implementation to report current time (in 
> timer resolution ticks) as a 64-bit counter. If the hardware implements 
> only N<64 bits, this again means that the implementation must add a 
> non-trivial software layer, which will be invoked at least once every 2^N 
> ticks, which assumes there is a way to generate an interrupt from the timer 
> that can be associated with the timer implementation internal code, and is 
> not visible to the application.  But if the timer does not generate an 
> interrupt, and instead en-queues an event buffer to a hardware queue, which 
> is mapped so that the entire 'timeout' delivery is implemented in hardware, 
> the timer implementation software would not have a means to insert itself 
> into the timer wrap-around occurrence.
>
Tick runs with timer resolution (e.g. increments every 1us). Current tick 
is the current time in ticks. Timeouts are requested in ticks and HW 
wheels/or rings run in tick resolution.
 

> In my original proposal, the interface supports timers with any number of 
> bits, and allows it to wrap-around without affecting the application. Under 
> my proposal, if the application needs to use the timer to track absolute 
> time, all it needs to do is create a single timeout with an interval close 
> to the available maximum, and when that timeout expires, increment a 
> software "time base" variable with the number of ticks that the long 
> timeout actually took to expire, and use that 'time base' to keep track of 
> absolute time with any number of bits it requires.
>
Also in your design SW needs to understand current time in order to insert 
the timeout request into right slot in HW wheel, etc, right?. The current 
slot defines the current time (tick). 

 

> +/**
> + * Request timeout with an absolute timer tick
> + *
> + * When tick reaches tmo_tick, the timer enqueues the timeout 
> notification into
> + * the destination queue.
> + *
> + * @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.
> + *
> + * @return Timeout handle if successful, otherwise ODP_TIMER_TMO_INVALID
> + */
> +odp_timer_tmo_t odp_timer_absolute_tmo(odp_timer_t timer, uint64_t 
> tmo_tick,
> +                                      odp_queue_t queue, odp_buffer_t 
> buf);
> +
>
> This function's absolute timeout interval raises similar concerns as the 
> prior function, as it requires the timer to keep track of absolute time, 
> which may require a non-trivial software addition to the timer 
> implementation.
>
Absolute is needed to have both low jitter/skew and overload protection. 
Also relative (more work to compensate jitter/skew) and periodic (dangerous 
in overload) timeouts can be provided also.
 
-Petri

One major problem with this API is that I don't see how resources are
allocates in advance (i.e. when a flow is set up) so that we can avoid
errors (e.g. out of resource) while processing packets for existing flows
(requesting/resetting/cancelling timers). I desperately want to avoid all
types of fatal errors after a flow has been set up. Not being able to
request/reset a timer for an existing flow is fatal for that flow, if we
can't supervise it or properly do retransmissions etc we will have to tear
it down. Not being able to transmit a packet is not a fatal error, packet
loss can happen anywhere and anytime, increase some suitable counter if
this happens and is noticed (by HW or SW).

The consequence of this is that the needs to be a call for reserving timer
resources (called during flow setup) which may fail and return an error (if
this happens, the application might decide not to set up the flow and
report this to the control plane). And the calls for arming, disarming and
resetting timers must not fail (all necessary resources preallocated), no
need for return values and error codes. Calling them with an invalid timer
is a programming error and should crash the process (e.g. call abort()).
Calling them with a "too early" timeout is not an error, the timer should
just expire immediately. If the call returns, it has succeeded. Just like
send() in OSE!

To cater for different implementations, I *require* a reset function which
just moves (a possibly expired) timer/timeout into the future. Requiring
separate cancel and request calls will often be suboptimal. A data
structure commonly used to implement priority queues (which is what we
might want to use for a SW implementation) is the heap and just moving an
existing node instead of removing and re-inserting it will likely cut the
overhead in half. Any implementation that does not benefit from the reset
operation can just call the individual cancel and request operations from
it.

> +uint64_t odp_timer_resolution(odp_timer_t timer);
I think it is better to specify timer resolution as frequency (Hz).
Frequencies go up all the time, this makes second-based metrics less and
less accurate.

For the original use case (e.g. flow supervision and retransmission, timers
mostly reset and seldom expire), we don't strictly need to use absolute
time as a little drift is not a fatal problem.
For the second use case (driving packet scheduling, timers always expire),
drift must be avoided and I think we should use absolute time.
Conclusion, we should have two different timer API's, focused on their
respective use cases.

create timer, remove timer - these calls allocate and free resources needed
for the timer, create timer returns a status/error code.
request timeout, cancel timeout, reset timeout - these calls operate on an
existing timer and do not allocate any new resources, no return codes (if
the call returns it succeeded), cancel and reset timeout can operate on
both active (armed) and expired timeouts as the application (making these
calls) and the timer implementation (e.g. HW or SW on another core) may
execute asynchronously and thus create race conditions.



On 11 March 2014 16:19, Petri Savolainen <petri.savolainen@linaro.org>wrote:

> Hi,
>
>
> On Monday, 10 March 2014 21:17:49 UTC+2, Rosenboim, Leonid wrote:
>
>> Petri,
>>
>> I agree with the new top-level abstraction which separates a timer (which
>> could be mapped to a hardware resource), and a timeout, which is an
>> individual request of service. I think it makes perfect sense.
>>
>> I do have some reservation with regards to some of the details:
>>
>> +/**
>> + * Create a timer
>> + *
>> + * Creates a new timer with requested properties.
>> + *
>> + * @param name       Name
>> + * @param pool       Buffer pool for allocating timeout notifications
>> + * @param resolution Timeout resolution in nanoseconds
>> + * @param min_tmo    Minimum timeout duration in nanoseconds
>> + * @param max_tmo    Maximum timeout duration in nanoseconds
>> + *
>> + * @return Timer handle if successful, otherwise ODP_TIMER_INVALID
>> + */
>> +odp_timer_t odp_timer_create(const char *name, odp_buffer_pool_t pool,
>> +                            uint64_t resolution, uint64_t min_tmo,
>> +                            uint64_t max_tmo);
>>
>> I think this collection of requirements of a timer implementation is
>> casting a net that is too wide, or in other words tries to pull the rope
>> from both ends. Specifically, if a hardware timer implements N bits in a
>> register, the resolution would be the number of nanoseconds that the timer
>> increments by 1 (which is here termed as "timer tick").
>>
> Resolution defines the maximum interval between two ticks, ticks can also
> have shorter interval (better resolution). From the three, resolution is
> more important to application than max_tmo, because it can cascade timeouts
> (instead of a single long timeout) but it cannot do anything to improve
> resolution.
>
>
>> This particular timer would wrap-around every 2^N nanoseconds, which
>> would be the maximum interval. Oh the other end, the function prototype
>> seems to be suggesting that the application may request an arbitrary
>> maximum interval, possibly larger than the wrap-around interval, which will
>> be impossible to satisfy without a significant software portion in the
>> implementation.
>>
> In other words, of the three parameters: 'resolution', 'min_tmo',
>> 'max_tmo', I think you need to chose _one_ to remain part of the timer
>> spec, and leave the other two for the implementation do derive and report
>> via the 'get' functions.
>>
>>
>> Max_tmo is resolution*max_ticks. Typically max_ticks depends on timer
> memory usage (more ticks => larger rings etc => more memory). Max tmo can
> be used to optimize timer memory usage. You are correct max_tmo should be
> hint for timer implementation, not a strict requirement. Application should
> check what's the max tmo after timer has been created. Min is also a hint
> to optimize (e.g. application may need micro sec level resolution, but ask
> millisecond level timeouts).
>
>
>
>
>> +/**
>> + * Current timer tick
>> + *
>> + * @param timer Timer
>> + *
>> + * @return Current time in timer ticks
>> + */
>> +uint64_t odp_timer_current_tick(odp_timer_t timer);
>> +
>>
>> I assume that the intention here is to have the timer report a tick
>> counter that only wraps around every N^64 ticks.
>>
>> This function requires the timer implementation to report current time
>> (in timer resolution ticks) as a 64-bit counter. If the hardware implements
>> only N<64 bits, this again means that the implementation must add a
>> non-trivial software layer, which will be invoked at least once every 2^N
>> ticks, which assumes there is a way to generate an interrupt from the timer
>> that can be associated with the timer implementation internal code, and is
>> not visible to the application.  But if the timer does not generate an
>> interrupt, and instead en-queues an event buffer to a hardware queue, which
>> is mapped so that the entire 'timeout' delivery is implemented in hardware,
>> the timer implementation software would not have a means to insert itself
>> into the timer wrap-around occurrence.
>>
> Tick runs with timer resolution (e.g. increments every 1us). Current tick
> is the current time in ticks. Timeouts are requested in ticks and HW
> wheels/or rings run in tick resolution.
>
>
>> In my original proposal, the interface supports timers with any number of
>> bits, and allows it to wrap-around without affecting the application. Under
>> my proposal, if the application needs to use the timer to track absolute
>> time, all it needs to do is create a single timeout with an interval close
>> to the available maximum, and when that timeout expires, increment a
>> software "time base" variable with the number of ticks that the long
>> timeout actually took to expire, and use that 'time base' to keep track of
>> absolute time with any number of bits it requires.
>>
> Also in your design SW needs to understand current time in order to insert
> the timeout request into right slot in HW wheel, etc, right?. The current
> slot defines the current time (tick).
>
>
>
>> +/**
>> + * Request timeout with an absolute timer tick
>> + *
>> + * When tick reaches tmo_tick, the timer enqueues the timeout
>> notification into
>> + * the destination queue.
>> + *
>> + * @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.
>> + *
>> + * @return Timeout handle if successful, otherwise ODP_TIMER_TMO_INVALID
>> + */
>> +odp_timer_tmo_t odp_timer_absolute_tmo(odp_timer_t timer, uint64_t
>> tmo_tick,
>> +                                      odp_queue_t queue, odp_buffer_t
>> buf);
>> +
>>
>> This function's absolute timeout interval raises similar concerns as the
>> prior function, as it requires the timer to keep track of absolute time,
>> which may require a non-trivial software addition to the timer
>> implementation.
>>
> Absolute is needed to have both low jitter/skew and overload protection.
> Also relative (more work to compensate jitter/skew) and periodic (dangerous
> in overload) timeouts can be provided also.
>
> -Petri
>
>  --
> You received this message because you are subscribed to the Google Groups
> "LNG ODP Sub-team - lng-odp@linaro.org" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to lng-odp+unsubscribe@linaro.org.
> To post to this group, send email to lng-odp@linaro.org.
> Visit this group at http://groups.google.com/a/linaro.org/group/lng-odp/.
> To view this discussion on the web visit
> https://groups.google.com/a/linaro.org/d/msgid/lng-odp/205895d0-6624-4f51-b587-4e0ca01d20b3%40linaro.org<https://groups.google.com/a/linaro.org/d/msgid/lng-odp/205895d0-6624-4f51-b587-4e0ca01d20b3%40linaro.org?utm_medium=email&utm_source=footer>
> .
>
> For more options, visit https://groups.google.com/a/linaro.org/d/optout.
>