Adapt devalarm to the new unset_alarm [2/2]
authorBarret Rhoden <brho@cs.berkeley.edu>
Fri, 15 Jul 2016 19:35:05 +0000 (15:35 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Tue, 19 Jul 2016 15:43:10 +0000 (11:43 -0400)
Devalarm was trying, and failing, to handle the old style of unset_alarm().
Specifically, in its kref release, it was freeing the memory after an
unset.  The unset would succeed, but the RKM was still pending.  I had this
happen; it was fun.

This also fixes a minor bug with firing FD taps outside the lock.  At
least it looked wrong.

All in all, I'm not super happy with the sync in use here.  Here's the
issue.  We need to sync between unset and set.  unset blocks, so we need
some sleeping sync.  We also need some sync between the handler and other
operations (e.g. on 'count' and the timerfd wakeup).  Since unset waits-on
the handler, we can't use the same sync for "unset-to-set" and
"handler-to-others".  Finally, we need something for the timerfd that
allows aborts, such as the CV (or maybe a hacked rendez, but that is just a
CV under the hood).

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/drivers/dev/alarm.c
kern/include/devalarm.h

index dc8b0a9..31a3670 100644 (file)
@@ -118,22 +118,19 @@ static void proc_alarm_handler(struct alarm_waiter *a_waiter)
 {
        struct proc_alarm *a = container_of(a_waiter, struct proc_alarm, a_waiter);
 
 {
        struct proc_alarm *a = container_of(a_waiter, struct proc_alarm, a_waiter);
 
-       /* We need the alarm to *not* hold the tchain lock (i.e. not in IRQ ctx),
-        * o/w we could deadlock.  The lock ordering is alarm_lock->tchain_lock. */
-       assert(!a_waiter->holds_tchain_lock);
        cv_lock(&a->cv);
        a->count++;
        cv_lock(&a->cv);
        a->count++;
-       if (a->should_stop || !a->period) {
+       if (!a->period) {
                a_waiter->wake_up_time = 0;
                a_waiter->wake_up_time = 0;
-               a->armed = FALSE;
        } else {
        } else {
+               /* TODO: use an alarm helper, once we switch over to nsec */
                a_waiter->wake_up_time += a->period;
                set_alarm(a->proc->alarmset.tchain, a_waiter);
        }
        __cv_broadcast(&a->cv);
                a_waiter->wake_up_time += a->period;
                set_alarm(a->proc->alarmset.tchain, a_waiter);
        }
        __cv_broadcast(&a->cv);
-       cv_unlock(&a->cv);
        /* Fires taps for both Qtimer and Qcount. */
        alarm_fire_taps(a, FDTAP_FILT_WRITTEN | FDTAP_FILT_READABLE);
        /* Fires taps for both Qtimer and Qcount. */
        alarm_fire_taps(a, FDTAP_FILT_WRITTEN | FDTAP_FILT_READABLE);
+       cv_unlock(&a->cv);
 }
 
 void devalarm_init(struct proc *p)
 }
 
 void devalarm_init(struct proc *p)
@@ -300,6 +297,7 @@ static struct chan *alarmopen(struct chan *c, int omode)
                        kref_init(&a->kref, alarm_release, 1);
                        SLIST_INIT(&a->fd_taps);
                        cv_init(&a->cv);
                        kref_init(&a->kref, alarm_release, 1);
                        SLIST_INIT(&a->fd_taps);
                        cv_init(&a->cv);
+                       qlock_init(&a->qlock);
                        init_awaiter(&a->a_waiter, proc_alarm_handler);
                        spin_lock(&p->alarmset.lock);
                        a->id = p->alarmset.id_counter++;
                        init_awaiter(&a->a_waiter, proc_alarm_handler);
                        spin_lock(&p->alarmset.lock);
                        a->id = p->alarmset.id_counter++;
@@ -444,29 +442,28 @@ static long alarmread(struct chan *c, void *ubuf, long n, int64_t offset)
 /* Helper, sets the procalarm to hexval (abs TSC ticks).  0 disarms. */
 static void set_proc_alarm(struct proc_alarm *a, uint64_t hexval)
 {
 /* Helper, sets the procalarm to hexval (abs TSC ticks).  0 disarms. */
 static void set_proc_alarm(struct proc_alarm *a, uint64_t hexval)
 {
-       cv_lock(&a->cv);
        /* Due to how we have to maintain 'count', we need to strictly account for
        /* Due to how we have to maintain 'count', we need to strictly account for
-        * the firings of the alarm.  Easiest thing is to disarm it, make sure it is
-        * off, reset everything, then rearm it. */
-       while (a->armed) {
-               a->should_stop = TRUE;
-               if (unset_alarm(a->proc->alarmset.tchain, &a->a_waiter)) {
-                       a->armed = FALSE;
-                       break;
-               }
-               /* We didn't find it on the tchain, which means it has left the chain,
-                * but hasn't fired yet.  We need to block til it fired and disarmed
-                * itself */
-               cv_wait(&a->cv);
-       }
-       a->should_stop = FALSE;
+        * the firings of the alarm.  Easiest thing is to disarm it, reset
+        * everything, then rearm it.  Note that if someone is blocked on count = 0,
+        * they may still be blocked until the next time the alarm fires.
+        *
+        * unset waits on the handler, which grabs the cv lock, so we don't grab the
+        * cv lock.  However, we still need to protect ourselves from multiple
+        * setters trying to run this at once.  Unset actually can handle being
+        * called concurrently, but alarm setters can't, nor can it handle the
+        * unsets and sets getting out of sync.  For instance, two unsets followed
+        * by two sets would be a bug.  Likewise, setting the awaiter value while it
+        * is on a tchain is a bug.  The qlock prevents that. */
+       qlock(&a->qlock);
+       unset_alarm(a->proc->alarmset.tchain, &a->a_waiter);
+       cv_lock(&a->cv);
        a->count = 0;
        if (hexval) {
        a->count = 0;
        if (hexval) {
-               a->armed = TRUE;
                set_awaiter_abs(&a->a_waiter, hexval);
                set_alarm(a->proc->alarmset.tchain, &a->a_waiter);
        }
        cv_unlock(&a->cv);
                set_awaiter_abs(&a->a_waiter, hexval);
                set_alarm(a->proc->alarmset.tchain, &a->a_waiter);
        }
        cv_unlock(&a->cv);
+       qunlock(&a->qlock);
 }
 
 /* Note that in read and write we have an open chan, which means we have an
 }
 
 /* Note that in read and write we have an open chan, which means we have an
index 41f85ac..1b99215 100644 (file)
 struct proc_alarm {
        TAILQ_ENTRY(proc_alarm)         link;
        int                                                     id;
 struct proc_alarm {
        TAILQ_ENTRY(proc_alarm)         link;
        int                                                     id;
-       bool                                            armed;
-       bool                                            should_stop;
        struct kref                                     kref;
        struct alarm_waiter                     a_waiter;
        struct cond_var                         cv;
        struct kref                                     kref;
        struct alarm_waiter                     a_waiter;
        struct cond_var                         cv;
+       qlock_t                                         qlock;
        struct proc                                     *proc;
        struct fdtap_slist                      fd_taps;
        unsigned long                           period;
        struct proc                                     *proc;
        struct fdtap_slist                      fd_taps;
        unsigned long                           period;