prof: fix disgusting synchronzation
authorBarret Rhoden <brho@cs.berkeley.edu>
Sun, 12 May 2019 22:00:43 +0000 (18:00 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Sun, 12 May 2019 22:00:43 +0000 (18:00 -0400)
commit37a590d0f6db27a67fe74515e0db95f5859b5d9f
tree885a5716ca932cf35f9278689dafd8c1b124881c
parent299f4b5e23e34faa6225823d294be431898f189b
prof: fix disgusting synchronzation

The profiler's sync code was always an annoyance - the first version was
nasty and I found bugs with it.  The second version (current code) had
layers of krefs and qlocks and whatnot, obfuscating things.  I didn't
see the bug at the time, but syzbot found the bug.  Absence of evidence
is not the evidence of absence.

The root of the issue was that the kref was used to handle the
tear-down, but the open / setup code assumed a close / cleanup was
completed.  If a core kept a kref around between a quick close-open
pair, then we'd crash.  Specifically, this would die:

Core 1 Core 2
-------------------------------------
open
  set ref = 1
start BT sample
kref_get (ref == 2)
close
  decref, ref == 1

open
  panic!  (ref != 0)
eventually decref

Fixing it requires blocking close / cleanup until all of the users are
done (users being the sampler pushers).  That's a great use for RCU.

This commit replaces the kref with RCU, and fixes a bunch of other nasty
things along the way.  To do it properly, I created struct profiler to
encapsulate the qio and the per-core array, and protected that entire
thing with RCU.

This resulted in a lot of cleanups.  Various assertions and checks for
the existence of profiler_queue and the cpu buffer array all disappear.
I also explicitly documented the synchronization rules, and got rid of
the superfluous profiler mutex.

Additionally, a lot of the error handling goes away.  Some of it was due
to completely unnecessary use of waserror() - e.g. catching errors for
functions that don't throw.  There was also an unmatched waserror() in
the old code.  That's all gone.

Other minor fixups:
- kprof couldn't handle an error in profiler_setup().  If it failed,
we'd just say the profiler was opened.
- kmalloc(x, 0) -> kmalloc(x, MEM_ATOMIC)
- WRITE_ONCE() for the unsynched configuration variable writes.  Note
that no one in our codebase even uses those, and their usage is a little
hokey.

Anyway, there might be some bugs here still, but hopefully it's a little
better and more clear.

Reported-by: syzbot+e8998ed113c341947438@syzkaller.appspotmail.com
Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/drivers/dev/kprof.c
kern/include/profiler.h
kern/src/profiler.c