From 212fe157863695dd526be6320ddc84ce6ff5d392 Mon Sep 17 00:00:00 2001 From: "Martine S. Lenders" Date: Thu, 9 Apr 2020 23:21:42 +0200 Subject: [PATCH 1/3] xtimer: set now pointer correctly in _update_short_timers() loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes `xtimer` to use `xtimer_now64()` instead of `xtimer_now()` for updating the `*now` variable during the iteration in `_update_short_timers()` function. The same function is used to initialize `*now` in `_timer_callback()` below. While using `xtimer_now()` in this iteration step does not hinder the proper execution of all timers in the short term timers (for those the `xtimer` module only looks at the `start_time` member, not the `long_start_time` member) at least for the current long term time window (I did not test higher cases), it sets the `long_start_time` member to 0 for all timers following in the list of timers after this iteration step. However, external modules that rely on this to be correct, e.g. evtimer [1], fail their calculations when trying to compare to the current value to `xtimer_now64()`. [1] https://github.com/RIOT-OS/RIOT/blob/11f3d68/sys/evtimer/evtimer.c#L118-L121 Co-Authored-By: Cenk Gündoğan --- sys/xtimer/xtimer_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sys/xtimer/xtimer_core.c b/sys/xtimer/xtimer_core.c index cdc34ffcac..236a6f0408 100644 --- a/sys/xtimer/xtimer_core.c +++ b/sys/xtimer/xtimer_core.c @@ -292,7 +292,7 @@ static inline void _update_short_timers(uint64_t *now) /* assign new head */ timer = timer_list_head; /* update current_time */ - *now = _xtimer_now(); + *now = _xtimer_now64(); } else { timer->offset -= elapsed; From 4aa4a17071f79d2c2eb4c9e95db8474ed63abff6 Mon Sep 17 00:00:00 2001 From: "Martine S. Lenders" Date: Thu, 9 Apr 2020 23:33:23 +0200 Subject: [PATCH 2/3] tests/xtimer: add regression test for long_start_time update bug Co-Authored-By: Julian Holzwarth --- tests/xtimer_now32_overflow/Makefile | 8 ++ tests/xtimer_now32_overflow/README.md | 6 ++ tests/xtimer_now32_overflow/main.c | 91 +++++++++++++++++++++ tests/xtimer_now32_overflow/tests/01-run.py | 48 +++++++++++ 4 files changed, 153 insertions(+) create mode 100644 tests/xtimer_now32_overflow/Makefile create mode 100644 tests/xtimer_now32_overflow/README.md create mode 100644 tests/xtimer_now32_overflow/main.c create mode 100755 tests/xtimer_now32_overflow/tests/01-run.py diff --git a/tests/xtimer_now32_overflow/Makefile b/tests/xtimer_now32_overflow/Makefile new file mode 100644 index 0000000000..748bec036a --- /dev/null +++ b/tests/xtimer_now32_overflow/Makefile @@ -0,0 +1,8 @@ +include ../Makefile.tests_common + +USEMODULE += fmt +USEMODULE += xtimer + +DISABLE_MODULE += auto_init_xtimer + +include $(RIOTBASE)/Makefile.include diff --git a/tests/xtimer_now32_overflow/README.md b/tests/xtimer_now32_overflow/README.md new file mode 100644 index 0000000000..178c6d2390 --- /dev/null +++ b/tests/xtimer_now32_overflow/README.md @@ -0,0 +1,6 @@ +Description +=========== + +This is a regression test for when `xtimer_now64()` becomes greater than +`UINT32_MAX` and the internal state machine fails to update the timers state +correctly. diff --git a/tests/xtimer_now32_overflow/main.c b/tests/xtimer_now32_overflow/main.c new file mode 100644 index 0000000000..b296679d26 --- /dev/null +++ b/tests/xtimer_now32_overflow/main.c @@ -0,0 +1,91 @@ +/* + * Copyright (C) 2020 Freie Universität Berlin + * + * This file is subject to the terms and conditions of the GNU Lesser + * General Public License v2.1. See the file LICENSE in the top level + * directory for more details. + */ + +/** + * @{ + * + * @file + * @author Martine Lenders + */ + +#include + +#include "fmt.h" +#include "kernel_defines.h" +#include "msg.h" +#include "test_utils/expect.h" +#include "thread.h" +#include "xtimer.h" + +#define MAIN_MSG_QUEUE_SIZE (4U) +#define TIMERS_NUMOF (3U) + +msg_t _main_msg_queue[MAIN_MSG_QUEUE_SIZE]; +static const uint64_t _timers_offsets[TIMERS_NUMOF] = { + /* MUST ASCEND */ + 1 * US_PER_SEC, + 2 * US_PER_SEC, + 3 * US_PER_SEC, +}; + +int main(void) +{ + xtimer_t timers[TIMERS_NUMOF]; + msg_t msgs[TIMERS_NUMOF]; + uint64_t start; + + expect(ARRAY_SIZE(_timers_offsets) == TIMERS_NUMOF); + msg_init_queue(_main_msg_queue, MAIN_MSG_QUEUE_SIZE); + /* ensure that xtimer_now64() is greater than UINT32_MAX */ + _xtimer_current_time = (2LLU << 32U); + xtimer_init(); + print_str("Setting "); + print_u32_dec(TIMERS_NUMOF); + print_str(" timers:\n"); + for (unsigned i = 0; i < TIMERS_NUMOF; i++) { + msgs[i].content.value = i; + print_str(" #"); + print_u32_dec(i); + print_str(" in "); + print_u64_dec(_timers_offsets[i]); + print_str(" usec\n"); + } + print_str("now="); + start = xtimer_now64().ticks64; + print_u64_dec(start); + print_str("\n"); + expect(start > UINT32_MAX); + /* set timers after all were printed for better timing */ + for (unsigned i = 0; i < TIMERS_NUMOF; i++) { + xtimer_set_msg64(&timers[i], _timers_offsets[i], &msgs[i], + thread_getpid()); + expect(timers[i].long_start_time > 0); + } + while (1) { + msg_t msg; + + msg_receive(&msg); + print_str("#"); + print_u32_dec(msg.content.value); + print_str(":now="); + print_u64_dec((uint64_t)xtimer_now64().ticks64); + print_str("\n"); + for (unsigned i = 0; i <= msg.content.value; i++) { + /* all fired timers expired */ + expect(timers[i].long_start_time == 0); + } + for (unsigned i = (msg.content.value + 1); i <= TIMERS_NUMOF; i++) { + /* upper half of remaing_timers' start_time stays above 0 as it is + * based on xtimer_now64() during the timer's callback execution */ + expect(timers[i].long_start_time > 0); + } + } + return 0; +} + +/** @} */ diff --git a/tests/xtimer_now32_overflow/tests/01-run.py b/tests/xtimer_now32_overflow/tests/01-run.py new file mode 100755 index 0000000000..d2afac471e --- /dev/null +++ b/tests/xtimer_now32_overflow/tests/01-run.py @@ -0,0 +1,48 @@ +#!/usr/bin/env python3 + +# Copyright (C) 2018 Freie Universität Berlin +# +# This file is subject to the terms and conditions of the GNU Lesser +# General Public License v2.1. See the file LICENSE in the top level +# directory for more details. + +import sys +import time +from testrunner import run + + +def _get_largest_timeout_difference(timeouts): + next_timeout = min(timeouts) + # get largest difference between all timeouts + timeout = max(a - b for a in timeouts for b in timeouts) + # check if smallest timeout (difference to 0) is the largest difference + if timeout < next_timeout: + timeout = next_timeout + return timeout + + +def testfunc(child): + timers = {} + child.expect(r"Setting (\d+) timers:") + timers_numof = int(child.match.group(1)) + for i in range(timers_numof): + child.expect(r" #(\d+) in (\d+) usec") + assert i == int(child.match.group(1)) + timers[i] = int(child.match.group(2)) + assert timers_numof == len(timers) + check_time = int(time.time()) + child.expect(r"now=(\d+)") + offset = int(child.match.group(1)) + # get largest possible timeout for expects below + timeout = _get_largest_timeout_difference(timers.values()) / (10**6) + 1 + for i in range(timers_numof): + child.expect(r"#(\d):now=(\d+)", timeout=timeout) + t = int(child.match.group(1)) + now = int(child.match.group(2)) + assert (int(time.time()) - check_time) >= (timers[t] / 10**6) + expected = timers[t] + offset + assert expected <= now + + +if __name__ == "__main__": + sys.exit(run(testfunc)) From 085c62ecdc9b4741566a1a708adea8e10b61d40d Mon Sep 17 00:00:00 2001 From: "Martine S. Lenders" Date: Fri, 10 Apr 2020 01:07:54 +0200 Subject: [PATCH 3/3] xtimer_core: piggy-back style fixes --- sys/xtimer/xtimer_core.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sys/xtimer/xtimer_core.c b/sys/xtimer/xtimer_core.c index 236a6f0408..59d22560c5 100644 --- a/sys/xtimer/xtimer_core.c +++ b/sys/xtimer/xtimer_core.c @@ -169,7 +169,8 @@ static inline void _schedule_earliest_lltimer(uint32_t now) } /** - * @brief compare two timers. return true if timerA expires earlier than or equal to timerB and false otherwise. + * @brief compare two timers. return true if timerA expires earlier than or + * equal to timerB and false otherwise. */ static bool _timer_comparison(xtimer_t* timerA, xtimer_t* timerB, uint32_t now) { @@ -278,7 +279,7 @@ static inline void _update_short_timers(uint64_t *now) /* make sure we don't fire too early */ if (timer->offset > elapsed) { - while(_xtimer_now() - timer->start_time < timer->offset) {} + while (_xtimer_now() - timer->start_time < timer->offset) {} } /* advance list */ timer_list_head = timer->next;