Use FD taps for event delivery for #alarm
authorBarret Rhoden <brho@cs.berkeley.edu>
Mon, 11 Apr 2016 17:57:54 +0000 (13:57 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Fri, 15 Apr 2016 14:29:13 +0000 (10:29 -0400)
It turns out that FD taps work for #alarm, and they do a better job than
the old ev_q.  Now the user can tell us what it wants to hear for a
particular tap (e.g. the alarmid) instead of the kernel having to imply
that.  Plus, now we use just the one mechanism for asking the kernel to
send to an event when something happens (FD tap).

This required a few changes to userspace.  The subtlest is that
devalarm_set_evq takes timerfd instead of ctlfd.  That's actually a sign
that the API might be too coupled to the implementation.  The nicer
cleanups in userspace are:

- tests/alarm.c and PVC alarms use the devalarm interfaces (those functions
  predated that interface)
- devalarm provides a way to extract the ID, instead of functions checking
  ev_msg->ev_arg2

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

index 254ed77..5e1907e 100644 (file)
@@ -4,16 +4,18 @@
  *
  * #alarm: a device for registering per-process alarms.
  *
- * Allows a process to set up alarms, where the kernel will send an event to a
- * posted ev_q at a certain TSC time.
+ * Allows a process to set up alarms, which they can tap to get events at a
+ * certain TSC time.
  *
  * Every process has their own alarm sets and view of #alarm; gen and friends
  * look at current's alarmset when it is time to gen or open a file.
  *
  * To use, first open #alarm/clone, and that gives you an alarm directory aN,
- * where N is ID of the alarm.  ctl takes two commands: "evq POINTER" (to tell
- * the kernel the pointer of your ev_q) and "cancel", to stop an alarm.  timer
- * takes just the value (in absolute tsc time) to fire the alarm.
+ * where N is ID of the alarm.
+ *
+ * ctl takes one command: "cancel", to stop an alarm.  timer takes just the
+ * value (in absolute tsc time) to fire the alarm.  To find out about the timer
+ * firing, put an FD tap on the timer for FDTAP_FILT_WRITTEN.
  *
  * While each process has a separate view of #alarm, it is possible to post a
  * chan to Qctl or Qtimer to #srv.  If another proc has your Qtimer, it can set
@@ -50,7 +52,6 @@
 #include <kref.h>
 #include <atomic.h>
 #include <alarm.h>
-#include <event.h>
 #include <umem.h>
 #include <devalarm.h>
 
@@ -90,19 +91,24 @@ static void alarm_release(struct kref *kref)
        kfree(a);
 }
 
+static void alarm_fire_taps(struct proc_alarm *a, int filter)
+{
+       struct fd_tap *tap_i;
+
+       if (SLIST_EMPTY(&a->fd_taps))
+               return;
+       /* TODO: (RCU) Locking to protect the list and the tap's existence. */
+       spin_lock(&a->tap_lock);
+       SLIST_FOREACH(tap_i, &a->fd_taps, link)
+               fire_tap(tap_i, filter);
+       spin_unlock(&a->tap_lock);
+}
+
 static void proc_alarm_handler(struct alarm_waiter *a_waiter)
 {
        struct proc_alarm *a = container_of(a_waiter, struct proc_alarm, a_waiter);
-       struct event_queue *ev_q = ACCESS_ONCE(a->ev_q);
-       struct event_msg msg;
-       if (!ev_q || !a->proc) {
-               printk("[kernel] proc_alarm, bad ev_q %p or proc %p\n", ev_q, a->proc);
-               return;
-       }
-       memset(&msg, 0, sizeof(struct event_msg));
-       msg.ev_type = EV_ALARM;
-       msg.ev_arg2 = a->id;
-       send_event(a->proc, ev_q, &msg, 0);
+
+       alarm_fire_taps(a, FDTAP_FILT_WRITTEN);
 }
 
 void devalarm_init(struct proc *p)
@@ -253,6 +259,8 @@ static struct chan *alarmopen(struct chan *c, int omode)
                case Qclone:
                        a = kzmalloc(sizeof(struct proc_alarm), MEM_WAIT);
                        kref_init(&a->kref, alarm_release, 1);
+                       SLIST_INIT(&a->fd_taps);
+                       spinlock_init(&a->tap_lock);
                        init_awaiter(&a->a_waiter, proc_alarm_handler);
                        spin_lock(&p->alarmset.lock);
                        a->id = p->alarmset.id_counter++;
@@ -373,17 +381,7 @@ static long alarmwrite(struct chan *c, void *ubuf, long n, int64_t unused)
                        }
                        if (cb->nf < 1)
                                error(EFAIL, "short control request");
-                       if (!strcmp(cb->f[0], "evq")) {
-                               if (cb->nf < 2)
-                                       error(EFAIL, "evq needs a pointer");
-                               /* i think it's safe to do a stroul on a parsecmd.  it's kernel
-                                * memory, and space or 0 terminated */
-                               hexval = strtoul(cb->f[1], 0, 16);
-                               /* This is just to help userspace - event code can handle it */
-                               if (!is_user_rwaddr((void *)hexval, sizeof(struct event_queue)))
-                                       error(EFAIL, "Non-user ev_q pointer");
-                               p_alarm->ev_q = (struct event_queue *)hexval;
-                       } else if (!strcmp(cb->f[0], "cancel")) {
+                       if (!strcmp(cb->f[0], "cancel")) {
                                unset_alarm(p_alarm->proc->alarmset.tchain, &p_alarm->a_waiter);
                        } else {
                                error(EFAIL, "%s: not implemented", cb->f[0]);
@@ -414,6 +412,62 @@ static long alarmwrite(struct chan *c, void *ubuf, long n, int64_t unused)
        return n;
 }
 
+static int alarm_tapfd(struct chan *c, struct fd_tap *tap, int cmd)
+{
+       int ret;
+       struct proc_alarm *a = QID2A(c->qid);
+
+       /* We don't actually support HANGUP, but epoll implies it. */
+       #define ALARM_LEGAL_TIMER_TAPS (FDTAP_FILT_WRITTEN | FDTAP_FILT_HANGUP)
+
+       switch (TYPE(c->qid)) {
+       case Qtimer:
+               if (tap->filter & ~ALARM_LEGAL_TIMER_TAPS) {
+                       set_error(ENOSYS, "Unsupported #%s tap, must be %p", devname(),
+                                         ALARM_LEGAL_TIMER_TAPS);
+                       return -1;
+               }
+               spin_lock(&a->tap_lock);
+               switch (cmd) {
+               case (FDTAP_CMD_ADD):
+                       SLIST_INSERT_HEAD(&a->fd_taps, tap, link);
+                       ret = 0;
+                       break;
+               case (FDTAP_CMD_REM):
+                       SLIST_REMOVE(&a->fd_taps, tap, fd_tap, link);
+                       ret = 0;
+                       break;
+               default:
+                       set_error(ENOSYS, "Unsupported #%s tap command %p",
+                                         devname(), cmd);
+                       ret = -1;
+               }
+               spin_unlock(&a->tap_lock);
+               return ret;
+       default:
+               set_error(ENOSYS, "Can't tap #%s file type %d", devname(),
+                         c->qid.path);
+               return -1;
+       }
+}
+
+static char *alarm_chaninfo(struct chan *ch, char *ret, size_t ret_l)
+{
+       struct proc_alarm *a = QID2A(ch->qid);
+
+       switch (TYPE(ch->qid)) {
+       case Qctl:
+       case Qtimer:
+               snprintf(ret, ret_l, "Id %d, %s, expires %llu", a->id,
+                        SLIST_EMPTY(&a->fd_taps) ? "untapped" : "tapped",
+                        a->a_waiter.wake_up_time);
+               break;
+       default:
+               return devchaninfo(ch, ret, ret_l);
+       }
+       return ret;
+}
+
 struct dev alarmdevtab __devtab = {
        .name = "alarm",
 
@@ -433,5 +487,6 @@ struct dev alarmdevtab __devtab = {
        .remove = alarmremove,
        .wstat = alarmwstat,
        .power = devpower,
-       .chaninfo = devchaninfo,
+       .chaninfo = alarm_chaninfo,
+       .tapfd = alarm_tapfd,
 };
index 53a8938..7a0a1fe 100644 (file)
@@ -9,8 +9,8 @@
 #include <sys/queue.h>
 #include <kref.h>
 #include <alarm.h>
-#include <event.h>
 #include <atomic.h>
+#include <fdtap.h>
 
 struct proc_alarm {
        TAILQ_ENTRY(proc_alarm)         link;
@@ -18,7 +18,8 @@ struct proc_alarm {
        struct kref                                     kref;
        struct alarm_waiter                     a_waiter;
        struct proc                                     *proc;
-       struct event_queue                      *ev_q;
+       spinlock_t                                      tap_lock;
+       struct fdtap_slist                      fd_taps;
 };
 TAILQ_HEAD(proc_alarm_list, proc_alarm);
 
index 8f3300c..68b310d 100644 (file)
@@ -10,6 +10,7 @@
 #include <unistd.h>
 #include <parlib/event.h>
 #include <benchutil/measure.h>
+#include <benchutil/alarm.h>
 #include <parlib/uthread.h>
 #include <parlib/timing.h>
 
@@ -24,39 +25,18 @@ static void handle_alarm(struct event_msg *ev_msg, unsigned int ev_type,
                          void *data)
 {
        assert(ev_type == EV_ALARM);
-       printf("\tAlarm fired!, id %d\n", ev_msg ? ev_msg->ev_arg2 : 55555);
+       printf("\tAlarm fired!, id %p\n", devalarm_get_id(ev_msg));
 }
 
 int main(int argc, char **argv)
 {
        int ctlfd, timerfd, alarm_nr, ret, srvfd;
        char buf[20];
-       char path[32];
        struct event_queue *ev_q;
 
        printf("Starting alarm test\n");
-       /* standard 9ns stuff: clone and read it to get our path, ending up with the
-        * ctlfd and timerfd for #alarm/aN/{ctl,timer}.  if you plan to fork, you
-        * can open CLOEXEC. */
-       ctlfd = open("#alarm/clone", O_RDWR | O_CLOEXEC);
-       if (ctlfd < 0) {
-               perror("Can't clone an alarm");
-               exit(-1);
-       }
-       ret = read(ctlfd, buf, sizeof(buf) - 1);
-       if (ret <= 0) {
-               if (!ret)
-                       printf("Got early EOF from ctl\n");
-               else
-                       perror("Can't read ctl");
-               exit(-1);
-       }
-       buf[ret] = 0;
-       snprintf(path, sizeof(path), "#alarm/a%s/timer", buf);
-       /* Don't open CLOEXEC if you want to post it to srv later */
-       timerfd = open(path, O_RDWR);
-       if (timerfd < 0) {
-               perror("Can't open timer");
+       if (devalarm_get_fds(&ctlfd, &timerfd, 0)) {
+               perror("Alarm setup");
                exit(-1);
        }
        /* Since we're doing SPAM_PUBLIC later, we actually don't need a big ev_q.
@@ -72,31 +52,23 @@ int main(int argc, char **argv)
         * we have concurrent preempts/yields. */
        ev_q->ev_flags = EVENT_IPI | EVENT_SPAM_PUBLIC | EVENT_WAKEUP;
        /* Register the ev_q for our alarm */
-       ret = snprintf(path, sizeof(path), "evq %llx", ev_q);
-       ret = write(ctlfd, path, ret);
-       if (ret <= 0) {
-               perror("Failed to write ev_q");
+       if (devalarm_set_evq(timerfd, ev_q, 0xdeadbeef)) {
+               perror("Failed to set evq");
                exit(-1);
        }
        /* Try to set, then cancel before it should go off */
-       ret = snprintf(buf, sizeof(buf), "%llx", read_tsc() + sec2tsc(1));
-       ret = write(timerfd, buf, ret);
-       if (ret <= 0) {
+       if (devalarm_set_time(timerfd, read_tsc() + sec2tsc(1))) {
                perror("Failed to set timer");
                exit(-1);
        }
-       ret = snprintf(buf, sizeof(buf), "cancel");
-       ret = write(ctlfd, buf, ret);
-       if (ret <= 0) {
+       if (devalarm_disable(ctlfd)) {
                perror("Failed to cancel timer");
                exit(-1);
        }
        uthread_sleep(2);
        printf("No alarm should have fired yet\n");
        /* Try to set and receive */
-       ret = snprintf(buf, sizeof(buf), "%llx", read_tsc() + sec2tsc(1));
-       ret = write(timerfd, buf, ret);
-       if (ret <= 0) {
+       if (devalarm_set_time(timerfd, read_tsc() + sec2tsc(1))) {
                perror("Failed to set timer");
                exit(-1);
        }
index 553181c..db70fe4 100644 (file)
@@ -42,9 +42,9 @@ int main(int argc, char **argv)
                return -1;
        if (devalarm_get_fds(&ctlfd2, &timerfd2, 0))
                return -1;
-       if (devalarm_set_evq(ctlfd1, evq1))
+       if (devalarm_set_evq(timerfd1, evq1, 0))
                return -1;
-       if (devalarm_set_evq(ctlfd2, evq2))
+       if (devalarm_set_evq(timerfd2, evq2, 0))
                return -1;
        now = read_tsc();
        /* with this setup and the early sleep, two fires, then one.  but we'll
index f76c596..f740877 100644 (file)
@@ -72,13 +72,17 @@ int devalarm_get_fds(int *ctlfd_r, int *timerfd_r, int *alarmid_r)
        return 0;
 }
 
-int devalarm_set_evq(int ctlfd, struct event_queue *ev_q)
+int devalarm_set_evq(int timerfd, struct event_queue *ev_q, int alarmid)
 {
-       int ret;
-       char path[32];
-       ret = snprintf(path, sizeof(path), "evq %llx", ev_q);
-       ret = write(ctlfd, path, ret);
-       if (ret <= 0)
+       struct fd_tap_req tap_req = {0};
+
+       tap_req.fd = timerfd;
+       tap_req.cmd = FDTAP_CMD_ADD;
+       tap_req.filter = FDTAP_FILT_WRITTEN;
+       tap_req.ev_id = EV_ALARM;
+       tap_req.ev_q = ev_q;
+       tap_req.data = (void*)(long)alarmid;
+       if (sys_tap_fds(&tap_req, 1) != 1)
                return -1;
        return 0;
 }
@@ -94,6 +98,13 @@ int devalarm_set_time(int timerfd, uint64_t tsc_time)
        return 0;
 }
 
+int devalarm_get_id(struct event_msg *ev_msg)
+{
+       if (!ev_msg)
+               return -1;
+       return (int)(long)ev_msg->ev_arg3;
+}
+
 int devalarm_disable(int ctlfd)
 {
        int ret = write(ctlfd, "cancel", sizeof("cancel"));
@@ -162,7 +173,7 @@ static void init_alarm_service(void)
        reset_tchain_times(&global_tchain);
 
        if (devalarm_get_fds(&ctlfd, &timerfd, &alarmid)) {
-               perror("devalarm_get_fds");
+               perror("Useralarm: devalarm_get_fds");
                return;
        }
        /* Since we're doing SPAM_PUBLIC later, we actually don't need a big ev_q.
@@ -177,7 +188,7 @@ static void init_alarm_service(void)
         * __trigger can handle spurious upcalls.  If it ever is not okay, then use
         * an INDIR (probably with SPAM_INDIR too) instead of SPAM_PUBLIC. */
        ev_q->ev_flags = EVENT_IPI | EVENT_SPAM_PUBLIC | EVENT_WAKEUP;
-       if (devalarm_set_evq(ctlfd, ev_q)) {
+       if (devalarm_set_evq(timerfd, ev_q, alarmid)) {
                perror("set_alarm_evq");
                return;
        }
@@ -324,7 +335,7 @@ static void handle_user_alarm(struct event_msg *ev_msg, unsigned int ev_type,
                               void *data)
 {
        assert(ev_type == EV_ALARM);
-       if (ev_msg && (ev_msg->ev_arg2 == global_tchain.alarmid))
+       if (devalarm_get_id(ev_msg) == global_tchain.alarmid)
                __trigger_tchain(&global_tchain);
 }
 
index 79fd3f8..e032693 100644 (file)
@@ -41,8 +41,9 @@ __BEGIN_DECLS
 /* Low-level alarm interface */
 
 int devalarm_get_fds(int *ctlfd_r, int *timerfd_r, int *alarmid_r);
-int devalarm_set_evq(int ctlfd, struct event_queue *ev_q);
+int devalarm_set_evq(int timerfd, struct event_queue *ev_q, int alarmid);
 int devalarm_set_time(int timerfd, uint64_t tsc_time);
+int devalarm_get_id(struct event_msg *ev_msg);
 int devalarm_disable(int ctlfd);
 
 /* Alarm service */
index 46d8948..7affaa9 100644 (file)
@@ -17,6 +17,7 @@
 #include <fcntl.h>
 #include <unistd.h>
 #include <benchutil/pvcalarm.h>
+#include <benchutil/alarm.h>
 
 /* Different states for enabling/disabling the per-vcore alarms. */
 enum {
@@ -78,11 +79,7 @@ static int init_global_pvcalarm()
  * time */
 static void run_pvcalarm(struct pvcalarm_data *pvcalarm_data, uint64_t offset)
 {
-       int ret;
-       char buf[20];
-       ret = snprintf(buf, sizeof(buf), "%llx", read_tsc() + offset);
-       ret = write(pvcalarm_data->timerfd, buf, ret);
-       if (ret <= 0) {
+       if (devalarm_set_time(pvcalarm_data->timerfd, read_tsc() + offset)) {
                perror("Useralarm: Failed to set timer");
                return;
        }
@@ -100,9 +97,7 @@ static void start_pvcalarm(struct pvcalarm_data *pvcalarm_data, uint64_t offset)
 /* Stop the pvc alarm associated with pvcalarm_data */
 static void stop_pvcalarm(struct pvcalarm_data *pvcalarm_data)
 {
-       int ret;
-       ret = write(pvcalarm_data->ctlfd, "cancel", sizeof("cancel"));
-       if (ret <= 0) {
+       if (devalarm_disable(pvcalarm_data->ctlfd)) {
                printf("Useralarm: unable to disarm alarm!\n");
                return;
        }
@@ -178,30 +173,11 @@ int disable_pvcalarms()
  * online and the pvcalarm service is active */
 static void init_pvcalarm(struct pvcalarm_data *pvcalarm_data, int vcoreid)
 {
-       int ctlfd, timerfd, alarmid, ev_flags, ret;
-       char buf[20];
-       char path[32];
+       int ctlfd, timerfd, alarmid, ev_flags;
        struct event_queue *ev_q;
 
-       ctlfd = open("#alarm/clone", O_RDWR | O_CLOEXEC);
-       if (ctlfd < 0) {
-               perror("Pvcalarm: Can't clone an alarm");
-               return;
-       }
-       ret = read(ctlfd, buf, sizeof(buf) - 1);
-       if (ret <= 0) {
-               if (!ret)
-                       printf("Pvcalarm: Got early EOF from ctl\n");
-               else
-                       perror("Pvcalarm: Can't read ctl");
-               return;
-       }
-       buf[ret] = 0;
-       alarmid = atoi(buf);
-       snprintf(path, sizeof(path), "#alarm/a%s/timer", buf);
-       timerfd = open(path, O_RDWR | O_CLOEXEC);
-       if (timerfd < 0) {
-               perror("Pvcalarm: Can't open timer");
+       if (devalarm_get_fds(&ctlfd, &timerfd, &alarmid)) {
+               perror("Pvcalarm: alarm setup");
                return;
        }
        register_ev_handler(EV_ALARM, handle_pvcalarm, 0);
@@ -213,10 +189,8 @@ static void init_pvcalarm(struct pvcalarm_data *pvcalarm_data, int vcoreid)
        }
        ev_q->ev_vcore = vcoreid;
        ev_q->ev_flags = ev_flags;
-       ret = snprintf(path, sizeof(path), "evq %llx", ev_q);
-       ret = write(ctlfd, path, ret);
-       if (ret <= 0) {
-               perror("Pvcalarm: Failed to write ev_q");
+       if (devalarm_set_evq(timerfd, ev_q, alarmid)) {
+               perror("Pvcalarm: Failed to set evq");
                return;
        }
        /* now the alarm is all set, just need to write the timer whenever we want
@@ -261,7 +235,8 @@ static void handle_pvcalarm(struct event_msg *ev_msg, unsigned int ev_type,
                             void *data)
 {
        struct pvcalarm_data *pvcalarm_data = &global_pvcalarm.data[vcore_id()];
-       if (!ev_msg || (ev_msg->ev_arg2 != pvcalarm_data->alarmid))
+
+       if (devalarm_get_id(ev_msg) != pvcalarm_data->alarmid)
                return;
        if (!__vcore_preamble()) return;
        global_pvcalarm.handler(ev_msg, ev_type, data);