alarm: rework alarm virtualization

Properly account for:

- X
- Y
- Z

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

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

examples/tests/multi_alarm_dont_ignore_previous_overflow/main.c

@@ -0,0 +1,24 @@
+#include <stdio.h>
+#include <stdlib.h>
+
+#include <libtock-sync/services/alarm.h>
+
+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();
+  }
+}

libtock/services/alarm.c

@@ -1,10 +1,39 @@
 #include "alarm.h"
 #include <assert.h>
-#include <limits.h>
 #include <stdlib.h>
 
 #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