Re: [RFCv2 1/2] misc: Add timer API


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 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
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 ?


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;
^^
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;
+
+struct sys_timer_queue {
+ nano_thread_id_t __thread_id;
+ struct sys_timer *__head;
^^^
I'm not sure why you use double-underscores in structure field names...?

+ size_t stack_size;
+ char stack[0];
+};
+
+typedef void (*sys_timer_func)(void *user_data);
+
+struct sys_timer {
+ struct nano_timer __nano_timer;
^^
also here

+ sys_timer_func func;
+ void *user_data;
+ struct sys_timer *__next;
^^
and here

[..snip..]

+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();
^^^^^^^^^^^^^^

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

+}
+
+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

Join devel@lists.zephyrproject.org to automatically receive all group messages.