Re: [RFC][PATCH 00/11] perf pmu interface -v2

[Peter Zijlstra <peterz@infradead.org>]

On Fri, 2010-07-02 at 11:57 +0900, Paul Mundt wrote:
> At the moment it's not an issue since we have big enough counters that
> overflows don't really happen, especially if we're primarily using them
> for one-shot measuring.
> 
> SH-4A style counters behave in such a fashion that we have 2 general
> purpose counters, and 2 counters for measuring bus transactions. These
> bus counters can optionally be disabled and used in a chained mode to
> provide the general purpose counters a 64-bit counter (the actual
> validity in the upper half of the chained counter varies depending on the
> CPUs, but all of them can do at least 48-bits when chained). 

Right, so I was reading some of that code and I couldn't actually find
where you keep consistency between the hardware counter value and the
stored prev_count value.

That is, suppose I'm counting, the hardware starts at 0, hwc->prev_count
= 0 and event->count = 0.

At some point, x we context switch this task away, so we ->disable(),
which disables the counter and updates the values, so at that time
hwc->prev = x and event->count = x, right?

Now suppose we schedule the task back in, so we do ->enable(), then what
happens? sh_pmu_enable() finds an unused index, (disables it for some
reason.. it should already be cleared if its not used, but I guess a few
extra hardware writes dont hurt) and calls sh4a_pmu_enable() on it.

sh4a_pmu_enable() does 3 writes:

  PPC_PMCAT -- does this clear the counter value?
  PPC_CCBR  -- writes the ->config bits
  PPC_CCBR  (adds CCBR_DUC, couldn't this be done in the 
             previous write to this reg?)

Now assuming that enable does indeed clear the hardware counter value,
shouldn't you also set hwc->prev_count to 0 again? Otherwise the next
update will see a massive jump?

Alternatively you could write the hwc->prev_count value back to the
register.

If you eventually want to drop the chained counter support I guess it
would make sense to have sh_perf_event_update() read and clear the
counter so that you're always 0 based and then enforce an update from
the arch tick hander so you never overflow.