On Thu, Sep 23, 2010 at 03:39:51PM +0800, Deng-Cheng Zhu wrote: > 2010/9/22 Matt Fleming <matt@xxxxxxxxxxxxxxxxx>: > > 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!
