Re: [PATCH v6 4/7] MIPS: add support for hardware performance events (skeleton)

[Matt Fleming <matt@console-pimps.org>]

On Thu, Sep 23, 2010 at 03:39:51PM +0800, Deng-Cheng Zhu wrote:
> 2010/9/22 Matt Fleming <matt@console-pimps.org>:
> > I'm probably just being stupid but I can't figure out what this snippet
> > of code is doing. You're checking to see if the counter has overflowed,
> > and if it has then you just clear the overflow bit? I would have
> > expected you to reset the counter to 0.
> [DC]: Maybe my original code comment for the member msbs of the struct
> cpu_hw_events is too simple. And here is more: Unlike the perf counters in
> some other architectures, a 32bit MIPS perf counter, for example, will
> generate an interrupt after 0x7fffffff. But we want the operation to look
> like: 0x0 -> 0xffffffff -> interrupt. So there's a "pseudo" signal halfway.
> Please also note that the counter value will be brought back to 0 soon
> after it reaches 0x80000000 *each time*.

Ah OK, thanks.

> > Having conditional code like this is a pretty sure sign that you haven't
> > separated support for the various performance hardware properly. Have
> > you had a look at how SH uses a registration interface to register
> > sh_pmus?  Ideally all the internals for each type of perfcounter
> > hardware should be in their own file.
> [DC]: It does look ugly. It should be easy to put conditional code into
> perf_event_[mipsxx|loongson2|rm9000].c. I'll post a patch to fix it when
> the whole thing is accepted.

The potential problem with doing a cleanup patch after this series has
been merged is that it will modify most of the code in this patch
series - essentially rewriting it. I don't have a strong opinion
either way but Ralf may.

> > SH also has this problem that it doesn't have any sort of performance
> > counter interrupt and so can't check for overflow. This lack of
> > interrupt really needs to be solved generically in perf as it's a
> > problem that effects quite a few architectures. I suspect that using a
> > hrtimer instead of piggy-backing the timer interrupt would make the core
> > perf guys happier. Writing support for this is on my ever-growing list
> > of things todo. I've already started on some patches for the perf tool
> > so that it's possible to sample counters even though there is no
> > periodic interrupt (but that's a different problem).
> [DC]: Please add me into your post list when your patches are ready :-)
> Finally, thanks for your time to review the code.

Sure, I will do. No problem!