alarm: rework alarm virtualization

Fixes two bugs in alarm virtualization.

1. Addresses an unaccounted for case when inserting new alarms into the
   global list of outstanding alarms. In particular, it's possible for
   only `cur` to overflow and still happen before the alarm being added
   (see inline comments added to `root_insert`.

2. Rewrites the virtualization to avoid the possibility of alarms being
   added while the virtualizing upcall is iterating for expired alarms.

Author: alevy

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,118 @@ 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_pop();
+
+  // 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.
+  assert(head != NULL && !is_within(scheduled, head->reference, head->dt));
+  head->next  = NULL;
+  head->prev  = NULL;
+  tocall      = head;
+  tocall_last = head;
+
+  // 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 +304,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 +357,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 +377,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 +388,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