* [PATCH] MIPS: oprofile: Fix for BUG: using smp_processor_id() in preemptible [00000000] code: oprofiled/1362
@ 2013-08-01 15:10 Jerin Jacob
2013-08-01 16:31 ` Ralf Baechle
0 siblings, 1 reply; 3+ messages in thread
From: Jerin Jacob @ 2013-08-01 15:10 UTC (permalink / raw)
To: ralf; +Cc: Jerin Jacob, linux-mips, mbhat, jchandra
current_cpu_type() is not preemption-safe.
If CONFIG_PREEMPT is enabled then mipsxx_reg_setup() can be called from preemptible state.
Added get_cpu()/put_cpu() pair to make it preemption-safe.
This was found while testing oprofile with CONFIG_DEBUG_PREEMPT enable.
/usr/zntestsuite # opcontrol --init
/usr/zntestsuite # opcontrol --setup --event=L2_CACHE_ACCESSES:500 --event=L2_CACHE_MISSES:500 --no-vmlinux
/usr/zntestsuite # opcontrol --start
Using 2.6+ OProfile kernel interface.
BUG: using smp_processor_id() in preemptible [00000000] code: oprofiled/1362
caller is mipsxx_reg_setup+0x11c/0x164
CPU: 0 PID: 1362 Comm: oprofiled Not tainted 3.10.4 #18
Stack : 00000006 70757465 00000000 00000000 00000000 00000000 80b173f6 00000037
80b10000 00000000 80b21614 88f5a220 00000000 00000000 00000000 00000000
00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000 89c49c00 89c49c2c 80721254 807b7927 8012c1d0
80b10000 80721254 00000000 00000552 88f5a220 80b1335c 807b78e6 89c49ba8
...
Call Trace:
[<801099a4>] show_stack+0x64/0x7c
[<80665520>] dump_stack+0x20/0x2c
[<803a2250>] debug_smp_processor_id+0xe0/0xf0
[<8052df24>] mipsxx_reg_setup+0x11c/0x164
[<8052cd70>] op_mips_setup+0x24/0x4c
[<80529cfc>] oprofile_setup+0x5c/0x12c
[<8052b9f8>] event_buffer_open+0x78/0xf8
[<801c3150>] do_dentry_open.isra.15+0x2b8/0x3b0
[<801c3270>] finish_open+0x28/0x4c
[<801d49b8>] do_last.isra.41+0x2cc/0xd00
[<801d54a0>] path_openat+0xb4/0x4c4
[<801d5c44>] do_filp_open+0x3c/0xac
[<801c4744>] do_sys_open+0x110/0x1f4
[<8010f47c>] stack_done+0x20/0x44
Signed-off-by: Jerin Jacob <jerinjacobk@gmail.com>
---
arch/mips/oprofile/op_model_mipsxx.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/mips/oprofile/op_model_mipsxx.c b/arch/mips/oprofile/op_model_mipsxx.c
index e4b1140..20a80ea 100644
--- a/arch/mips/oprofile/op_model_mipsxx.c
+++ b/arch/mips/oprofile/op_model_mipsxx.c
@@ -147,7 +147,7 @@ static struct mipsxx_register_config {
static void mipsxx_reg_setup(struct op_counter_config *ctr)
{
- unsigned int counters = op_model_mipsxx_ops.num_counters;
+ unsigned int curr_cpu, counters = op_model_mipsxx_ops.num_counters;
int i;
/* Compute the performance counter control word. */
@@ -166,8 +166,10 @@ static void mipsxx_reg_setup(struct op_counter_config *ctr)
reg.control[i] |= M_PERFCTL_USER;
if (ctr[i].exl)
reg.control[i] |= M_PERFCTL_EXL;
- if (current_cpu_type() == CPU_XLR)
+ curr_cpu = get_cpu();
+ if (cpu_data[curr_cpu].cputype == CPU_XLR)
reg.control[i] |= M_PERFCTL_COUNT_ALL_THREADS;
+ put_cpu();
reg.counter[i] = 0x80000000 - ctr[i].count;
}
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] MIPS: oprofile: Fix for BUG: using smp_processor_id() in preemptible [00000000] code: oprofiled/1362
2013-08-01 15:10 [PATCH] MIPS: oprofile: Fix for BUG: using smp_processor_id() in preemptible [00000000] code: oprofiled/1362 Jerin Jacob
@ 2013-08-01 16:31 ` Ralf Baechle
2013-08-02 10:27 ` Jerin Jacob
0 siblings, 1 reply; 3+ messages in thread
From: Ralf Baechle @ 2013-08-01 16:31 UTC (permalink / raw)
To: Jerin Jacob; +Cc: linux-mips, mbhat, jchandra
On Thu, Aug 01, 2013 at 08:40:41PM +0530, Jerin Jacob wrote:
> current_cpu_type() is not preemption-safe.
> If CONFIG_PREEMPT is enabled then mipsxx_reg_setup() can be called from preemptible state.
> Added get_cpu()/put_cpu() pair to make it preemption-safe.
>
> This was found while testing oprofile with CONFIG_DEBUG_PREEMPT enable.
[...]
Interesting. I wonder how many more of this kind of bug we got lurking
around.
In case of oprofile, we silently assume that all processors have the same
number of counters, the same style and number of counters. So it'd be
ok to just look at the boot CPU which is even simpler than your fix.
What do you think about below fix?
Thanks,
Ralf
arch/mips/include/asm/cpu-features.h | 2 ++
arch/mips/oprofile/op_model_mipsxx.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/mips/include/asm/cpu-features.h b/arch/mips/include/asm/cpu-features.h
index 1dc0860..fa44f3e 100644
--- a/arch/mips/include/asm/cpu-features.h
+++ b/arch/mips/include/asm/cpu-features.h
@@ -17,6 +17,8 @@
#define current_cpu_type() current_cpu_data.cputype
#endif
+#define boot_cpu_type() cpu_data[0].cputype
+
/*
* SMP assumption: Options of CPU 0 are a superset of all processors.
* This is true for all known MIPS systems.
diff --git a/arch/mips/oprofile/op_model_mipsxx.c b/arch/mips/oprofile/op_model_mipsxx.c
index e4b1140..3a2b6e9 100644
--- a/arch/mips/oprofile/op_model_mipsxx.c
+++ b/arch/mips/oprofile/op_model_mipsxx.c
@@ -166,7 +166,7 @@ static void mipsxx_reg_setup(struct op_counter_config *ctr)
reg.control[i] |= M_PERFCTL_USER;
if (ctr[i].exl)
reg.control[i] |= M_PERFCTL_EXL;
- if (current_cpu_type() == CPU_XLR)
+ if (boot_cpu_type() == CPU_XLR)
reg.control[i] |= M_PERFCTL_COUNT_ALL_THREADS;
reg.counter[i] = 0x80000000 - ctr[i].count;
}
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] MIPS: oprofile: Fix for BUG: using smp_processor_id() in preemptible [00000000] code: oprofiled/1362
2013-08-01 16:31 ` Ralf Baechle
@ 2013-08-02 10:27 ` Jerin Jacob
0 siblings, 0 replies; 3+ messages in thread
From: Jerin Jacob @ 2013-08-02 10:27 UTC (permalink / raw)
To: Ralf Baechle; +Cc: linux-mips, mbhat, jchandra
On Thu, Aug 01, 2013 at 06:31:05PM +0200, Ralf Baechle wrote:
> On Thu, Aug 01, 2013 at 08:40:41PM +0530, Jerin Jacob wrote:
>
> > current_cpu_type() is not preemption-safe.
> > If CONFIG_PREEMPT is enabled then mipsxx_reg_setup() can be called from preemptible state.
> > Added get_cpu()/put_cpu() pair to make it preemption-safe.
> >
> > This was found while testing oprofile with CONFIG_DEBUG_PREEMPT enable.
> [...]
>
> Interesting. I wonder how many more of this kind of bug we got lurking
> around.
>
> In case of oprofile, we silently assume that all processors have the same
> number of counters, the same style and number of counters. So it'd be
> ok to just look at the boot CPU which is even simpler than your fix.
>
> What do you think about below fix?
yes, it make sense to compare the cpu_type against boot cpu.
Acked-by: Jerin Jacob <jerinjacobk@gmail.com>
>
> Thanks,
>
> Ralf
>
> arch/mips/include/asm/cpu-features.h | 2 ++
> arch/mips/oprofile/op_model_mipsxx.c | 2 +-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/include/asm/cpu-features.h b/arch/mips/include/asm/cpu-features.h
> index 1dc0860..fa44f3e 100644
> --- a/arch/mips/include/asm/cpu-features.h
> +++ b/arch/mips/include/asm/cpu-features.h
> @@ -17,6 +17,8 @@
> #define current_cpu_type() current_cpu_data.cputype
> #endif
>
> +#define boot_cpu_type() cpu_data[0].cputype
> +
> /*
> * SMP assumption: Options of CPU 0 are a superset of all processors.
> * This is true for all known MIPS systems.
> diff --git a/arch/mips/oprofile/op_model_mipsxx.c b/arch/mips/oprofile/op_model_mipsxx.c
> index e4b1140..3a2b6e9 100644
> --- a/arch/mips/oprofile/op_model_mipsxx.c
> +++ b/arch/mips/oprofile/op_model_mipsxx.c
> @@ -166,7 +166,7 @@ static void mipsxx_reg_setup(struct op_counter_config *ctr)
> reg.control[i] |= M_PERFCTL_USER;
> if (ctr[i].exl)
> reg.control[i] |= M_PERFCTL_EXL;
> - if (current_cpu_type() == CPU_XLR)
> + if (boot_cpu_type() == CPU_XLR)
> reg.control[i] |= M_PERFCTL_COUNT_ALL_THREADS;
> reg.counter[i] = 0x80000000 - ctr[i].count;
> }
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-08-02 10:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-01 15:10 [PATCH] MIPS: oprofile: Fix for BUG: using smp_processor_id() in preemptible [00000000] code: oprofiled/1362 Jerin Jacob
2013-08-01 16:31 ` Ralf Baechle
2013-08-02 10:27 ` Jerin Jacob
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.