From eda4c027dadf8a8e62da1342e855aff91285a3ab Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Mon, 28 Jan 2019 09:35:27 -0800 Subject: [PATCH] misc/dlist: Swap insertion API for a faster one The sys_dlist_insert_*() functions had a behavior where a NULL argument for the insertion position to sys_dlist_insert_after/before() was interpreted as "the end of the list". We never used that convention (except in one spot internal to dlist.h which was not itself used anywhere), and of course already have an API for appending and prepending to a list. In practice this was a performance disaster. The NULL check is virtually never provable statically by the compiler, so that test and branch is present always. And worse, the check and call to another function was pushing this beyond the complexity limit for gcc to inline a function (at -Os optimization anyway), forcing us to use function calls for what should be a ~8 instruction sequence. The upshot is that dlist insertions were 2-3x slower than they needed to be. Deprecate these older APIs and introduce a new sys_dlist_insert() call which can be much better optimized. Signed-off-by: Andy Ross --- include/misc/dlist.h | 43 +++++++++++++++------------------ kernel/poll.c | 3 +-- kernel/sched.c | 4 +-- kernel/timeout.c | 3 +-- tests/kernel/common/src/dlist.c | 5 ++-- 5 files changed, 25 insertions(+), 33 deletions(-) diff --git a/include/misc/dlist.h b/include/misc/dlist.h index ba0795233ed..e16a70d8dc1 100644 --- a/include/misc/dlist.h +++ b/include/misc/dlist.h @@ -22,6 +22,7 @@ #include #include +#include #ifdef __cplusplus extern "C" { @@ -429,19 +430,22 @@ static inline void sys_dlist_prepend(sys_dlist_t *list, sys_dnode_t *node) } /** - * @brief insert node after a node + * @brief Insert a node into a list * - * Insert a node after a specified node in a list. - * This and other sys_dlist_*() functions are not thread safe. + * Insert a node before a specified node in a dlist. * - * @param list the doubly-linked list to operate on - * @param insert_point the insert point in the list: if NULL, insert at head - * @param node the element to append - * - * @return N/A + * @param successor the position before which "node" will be inserted + * @param node the element to insert */ +static inline void sys_dlist_insert(sys_dnode_t *successor, sys_dnode_t *node) +{ + node->prev = successor->prev; + node->next = successor; + successor->prev->next = node; + successor->prev = node; +} -static inline void sys_dlist_insert_after(sys_dlist_t *list, +static inline void __deprecated sys_dlist_insert_after(sys_dlist_t *list, sys_dnode_t *insert_point, sys_dnode_t *node) { if (insert_point == NULL) { @@ -454,20 +458,7 @@ static inline void sys_dlist_insert_after(sys_dlist_t *list, } } -/** - * @brief insert node before a node - * - * Insert a node before a specified node in a list. - * This and other sys_dlist_*() functions are not thread safe. - * - * @param list the doubly-linked list to operate on - * @param insert_point the insert point in the list: if NULL, insert at tail - * @param node the element to insert - * - * @return N/A - */ - -static inline void sys_dlist_insert_before(sys_dlist_t *list, +static inline void __deprecated sys_dlist_insert_before(sys_dlist_t *list, sys_dnode_t *insert_point, sys_dnode_t *node) { if (insert_point == NULL) { @@ -508,7 +499,11 @@ static inline void sys_dlist_insert_at(sys_dlist_t *list, sys_dnode_t *node, while (pos && !cond(pos, data)) { pos = sys_dlist_peek_next(list, pos); } - sys_dlist_insert_before(list, pos, node); + if (pos != NULL) { + sys_dlist_insert(pos, node); + } else { + sys_dlist_append(list, node); + } } } diff --git a/kernel/poll.c b/kernel/poll.c index 6c0dff797b0..ab260ef99c8 100644 --- a/kernel/poll.c +++ b/kernel/poll.c @@ -91,8 +91,7 @@ static inline void add_event(sys_dlist_t *events, struct k_poll_event *event, SYS_DLIST_FOR_EACH_CONTAINER(events, pending, _node) { if (_is_t1_higher_prio_than_t2(poller->thread, pending->poller->thread)) { - sys_dlist_insert_before(events, &pending->_node, - &event->_node); + sys_dlist_insert(&pending->_node, &event->_node); return; } } diff --git a/kernel/sched.c b/kernel/sched.c index e757624ee0e..6b959c168ca 100644 --- a/kernel/sched.c +++ b/kernel/sched.c @@ -573,8 +573,8 @@ void _priq_dumb_add(sys_dlist_t *pq, struct k_thread *thread) SYS_DLIST_FOR_EACH_CONTAINER(pq, t, base.qnode_dlist) { if (_is_t1_higher_prio_than_t2(thread, t)) { - sys_dlist_insert_before(pq, &t->base.qnode_dlist, - &thread->base.qnode_dlist); + sys_dlist_insert(&t->base.qnode_dlist, + &thread->base.qnode_dlist); return; } } diff --git a/kernel/timeout.c b/kernel/timeout.c index 25107a972e3..8654376ba0e 100644 --- a/kernel/timeout.c +++ b/kernel/timeout.c @@ -87,8 +87,7 @@ void _add_timeout(struct _timeout *to, _timeout_func_t fn, s32_t ticks) if (t->dticks > to->dticks) { t->dticks -= to->dticks; - sys_dlist_insert_before(&timeout_list, - &t->node, &to->node); + sys_dlist_insert(&t->node, &to->node); break; } to->dticks -= t->dticks; diff --git a/tests/kernel/common/src/dlist.c b/tests/kernel/common/src/dlist.c index 95ba0eaa98c..4c60ff528c5 100644 --- a/tests/kernel/common/src/dlist.c +++ b/tests/kernel/common/src/dlist.c @@ -170,7 +170,7 @@ static inline bool verify_tail_head(sys_dlist_t *list, * @brief Verify doubly linked list funtionalities * * @see sys_dlist_append(), sys_dlist_remove(), sys_dlist_prepend(), - * sys_dlist_remove(), sys_dlist_insert_after(), sys_dlist_peek_next() + * sys_dlist_remove(), sys_dlist_insert(), sys_dlist_peek_next() * SYS_DLIST_ITERATE_FROM_NODE() */ void test_dlist(void) @@ -238,8 +238,7 @@ void test_dlist(void) "test_list node links are wrong"); /* Inserting node 4 after node 2 */ - sys_dlist_insert_after(&test_list, &test_node_2.node, - &test_node_4.node); + sys_dlist_insert(test_node_2.node.next, &test_node_4.node); zassert_true((verify_tail_head(&test_list, &test_node_2.node, &test_node_3.node, false)),