Here is a new version of oprofile against 2.5.39. Thanks Andi,
Christoph, and Alan for your comments. I think I should have fixed
the things you mentioned.
As before, the full patch is available here :
http://oprofile.sf.net/oprofile-2.5/oprofile-2.5-all.diff [100k]
with usage notes :
http://oprofile.sf.net/oprofile-2.5.html
and more readable broken-out patches (but not applyable) :
http://oprofile.sf.net/oprofile-2.5/
Changes from last time :
o More comments on the fiddly bits
o Fix for NMI on shutdown
o Added stats
o avoid some mis-attributed samples
o Re-worked FS stuff
o Moved x86 stuff into arch/
o Fixed UP build
o various fixes and cleanups
regards
john
John Levon <[email protected]> writes:
Can you explain what you need the context switch hook for ?
I don't think it's a good idea to put a hook at such a critical place.
2.4 oprofile worked without such a hook, no ?
-Andi
On Sun, Sep 29, 2002 at 04:29:14AM +0200, Andi Kleen wrote:
> Can you explain what you need the context switch hook for ?
Hmm, I tried to explain this in comments in the patch ...
> I don't think it's a good idea to put a hook at such a critical place.
... but I obviously didn't do a very good job.
We need a context to look up the EIP against when we process each sample
in buffer_sync.c. We could just log current at sample time along with
EIP/event, but why would it be preferrable to just logging the same
information once when it's needed ?
Basically it's a matter of :
task_struct *
EIP/Event
EIP/Event
EIP/Event
EIP/Event
....
versus
task_struct */EIP/Event
task_struct */EIP/Event
task_struct */EIP/Event
task_struct */EIP/Event
task_struct */EIP/Event
....
Where task_struct is the same as the previous entry for the vast
majority of entries.
> 2.4 oprofile worked without such a hook, no ?
Sure, but it was ugly as hell (and worked completely differently)
regards
john
--
"When your name is Winner, that's it. You don't need a nickname."
- Loser Lane
>
> Basically it's a matter of :
>
> task_struct *
> EIP/Event
> EIP/Event
> EIP/Event
> EIP/Event
> ....
I think you can easily do that by keeping state per cpu in the
NMI handler.
if (current == __get_cpu_var(oprofile_cpustate)) {
/* log current */
__get_cpu_var(oprofile_cpustate) = current;
} else {
/* do nothing */
}
/* log EIP */
[or when you are an module use an cache line padded array indexed with
smp_processor_id - per cpu data doesn't work from modules]
This is even more efficient because when the NMI rate is lower than
the task switch frequency (which is not unlikely) then you'll avoid
many useless task_struct loggings.
-Andi
> I think you can easily do that by keeping state per cpu in the
> NMI handler.
>
> if (current == __get_cpu_var(oprofile_cpustate)) {
^^
Should be != of course.
-Andi
> /* log current */
> __get_cpu_var(oprofile_cpustate) = current;
> } else {
> /* do nothing */
> }
> /* log EIP */
On Sun, Sep 29, 2002 at 05:08:07AM +0200, Andi Kleen wrote:
> I think you can easily do that by keeping state per cpu in the
> NMI handler.
You're quite right, it's simpler as well.
I had a distinct memory of not doing this because it wouldn't work
originally, but I forget what the reasoning was.
> smp_processor_id - per cpu data doesn't work from modules]
yes, which was why I don't use it already.
[Are there any advantages to per_cpu stuff over hand-coding, other
than readability ??]
thanks
john
--
"When your name is Winner, that's it. You don't need a nickname."
- Loser Lane
Andi Kleen wrote:
>
> smp_processor_id
Using smp_processor_id() is usually a bug.
Please default to using get_cpu()/put_cpu().
- It pins the caller onto that CPU if the kernel is preemptive.
- You may not sleep inside get_cpu/put_cpu.
- If you do something which might sleep inside get_cpu/put_cpu,
you get might_sleep() warnings.
Probably, smp_processor_id() needs to be renamed to something
which is hard to type.
On Sat, Sep 28, 2002 at 08:47:06PM -0700, Andrew Morton wrote:
> Using smp_processor_id() is usually a bug.
Well, this is in-interrupt code.
> - If you do something which might sleep inside get_cpu/put_cpu,
> you get might_sleep() warnings.
Certainly handy (but I don't see it's useful in this particular code,
really)
regards
john
--
"When your name is Winner, that's it. You don't need a nickname."
- Loser Lane
> [Are there any advantages to per_cpu stuff over hand-coding, other
> than readability ??]
As well as allocating the areas on separate cachelines and packing it
all together so you dont waste 1 cacheline minus a small amount times
NR_CPUS of memory, on NUMA boxes we will allocate the areas from local
memory.
Oh yeah we can also do some tricks to make the address generation
slightly quicker (eg we will probably dedicate r13 to be a pointer
to the start of the per cpu area on ppc64).
Anton