diff --git a/src/core/timeouts.c b/src/core/timeouts.c index 90a5568e..b1d94a9f 100644 --- a/src/core/timeouts.c +++ b/src/core/timeouts.c @@ -67,7 +67,7 @@ #endif /* LWIP_DEBUG_TIMERNAMES */ /* Check if timer's expiry time is greater than time and care about u32_t wraparounds */ -#define TIMER_LESS_THAN(timer, t) ( (((timer)->time == (t)) || (((u32_t)((timer)->time-(t))) > 0x7fffffff)) ? 1 : 0 ) +#define TIME_LESS_THAN(t, compare_to) ( (((t) == (compare_to)) || (((u32_t)((t)-(compare_to))) > 0x7fffffff)) ? 1 : 0 ) /** This array contains all stack-internal cyclic timers. To get the number of * timers, use LWIP_ARRAYSIZE() */ @@ -115,6 +115,8 @@ const int lwip_num_cyclic_timers = LWIP_ARRAYSIZE(lwip_cyclic_timers); /** The one and only timeout list */ static struct sys_timeo *next_timeout; +static u32_t current_timeout_due_time; + #if LWIP_TESTMODE struct sys_timeo** lwip_sys_timers_get_next_timout(void) @@ -173,15 +175,30 @@ tcp_timer_needed(void) * * @param arg unused argument */ -static void -cyclic_timer(void *arg) +#if !LWIP_TESTMODE +static +#endif +void +lwip_cyclic_timer(void *arg) { + u32_t now; + u32_t next_timeout; const struct lwip_cyclic_timer *cyclic = (const struct lwip_cyclic_timer *)arg; + #if LWIP_DEBUG_TIMERNAMES LWIP_DEBUGF(TIMERS_DEBUG, ("tcpip: %s()\n", cyclic->handler_name)); #endif cyclic->handler(); - sys_timeout(cyclic->interval_ms, cyclic_timer, arg); + + now = sys_now(); + next_timeout = (u32_t)(current_timeout_due_time + cyclic->interval_ms); + if (TIME_LESS_THAN(next_timeout, now)) { + /* timer would immediately expire again -> "overload" -> restart without any correction */ + sys_timeout(cyclic->interval_ms, lwip_cyclic_timer, arg); + } else { + /* correct cyclic interval with handler execution delay and sys_check_timeouts jitter */ + sys_timeout((u32_t)(next_timeout - now), lwip_cyclic_timer, arg); + } } /** Initialize this module */ @@ -192,7 +209,7 @@ void sys_timeouts_init(void) for (i = (LWIP_TCP ? 1 : 0); i < LWIP_ARRAYSIZE(lwip_cyclic_timers); i++) { /* we have to cast via size_t to get rid of const warning (this is OK as cyclic_timer() casts back to const* */ - sys_timeout(lwip_cyclic_timers[i].interval_ms, cyclic_timer, LWIP_CONST_CAST(void *, &lwip_cyclic_timers[i])); + sys_timeout(lwip_cyclic_timers[i].interval_ms, lwip_cyclic_timer, LWIP_CONST_CAST(void *, &lwip_cyclic_timers[i])); } } @@ -230,7 +247,7 @@ sys_timeout(u32_t msecs, sys_timeout_handler handler, void *arg) timeout->next = NULL; timeout->h = handler; timeout->arg = arg; - timeout->time = (u32_t)(now + msecs); /* overflow handled by TIMER_LESS_THAN macro */ + timeout->time = (u32_t)(now + msecs); /* overflow handled by TIME_LESS_THAN macro */ #if LWIP_DEBUG_TIMERNAMES timeout->handler_name = handler_name; LWIP_DEBUGF(TIMERS_DEBUG, ("sys_timeout: %p msecs=%"U32_F" handler=%s arg=%p\n", @@ -241,12 +258,12 @@ sys_timeout(u32_t msecs, sys_timeout_handler handler, void *arg) next_timeout = timeout; return; } - if (TIMER_LESS_THAN(timeout, next_timeout->time)) { + if (TIME_LESS_THAN(timeout->time, next_timeout->time)) { timeout->next = next_timeout; next_timeout = timeout; } else { for (t = next_timeout; t != NULL; t = t->next) { - if ((t->next == NULL) || TIMER_LESS_THAN(timeout, t->next->time)) { + if ((t->next == NULL) || TIME_LESS_THAN(timeout->time, t->next->time)) { timeout->next = t->next; t->next = timeout; break; @@ -322,12 +339,13 @@ sys_check_timeouts(void) had_one = 0; tmptimeout = next_timeout; now = sys_now(); - if (tmptimeout && TIMER_LESS_THAN(tmptimeout, now)) { + if (tmptimeout && TIME_LESS_THAN(tmptimeout->time, now)) { /* timeout has expired */ had_one = 1; next_timeout = tmptimeout->next; handler = tmptimeout->h; arg = tmptimeout->arg; + current_timeout_due_time = tmptimeout->time; #if LWIP_DEBUG_TIMERNAMES if (handler != NULL) { LWIP_DEBUGF(TIMERS_DEBUG, ("sct calling h=%s arg=%p\n", @@ -373,7 +391,7 @@ sys_timeouts_sleeptime(void) return 0xffffffff; } now = sys_now(); - if (TIMER_LESS_THAN(next_timeout, now)) { + if (TIME_LESS_THAN(next_timeout->time, now)) { return 0; } else { return (u32_t)(next_timeout->time - now); diff --git a/src/include/lwip/timeouts.h b/src/include/lwip/timeouts.h index b424a7d7..f28fa9e6 100644 --- a/src/include/lwip/timeouts.h +++ b/src/include/lwip/timeouts.h @@ -116,6 +116,7 @@ void sys_timeouts_mbox_fetch(sys_mbox_t *mbox, void **msg); #if LWIP_TESTMODE struct sys_timeo** lwip_sys_timers_get_next_timout(void); +void lwip_cyclic_timer(void *arg); #endif #endif /* LWIP_TIMERS */ diff --git a/test/unit/core/test_timers.c b/test/unit/core/test_timers.c index eae4a88f..e8318063 100644 --- a/test/unit/core/test_timers.c +++ b/test/unit/core/test_timers.c @@ -31,7 +31,65 @@ static void dummy_handler(void* arg) fired[index] = 1; } -/* reproduce bug bug #52748: the bug in timeouts.c */ +#define HANDLER_EXECUTION_TIME 5 +static int cyclic_fired; +static void dummy_cyclic_handler(void) +{ + cyclic_fired = 1; + lwip_sys_now += HANDLER_EXECUTION_TIME; +} + +struct lwip_cyclic_timer test_cyclic = {10, dummy_cyclic_handler}; + +void do_test_cyclic_timers(u32_t offset) +{ + struct sys_timeo** list_head = lwip_sys_timers_get_next_timout(); + + /* verify normal timer expiration */ + lwip_sys_now = offset + 0; + sys_timeout(test_cyclic.interval_ms, lwip_cyclic_timer, &test_cyclic); + + cyclic_fired = 0; + sys_check_timeouts(); + fail_unless(cyclic_fired == 0); + + lwip_sys_now = offset + test_cyclic.interval_ms; + sys_check_timeouts(); + fail_unless(cyclic_fired == 1); + + fail_unless((*list_head)->time == (u32_t)(lwip_sys_now + test_cyclic.interval_ms - HANDLER_EXECUTION_TIME)); + + sys_untimeout(lwip_cyclic_timer, &test_cyclic); + + + /* verify "overload" - next cyclic timer execution is already overdue twice */ + lwip_sys_now = offset + 0; + sys_timeout(test_cyclic.interval_ms, lwip_cyclic_timer, &test_cyclic); + + cyclic_fired = 0; + sys_check_timeouts(); + fail_unless(cyclic_fired == 0); + + lwip_sys_now = offset + 2*test_cyclic.interval_ms; + sys_check_timeouts(); + fail_unless(cyclic_fired == 1); + + fail_unless((*list_head)->time == (u32_t)(lwip_sys_now + test_cyclic.interval_ms)); +} + +START_TEST(test_cyclic_timers) +{ + LWIP_UNUSED_ARG(_i); + + /* check without u32_t wraparound */ + do_test_cyclic_timers(0); + + /* check with u32_t wraparound */ + do_test_cyclic_timers(0xfffffff0); +} +END_TEST + +/* reproduce bug #52748: the bug in timeouts.c */ START_TEST(test_bug52748) { LWIP_UNUSED_ARG(_i); @@ -63,15 +121,11 @@ START_TEST(test_bug52748) } END_TEST -START_TEST(test_timers) +void do_test_timers(u32_t offset) { struct sys_timeo** list_head = lwip_sys_timers_get_next_timout(); - - LWIP_UNUSED_ARG(_i); - - /* check without u32_t wraparound */ - - lwip_sys_now = 100; + + lwip_sys_now = offset + 0; sys_timeout(10, dummy_handler, LWIP_PTR_NUMERIC_CAST(void*, 0)); fail_unless(sys_timeouts_sleeptime() == 10); @@ -115,53 +169,17 @@ START_TEST(test_timers) sys_untimeout(dummy_handler, LWIP_PTR_NUMERIC_CAST(void*, 0)); sys_untimeout(dummy_handler, LWIP_PTR_NUMERIC_CAST(void*, 1)); sys_untimeout(dummy_handler, LWIP_PTR_NUMERIC_CAST(void*, 2)); +} - /* check u32_t wraparound */ +START_TEST(test_timers) +{ + LWIP_UNUSED_ARG(_i); - lwip_sys_now = 0xfffffff5; + /* check without u32_t wraparound */ + do_test_timers(0); - sys_timeout(10, dummy_handler, LWIP_PTR_NUMERIC_CAST(void*, 0)); - fail_unless(sys_timeouts_sleeptime() == 10); - sys_timeout(20, dummy_handler, LWIP_PTR_NUMERIC_CAST(void*, 1)); - fail_unless(sys_timeouts_sleeptime() == 10); - sys_timeout( 5, dummy_handler, LWIP_PTR_NUMERIC_CAST(void*, 2)); - fail_unless(sys_timeouts_sleeptime() == 5); - - /* linked list correctly sorted? */ - fail_unless((*list_head)->time == (u32_t)(lwip_sys_now + 5)); - fail_unless((*list_head)->next->time == (u32_t)(lwip_sys_now + 10)); - fail_unless((*list_head)->next->next->time == (u32_t)(lwip_sys_now + 20)); - - /* check timers expire in correct order */ - memset(&fired, 0, sizeof(fired)); - - lwip_sys_now += 4; - sys_check_timeouts(); - fail_unless(fired[2] == 0); - - lwip_sys_now += 1; - sys_check_timeouts(); - fail_unless(fired[2] == 1); - - lwip_sys_now += 4; - sys_check_timeouts(); - fail_unless(fired[0] == 0); - - lwip_sys_now += 1; - sys_check_timeouts(); - fail_unless(fired[0] == 1); - - lwip_sys_now += 9; - sys_check_timeouts(); - fail_unless(fired[1] == 0); - - lwip_sys_now += 1; - sys_check_timeouts(); - fail_unless(fired[1] == 1); - - sys_untimeout(dummy_handler, LWIP_PTR_NUMERIC_CAST(void*, 0)); - sys_untimeout(dummy_handler, LWIP_PTR_NUMERIC_CAST(void*, 1)); - sys_untimeout(dummy_handler, LWIP_PTR_NUMERIC_CAST(void*, 2)); + /* check with u32_t wraparound */ + do_test_timers(0xfffffff0); } END_TEST @@ -171,6 +189,7 @@ timers_suite(void) { testfunc tests[] = { TESTFUNC(test_bug52748), + TESTFUNC(test_cyclic_timers), TESTFUNC(test_timers) }; testfunc tests_unused[] = {