alarm: Force unset_alarm to grab the CV lock
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 16 Oct 2018 14:20:40 +0000 (10:20 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Mon, 22 Oct 2018 23:35:51 +0000 (19:35 -0400)
The issue here is that unset_alarm() is now used to mean "is the alarm
service done with the alarm."  Previously, it was "has the function run
yet."

This all is overly fucked up, due to a bunch of things:
- The alarm callbacks allegedly can block.  Not sure if any of them
  actually need that.
- Unset blocks until a CB is done, meaning it might sleep.
- So we need the CV and flags, and need to do things *after* the
  function pointer runs.
- That means the caller/user needs to be aware of the alarm service, and
  can't reuse/free the alarm right away.
- Alarms can re-arm from within their callbacks, and the service doesn't
  know about it.  Hell, what if they put themselves on another timer
chain?

Even with this change, we still have some issues.  Say you set_alarm()
from within a callback.  Then that alarm fires and runs before the alarm
service finishes it's CV work?  This could easily happen with a global
tchain (like parlib provides).  It could also happen with a per-core
timer chain, if the callback calls set_alarm() and then blocks.  Anyway,
under this situation, we could have *two* contexts trying to finish the
same alarm at the same time, where one of them clears is_running while
the other is still running.

Oh, and if an alarm CB blocks, under the current implementation, it'll
block the other unrelated alarms after it.  Not ideal.

The longer term fix might be to not allow alarm CBs to block (in
userspace, this means no uthread mutexes, which is actually already the
case since they run in vcore context).  This actually makes it a lot
more like Linux's timer stuff: they run in softirq context only (not
hard irq), though their del_timer_sync is a bit nasty.  They allow
mod_timer() to be called from within the CB, though maybe a little
discipline could help out with some of the corner cases.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/src/alarm.c
user/parlib/alarm.c

index d75eb1d..21afc53 100644 (file)
@@ -296,14 +296,6 @@ bool unset_alarm(struct timer_chain *tchain, struct alarm_waiter *waiter)
                return true;
        }
 
-       /* A common case is that it already finished.  We need the CV lock farther
-        * below so that we don't miss the signal.  You don't need it if you can see
-        * the signal (is_running == false) is already sent. */
-       if (!waiter->is_running) {
-               spin_unlock_irqsave(&tchain->lock);
-               return false;
-       }
-
        /* no_rearm is set and checked under the tchain lock.  It is cleared when
         * unset completes, outside the lock.  That is safe since we know the alarm
         * service is no longer aware of waiter (either the handler ran or we
index 3cd3cee..4769f9e 100644 (file)
@@ -423,14 +423,6 @@ static bool __tc_unset_alarm(struct timer_chain *tchain,
                return true;
        }
 
-       /* A common case is that it already finished.  We need the CV lock farther
-        * below so that we don't miss the signal.  You don't need it if you can see
-        * the signal (is_running == false) is already sent. */
-       if (!waiter->is_running) {
-               spin_pdr_unlock(&tchain->lock);
-               return false;
-       }
-
        /* no_rearm is set and checked under the tchain lock.  It is cleared when
         * unset completes, outside the lock.  That is safe since we know the alarm
         * service is no longer aware of waiter (either the handler ran or we