Date
1 - 11 of 11
[RFCv2 1/2] misc: Add timer API
|
Luiz Augusto von Dentz
From: Luiz Augusto von Dentz <luiz.von.dentz(a)intel.com>
This adds a timer fiber which can be used track timeouts removing the need to use one delayed fiber per timeout. Change-Id: Id13347fbc69b1e83bca22094fbeb741e045ed516 Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz(a)intel.com> --- include/misc/timer.h | 106 ++++++++++++++++++++++++ misc/Kconfig | 37 +++++++++ misc/Makefile | 1 + misc/timer.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++++++ net/bluetooth/Kconfig | 1 + 5 files changed, 369 insertions(+) create mode 100644 include/misc/timer.h create mode 100644 misc/timer.c diff --git a/include/misc/timer.h b/include/misc/timer.h new file mode 100644 index 0000000..d3ce695 --- /dev/null +++ b/include/misc/timer.h @@ -0,0 +1,106 @@ +/** @file + * @brief System timer APIs. + */ + +/* + * Copyright (c) 2016 Intel Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +extern struct sys_timer_queue *__global_tq; + +struct sys_timer_queue { + nano_thread_id_t __thread_id; + struct sys_timer *__head; + size_t stack_size; + char stack[0]; +}; + +typedef void (*sys_timer_func)(void *user_data); + +struct sys_timer { + struct nano_timer __nano_timer; + sys_timer_func func; + void *user_data; + struct sys_timer *__next; +}; + +/** + * @brief Init timer queue + * + * @param tq Timer queue object that should be initialized + * @param stack_size The stack size in bytes + */ +int sys_timer_queue_init(struct sys_timer_queue *tq, size_t stack_size); + +/** + * @brief Add timer to a timer queue + * + * Both timer and timer queue must be initialized. + * + * @param tq Timer queue object to add the timer + * @param t Timer object to be added + * @param ticks Amount of time in ticks to wait + * + * @return Zero on success or (negative) error code otherwise. + */ +int sys_timer_queue_add(struct sys_timer_queue *tq, struct sys_timer *t, + int ticks); + +/** + * @brief Cancel timer + * + * Remove timer from timer queue and stop it so its callback is not called. + * + * @param tq Timer queue object where the timer was added + * @param t Timer object to be canceled + * + * @return Zero on success or (negative) error code otherwise. + */ +int sys_timer_queue_cancel(struct sys_timer_queue *tq, struct sys_timer *t); + +/** + * @brief Init timer + * + * @param t Timer object to be initialized + * @param func Function callback to be called when timer expires + * @param user_data Data to be passed in the callback + * + * @return Zero on success or (negative) error code otherwise. + */ +int sys_timer_init(struct sys_timer *t, sys_timer_func func, void *user_data); + +/** + * @brief Add timer to the global timer queue + * + * Timer must be initialized. + * + * @param _t Timer object to be added + * @param _ticks Amount of time in ticks to wait + * + * @return Zero on success or (negative) error code otherwise. + */ +#define sys_timer_add(_t, _ticks) sys_timer_queue_add(__global_tq, _t, _ticks) + +/** + * @brief Cancel global timer + * + * Remove timer from the global timer queue and stop it so its callback is not + * called. + * + * @param _t Timer object to be canceled + * + * @return Zero on success or (negative) error code otherwise. + */ +#define sys_timer_cancel(_t) sys_timer_queue_cancel(__global_tq, _t) diff --git a/misc/Kconfig b/misc/Kconfig index 4c05169..a9bd80f 100644 --- a/misc/Kconfig +++ b/misc/Kconfig @@ -374,3 +374,40 @@ config REBOOT needed to perform a "safe" reboot (e.g. SYSTEM_CLOCK_DISABLE, to stop the system clock before issuing a reset). endmenu + +menu "Timer Options" + +config SYS_TIMER + bool + prompt "Enable System Timer" + default n + select NANO_TIMEOUTS + select NANO_TIMERS + help + Global switch for enabling sys_timer API. + +config SYS_TIMER_STACK_SIZE + int "Size of the global timer fiber stack" + depends on SYS_TIMER + default 512 + range 512 65536 + help + Size of the global timer fiber stack. This is the context from + which all global timer callbacks occur. + +config SYS_LOG_TIMER_LEVEL + int + prompt "System Timer Log level" + depends on SYS_LOG && SYS_TIMER + default 0 + range 0 4 + help + Sets log level for System Timers. + Levels are: + 0 OFF, do not write + 1 ERROR, only write SYS_LOG_ERR + 2 WARNING, write SYS_LOG_WRN in adition to previous level + 3 INFO, write SYS_LOG_INF in adition to previous levels + 4 DEBUG, write SYS_LOG_DBG in adition to previous levels + +endmenu diff --git a/misc/Makefile b/misc/Makefile index cb9a718..d57b8f2 100644 --- a/misc/Makefile +++ b/misc/Makefile @@ -3,5 +3,6 @@ obj-$(CONFIG_CPLUSPLUS) += cpp_virtual.o cpp_vtable.o \ cpp_init_array.o cpp_ctors.o cpp_dtors.o obj-$(CONFIG_PRINTK) += printk.o obj-$(CONFIG_REBOOT) += reboot.o +obj-$(CONFIG_SYS_TIMER) += timer.o obj-y += generated/ obj-y += debug/ diff --git a/misc/timer.c b/misc/timer.c new file mode 100644 index 0000000..90bf196 --- /dev/null +++ b/misc/timer.c @@ -0,0 +1,224 @@ +/** @file + * @brief Internal APIs for Bluetooth timer handling. + */ + +/* + * Copyright (c) 2016 Intel Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <nanokernel.h> +#include <toolchain.h> +#include <sections.h> +#include <string.h> +#include <errno.h> +#include <misc/stack.h> +#include <misc/timer.h> + +#define SYS_LOG_DOMAIN "System Timer" +#define SYS_LOG_LEVEL CONFIG_SYS_LOG_TIMER_LEVEL +#include <misc/sys_log.h> + +#if CONFIG_SYS_LOG_TIMER_LEVEL > 0 +/* Enabling debug increases stack size requirement considerably */ +#define STACK_DEBUG_EXTRA 512 +#else +#define STACK_DEBUG_EXTRA 0 +#endif + +#define STACK(name, size) \ + char __stack name[(size) + STACK_DEBUG_EXTRA] + +struct timer_queue { + struct sys_timer_queue tq; + + STACK(stack, CONFIG_SYS_TIMER_STACK_SIZE); +}; + +static struct timer_queue global_tq = { + .tq.stack_size = CONFIG_SYS_TIMER_STACK_SIZE, +}; + +struct sys_timer_queue *__global_tq = &global_tq.tq; + +static void timer_expired(struct sys_timer *t) +{ + SYS_LOG_DBG("t %p", t); + + /* Just to be safe stop the timer */ + nano_fiber_timer_stop(&t->__nano_timer); + + t->func(t->user_data); +} + +static int32_t timer_queue_ticks_remains(struct sys_timer_queue *tq) +{ + int32_t ticks; + + stack_analyze("timer queue stack", tq->stack, tq->stack_size); + + while (tq->__head) { + struct sys_timer *cur; + + /* If head is not expired sleep the remaining ticks */ + ticks = nano_timer_ticks_remain(&tq->__head->__nano_timer); + if (ticks > 0) { + goto done; + } + + /* Remove head since it has expired */ + cur = tq->__head; + tq->__head = tq->__head->__next; + + /* Execute callback */ + timer_expired(cur); + } + + /* No timers left, abort fiber */ + ticks = 0; + +done: + SYS_LOG_DBG("tq %p ticks %d", tq, ticks); + + return ticks; +} + +static void timer_queue_fiber(struct sys_timer_queue *tq) +{ + int32_t ticks; + + SYS_LOG_DBG("tq %p thread_id %d started", tq, tq->__thread_id); + + while ((ticks = timer_queue_ticks_remains(tq))) { + fiber_sleep(ticks); + } + + SYS_LOG_DBG("tq %p thread_id %d stopped", tq, tq->__thread_id); + + tq->__thread_id = 0; + fiber_abort(); +} + +static int timer_queue_start(struct sys_timer_queue *tq) +{ + if (tq->__thread_id) { + return 0; + } + + tq->__thread_id = fiber_start(tq->stack, tq->stack_size, + (nano_fiber_entry_t)timer_queue_fiber, + (int) tq, 0, 7, 0); + + return 0; +} + +int sys_timer_queue_init(struct sys_timer_queue *tq, size_t stack_size) +{ + if (!tq || !stack_size) { + return -EINVAL; + } + + tq->stack_size = stack_size; + + return 0; +} + +static inline void timer_queue_wakeup(struct sys_timer_queue *tq) +{ + if (sys_thread_self_get() != tq->__thread_id) { + fiber_wakeup(tq->__thread_id); + } +} + +int sys_timer_queue_add(struct sys_timer_queue *tq, struct sys_timer *t, int ticks) +{ + struct sys_timer *cur, *prev; + + SYS_LOG_DBG("tq %p t %p ticks %d", tq, t, ticks); + + if (!t || !t->func) + return -EINVAL; + + timer_queue_start(tq); + nano_timer_start(&t->__nano_timer, ticks); + + /* Sort the list of timers */ + for (cur = tq->__head, prev = NULL; cur; + prev = cur, cur = cur->__next) { + if (ticks < nano_timer_ticks_remain(&cur->__nano_timer)) { + break; + } + } + + t->__next = cur; + + if (prev) { + prev->__next = t; + } else { + tq->__head = t; + /* Wakeup timer queue fiber since there is a new head */ + timer_queue_wakeup(tq); + } + + return 0; +} + +int sys_timer_queue_cancel(struct sys_timer_queue *tq, struct sys_timer *t) +{ + struct sys_timer *cur, *prev; + + SYS_LOG_DBG("tq %p t %p", tq, t); + + if (!t) { + return -EINVAL; + } + + /* Lookup existing timers */ + for (cur = tq->__head, prev = NULL; cur; + prev = cur, cur = cur->__next) { + if (cur == t) { + break; + } + } + + if (!cur) { + return -ENOENT; + } + + nano_timer_stop(&t->__nano_timer); + + /* Remove timer for the queue */ + if (prev) { + prev->__next = t->__next; + } else { + tq->__head = t->__next; + /* Wakeup timer queue fiber since there is a new head */ + timer_queue_wakeup(tq); + } + + return 0; +} + +int sys_timer_init(struct sys_timer *t, sys_timer_func func, void *user_data) +{ + if (!t || !func) { + return -EINVAL; + } + + t->func = func; + t->user_data = user_data; + nano_timer_init(&t->__nano_timer, t); + + return 0; +} diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig index 2ffb0e0..aa2712b 100644 --- a/net/bluetooth/Kconfig +++ b/net/bluetooth/Kconfig @@ -129,6 +129,7 @@ config BLUETOOTH_CENTRAL config BLUETOOTH_CONN bool + select SYS_TIMER default n if BLUETOOTH_CONN -- 2.5.5
|
|
|
|
Tomasz Bursztyka
Hi Luiz,
+Giving the pointer on current sys_timer instead you could: +remove that pointer, and up to user to use CONTAINER_OF() if he wants to. (yes I am reusing ideas from Johan ;) ) + struct sys_timer *__next;Are you ever going to use another queue besides the system's one (the global one)? It could simplify a lot the API then. Unless I miss to see some use case for custom queue +And continuing on that idea, is it then worth asking to which queue you want to add a timer? + * @param t Timer object to be addedso you could remove tq parameter. +here as well. +And these 2 defines would also go away. And struct sys_timer_queue could also be private. Tomasz
|
|
|
|
Luiz Augusto von Dentz
Hi Tomasz,
On Wed, Apr 20, 2016 at 12:05 PM, Tomasz Bursztyka <tomasz.bursztyka(a)linux.intel.com> wrote: Hi Luiz,Yep, this actually saves a pointer and in fact is something I thought+ about but for some reason did not implement, anyway I will change to make use of CONTAINER_OF. Well a private/custom one could be useful in case the stack needs to+ struct sys_timer *__next; different, and I was actually thinking to include a priority later on. But yes it could be simplified in the first version so later on we can check for actual use cases. + -- Luiz Augusto von Dentz
|
|
|
|
Benjamin Walsh <benjamin.walsh@...>
On Tue, Apr 19, 2016 at 05:26:11PM +0300, Luiz Augusto von Dentz wrote:
From: Luiz Augusto von Dentz <luiz.von.dentz(a)intel.com>This is a good functionality to add to the kernel I think. Two things though: 1. It should probably end up in the kernel directory: I don't think this is a 'misc' functionality. 2. I am not sure about the naming. This is yet another timer library, when we already have nano timers and micro timers. What makes it special is its callback functionality. Can we try to get that into the name ? ^^ We try not to use double-underscores. Single one is enough, but since this is a global symbol, it should have a more descriptive name. + size_t stack_size;^^^ I'm not sure why you use double-underscores in structure field names...? + size_t stack_size;^^ also here + sys_timer_func func;^^ and here [..snip..] +static void timer_queue_fiber(struct sys_timer_queue *tq)^^^^^^^^^^^^^^ Don't do this: it's done automatically by the kernel when the fiber entry point runs to completion. I think it looks good, but I should not be doing a code review on the mailing list. I'll do the full review in gerrit when it ends up there. Cheers, Ben +}
|
|
|
|
Luiz Augusto von Dentz
Hi Benjamin,
On Thu, Apr 21, 2016 at 5:32 PM, Benjamin Walsh <benjamin.walsh(a)windriver.com> wrote: On Tue, Apr 19, 2016 at 05:26:11PM +0300, Luiz Augusto von Dentz wrote:Yep, I asked about this but nobody seems to be interested at thatFrom: Luiz Augusto von Dentz <luiz.von.dentz(a)intel.com>This is a good functionality to add to the kernel I think. Two things point so I used misc to start with. 2. I am not sure about the naming. This is yet another timer library,Sure we can add a timeout or callback to the name, but IMO this would be much more useful than nano_timer thus why I would like to promote it over it. I understood from the code that __ are used for internal/private^^ stuch, such as in uint64_t __noinit __start_tsc;, well without this being stated in the coding style it is kind hard to to guess. Okay, I will update it shortly.+ size_t stack_size;^^^ I think it looks good, but I should not be doing a code review on theActually I find these type of review much more convenient than gerrit, anyway you can find them there as well: https://gerrit.zephyrproject.org/r/#/c/1593/ Cheers, -- Luiz Augusto von Dentz
|
|
|
|
Luiz Augusto von Dentz
Hi,
On Thu, Apr 21, 2016 at 6:51 PM, Luiz Augusto von Dentz <luiz.dentz(a)gmail.com> wrote: Hi Benjamin,Under kernel there is either nanokernel or microkernel, not sure where a sys_* API should go? It is actually really messy to have directly in nanokernel.h since it is all missed up with #ifdef in between, perhaps we should have a system.h for sys_ API? Since nobody is suggesting anything here are some alternatives:2. I am not sure about the naming. This is yet another timer library,Sure we can add a timeout or callback to the name, but IMO this would sys_timer_callback*: quite long imo sys_callback: doesn't say much sys_timeout: I would favor this one I understood from the code that __ are used for internal/private^^ -- Luiz Augusto von Dentz
|
|
|
|
Benjamin Walsh <benjamin.walsh@...>
On Thu, Apr 21, 2016 at 06:51:19PM +0300, Luiz Augusto von Dentz wrote:
The only things that should be using double-underscore are theI understood from the code that __ are used for internal/private+extern struct sys_timer_queue *__global_tq;^^ attributes macros (e.g. __weak for __attribute__((__weak)))). There are other cases, such as symbols from linker scripts, ARM and ARC exception handlers, maybe others, but these are legacy from the old codebase, and should be changed when we have the time.
|
|
|
|
Benjamin Walsh <benjamin.walsh@...>
On Fri, Apr 22, 2016 at 03:22:45PM +0300, Luiz Augusto von Dentz wrote:
Hi,It uses nanokernel timers, so I would put it under nanokernel. I would prefer sys_callback I think: you add a callback with a timeoutSince nobody is suggesting anything here are some alternatives:2. I am not sure about the naming. This is yet another timer library,Sure we can add a timeout or callback to the name, but IMO this would shows that there is a timeout/timer IMHO. int sys_callback_queue_init(struct sys_timer_queue *tq, size_t stack_size) int sys_callback_add(struct sys_timer_queue *tq, struct sys_timer *t, int ticks) int sys_callback_cancel(struct sys_timer_queue *tq, struct sys_timer *t) int sys_callback_timer_init(struct sys_timer *t, sys_timer_func func, void *user_data) ^^^^^^^^^^^^^^^^^^ Not sure why this is not part of sys_callback_queue_init though... Or, how about 'sys_callout' ? --I understood from the code that __ are used for internal/private^^ Benjamin Walsh, SMTS Wind River Rocket www.windriver.com Zephyr kernel maintainer www.zephyrproject.org
|
|
|
|
Tomasz Bursztyka
Hi Benjamin,
Me too.2. I am not sure about the naming. This is yet another timer library,Since nobody is suggesting anything here are some alternatives:Sure we can add a timeout or callback to the name, but IMO this wouldwhen we already have nano timers and micro timers. What makes it special I would prefer sys_callback I think: you add a callback with a timeoutA timeout says all without people to read parameters. At a timeout usually, it means you want to do something, thus the callback. It's not suprizing timeout keyword is usually preferred in some user-land libraries on Linux or else. Tomasz
|
|
|
|
Benjamin Walsh <benjamin.walsh@...>
On Mon, Apr 25, 2016 at 02:08:00PM +0200, Tomasz Bursztyka wrote:
Hi Benjamin,Don't you think it would be confusing for a user that we have two timerMe too.2. I am not sure about the naming. This is yet another timer library,Since nobody is suggesting anything here are some alternatives:Sure we can add a timeout or callback to the name, but IMO this wouldwhen we already have nano timers and micro timers. What makes it special APIs and a timeout API ?
|
|
|
|
Luiz Augusto von Dentz
Hi Ben,
On Mon, Apr 25, 2016 at 6:50 PM, Benjamin Walsh <benjamin.walsh(a)windriver.com> wrote: On Mon, Apr 25, 2016 at 02:08:00PM +0200, Tomasz Bursztyka wrote:Well that was what I mentioned that the use of nano_timer is veryHi Benjamin,Don't you think it would be confusing for a user that we have two timerMe too.2. I am not sure about the naming. This is yet another timer library,Since nobody is suggesting anything here are some alternatives:Sure we can add a timeout or callback to the name, but IMO this wouldwhen we already have nano timers and micro timers. What makes it special limited, if you look at the unit test you will quickly realize how hard it is to use in practice. That said Im in favor of push this quickly and then decide the name later since we do depend in this functionality to implement some protocol requirements in Bluetooth and probably IP will switch to use it as well. -- Luiz Augusto von Dentz
|
|
|