From b3103efe93be698d340574e756792c9be036e06f Mon Sep 17 00:00:00 2001 From: Amit Aryeh Levy Date: Thu, 10 Oct 2024 16:23:25 -0700 Subject: [PATCH] alarm: rework alarm virtualization Properly account for: - X - Y - Z --- .../Makefile | 11 ++ .../README.md | 29 +++ .../main.c | 24 +++ libtock/services/alarm.c | 174 +++++++++++++++--- 4 files changed, 217 insertions(+), 21 deletions(-) create mode 100644 examples/tests/multi_alarm_dont_ignore_previous_overflow/Makefile create mode 100644 examples/tests/multi_alarm_dont_ignore_previous_overflow/README.md create mode 100644 examples/tests/multi_alarm_dont_ignore_previous_overflow/main.c diff --git a/examples/tests/multi_alarm_dont_ignore_previous_overflow/Makefile b/examples/tests/multi_alarm_dont_ignore_previous_overflow/Makefile new file mode 100644 index 000000000..54d6a7969 --- /dev/null +++ b/examples/tests/multi_alarm_dont_ignore_previous_overflow/Makefile @@ -0,0 +1,11 @@ +# Makefile for user application + +# Specify this directory relative to the current application. +TOCK_USERLAND_BASE_DIR = ../../.. + +# Which files to compile. +C_SRCS := $(wildcard *.c) + +# Include userland master makefile. Contains rules and flags for actually +# building the application. +include $(TOCK_USERLAND_BASE_DIR)/AppMakefile.mk diff --git a/examples/tests/multi_alarm_dont_ignore_previous_overflow/README.md b/examples/tests/multi_alarm_dont_ignore_previous_overflow/README.md new file mode 100644 index 000000000..ff47c4688 --- /dev/null +++ b/examples/tests/multi_alarm_dont_ignore_previous_overflow/README.md @@ -0,0 +1,29 @@ +# Test Multiple Alarms (Ensure previous overflowing alarms aren't ignore) + +This tests the virtual alarms available to userspace. It sets three +alarms alarms. The first two overflow and should result in multiple +alarms under the hood, and a third a 10 second alarm started after the +second. The first alarm is set first, but expires second (1 second +after overflow vs 10ms after overflow). The third alarm should expire +last. + +When successful, the first alarm should fire after a clock overflow + +1 second, while the third should fire after a clock overflow + 10ms + +10 seconds. + +If the virtual alarm library is buggy, this test might fail by never +firing the first alarm or firing it after the third alarm +(specifically almost another clock overflow) because either alarm +insertion or virtual alarm upcall handing misses it after the second +alarm fires. + +# Example Output + +Correct: + +(after an entire clock overflow, ~8 minutes) + +``` +1 10498816 10496827 +2 86100992 86099200 +``` diff --git a/examples/tests/multi_alarm_dont_ignore_previous_overflow/main.c b/examples/tests/multi_alarm_dont_ignore_previous_overflow/main.c new file mode 100644 index 000000000..5250a2c5f --- /dev/null +++ b/examples/tests/multi_alarm_dont_ignore_previous_overflow/main.c @@ -0,0 +1,24 @@ +#include +#include + +#include + +static void event_cb(uint32_t now, uint32_t expiration, void* ud) { + int i = (int)ud; + printf("%d %lu %lu\n", i, now, expiration); +} + +int main(void) { + libtock_alarm_t t1; + libtock_alarm_t t2; + + uint32_t overflow_ms = libtock_alarm_ticks_to_ms(UINT32_MAX); + + libtock_alarm_in_ms(overflow_ms + 1000, event_cb, (void*)1, &t1); + libtocksync_alarm_delay_ms(overflow_ms + 10); + libtock_alarm_in_ms(10000, event_cb, (void*)2, &t2); + + while (1) { + yield(); + } +} diff --git a/libtock/services/alarm.c b/libtock/services/alarm.c index 296b936e7..d9c123ae1 100644 --- a/libtock/services/alarm.c +++ b/libtock/services/alarm.c @@ -1,10 +1,39 @@ #include "alarm.h" #include -#include #include #define MAX_TICKS UINT32_MAX +/** \brief Checks if `now` is between `reference` and `dt`, meaning + * the alarm has not yet expired. + * + * Invariants: + * 1. `now` hasn't wrapped `reference`, i.e., in an infinite space, + * `now` - `reference` <= MAX_TICKS + * 2. `dt` is at most MAX_TICKS, such that `reference + dt` at most + * wraps back to `reference`. + * 3. `now` happens after `reference`, so if `now` is not "within" + * `reference` and `dt`, it is after, not before. + * 4. `now` may be larger or smaller than `reference` since ticks + * wrap. + * + * Corrolaries: + * - if `now` is larger than `reference`, `now - reference` is + * directly comparable to `dt` (1 & 2) + * - if `now` is less than reference, `now - reference` wraps, and is + * - comparable to `dt` because `now` is guaranteed to happened after + * `reference` (3) + * + * The result is that the check is fairly simple, but its simplicitly + * relies specifically on `now` never happening before `reference`, + * and thus tied to the specific logic in `alarm_upcall` (see inline + * comment on delaying executing callbacks until after checking all + * outstanding alarms). + */ +static bool is_within(uint32_t now, uint32_t reference, uint32_t dt) { + return now - reference < dt; +} + /** \brief Convert milliseconds to clock ticks * * WARNING: This function will assert if the output @@ -101,14 +130,20 @@ static void root_insert(libtock_alarm_ticks_t* alarm) { bool cur_overflows = (*cur)->reference > (UINT32_MAX - (*cur)->dt); // This alarm (`cur`) happens after the new alarm (`alarm`) if: - // - both overflow or neither overflow, and cur expiration is + // + // - both overflow or neither overflow and cur expiration is // larger than the new expiration - // - or, only cur overflows + // - only cur overflows, cur's reference is before new reference + // (both started this epoch, cur must expire next epoch) + // - only cur overflows, cur's reference is after new reference + // (cur started in previous epoch, cur also must expire this + // epoch) and cur's expiration is larger than the new + // expiration. // // If the new alarm overflows and this alarm doesn't, this alarm // happens _before_ the new alarm. if (((cur_overflows == new_overflows) && (cur_expiration > new_expiration)) || - cur_overflows) { + (cur_overflows && ((*cur)->reference < alarm->reference || cur_expiration > new_expiration))) { // insert before libtock_alarm_ticks_t* tmp = *cur; *cur = alarm; @@ -145,27 +180,125 @@ static libtock_alarm_ticks_t* root_peek(void) { return root; } +/** \brief Upcall for internal virtual alarms + * + * This upcall checks the ordered list of outstanding alarms for + * expired alarms, removes them from the list, and invokes their + * callbacks. + * + * Invariants: + * 1. The list of outstanding alarms is ordered by absolute expiration time. + * 2. The alarm event that invoked this upcall is for the current `head` of the list. + * 3. No alarms are added (or re-added) to the list between while + * iterating through alarms + * 4. For each alarm, `alarm->dt < MAX_TICKS + 1` + * + * Corrollaries: + * - If the head of the list hasn't expired, no alarms in the tail of + * the list have expired (1) + * - `scheduled` happens after `head->reference` (2) + * - `scheduled` happens before `head->reference + MAX_TICKS + 1` (4) + * - For each alarm in the list, `now` happens after + * `alarm->reference` (2, 3) + * - For each alarm, `scheduled` cannot have cannot have wrapped + * `alarm->reference` (1, 4) + * - For each alarm, if `scheduled` and `now` are not on the same side of + * `alarm->reference`, the alarm must have expired (1, 4) + * + * Critically, this upcall cannot allow any alarms to be added to the + * ordered list while iterating by invoking upcalls, as that could + * violate invariant (3) and result in `now` happening before some + * `alarm->reference`. + * + * Some alarms that expire between `now` and the end of the upcall may + * be "missed", which may mean they are delivered later. They should + * still show up first in the list at the end, so will fire next. + */ static void alarm_upcall(__attribute__ ((unused)) int kernel_now, - __attribute__ ((unused)) int scheduled, + int scheduled0, __attribute__ ((unused)) int unused2, __attribute__ ((unused)) void* opaque) { + // `tocall` is a temporary list to keep track of expired alarms to call later. + libtock_alarm_ticks_t* tocall = NULL; + libtock_alarm_ticks_t* tocall_last = NULL; + + // We want `scheduled` as unsigned so wrapping math works out correctly. + uint32_t scheduled = scheduled0; + + // Take the current tick value. We could use `kernel_now`, but would + // potentially unnecessarily delay some alarms. + uint32_t now; + libtock_alarm_command_read(&now); + + // We know this upcall is associated with the head, so it's easier + // to deal with. + libtock_alarm_ticks_t* head = root_peek(); + + // Formally, we should be able to just add head to + // `tocall`, but let's just be defensive just in case there is an + // errant alarm. + if (head != NULL && !is_within(scheduled, head->reference, head->dt)) { + // If for whatever reason, `head` hasn't expired, leave it at head + // and reset its alarm. + root_pop(); + head->next = NULL; + head->prev = tocall_last; + tocall = head; + tocall_last = head; + } else { + // errant alarm upcall? This should never happen, but oh well, + // doesn't hurt to reset the alarm at this point + } + + // Now iterate through the remaining alarms in case any of them have + // also expired. for (libtock_alarm_ticks_t* alarm = root_peek(); alarm != NULL; alarm = root_peek()) { - uint32_t now; - libtock_alarm_command_read(&now); - // has the alarm not expired yet? (distance from `now` has to be larger or - // equal to distance from current clock value. - if (alarm->dt > now - alarm->reference) { - libtock_alarm_command_set_absolute(alarm->reference, alarm->dt); + // has the alarm not expired yet? + // Three cases: + // 1. scheduled - reference is larger than now - reference: + // ticks have wrapped reference, alarm must have expired. + // 2. scheduled is no longer within reference + dt (alarm + // expired) + // 3. now is no longer within reference + dt (alarm expired) + // + // Simpler to check the non-expiring case first. + if ((now - alarm->reference >= scheduled - alarm->reference) && + is_within(scheduled, alarm->reference, alarm->dt) && + is_within(now, alarm->reference, alarm->dt)) { + // Nope, has not expired, nothing "after" this alarm has + // expired either, since we add alarms in expiration order. break; } else { + // Expired, add to `tocall` list. root_pop(); + alarm->next = NULL; + alarm->prev = tocall_last; + // If this expired, head must have also expired, so `tocall` and + // `tocall_last` are non-null, so just add to the end. + tocall_last->next = alarm; + } + } - if (alarm->callback) { - uint32_t expiration = alarm->reference + alarm->dt; - alarm->callback(now, expiration, alarm->ud); - } + for (libtock_alarm_ticks_t* alarm = tocall; alarm != NULL; alarm = tocall) { + alarm->prev = NULL; + tocall = alarm->next; + alarm->next = NULL; + if (alarm->callback) { + uint32_t expiration = alarm->reference + alarm->dt; + alarm->callback(now, expiration, alarm->ud); } } + + head = root_peek(); + if (head != NULL) { + // TODO(alevy): At this point, is it possible we've wrapped so far + // past `reference` that we might end up delaying a technically + // expired alarm by another timer wrap? I think technically yes, + // though techincally the interface only guarantees alarms will + // delay *at least* `dt`, so more is fine, and we could be delayed + // arbitrarily long by the kernel anyway. + libtock_alarm_command_set_absolute(head->reference, head->dt); + } } static int libtock_alarm_at_internal(uint32_t reference, uint32_t dt, libtock_alarm_callback cb, void* ud, @@ -178,10 +311,6 @@ static int libtock_alarm_at_internal(uint32_t reference, uint32_t dt, libtock_al alarm->next = NULL; root_insert(alarm); - int i = 0; - for (libtock_alarm_ticks_t* cur = root_peek(); cur != NULL; cur = cur->next) { - i++; - } if (root_peek() == alarm) { libtock_alarm_set_upcall((subscribe_upcall*)alarm_upcall, NULL); @@ -235,8 +364,10 @@ static void overflow_callback(__attribute__ ((unused)) uint32_t now, // schedule next intermediate alarm that will overflow tock_timer->overflows_left--; + const uint32_t max_ticks_in_ms = libtock_alarm_ticks_to_ms(MAX_TICKS); + const uint32_t max_ms_in_ticks = ms_to_ticks(max_ticks_in_ms); libtock_alarm_at(last_timer_fire_time, - MAX_TICKS, + max_ms_in_ticks, (libtock_alarm_callback) overflow_callback, (void*) tock_timer, &(tock_timer->alarm)); @@ -253,6 +384,7 @@ int libtock_alarm_in_ms(uint32_t ms, libtock_alarm_callback cb, void* opaque, li // and the remainder ticks to reach the target length of time. The overflows use the // `overflow_callback` for each intermediate overflow. const uint32_t max_ticks_in_ms = libtock_alarm_ticks_to_ms(MAX_TICKS); + const uint32_t max_ms_in_ticks = ms_to_ticks(max_ticks_in_ms); if (ms > max_ticks_in_ms) { // overflows_left is the number of intermediate alarms that need to be scheduled to reach the target // dt_ms. After the alarm in this block is scheduled, we have this many overflows left (hence the reason @@ -263,7 +395,7 @@ int libtock_alarm_in_ms(uint32_t ms, libtock_alarm_callback cb, void* opaque, li alarm->user_data = opaque; alarm->callback = cb; - return libtock_alarm_at(now, MAX_TICKS, (libtock_alarm_callback)overflow_callback, (void*)(alarm), + return libtock_alarm_at(now, max_ms_in_ticks, (libtock_alarm_callback)overflow_callback, (void*)(alarm), &(alarm->alarm)); } else { // No overflows needed