Hi,
I decided to try oprofile too :) Here's what I did:
# opcontrol --deinit
Stopping profiling.
Killing daemon.
Opening /proc/modules: No such file or directory
This was after having collected some data. The kernel is compiled without
module support. (I can provide config if necessary)
Vegard
opannotate[27361]: segfault at 0 ip 080d1f5b sp bfcbab6c error 4 in opannotate[8048000+117000]
BUG: using smp_processor_id() in preemptible [00000000] code: oprofiled/27301
caller is nmi_shutdown+0x11/0x60
Pid: 27301, comm: oprofiled Not tainted 2.6.26-rc7 #25
[<c028a90d>] debug_smp_processor_id+0xbd/0xc0
[<c045fba1>] nmi_shutdown+0x11/0x60
[<c045dd4a>] oprofile_shutdown+0x2a/0x60
[<c045eb70>] event_buffer_release+0x10/0x40
[<c0195f76>] __fput+0xb6/0x180
[<c01962e9>] fput+0x19/0x20
[<c0193297>] filp_close+0x47/0x70
[<c013869b>] put_files_struct+0x9b/0xb0
[<c01386f2>] exit_files+0x42/0x60
[<c013989d>] do_exit+0x16d/0x700
[<c014e1c6>] ? up_read+0x16/0x30
[<c0122953>] ? do_page_fault+0x2e3/0x700
[<c0194b90>] ? do_sync_write+0x0/0x110
[<c0139e61>] do_group_exit+0x31/0x90
[<c0139ecf>] sys_exit_group+0xf/0x20
[<c010831b>] sysenter_past_esp+0x78/0xd1
=======================
Hi,
Does this look correct? I didn't really play with preemption before, but
as far as I can tell, this is the right thing to do.
I don't really get why model->shutdown(msrs) is done only for one of the
CPUs, but my patch assumes that this is correct. (If that had been done
from inside nmi_shutdown() for each CPU, we wouldn't have had to get the
cpu var, and not needed to disable preemption.)
Please comment :-)
Vegard
From: Vegard Nossum <[email protected]>
Date: Sat, 21 Jun 2008 23:44:19 +0200
Subject: [PATCH] x86/oprofile: disable preemption in nmi_shutdown
BUG: using smp_processor_id() in preemptible [00000000] code: oprofiled/27301
caller is nmi_shutdown+0x11/0x60
Pid: 27301, comm: oprofiled Not tainted 2.6.26-rc7 #25
[<c028a90d>] debug_smp_processor_id+0xbd/0xc0
[<c045fba1>] nmi_shutdown+0x11/0x60
[<c045dd4a>] oprofile_shutdown+0x2a/0x60
Note that we don't need this for the other functions, since they are all
called with on_each_cpu() (which disables preemption for us anyway).
Signed-off-by: Vegard Nossum <[email protected]>
---
arch/x86/oprofile/nmi_int.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
index cc48d3f..4a177b4 100644
--- a/arch/x86/oprofile/nmi_int.c
+++ b/arch/x86/oprofile/nmi_int.c
@@ -269,12 +269,16 @@ static void nmi_cpu_shutdown(void *dummy)
static void nmi_shutdown(void)
{
- struct op_msrs *msrs = &__get_cpu_var(cpu_msrs);
+ struct op_msrs *msrs;
+
+ preempt_disable();
+ msrs = &__get_cpu_var(cpu_msrs);
nmi_enabled = 0;
on_each_cpu(nmi_cpu_shutdown, NULL, 0, 1);
unregister_die_notifier(&profile_exceptions_nb);
model->shutdown(msrs);
free_msrs();
+ preempt_enable();
}
static void nmi_cpu_start(void *dummy)
--
1.5.4.1
Hi Vegard,
Vegard Nossum <[email protected]> writes:
> Hi,
>
> Does this look correct? I didn't really play with preemption before, but
> as far as I can tell, this is the right thing to do.
>
> I don't really get why model->shutdown(msrs) is done only for one of the
> CPUs, but my patch assumes that this is correct. (If that had been done
> from inside nmi_shutdown() for each CPU, we wouldn't have had to get the
> cpu var, and not needed to disable preemption.)
>
> Please comment :-)
>
>
> Vegard
>
>
> From: Vegard Nossum <[email protected]>
> Date: Sat, 21 Jun 2008 23:44:19 +0200
> Subject: [PATCH] x86/oprofile: disable preemption in nmi_shutdown
>
> BUG: using smp_processor_id() in preemptible [00000000] code: oprofiled/27301
> caller is nmi_shutdown+0x11/0x60
> Pid: 27301, comm: oprofiled Not tainted 2.6.26-rc7 #25
> [<c028a90d>] debug_smp_processor_id+0xbd/0xc0
> [<c045fba1>] nmi_shutdown+0x11/0x60
> [<c045dd4a>] oprofile_shutdown+0x2a/0x60
>
> Note that we don't need this for the other functions, since they are all
> called with on_each_cpu() (which disables preemption for us anyway).
>
> Signed-off-by: Vegard Nossum <[email protected]>
> ---
> arch/x86/oprofile/nmi_int.c | 6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
> index cc48d3f..4a177b4 100644
> --- a/arch/x86/oprofile/nmi_int.c
> +++ b/arch/x86/oprofile/nmi_int.c
> @@ -269,12 +269,16 @@ static void nmi_cpu_shutdown(void *dummy)
>
> static void nmi_shutdown(void)
> {
> - struct op_msrs *msrs = &__get_cpu_var(cpu_msrs);
> + struct op_msrs *msrs;
> +
> + preempt_disable();
> + msrs = &__get_cpu_var(cpu_msrs);
> nmi_enabled = 0;
> on_each_cpu(nmi_cpu_shutdown, NULL, 0, 1);
> unregister_die_notifier(&profile_exceptions_nb);
> model->shutdown(msrs);
> free_msrs();
> + preempt_enable();
Have a look at get_cpu_var() and put_cpu_var(), that is exactly the
pattern.
Hannes
Hi,
On Sun, Jun 22, 2008 at 9:30 AM, Johannes Weiner <[email protected]> wrote:
> Vegard Nossum <[email protected]> writes:
>> static void nmi_shutdown(void)
>> {
>> - struct op_msrs *msrs = &__get_cpu_var(cpu_msrs);
>> + struct op_msrs *msrs;
>> +
>> + preempt_disable();
>> + msrs = &__get_cpu_var(cpu_msrs);
>> nmi_enabled = 0;
>> on_each_cpu(nmi_cpu_shutdown, NULL, 0, 1);
>> unregister_die_notifier(&profile_exceptions_nb);
>> model->shutdown(msrs);
>> free_msrs();
>> + preempt_enable();
>
> Have a look at get_cpu_var() and put_cpu_var(), that is exactly the
> pattern.
Thanks! How about this instead?
Vegard
From: Vegard Nossum <[email protected]>
Date: Sat, 21 Jun 2008 23:44:19 +0200
Subject: [PATCH] x86/oprofile: disable preemption in nmi_shutdown
BUG: using smp_processor_id() in preemptible [00000000] code: oprofiled/27301
caller is nmi_shutdown+0x11/0x60
Pid: 27301, comm: oprofiled Not tainted 2.6.26-rc7 #25
[<c028a90d>] debug_smp_processor_id+0xbd/0xc0
[<c045fba1>] nmi_shutdown+0x11/0x60
[<c045dd4a>] oprofile_shutdown+0x2a/0x60
Note that we don't need this for the other functions, since they are all
called with on_each_cpu() (which disables preemption for us anyway).
Signed-off-by: Vegard Nossum <[email protected]>
---
arch/x86/oprofile/nmi_int.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
index cc48d3f..2b6ad5b 100644
--- a/arch/x86/oprofile/nmi_int.c
+++ b/arch/x86/oprofile/nmi_int.c
@@ -269,12 +269,13 @@ static void nmi_cpu_shutdown(void *dummy)
static void nmi_shutdown(void)
{
- struct op_msrs *msrs = &__get_cpu_var(cpu_msrs);
+ struct op_msrs *msrs = &get_cpu_var(cpu_msrs);
nmi_enabled = 0;
on_each_cpu(nmi_cpu_shutdown, NULL, 0, 1);
unregister_die_notifier(&profile_exceptions_nb);
model->shutdown(msrs);
free_msrs();
+ put_cpu_var(cpu_msrs);
}
static void nmi_cpu_start(void *dummy)
--
1.5.4.1
* Vegard Nossum <[email protected]> wrote:
> From: Vegard Nossum <[email protected]>
> Date: Sat, 21 Jun 2008 23:44:19 +0200
> Subject: [PATCH] x86/oprofile: disable preemption in nmi_shutdown
>
> BUG: using smp_processor_id() in preemptible [00000000] code: oprofiled/27301
> caller is nmi_shutdown+0x11/0x60
> Pid: 27301, comm: oprofiled Not tainted 2.6.26-rc7 #25
> [<c028a90d>] debug_smp_processor_id+0xbd/0xc0
> [<c045fba1>] nmi_shutdown+0x11/0x60
> [<c045dd4a>] oprofile_shutdown+0x2a/0x60
>
> Note that we don't need this for the other functions, since they are
> all called with on_each_cpu() (which disables preemption for us
> anyway).
applied to tip/x86/oprofile - thanks Vegard.
Ingo