v2.6.26-rc7/oprofile: BUG: using smp_processor_id() in preemptible

v2.6.26-rc7/oprofile: BUG: using smp_processor_id() in preemptible

[Vegard Nossum]

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

[PATCH] x86/oprofile: disable preemption in nmi_shutdown

[Vegard Nossum]

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 <vegard.nossum@gmail.com>
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 <vegard.nossum@gmail.com>
---
 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

Re: [PATCH] x86/oprofile: disable preemption in nmi_shutdown

[Johannes Weiner]

Hi Vegard,

Vegard Nossum <vegard.nossum@gmail.com> 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 <vegard.nossum@gmail.com>
> 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 <vegard.nossum@gmail.com>
> ---
>  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

Re: [PATCH] x86/oprofile: disable preemption in nmi_shutdown

[Vegard Nossum]

Hi,

On Sun, Jun 22, 2008 at 9:30 AM, Johannes Weiner <hannes@saeurebad.de> wrote:
> Vegard Nossum <vegard.nossum@gmail.com> 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 <vegard.nossum@gmail.com>
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 <vegard.nossum@gmail.com>
---
 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

Re: [PATCH] x86/oprofile: disable preemption in nmi_shutdown

[Ingo Molnar]

* Vegard Nossum <vegard.nossum@gmail.com> wrote:

> From: Vegard Nossum <vegard.nossum@gmail.com>
> 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