Re: [PATCH][RFC] oprofile for 2.5.39

Re: [PATCH][RFC] oprofile for 2.5.39

[Andi Kleen]

John Levon <movement@marcelothewonderpenguin.com> 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

Re: [PATCH][RFC] oprofile for 2.5.39

[John Levon]

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

Re: [PATCH][RFC] oprofile for 2.5.39

[Andi Kleen]

> 
> 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
	

Re: [PATCH][RFC] oprofile for 2.5.39

[Andi Kleen]

> 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 */

Re: [PATCH][RFC] oprofile for 2.5.39

[John Levon]

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

Re: [PATCH][RFC] oprofile for 2.5.39

[Anton Blanchard]

> [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

Re: [PATCH][RFC] oprofile for 2.5.39

[Andrew Morton]

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.

Re: [PATCH][RFC] oprofile for 2.5.39

[John Levon]

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

[PATCH][RFC] oprofile for 2.5.39

[John Levon]

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