Skip to contentSkip to conversation

oprofile, x86: Fix race in nmi handler while starting

PATCH In review Robert Richter 2 participants 4 messages Jun 7, 2011 – Jun 17, 2011 x86

lore
Share & tools
lore

Robert Richter

author
#0 lore raw

PATCH 1/1 oprofile, x86: Fix race in nmi handler while starting

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 <[email protected]>
Cc <[email protected]> # .35+
Signed-off-by Robert Richter <[email protected]>
---
 arch/x86/oprofile/nmi_int.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)
arch/x86/oprofile/nmi_int.c +10 -4
index cf97500..68894fd 100644
@@ -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);
Signature · 1 line
1.7.5.rc3
#1 lore raw
Quoted 7 lines · 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.

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

author
#2 lore raw
Quoted 8 lines · Ingo Molnar
Quoted 5 lines · 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.

Quoted 8 lines
Quoted 2 lines

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

Signature · 2 lines
Advanced Micro Devices, Inc.
Operating System Research Center
#3 lore raw
Quoted 19 lines · Robert Richter
Quoted 9 lines · Ingo Molnar
Quoted 6 lines · 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.

Quoted 8 lines
Quoted 2 lines

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.

Quoted 6 lines

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