perf: Fix event enabling logic
authorBarret Rhoden <brho@cs.berkeley.edu>
Fri, 20 May 2016 16:25:21 +0000 (12:25 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Thu, 16 Jun 2016 15:48:37 +0000 (11:48 -0400)
commit2edf9e2326b3d95c34660afa0fecdfb17b9ace26
tree26f58497c6b072c17fad5f7c7e080924b1f56a89
parentb54b815425d2c37d630f4475921ec6dcb82706b2
perf: Fix event enabling logic

There were a bunch of subtle problems here.

First, we should never write 'pa' in perfmon_do_cores_alloc(), or anywhere
that is shared and racy.  But you'll notice that technically we weren't:

PMEV_SET_EN(cctx->fixed_counters[i].event, 1);

That's the local copy.  But, when we called the function that would have
used the Enable bit, we actually used the old shared one again!

tmp = perfmon_get_fixevent_mask(&pa->ev, i, fxctrl_value);

So now we just copy in and use a local variable, and to keep things clear,
we don't make changes to the event on a per-core basis.  If we have to in
the future, we'll operate on the local copy.

Second, the meaning of the Enable bit was wrong for fixed counters - it was
setting the interrupt (PMI).  We have a separate bit for that.

Third, it wasn't clear why we needed to enforce that Enable was on.  It's a
nice thing to do, but it looks like we need at least one bit set so the
kernel doesn't get confused about whether a counter is being used or not.

Next (fourthly?), we weren't clearing the overflow when we wanted an
interrupt.  If we ever had a situation where the counter had an old
overflow (say, it wasn't in sampling mode and just happened to overflow),
then we wouldn't get our interrupt.  We were doing this when we received an
actual interrupt - just not when we first set it up.  While we're at it, we
should be clearing overflow regardless of whether or not we want an
interrupt.  The regular counting style counter should track that too.

While we're at it, we shouldn't set trigger_count to whatever the user
asked for if the interrupt isn't on.  They probably said 0, but we might as
well make sure it is 0.

Finally, the perf counters need to be enabled twice.  We were doing it, but
for some reason we'd enable it in one place, then set some values, then set
it in the other place.  Technically the counter isn't enabled until this
last setting, but it's pretty ugly.  Now we just set the counter settings,
then turn it on.

It's also just nasty to have:

perfmon_enable_fix_event((int) ccno, FALSE);

Enable?  No, actually disable!  There are now two functions for that.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/arch/x86/perfmon.c