[PATCH] oprofile, x86: Fix race in nmi handler while starting

[PATCH] oprofile, x86: Fix race in nmi handler while starting

[Robert Richter]

In some rare cases, nmis are generated immediately after the nmi
handler of the cpu was started. This causes the counter not to be
enabled. Before enabling the nmi handlers we need to set variable
ctr_running first and make sure its value is written to memory.

Also, the patch makes all existing barriers a memory barrier instead
of a compiler barrier only.

Reported-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: <stable@kernel.org> # .35+
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/oprofile/nmi_int.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c
index cf97500..68894fd 100644
--- a/arch/x86/oprofile/nmi_int.c
+++ b/arch/x86/oprofile/nmi_int.c
@@ -112,8 +112,10 @@ static void nmi_cpu_start(void *dummy)
 static int nmi_start(void)
 {
 	get_online_cpus();
-	on_each_cpu(nmi_cpu_start, NULL, 1);
 	ctr_running = 1;
+	/* make ctr_running visible to the nmi handler: */
+	smp_mb();
+	on_each_cpu(nmi_cpu_start, NULL, 1);
 	put_online_cpus();
 	return 0;
 }
@@ -504,15 +506,18 @@ static int nmi_setup(void)
 
 	nmi_enabled = 0;
 	ctr_running = 0;
-	barrier();
+	/* make variables visible to the nmi handler: */
+	smp_mb();
 	err = register_die_notifier(&profile_exceptions_nb);
 	if (err)
 		goto fail;
 
 	get_online_cpus();
 	register_cpu_notifier(&oprofile_cpu_nb);
-	on_each_cpu(nmi_cpu_setup, NULL, 1);
 	nmi_enabled = 1;
+	/* make nmi_enabled visible to the nmi handler: */
+	smp_mb();
+	on_each_cpu(nmi_cpu_setup, NULL, 1);
 	put_online_cpus();
 
 	return 0;
@@ -531,7 +536,8 @@ static void nmi_shutdown(void)
 	nmi_enabled = 0;
 	ctr_running = 0;
 	put_online_cpus();
-	barrier();
+	/* make variables visible to the nmi handler: */
+	smp_mb();
 	unregister_die_notifier(&profile_exceptions_nb);
 	msrs = &get_cpu_var(cpu_msrs);
 	model->shutdown(msrs);
-- 
1.7.5.rc3

Re: [PATCH] oprofile, x86: Fix race in nmi handler while starting

[Ingo Molnar]

* Robert Richter <robert.richter@amd.com> wrote:

> In some rare cases, nmis are generated immediately after the nmi 
> handler of the cpu was started. This causes the counter not to be 
> enabled. Before enabling the nmi handlers we need to set variable 
> ctr_running first and make sure its value is written to memory.
> 
> Also, the patch makes all existing barriers a memory barrier 
> instead of a compiler barrier only.

Could we *PLEASE* just do the obvious and turn oprofile into a perf 
user on x86 too, like ARM already does? Two years down the line and 
there's zero progress in that area. We want to obsolete the Oprofile 
PMU bits on x86, the infrastructure duplication causes non-trivial 
problems like this one.

Thanks,

	Ingo

Re: [PATCH] oprofile, x86: Fix race in nmi handler while starting

[Robert Richter]

On 09.06.11 03:18:54, Ingo Molnar wrote:
> 
> * Robert Richter <robert.richter@amd.com> wrote:
> 
> > In some rare cases, nmis are generated immediately after the nmi 
> > handler of the cpu was started. This causes the counter not to be 
> > enabled. Before enabling the nmi handlers we need to set variable 
> > ctr_running first and make sure its value is written to memory.
> > 
> > Also, the patch makes all existing barriers a memory barrier 
> > instead of a compiler barrier only.
> 
> Could we *PLEASE* just do the obvious and turn oprofile into a perf 
> user on x86 too, like ARM already does? Two years down the line and 
> there's zero progress in that area. We want to obsolete the Oprofile 
> PMU bits on x86, the infrastructure duplication causes non-trivial 
> problems like this one.

Ingo,

I would rather prefer to spend the effort to add full pmu support to
perf. AMD features missing are still IBS and nb performance counters
for family 15h. The oprofile kernel driver would become obsolete at
all with a new daemon using the perf syscall interface. So why
spending more time with porting this legacy driver to perf in the
kernel? It will be also hard to implement identical behaviour which
may raise concerns about backward compatibility of the kernel driver.

I admit, we have had a lot of bug fixes for oprofile in the past, but
most of them are part of maintaining existing code which we have to do
for distros anyway. There is almost no or only limited feature
development for oprofile now.

Hope this makes sense.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH] oprofile, x86: Fix race in nmi handler while starting

[Ingo Molnar]

* Robert Richter <robert.richter@amd.com> wrote:

> On 09.06.11 03:18:54, Ingo Molnar wrote:
> > 
> > * Robert Richter <robert.richter@amd.com> wrote:
> > 
> > > In some rare cases, nmis are generated immediately after the 
> > > nmi handler of the cpu was started. This causes the counter not 
> > > to be enabled. Before enabling the nmi handlers we need to set 
> > > variable ctr_running first and make sure its value is written 
> > > to memory.
> > > 
> > > Also, the patch makes all existing barriers a memory barrier 
> > > instead of a compiler barrier only.
> > 
> > Could we *PLEASE* just do the obvious and turn oprofile into a 
> > perf user on x86 too, like ARM already does? Two years down the 
> > line and there's zero progress in that area. We want to obsolete 
> > the Oprofile PMU bits on x86, the infrastructure duplication 
> > causes non-trivial problems like this one.
> 
> Ingo,
> 
> I would rather prefer to spend the effort to add full pmu support 
> to perf. AMD features missing are still IBS and nb performance 
> counters for family 15h. The oprofile kernel driver would become 
> obsolete at all with a new daemon using the perf syscall interface.

Agreed - i just don't see *any* progress on the IBS front: more time 
seems to be spent on fixing and sometimes even extending oprofile 
than on any IBS work.

Thanks,

	Ingo