public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [mm patch] oprofile: backtrace operation does not initialized
@ 2004-12-08  9:30 Akinobu Mita
  2004-12-08  9:34 ` Akinobu Mita
  2004-12-08 16:00 ` John Levon
  0 siblings, 2 replies; 16+ messages in thread
From: Akinobu Mita @ 2004-12-08  9:30 UTC (permalink / raw)
  To: phil.el; +Cc: John Levon, linux-kernel

Hello.

When I forced the oprofile to use timer interrupt with specifying
"timer=1" module parameter. "oprofile_operations->backtrace" did
not initialized on i386.

Please apply this patch, or make oprofile initialize the backtrace
operation in case of using timer interrupt in your preferable way.

---

Signed-off-by: Akinobu Mita <amgta@yacht.ocn.ne.jp>

 arch/alpha/oprofile/common.c |    3 +++
 arch/arm/oprofile/init.c     |    3 +++
 arch/i386/oprofile/init.c    |    5 ++++-
 arch/ia64/oprofile/init.c    |    5 ++++-
 arch/ppc64/oprofile/common.c |    3 +++
 drivers/oprofile/oprof.c     |    4 ++--
 include/linux/oprofile.h     |    2 ++
 7 files changed, 21 insertions(+), 4 deletions(-)

diff -Nurp 2.6-mm.orig/arch/alpha/oprofile/common.c 2.6-mm/arch/alpha/oprofile/common.c
--- 2.6-mm.orig/arch/alpha/oprofile/common.c	2004-12-07 00:11:29.000000000 +0900
+++ 2.6-mm/arch/alpha/oprofile/common.c	2004-12-07 23:35:48.000000000 +0900
@@ -143,6 +143,9 @@ oprofile_arch_init(struct oprofile_opera
 {
 	struct op_axp_model *lmodel = NULL;
 
+	if (ops->force_timer)
+		return;
+
 	switch (implver()) {
 	case IMPLVER_EV4:
 		lmodel = &op_model_ev4;
diff -Nurp 2.6-mm.orig/arch/arm/oprofile/init.c 2.6-mm/arch/arm/oprofile/init.c
--- 2.6-mm.orig/arch/arm/oprofile/init.c	2004-12-07 00:11:32.000000000 +0900
+++ 2.6-mm/arch/arm/oprofile/init.c	2004-12-07 23:37:26.000000000 +0900
@@ -14,6 +14,9 @@
 
 void __init oprofile_arch_init(struct oprofile_operations *ops)
 {
+	if (ops->force_timer)
+		return;
+
 #ifdef CONFIG_CPU_XSCALE
 	pmu_init(ops, &op_xscale_spec);
 #endif
diff -Nurp 2.6-mm.orig/arch/i386/oprofile/init.c 2.6-mm/arch/i386/oprofile/init.c
--- 2.6-mm.orig/arch/i386/oprofile/init.c	2004-12-07 00:11:15.000000000 +0900
+++ 2.6-mm/arch/i386/oprofile/init.c	2004-12-07 23:29:31.000000000 +0900
@@ -25,6 +25,10 @@ void __init oprofile_arch_init(struct op
 {
 	int ret __attribute_used__ = -ENODEV;
 
+	ops->backtrace = x86_backtrace;
+	if (ops->force_timer)
+		return;
+
 #ifdef CONFIG_X86_LOCAL_APIC
 	ret = nmi_init(ops);
 #endif
@@ -32,7 +36,6 @@ void __init oprofile_arch_init(struct op
 	if (ret < 0)
 		ret = nmi_timer_init(ops);
 #endif
-	ops->backtrace = x86_backtrace;
 }
 
 
diff -Nurp 2.6-mm.orig/arch/ia64/oprofile/init.c 2.6-mm/arch/ia64/oprofile/init.c
--- 2.6-mm.orig/arch/ia64/oprofile/init.c	2004-12-07 00:11:29.000000000 +0900
+++ 2.6-mm/arch/ia64/oprofile/init.c	2004-12-07 23:35:02.000000000 +0900
@@ -18,11 +18,14 @@ extern void ia64_backtrace(struct pt_reg
 
 void __init oprofile_arch_init(struct oprofile_operations * ops)
 {
+	ops->backtrace = ia64_backtrace;
+	if (ops->force_timer)
+		return;
+
 #ifdef CONFIG_PERFMON
 	/* perfmon_init() can fail, but we have no way to report it */
 	perfmon_init(ops);
 #endif
-	ops->backtrace = ia64_backtrace;
 }
 
 
diff -Nurp 2.6-mm.orig/arch/ppc64/oprofile/common.c 2.6-mm/arch/ppc64/oprofile/common.c
--- 2.6-mm.orig/arch/ppc64/oprofile/common.c	2004-12-07 00:11:30.000000000 +0900
+++ 2.6-mm/arch/ppc64/oprofile/common.c	2004-12-07 23:36:59.000000000 +0900
@@ -129,6 +129,9 @@ void __init oprofile_arch_init(struct op
 {
 	unsigned int pvr;
 
+	if (ops->force_timer)
+		return;
+
 	pvr = mfspr(SPRN_PVR);
 
 	switch (PVR_VER(pvr)) {
diff -Nurp 2.6-mm.orig/drivers/oprofile/oprof.c 2.6-mm/drivers/oprofile/oprof.c
--- 2.6-mm.orig/drivers/oprofile/oprof.c	2004-12-07 00:12:07.000000000 +0900
+++ 2.6-mm/drivers/oprofile/oprof.c	2004-12-07 22:51:21.000000000 +0900
@@ -159,10 +159,10 @@ static int __init oprofile_init(void)
 	oprofile_timer_init(&oprofile_ops);
 
 	if (timer) {
+		oprofile_ops.force_timer = 1;
 		printk(KERN_INFO "oprofile: using timer interrupt.\n");
-	} else {
-		oprofile_arch_init(&oprofile_ops);
 	}
+	oprofile_arch_init(&oprofile_ops);
 
 	err = oprofilefs_register();
 	if (err)
diff -Nurp 2.6-mm.orig/include/linux/oprofile.h 2.6-mm/include/linux/oprofile.h
--- 2.6-mm.orig/include/linux/oprofile.h	2004-12-07 00:12:32.000000000 +0900
+++ 2.6-mm/include/linux/oprofile.h	2004-12-07 22:50:07.000000000 +0900
@@ -39,6 +39,8 @@ struct oprofile_operations {
 	void (*backtrace)(struct pt_regs * const regs, unsigned int depth);
 	/* CPU identification string. */
 	char * cpu_type;
+	/* Force use of timer interrupt */
+	int force_timer;
 };
 
 /**



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-08  9:30 [mm patch] oprofile: backtrace operation does not initialized Akinobu Mita
@ 2004-12-08  9:34 ` Akinobu Mita
  2004-12-08 11:13   ` Ingo Molnar
  2004-12-08 16:00 ` John Levon
  1 sibling, 1 reply; 16+ messages in thread
From: Akinobu Mita @ 2004-12-08  9:34 UTC (permalink / raw)
  To: phil.el; +Cc: John Levon, linux-kernel

On Wednesday 08 December 2004 18:30, Akinobu Mita wrote:

> Please apply this patch, or make oprofile initialize the backtrace
> operation in case of using timer interrupt in your preferable way.

Acutually, I want this change to work my own tailor-made profiler.
It simply moves the point where timer_hook is called.

e.g. move timer_hook to the entry point of schedule().

--- 2.6-mm/kernel/profile.c.orig	2004-12-07 23:52:56.000000000 +0900
+++ 2.6-mm/kernel/profile.c	2004-12-07 23:53:29.000000000 +0900
@@ -383,8 +383,6 @@ void profile_hit(int type, void *__pc)
 
 void profile_tick(int type, struct pt_regs *regs)
 {
-	if (type == CPU_PROFILING && timer_hook)
-		timer_hook(regs);
 	if (!user_mode(regs) && cpu_isset(smp_processor_id(), prof_cpu_mask))
 		profile_hit(type, (void *)profile_pc(regs));
 }
--- 2.6-mm/kernel/sched.c.orig	2004-12-08 16:08:13.000000000 +0900
+++ 2.6-mm/kernel/sched.c	2004-12-08 16:10:01.000000000 +0900
@@ -2665,6 +2665,30 @@ EXPORT_SYMBOL(sub_preempt_count);
 
 #endif
 
+#ifdef __i386__
+#define GET_CURRENT_REGS(regs)						\
+do {									\
+	__asm__ __volatile__("movl %%ebx,%0" : "=m"(regs.ebx));	\
+	__asm__ __volatile__("movl %%ecx,%0" : "=m"(regs.ecx));	\
+	__asm__ __volatile__("movl %%edx,%0" : "=m"(regs.edx));	\
+	__asm__ __volatile__("movl %%esi,%0" : "=m"(regs.esi));	\
+	__asm__ __volatile__("movl %%edi,%0" : "=m"(regs.edi));	\
+	__asm__ __volatile__("movl %%ebp,%0" : "=m"(regs.ebp));	\
+	__asm__ __volatile__("movl %%eax,%0" : "=m"(regs.eax));	\
+	__asm__ __volatile__("movl %%esp,%0" : "=m"(regs.esp));	\
+	__asm__ __volatile__("movw %%ss, %%ax;" :"=a"(regs.xss));	\
+	__asm__ __volatile__("movw %%cs, %%ax;" :"=a"(regs.xcs));	\
+	__asm__ __volatile__("movw %%ds, %%ax;" :"=a"(regs.xds));	\
+	__asm__ __volatile__("movw %%es, %%ax;" :"=a"(regs.xes));	\
+	__asm__ __volatile__("pushfl; popl %0" :"=m"(regs.eflags));	\
+									\
+	regs.eip = (unsigned long)current_text_addr();			\
+} while(0)
+
+#else
+#error please define get_current_regs()
+#endif
+
 /*
  * schedule() is the main scheduler function.
  */
@@ -2692,7 +2716,12 @@ asmlinkage void __sched schedule(void)
 			dump_stack();
 		}
 	}
-	profile_hit(SCHED_PROFILING, __builtin_return_address(0));
+	if (timer_hook) {
+		struct pt_regs regs;
+
+		GET_CURRENT_REGS(regs);
+		timer_hook(&regs);
+	}
 
 need_resched:
 	preempt_disable();



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-08  9:34 ` Akinobu Mita
@ 2004-12-08 11:13   ` Ingo Molnar
  2004-12-08 11:37     ` Akinobu Mita
  0 siblings, 1 reply; 16+ messages in thread
From: Ingo Molnar @ 2004-12-08 11:13 UTC (permalink / raw)
  To: Akinobu Mita; +Cc: phil.el, John Levon, linux-kernel


* Akinobu Mita <amgta@yacht.ocn.ne.jp> wrote:

> -	profile_hit(SCHED_PROFILING, __builtin_return_address(0));
> +	if (timer_hook) {
> +		struct pt_regs regs;
> +
> +		GET_CURRENT_REGS(regs);
> +		timer_hook(&regs);
> +	}

ugh. nack.

	Ingo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-08 11:13   ` Ingo Molnar
@ 2004-12-08 11:37     ` Akinobu Mita
  0 siblings, 0 replies; 16+ messages in thread
From: Akinobu Mita @ 2004-12-08 11:37 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: phil.el, John Levon, linux-kernel

On Wednesday 08 December 2004 20:13, Ingo Molnar wrote:
> * Akinobu Mita <amgta@yacht.ocn.ne.jp> wrote:
> > -	profile_hit(SCHED_PROFILING, __builtin_return_address(0));
> > +	if (timer_hook) {
> > +		struct pt_regs regs;
> > +
> > +		GET_CURRENT_REGS(regs);
> > +		timer_hook(&regs);
> > +	}
>
> ugh. nack.
>

This second patch is not intended for inclusion.
It's my own tailor-made profiler. Actually it breaks all architectures
except for i386 with CONFIG_PROFILING.

It just demonstrates why I want to apply the first patch.
It fixes specifying "timer=1" as oprofile module parameter avoids to
set oprofile_operations.backtrace.




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-08  9:30 [mm patch] oprofile: backtrace operation does not initialized Akinobu Mita
  2004-12-08  9:34 ` Akinobu Mita
@ 2004-12-08 16:00 ` John Levon
  2004-12-08 22:31   ` Greg Banks
  1 sibling, 1 reply; 16+ messages in thread
From: John Levon @ 2004-12-08 16:00 UTC (permalink / raw)
  To: Akinobu Mita; +Cc: phil.el, linux-kernel, gnb

On Wed, Dec 08, 2004 at 06:30:51PM +0900, Akinobu Mita wrote:

> When I forced the oprofile to use timer interrupt with specifying
> "timer=1" module parameter. "oprofile_operations->backtrace" did
> not initialized on i386.
> 
> Please apply this patch, or make oprofile initialize the backtrace
> operation in case of using timer interrupt in your preferable way.

I don't like this patch. The arches should just set the backtrace
always, then try to init the hardware. oprofile_init() should then force
the timer ops as needed.

Greg?

john

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-08 16:00 ` John Levon
@ 2004-12-08 22:31   ` Greg Banks
  2004-12-08 23:56     ` Philippe Elie
  0 siblings, 1 reply; 16+ messages in thread
From: Greg Banks @ 2004-12-08 22:31 UTC (permalink / raw)
  To: John Levon; +Cc: Akinobu Mita, phil.el, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 855 bytes --]

On Wed, Dec 08, 2004 at 04:00:55PM +0000, John Levon wrote:
> On Wed, Dec 08, 2004 at 06:30:51PM +0900, Akinobu Mita wrote:
> 
> > When I forced the oprofile to use timer interrupt with specifying
> > "timer=1" module parameter. "oprofile_operations->backtrace" did
> > not initialized on i386.
> > 
> > Please apply this patch, or make oprofile initialize the backtrace
> > operation in case of using timer interrupt in your preferable way.
> 
> I don't like this patch. The arches should just set the backtrace
> always, then try to init the hardware. oprofile_init() should then force
> the timer ops as needed.
> 
> Greg?

Agreed, that's a cleaner approach.  The attached patch (untested)
implements that.  Akinobu-san, can you please test the patch?

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.

[-- Attachment #2: oprofile-timer-backtrace-fix --]
[-- Type: text/plain, Size: 878 bytes --]

Allow stack tracing to work when sampling on timer is forced
using the timer=1 boot option.  Reported by Akinobu Mita.

Signed-off-by: Greg Banks <gnb@melbourne.sgi.com>
---
 oprof.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)


Index: linux/drivers/oprofile/oprof.c
===================================================================
--- linux.orig/drivers/oprofile/oprof.c	2004-12-04 19:43:37.%N +1100
+++ linux/drivers/oprofile/oprof.c	2004-12-09 09:25:02.%N +1100
@@ -155,13 +155,11 @@ static int __init oprofile_init(void)
 {
 	int err = 0;
 
-	/* this is our fallback case */
-	oprofile_timer_init(&oprofile_ops);
+	oprofile_arch_init(&oprofile_ops);
 
 	if (timer) {
 		printk(KERN_INFO "oprofile: using timer interrupt.\n");
-	} else {
-		oprofile_arch_init(&oprofile_ops);
+		oprofile_timer_init(&oprofile_ops);
 	}
 
 	err = oprofilefs_register();

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-08 22:31   ` Greg Banks
@ 2004-12-08 23:56     ` Philippe Elie
  2004-12-09  0:39       ` Greg Banks
  0 siblings, 1 reply; 16+ messages in thread
From: Philippe Elie @ 2004-12-08 23:56 UTC (permalink / raw)
  To: Greg Banks; +Cc: John Levon, Akinobu Mita, linux-kernel

On Thu, 09 Dec 2004 at 09:31 +0000, Greg Banks wrote:

> On Wed, Dec 08, 2004 at 04:00:55PM +0000, John Levon wrote:
> > On Wed, Dec 08, 2004 at 06:30:51PM +0900, Akinobu Mita wrote:
> > 
> > > When I forced the oprofile to use timer interrupt with specifying
> > > "timer=1" module parameter. "oprofile_operations->backtrace" did
> > > not initialized on i386.
> > > 
> > > Please apply this patch, or make oprofile initialize the backtrace
> > > operation in case of using timer interrupt in your preferable way.
> > 
> > I don't like this patch. The arches should just set the backtrace
> > always, then try to init the hardware. oprofile_init() should then force
> > the timer ops as needed.
> > 
> > Greg?
> 
> Agreed, that's a cleaner approach.  The attached patch (untested)
> implements that.  Akinobu-san, can you please test the patch?

> +++ linux/drivers/oprofile/oprof.c	2004-12-09 09:25:02.%N +1100
> @@ -155,13 +155,11 @@ static int __init oprofile_init(void)
>  {
>  	int err = 0;
>  
> -	/* this is our fallback case */
> -	oprofile_timer_init(&oprofile_ops);
> +	oprofile_arch_init(&oprofile_ops);

oprofile_arch_init() --> nmi_init() which setup oprofile_ops->setup/shutdown
etc.

>  	if (timer) {
>  		printk(KERN_INFO "oprofile: using timer interrupt.\n");
> -	} else {
> -		oprofile_arch_init(&oprofile_ops);
> +		oprofile_timer_init(&oprofile_ops);
>  	}

oprofile_timer_init doesn't reset op->setup/shutdown, I don't like the idea to
reset them in oprofile_timer_init() it's error prone. John any idea ?

-- 
Philippe Elie


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-08 23:56     ` Philippe Elie
@ 2004-12-09  0:39       ` Greg Banks
  2004-12-09  1:46         ` John Levon
  0 siblings, 1 reply; 16+ messages in thread
From: Greg Banks @ 2004-12-09  0:39 UTC (permalink / raw)
  To: Philippe Elie; +Cc: Greg Banks, John Levon, Akinobu Mita, linux-kernel

On Thu, Dec 09, 2004 at 12:56:23AM +0100, Philippe Elie wrote:
> 
> oprofile_timer_init doesn't reset op->setup/shutdown, I don't like the idea to
> reset them in oprofile_timer_init() it's error prone. John any idea ?

A better long term solution would be not to change the ops structure
but to copy and chain it, as I did in my experimental cswitch patches.
The approach I took allows for a new fake event which can be selected
at runtime with --event instead of with a boot option.

But for now I don't see any drama with leaving in the ->setup() and
->shutdown() methods when rewriting the ops structure.  Ditto for
the ->create_files() methods.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-09  0:39       ` Greg Banks
@ 2004-12-09  1:46         ` John Levon
  2004-12-09  1:50           ` Greg Banks
  0 siblings, 1 reply; 16+ messages in thread
From: John Levon @ 2004-12-09  1:46 UTC (permalink / raw)
  To: Greg Banks; +Cc: Philippe Elie, Akinobu Mita, linux-kernel

On Thu, Dec 09, 2004 at 11:39:06AM +1100, Greg Banks wrote:

> But for now I don't see any drama with leaving in the ->setup() and
> ->shutdown() methods when rewriting the ops structure.  Ditto for
> the ->create_files() methods.

Wouldn't this mean that we try to set up the NMI stuff regardless of
forcing the timer ? I can imagine a flaky system where somebody needs to
avoid going near that stuff.

timer_init() making sure to set all fields seems reasonable to me.  Or
oprofile_init() could grab ->backtrace, memset the structure, then
replace ->backtrace...

john

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-09  1:46         ` John Levon
@ 2004-12-09  1:50           ` Greg Banks
  2004-12-09  2:23             ` John Levon
  2004-12-09 14:22             ` Akinobu Mita
  0 siblings, 2 replies; 16+ messages in thread
From: Greg Banks @ 2004-12-09  1:50 UTC (permalink / raw)
  To: John Levon; +Cc: Greg Banks, Philippe Elie, Akinobu Mita, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 778 bytes --]

On Thu, Dec 09, 2004 at 01:46:22AM +0000, John Levon wrote:
> On Thu, Dec 09, 2004 at 11:39:06AM +1100, Greg Banks wrote:
> 
> > But for now I don't see any drama with leaving in the ->setup() and
> > ->shutdown() methods when rewriting the ops structure.  Ditto for
> > the ->create_files() methods.
> 
> Wouldn't this mean that we try to set up the NMI stuff regardless of
> forcing the timer ? I can imagine a flaky system where somebody needs to
> avoid going near that stuff.
> 
> timer_init() making sure to set all fields seems reasonable to me.  Or
> oprofile_init() could grab ->backtrace, memset the structure, then
> replace ->backtrace...

Ok, how about this patch?

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.

[-- Attachment #2: oprofile-timer-backtrace-fix-2 --]
[-- Type: text/plain, Size: 1429 bytes --]

Allow stack tracing to work when sampling on timer is forced
using the timer=1 boot option.  Reported by Akinobu Mita.

Signed-off-by: Greg Banks <gnb@melbourne.sgi.com>
---
 oprof.c     |    6 ++----
 timer_int.c |    3 +++
 2 files changed, 5 insertions(+), 4 deletions(-)

Index: linux/drivers/oprofile/oprof.c
===================================================================
--- linux.orig/drivers/oprofile/oprof.c	2004-12-04 19:43:37.%N +1100
+++ linux/drivers/oprofile/oprof.c	2004-12-09 09:25:02.%N +1100
@@ -155,13 +155,11 @@ static int __init oprofile_init(void)
 {
 	int err = 0;
 
-	/* this is our fallback case */
-	oprofile_timer_init(&oprofile_ops);
+	oprofile_arch_init(&oprofile_ops);
 
 	if (timer) {
 		printk(KERN_INFO "oprofile: using timer interrupt.\n");
-	} else {
-		oprofile_arch_init(&oprofile_ops);
+		oprofile_timer_init(&oprofile_ops);
 	}
 
 	err = oprofilefs_register();
Index: linux/drivers/oprofile/timer_int.c
===================================================================
--- linux.orig/drivers/oprofile/timer_int.c	2004-12-04 19:43:37.%N +1100
+++ linux/drivers/oprofile/timer_int.c	2004-12-09 12:48:52.%N +1100
@@ -37,6 +37,9 @@ static void timer_stop(void)
 
 void __init oprofile_timer_init(struct oprofile_operations * ops)
 {
+	ops->create_files = NULL;
+	ops->setup = NULL;
+	ops->shutdown = NULL;
 	ops->start = timer_start;
 	ops->stop = timer_stop;
 	ops->cpu_type = "timer";

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-09  1:50           ` Greg Banks
@ 2004-12-09  2:23             ` John Levon
  2004-12-09 14:22             ` Akinobu Mita
  1 sibling, 0 replies; 16+ messages in thread
From: John Levon @ 2004-12-09  2:23 UTC (permalink / raw)
  To: Greg Banks; +Cc: Philippe Elie, Akinobu Mita, linux-kernel

On Thu, Dec 09, 2004 at 12:50:24PM +1100, Greg Banks wrote:

> Ok, how about this patch?

Fine by me

john

> Allow stack tracing to work when sampling on timer is forced
> using the timer=1 boot option.  Reported by Akinobu Mita.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-09  1:50           ` Greg Banks
  2004-12-09  2:23             ` John Levon
@ 2004-12-09 14:22             ` Akinobu Mita
  2004-12-09 14:53               ` Akinobu Mita
  2004-12-09 15:23               ` Greg Banks
  1 sibling, 2 replies; 16+ messages in thread
From: Akinobu Mita @ 2004-12-09 14:22 UTC (permalink / raw)
  To: Greg Banks, John Levon; +Cc: Greg Banks, Philippe Elie, linux-kernel

On Thursday 09 December 2004 10:50, Greg Banks wrote:
> On Thu, Dec 09, 2004 at 01:46:22AM +0000, John Levon wrote:
> > On Thu, Dec 09, 2004 at 11:39:06AM +1100, Greg Banks wrote:
> > > But for now I don't see any drama with leaving in the ->setup() and
> > > ->shutdown() methods when rewriting the ops structure.  Ditto for
> > > the ->create_files() methods.
> >
> > Wouldn't this mean that we try to set up the NMI stuff regardless of
> > forcing the timer ? I can imagine a flaky system where somebody needs to
> > avoid going near that stuff.
> >
> > timer_init() making sure to set all fields seems reasonable to me.  Or
> > oprofile_init() could grab ->backtrace, memset the structure, then
> > replace ->backtrace...
>
> Ok, how about this patch?

Thanks, but..

This patch is broken on several architectures (sparc64, sh, parisc, s390).
Even though i386 without CONFIG_X86_LOCAL_APIC and CONFIG_X86_IO_APIC.

Since the timer interrupt is the only way of getting sampling for oprofile
on such environments. if no module parameters specified (i.e. timer == 0),
then oprofile_timer_init() is never called. and I have got this error:


Unable to handle kernel NULL pointer dereference at virtual address 00000000
 printing eip:
e81a97b0
*pde = 0b690001
Oops: 0000 [#1]
PREEMPT DEBUG_PAGEALLOC
Modules linked in: oprofile 3c59x microcode ntfs video
CPU:    0
EIP:    0060:[<e81a97b0>]    Not tainted VLI
EFLAGS: 00010246   (2.6.10-rc2-mm4) 
EIP is at oprofilefs_str_to_user+0x10/0x2c [oprofile]
eax: 00000000   ebx: 00000000   ecx: ffffffff   edx: 00000000
esi: c7169f54   edi: 00000000   ebp: c7616f6c   esp: c7616f68
ds: 007b   es: 007b   ss: 0068
Process cat (pid: 3366, threadinfo=c7616000 task=c716da60)
Stack: c7616fa8 c7616f90 c0171a80 00000000 0804d888 00001000 c7616fa8 c7169f54 
       fffffff7 0804d888 c7616fbc c0171cfe c7169f54 0804d888 00001000 c7616fa8 
       00000000 00000000 00000000 00000003 00001000 c7616000 c0103d41 00000003 
Call Trace:
 [<c0104397>] show_stack+0x6f/0x88
 [<c01044c6>] show_registers+0xfe/0x160
 [<c01046f1>] die+0x13d/0x288
 [<c0112c74>] do_page_fault+0x324/0x6a1
 [<c0103f3b>] error_code+0x2b/0x30
 [<c0171a80>] vfs_read+0x88/0x104
 [<c0171cfe>] sys_read+0x3a/0x64
 [<c0103d41>] sysenter_past_esp+0x52/0x71
Code: 53 58 89 43 54 89 43 4c 89 53 50 89 43 44 89 53 48 89 d8 8b 5d fc c9 c3 8d 76 00 55 89 e5 8b 55 08 57 31 c0 b9 ff ff ff ff 89 d7 <f2> ae f7 d1 49 51 52 ff 75 14 ff 75 10 ff 75 0c e8 97 70 ff d7 





^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-09 14:22             ` Akinobu Mita
@ 2004-12-09 14:53               ` Akinobu Mita
  2004-12-09 14:55                 ` John Levon
  2004-12-09 15:23               ` Greg Banks
  1 sibling, 1 reply; 16+ messages in thread
From: Akinobu Mita @ 2004-12-09 14:53 UTC (permalink / raw)
  To: Greg Banks, John Levon; +Cc: Greg Banks, Philippe Elie, linux-kernel

On Thursday 09 December 2004 23:22, Akinobu Mita wrote:

> Since the timer interrupt is the only way of getting sampling for oprofile
> on such environments. if no module parameters specified (i.e. timer == 0),
> then oprofile_timer_init() is never called. and I have got this error:

My first patch is not so bad, I think.

Or, It may be better to revert the return type of oprofile_arch_init()
from "void" to "int"  to get the result of initialization.
Though I don't know when/why its interface was changed. and some
architectures (ppc, sh64, m32r) remain to have old interfaces in -mm.




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-09 14:53               ` Akinobu Mita
@ 2004-12-09 14:55                 ` John Levon
  2004-12-09 15:27                   ` Greg Banks
  0 siblings, 1 reply; 16+ messages in thread
From: John Levon @ 2004-12-09 14:55 UTC (permalink / raw)
  To: Akinobu Mita; +Cc: Greg Banks, Philippe Elie, linux-kernel

On Thu, Dec 09, 2004 at 11:53:53PM +0900, Akinobu Mita wrote:

> Or, It may be better to revert the return type of oprofile_arch_init()
> from "void" to "int"  to get the result of initialization.
> Though I don't know when/why its interface was changed. and some
> architectures (ppc, sh64, m32r) remain to have old interfaces in -mm.

I've also lost track of why we don't return -ENODEV from the
arch_init().

We should set -ENODEV after setting ->backtrace as appropriate. Then the
code should do what it used to do:

    138         int err = oprofile_arch_init(&oprofile_ops);
    139
    140         if (err == -ENODEV || timer) {
    141                 timer_init(&oprofile_ops);
    142                 err = 0;
    143         } else if (err) {
    144                 goto out;
    145         }

john

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-09 14:22             ` Akinobu Mita
  2004-12-09 14:53               ` Akinobu Mita
@ 2004-12-09 15:23               ` Greg Banks
  1 sibling, 0 replies; 16+ messages in thread
From: Greg Banks @ 2004-12-09 15:23 UTC (permalink / raw)
  To: Akinobu Mita; +Cc: Greg Banks, John Levon, Philippe Elie, linux-kernel

On Thu, Dec 09, 2004 at 11:22:27PM +0900, Akinobu Mita wrote:
> On Thursday 09 December 2004 10:50, Greg Banks wrote:
> > Ok, how about this patch?
> [...]
> Since the timer interrupt is the only way of getting sampling for oprofile
> on such environments. if no module parameters specified (i.e. timer == 0),
> then oprofile_timer_init() is never called. and I have got this error:

Bother, back to the drawing board.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [mm patch] oprofile: backtrace operation does not initialized
  2004-12-09 14:55                 ` John Levon
@ 2004-12-09 15:27                   ` Greg Banks
  0 siblings, 0 replies; 16+ messages in thread
From: Greg Banks @ 2004-12-09 15:27 UTC (permalink / raw)
  To: John Levon; +Cc: Akinobu Mita, Philippe Elie, linux-kernel

On Thu, Dec 09, 2004 at 02:55:19PM +0000, John Levon wrote:
> On Thu, Dec 09, 2004 at 11:53:53PM +0900, Akinobu Mita wrote:
> 
> > Or, It may be better to revert the return type of oprofile_arch_init()
> > from "void" to "int"  to get the result of initialization.
> 
> I've also lost track of why we don't return -ENODEV from the
> arch_init().

I didn't see a good reason for that change either.  Version 0.2
of the callgraph patch had the int return and worked fine.  Let's
go back.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2004-12-09 15:28 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-12-08  9:30 [mm patch] oprofile: backtrace operation does not initialized Akinobu Mita
2004-12-08  9:34 ` Akinobu Mita
2004-12-08 11:13   ` Ingo Molnar
2004-12-08 11:37     ` Akinobu Mita
2004-12-08 16:00 ` John Levon
2004-12-08 22:31   ` Greg Banks
2004-12-08 23:56     ` Philippe Elie
2004-12-09  0:39       ` Greg Banks
2004-12-09  1:46         ` John Levon
2004-12-09  1:50           ` Greg Banks
2004-12-09  2:23             ` John Levon
2004-12-09 14:22             ` Akinobu Mita
2004-12-09 14:53               ` Akinobu Mita
2004-12-09 14:55                 ` John Levon
2004-12-09 15:27                   ` Greg Banks
2004-12-09 15:23               ` Greg Banks

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox