From: Robert R. <rob...@am...> - 2011-06-07 17:09:47
|
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 <sur...@am...> Cc: <st...@ke...> # .35+ Signed-off-by: Robert Richter <rob...@am...> --- 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 |
From: Ingo M. <mi...@el...> - 2011-06-09 07:19:19
|
* Robert Richter <rob...@am...> 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 |
From: Robert R. <rob...@am...> - 2011-06-16 15:26:16
|
On 09.06.11 03:18:54, Ingo Molnar wrote: > > * Robert Richter <rob...@am...> 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 |
From: Ingo M. <mi...@el...> - 2011-06-17 08:06:38
|
* Robert Richter <rob...@am...> wrote: > On 09.06.11 03:18:54, Ingo Molnar wrote: > > > > * Robert Richter <rob...@am...> 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 |