perf: Shutdown profiler/sampling on kpctl close
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 14 Jun 2016 20:48:57 +0000 (16:48 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Fri, 17 Jun 2016 16:17:54 +0000 (12:17 -0400)
There are two options for stopping the profiler: manually with a stop
command or by closing the FD.  We now support both.

The issue is a generic one that affects all of the Plan 9 devices, and the
root of the problem is how to deal with errors.  Let's use kprof as an
example.

Previously, to stop the profiler, you'd need to `echo stop > kpctl`.  This
works fine, so long as some program actually does this.  If for some reason
your program dies, e.g. perf crashes or exits with an error, it does not
stop the profiler.

If you do `perf record sleep 10 &; kill $PERF-PID`, then the profiler will
keep running.  (Side note: devarch's 'perf' file shuts down on close, so
we're only collecting mmap and comm events).  We're stuck waiting on an
event that will never occur.

The way to deal with this is to shutdown when the FD is closed.  This works
because the FD is implicitly closed when the process exits for any reason.
This also could work in a distributed system - eventually we can time out a
mount RPC and shut things down.  So by tying teardown into closing the FD,
we get fault tolerance with one mechanism.

The downside to using a close() as the signal to shutdown is you can no
longer easily control the device from the shell.  If you 'echo start >
kpctl`, it will start and then immediately stop when `echo` exits and its
FDs are closed.  That is the tradeoff.

This is not a new tradeoff in Plan 9.  The same thing happens with the IP
stack.  If you `cat /net/tcp/clone`, you'll create the conversation, but
the conversation closes as soon as `cat` exits.  I think the netlogger has
a similar issue (need to keep it open for logging to work).  Basically,
devices vary!

Ultimately, the power and value of the text based interface to these
devices isn't actually that it is scriptable, but that it is machine
independent.  Scripting is a nice convenience.

Oh, I kept the "stop" command around so that we can stop and start the
profiling without having to close the FD.  You want close() to work as an
option/failsafe, not as the only way to stop the device.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/drivers/dev/kprof.c
tools/profile/perf/perf.c
tools/profile/perf/perf_core.c

index 453e195..387ea01 100644 (file)
@@ -51,6 +51,7 @@ struct kprof {
        struct alarm_waiter *alarms;
        bool mpstat_ipi;
        bool profiling;
+       bool opened;
        char *pdata;
        size_t psize;
 };
@@ -231,6 +232,7 @@ static void kprof_init(void)
 
        qlock_init(&kprof.lock);
        kprof.profiling = FALSE;
+       kprof.opened = FALSE;
        kprof.pdata = NULL;
        kprof.psize = 0;
 
@@ -316,6 +318,20 @@ static struct chan *kprof_open(struct chan *c, int omode)
                if (openmode(omode) != O_READ)
                        error(EPERM, ERROR_FIXME);
        }
+       switch ((int) c->qid.path) {
+       case Kprofctlqid:
+               /* We have one global profiler.  Only one FD may be opened at a time for
+                * it.  If we ever have separate profilers, we can create the profiler
+                * here, and every open would get a separate instance. */
+               qlock(&kprof.lock);
+               if (kprof.opened) {
+                       qunlock(&kprof.lock);
+                       error(EBUSY, "Global profiler is already open");
+               }
+               kprof.opened = TRUE;
+               qunlock(&kprof.lock);
+               break;
+       }
        c->mode = openmode(omode);
        c->flag |= COPEN;
        c->offset = 0;
@@ -324,6 +340,14 @@ static struct chan *kprof_open(struct chan *c, int omode)
 
 static void kprof_close(struct chan *c)
 {
+       if (c->flag & COPEN) {
+               switch ((int) c->qid.path) {
+               case Kprofctlqid:
+                       kprof_stop_profiler();
+                       kprof.opened = FALSE;
+                       break;
+               }
+       }
 }
 
 static long mpstat_read(void *va, long n, int64_t off)
index 40d838d..de5e774 100644 (file)
@@ -414,6 +414,9 @@ int main(int argc, char *argv[])
        }
        if (i == COUNT_OF(perf_cmds))
                global_usage();
+       /* This cleanup is optional - they'll all be dealt with when the program
+        * exits.  This means its safe for us to exit(-1) at any point in the
+        * program. */
        perf_free_context(pctx);
        perfconv_free_context(cctx);
        perf_finalize();
index 915d92c..88e8e1c 100644 (file)
@@ -552,11 +552,8 @@ struct perf_context *perf_create_context(const struct perf_context_config *cfg)
 
 void perf_free_context(struct perf_context *pctx)
 {
-       for (int i = 0; i < pctx->event_count; i++)
-               perf_close_event(pctx->perf_fd, pctx->events[i].ped);
-       perf_disable_sampling(pctx->kpctl_fd);
-       close(pctx->kpctl_fd);
-       close(pctx->perf_fd);
+       close(pctx->kpctl_fd);  /* disabled sampling */
+       close(pctx->perf_fd);   /* closes all events */
        free(pctx);
 }