Fixes re-arming alarms from IRQ handlers
authorBarret Rhoden <brho@cs.berkeley.edu>
Mon, 2 Mar 2015 15:56:19 +0000 (10:56 -0500)
committerBarret Rhoden <brho@cs.berkeley.edu>
Mon, 2 Mar 2015 15:56:19 +0000 (10:56 -0500)
Previously, you had to call __set_alarm(), since the tchain lock was
held.  This made it more difficult to work with, and easy to
accidentally use the wrong type of set_alarm().  For IRQ handlers, you'd
quickly deadlock.  But RKM handlers using __set_alarm() could silently
corrupt the tchain.

This commit pushes the logic about whether or not the tchain lock is
held into the alarm code, so users can call set_alarm (or reset) without
worrying about the context.

Also, this clarifies the differences between the cancellation guarantees
between IRQ and RKM handlers.  In short, if you fail to cancel an RKM
alarm, it is possible that the alarm handler has not executed yet, but
it will eventually.  In these cases, do not free the waiter til after it
executes.  A bad practice would be:
unset_alarm(some-rkm-alarm-waiter);
// think it has already fired and no refs to waiter exist
kfree(the-waiter);
You *can* do that with IRQ alarms.  That's the price you pay for using
the RKM alarm.

We can build something more intense so that you can sleep on an RKM
alarm's completion, but I won't do it until someone expresses a need
(and feedback so far is no one wants it).

kern/drivers/dev/kprof.c
kern/include/alarm.h
kern/src/alarm.c

index 92096bb..367445d 100644 (file)
@@ -98,8 +98,7 @@ static void oprof_alarm_handler(struct alarm_waiter *waiter,
                oprofile_add_backtrace(get_hwtf_pc(hw_tf), get_hwtf_fp(hw_tf));
        else
                oprofile_add_userpc(get_hwtf_pc(hw_tf));
-       /* we're an IRQ handler, so the tchain is locked */
-       __reset_alarm_rel(tchain, waiter, oprof_timer_period);
+       reset_alarm_rel(tchain, waiter, oprof_timer_period);
 }
 
 static struct chan*
@@ -155,7 +154,7 @@ static void setup_timers(void)
                struct timer_chain *tchain = &per_cpu_info[core_id()].tchain;
                kproftimer(get_hwtf_pc(hw_tf));
                set_awaiter_rel(waiter, 1000);
-               __set_alarm(tchain, waiter);
+               set_alarm(tchain, waiter);
        }
        struct timer_chain *tchain = &per_cpu_info[core_id()].tchain;
        struct alarm_waiter *waiter = kmalloc(sizeof(struct alarm_waiter), 0);
index 7ab1fa3..f65f7bc 100644 (file)
  * parameter in addition to the standard alarm_waiter.  RKM alarms are executed
  * when kernel messages are executed, which is out of IRQ context.  RKMs are
  * safer, since you can sleep (qlock, some kmalloc, etc) and you do not need
- * irqsave locks.  To use an IRQ alarm, init the waiter with init_awaiter_irq().
+ * irqsave locks.
+ *
+ * Another important difference between IRQ and RKM alarms comes when cancelling
+ * or unsetting an alarm.  When you cancel (unset or reset) an alarm, the alarm
+ * is yanked off the tchain.  If the waiter was on the chain, then it will not
+ * fire for both IRQ and RKM alarms.  If the waiter was not on the chain, then
+ * for IRQ alarms, this means that the alarm has already fired.  However, for
+ * RKM alarms, the alarm may have already fired or it may still be waiting to
+ * fire (sitting in an RKM queue).  It will fire at some point, but perhaps it
+ * has not fired yet.  It is also possibly (though extremely unlikely) that if
+ * you reset an RKM alarm that the new alarm actually happens before the old one
+ * (if the new RKM was sent to another core).  
+ * 
+ * To use an IRQ alarm, init the waiter with init_awaiter_irq().
  *
  * Quick howto, using the pcpu tchains:
  *     struct timer_chain *tchain = &per_cpu_info[core_id()].tchain;
  *     set_awaiter_rel(waiter, USEC);
  *     set_alarm(tchain, waiter);
  * If you want the HANDLER to run again, do this at the end of it:
- *     set_awaiter_rel(waiter, USEC);
- *     __set_alarm(tchain, waiter);
- * Do not call set_alarm() from within an alarm handler; you'll deadlock.
+ *     set_awaiter_rel(waiter, USEC);  // or whenever you want it to fire
+ *     set_alarm(tchain, waiter);
+ * or:
+ *     reset_alarm_rel(tchain, waiter, USEC);
+ *
  * Don't forget to manage your memory at some (safe) point:
  *     kfree(waiter);
  * In the future, we might have a slab for these.  You can get it from wherever
@@ -76,6 +91,7 @@ struct alarm_waiter {
        TAILQ_ENTRY(alarm_waiter)       next;
        bool                                            on_tchain;
        bool                                            irq_ok;
+       bool                                            holds_tchain_lock;
        bool                                            has_func;
 };
 TAILQ_HEAD(awaiters_tailq, alarm_waiter);              /* ideally not a LL */
@@ -106,14 +122,9 @@ void init_awaiter_irq(struct alarm_waiter *waiter,
 void set_awaiter_abs(struct alarm_waiter *waiter, uint64_t abs_time);
 void set_awaiter_rel(struct alarm_waiter *waiter, uint64_t usleep);
 void set_awaiter_inc(struct alarm_waiter *waiter, uint64_t usleep);
-/* Arms/disarms the alarm.  Hold the lock when calling __methods.  */
-void __set_alarm(struct timer_chain *tchain, struct alarm_waiter *waiter);
+/* Arms/disarms the alarm. */
 void set_alarm(struct timer_chain *tchain, struct alarm_waiter *waiter);
 bool unset_alarm(struct timer_chain *tchain, struct alarm_waiter *waiter);
-bool __reset_alarm_abs(struct timer_chain *tchain, struct alarm_waiter *waiter,
-                       uint64_t abs_time);
-bool __reset_alarm_rel(struct timer_chain *tchain, struct alarm_waiter *waiter,
-                       uint64_t usleep);
 bool reset_alarm_abs(struct timer_chain *tchain, struct alarm_waiter *waiter,
                      uint64_t abs_time);
 bool reset_alarm_rel(struct timer_chain *tchain, struct alarm_waiter *waiter,
index 0ab20f7..e541a78 100644 (file)
@@ -52,6 +52,7 @@ static void __init_awaiter(struct alarm_waiter *waiter)
 {
        waiter->wake_up_time = ALARM_POISON_TIME;
        waiter->on_tchain = FALSE;
+       waiter->holds_tchain_lock = FALSE;
        if (!waiter->has_func)
                sem_init_irqsave(&waiter->sem, 0);
 }
@@ -134,11 +135,14 @@ static void wake_awaiter(struct alarm_waiter *waiter,
                          struct hw_trapframe *hw_tf)
 {
        if (waiter->has_func) {
-               if (waiter->irq_ok)
+               if (waiter->irq_ok) {
+                       waiter->holds_tchain_lock = TRUE;
                        waiter->func_irq(waiter, hw_tf);
-               else
+                       waiter->holds_tchain_lock = FALSE;
+               } else {
                        send_kernel_message(core_id(), __run_awaiter, (long)waiter,
                                            0, 0, KMSG_ROUTINE);
+               }
        } else {
                sem_up(&waiter->sem); /* IRQs are disabled, can call sem_up directly */
        }
@@ -228,22 +232,23 @@ static bool __insert_awaiter(struct timer_chain *tchain,
        panic("Could not find a spot for awaiter %p\n", waiter);
 }
 
-/* Sets the alarm.  If it is a kthread-style alarm (func == 0), sleep on it
- * later.  This version assumes you have the lock held.  That only makes sense
- * from alarm handlers, which are called with this lock held from IRQ context */
-void __set_alarm(struct timer_chain *tchain, struct alarm_waiter *waiter)
+static void __set_alarm(struct timer_chain *tchain, struct alarm_waiter *waiter)
 {
        if (__insert_awaiter(tchain, waiter))
                reset_tchain_interrupt(tchain);
 }
 
-/* Sets the alarm.  Don't call this from an alarm handler, since you already
- * have the lock held.  Call __set_alarm() instead. */
+/* Sets the alarm.  If it is a kthread-style alarm (func == 0), sleep on it
+ * later. */
 void set_alarm(struct timer_chain *tchain, struct alarm_waiter *waiter)
 {
-       spin_lock_irqsave(&tchain->lock);
-       __set_alarm(tchain, waiter);
-       spin_unlock_irqsave(&tchain->lock);
+       if (waiter->holds_tchain_lock) {
+               __set_alarm(tchain, waiter);
+       } else {
+               spin_lock_irqsave(&tchain->lock);
+               __set_alarm(tchain, waiter);
+               spin_unlock_irqsave(&tchain->lock);
+       }
 }
 
 /* Helper, rips the waiter from the tchain, knowing that it is on the list.
@@ -274,6 +279,7 @@ static bool __remove_awaiter(struct timer_chain *tchain,
  * disarmed before the alarm went off, FALSE if it already fired. */
 bool unset_alarm(struct timer_chain *tchain, struct alarm_waiter *waiter)
 {
+       assert(!waiter->holds_tchain_lock);     /* Don't call from within a handler */
        spin_lock_irqsave(&tchain->lock);
        bool ret = waiter->on_tchain;
        if (ret && __remove_awaiter(tchain, waiter))
@@ -290,8 +296,8 @@ bool unset_alarm(struct timer_chain *tchain, struct alarm_waiter *waiter)
  * tchain.  Either way, this will put the waiter on the list, set to go off at
  * abs_time.  If you know the alarm has fired, don't call this.  Just set the
  * awaiter, and then set_alarm() */
-bool __reset_alarm_abs(struct timer_chain *tchain, struct alarm_waiter *waiter,
-                       uint64_t abs_time)
+static bool __reset_alarm_abs(struct timer_chain *tchain,
+                              struct alarm_waiter *waiter, uint64_t abs_time)
 {
        /* The tchain's lock is held */
        bool ret = waiter->on_tchain;
@@ -306,8 +312,8 @@ bool __reset_alarm_abs(struct timer_chain *tchain, struct alarm_waiter *waiter,
        return ret;
 }
 
-bool __reset_alarm_rel(struct timer_chain *tchain, struct alarm_waiter *waiter,
-                       uint64_t usleep)
+static bool __reset_alarm_rel(struct timer_chain *tchain,
+                              struct alarm_waiter *waiter, uint64_t usleep)
 {
        uint64_t now, then;
        now = read_tsc();
@@ -319,18 +325,28 @@ bool __reset_alarm_rel(struct timer_chain *tchain, struct alarm_waiter *waiter,
 bool reset_alarm_abs(struct timer_chain *tchain, struct alarm_waiter *waiter,
                      uint64_t abs_time)
 {
-       spin_lock_irqsave(&tchain->lock);
-       bool ret = __reset_alarm_abs(tchain, waiter, abs_time);
-       spin_unlock_irqsave(&tchain->lock);
+       bool ret;
+       if (waiter->holds_tchain_lock) {
+               ret = __reset_alarm_abs(tchain, waiter, abs_time);
+       } else {
+               spin_lock_irqsave(&tchain->lock);
+               ret = __reset_alarm_abs(tchain, waiter, abs_time);
+               spin_unlock_irqsave(&tchain->lock);
+       }
        return ret;
 }
 
 bool reset_alarm_rel(struct timer_chain *tchain, struct alarm_waiter *waiter,
                      uint64_t usleep)
 {
-       spin_lock_irqsave(&tchain->lock);
-       bool ret =__reset_alarm_rel(tchain, waiter, usleep);
-       spin_unlock_irqsave(&tchain->lock);
+       bool ret;
+       if (waiter->holds_tchain_lock) {
+               ret =__reset_alarm_rel(tchain, waiter, usleep);
+       } else {
+               spin_lock_irqsave(&tchain->lock);
+               ret =__reset_alarm_rel(tchain, waiter, usleep);
+               spin_unlock_irqsave(&tchain->lock);
+       }
        return ret;
 }