oprofile fixes for v2.6.37

GIT PULL In review Robert Richter 2 participants 7 messages Dec 29, 2010 – Jan 3, 2011 x86

lore

Share & tools

lore

Robert Richter

author

2010-12-29 14:47 UTC #0 lore raw

Ingo,

please pull oprofile fixes for v2.6.37 (tip/perf/urgent):

git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent

(uploaded but not yet sync'ed until now)

Thanks,

-Robert

The following changes since commit e1e359273576ee8fe27021356b064c772ed29af3:

ring_buffer: Off-by-one and duplicate events in ring_buffer_read_page (2010-12-23 12:09:30 -0500)

are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent

Andrew Morton (1):
arch/x86/oprofile/op_model_amd.c: perform initialisation on a single CPU

 arch/x86/oprofile/op_model_amd.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

Ingo Molnar

2010-12-29 17:02 UTC #1 lore raw

Quoted 17 lines · Robert Richter

Ingo,
please pull oprofile fixes for v2.6.37 (tip/perf/urgent):
git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
(uploaded but not yet sync'ed until now)
Thanks,
-Robert
The following changes since commit e1e359273576ee8fe27021356b064c772ed29af3:
ring_buffer: Off-by-one and duplicate events in ring_buffer_read_page (2010-12-23 12:09:30 -0500)

Quoted 8 lines

are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
Andrew Morton (1):
arch/x86/oprofile/op_model_amd.c: perform initialisation on a single CPU
 arch/x86/oprofile/op_model_amd.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

Hm, i'm not sure this fix is correct:

static int op_amd_init(struct oprofile_operations *ops) {
+ /*

+        * init_ibs() preforms implictly cpu-local operations, so pin this
+        * thread to its current CPU
+        */
+       preempt_disable();
        init_ibs();

+ preempt_enable();

If init_ibs() is indeed CPU local, then it needs to be called on all CPUs. Does that happen and if not why not? AFAICS it's only called on one CPU.

Thanks,

Ingo

Robert Richter

author

2010-12-30 13:09 UTC #2 lore raw

Quoted 12 lines · Ingo Molnar

Hm, i'm not sure this fix is correct:
static int op_amd_init(struct oprofile_operations *ops) {
(quoted diff) +6 -0
+       /*
+        * init_ibs() preforms implictly cpu-local operations, so pin this
+        * thread to its current CPU
+        */
+       preempt_disable();
        init_ibs();
+       preempt_enable();

Quoted 2 lines

If init_ibs() is indeed CPU local, then it needs to be called on all CPUs. Does that happen and if not why not? AFAICS it's only called on one CPU.

It is correct to run init_ibs() only on one cpu. It only checks the ibs capabilities and sets up pci devices (if necessary). It runs only on one cpu but operates with the local APIC and some MSRs, thus it is better to disable preemption.

The cpu setup itself runs in op_amd_setup_ctrs() which is per cpu.

-Robert

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center

Ingo Molnar

2010-12-30 17:38 UTC #3 lore raw

Quoted 14 lines · Robert Richter

Quoted 12 lines · Ingo Molnar
Hm, i'm not sure this fix is correct:
static int op_amd_init(struct oprofile_operations *ops) {
(quoted diff) +6 -0
+       /*
+        * init_ibs() preforms implictly cpu-local operations, so pin this
+        * thread to its current CPU
+        */
+       preempt_disable();
        init_ibs();
+       preempt_enable();

Quoted 7 lines

Quoted 2 lines
If init_ibs() is indeed CPU local, then it needs to be called on all CPUs. Does that happen and if not why not? AFAICS it's only called on one CPU.
It is correct to run init_ibs() only on one cpu. It only checks the ibs capabilities and sets up pci devices (if necessary). It runs only on one cpu but operates with the local APIC and some MSRs, thus it is better to disable preemption.

Ok, but in that case the prempt_disable()/enable() should be put into init_ibs(), not be open-coded at the caller like that.

The comment about its cpu-localness could move to init_ibs() as well.

Thanks,

Ingo

Robert Richter

author

2011-01-03 11:30 UTC #4 lore raw

PATCH 1/1 arch/x86/oprofile/op_model_amd.c: perform initialisation on a single CPU

Quoted 25 lines · Ingo Molnar

Quoted 14 lines · Robert Richter
Quoted 12 lines · Ingo Molnar
Hm, i'm not sure this fix is correct:
static int op_amd_init(struct oprofile_operations *ops) {
(quoted diff) +6 -0
+       /*
+        * init_ibs() preforms implictly cpu-local operations, so pin this
+        * thread to its current CPU
+        */
+       preempt_disable();
        init_ibs();
+       preempt_enable();
Quoted 7 lines
Quoted 2 lines
If init_ibs() is indeed CPU local, then it needs to be called on all CPUs. Does that happen and if not why not? AFAICS it's only called on one CPU.
It is correct to run init_ibs() only on one cpu. It only checks the ibs capabilities and sets up pci devices (if necessary). It runs only on one cpu but operates with the local APIC and some MSRs, thus it is better to disable preemption.

Quoted 4 lines

Ok, but in that case the prempt_disable()/enable() should be put into init_ibs(), not be open-coded at the caller like that.
The comment about its cpu-localness could move to init_ibs() as well.

Ingo,

the patch below addresses this. Please apply to tip/perf/urgent.

Thanks,

-Robert

Quoted 1 line

From 41659787561d3b384dac0b8458de95e35d8f2a34 Mon Sep 17 00:00:00 2001

From: Robert Richter <[email protected]>
Date: Mon, 3 Jan 2011 10:36:53 +0100
Subject: [PATCH] arch/x86/oprofile/op_model_amd.c: perform initialisation on a single CPU

Disable preemption in init_ibs(). The function only checks the ibs capabilities and sets up pci devices (if necessary). It runs only on one cpu but operates with the local APIC and some MSRs, thus it is better to disable preemption.

[    7.034377] BUG: using smp_processor_id() in preemptible [00000000] code:
modprobe/483
[    7.034385] caller is setup_APIC_eilvt+0x155/0x180
[    7.034389] Pid: 483, comm: modprobe Not tainted 2.6.37-rc1-20101110+ #1
[    7.034392] Call Trace:
[    7.034400]  [<ffffffff812a2b72>] debug_smp_processor_id+0xd2/0xf0
[    7.034404]  [<ffffffff8101e985>] setup_APIC_eilvt+0x155/0x180
[    7.034413]  [<ffffffffa002e168>] op_amd_init+0x88/0x2b0 [oprofile]
[    7.034420]  [<ffffffffa0043000>] ? oprofile_init+0x0/0x42 [oprofile]
[    7.034425]  [<ffffffffa0043315>] op_nmi_init+0x249/0x2af [oprofile]
[    7.034431]  [<ffffffffa00430b4>] oprofile_arch_init+0x11/0x29 [oprofile]
[    7.034437]  [<ffffffffa0043010>] oprofile_init+0x10/0x42 [oprofile]
[    7.034441]  [<ffffffff810001e3>] do_one_initcall+0x43/0x170
[    7.034445]  [<ffffffff8108a52a>] sys_init_module+0xba/0x200
[    7.034449]  [<ffffffff8100285b>] system_call_fastpath+0x16/0x1b

Addresses https://bugzilla.kernel.org/show_bug.cgi?id=22812

Reported-by <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Dan Carpenter <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Andrew Morton <[email protected]>

Cc <[email protected]> [2.6.37.x]

Signed-off-by Robert Richter <[email protected]>

---
 arch/x86/oprofile/op_model_amd.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

arch/x86/oprofile/op_model_amd.c +15 -6

index a011bcc..8d7d883 100644
@@ -630,7 +630,14 @@ static int __init_ibs_nmi(void)
 	return 0;
 }
 
-/* initialize the APIC for the IBS interrupts if available */
+/*
+ * check and reserve APIC extended interrupt LVT offset for IBS if
+ * available
+ *
+ * init_ibs() preforms implicitly cpu-local operations, so pin this
+ * thread to its current CPU
+ */
+
 static void init_ibs(void)
 {
 	ibs_caps = get_ibs_caps();

@@ -638,13 +645,15 @@ static void init_ibs(void)
 	if (!ibs_caps)
 		return;
 
-	if (__init_ibs_nmi()) {
+	preempt_disable();
+
+	if (__init_ibs_nmi())
 		ibs_caps = 0;
-		return;
-	}
+	else
+		printk(KERN_INFO "oprofile: AMD IBS detected (0x%08x)\n",
+		       (unsigned)ibs_caps);
 
-	printk(KERN_INFO "oprofile: AMD IBS detected (0x%08x)\n",
-	       (unsigned)ibs_caps);
+	preempt_enable();
 }
 
 static int (*create_arch_files)(struct super_block *sb, struct dentry *root);

Signature · 7 lines

1.7.3.2



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

Ingo Molnar

2011-01-03 12:04 UTC #5 lore raw

Re: [PATCH] arch/x86/oprofile/op_model_amd.c: perform initialisation on a single CPU

Quoted 3 lines · Robert Richter

static void init_ibs(void)
{
ibs_caps = get_ibs_caps();

this get_ibs_caps() call is percpu too - still it now runs with preempt off.

Review

Quoted 2 lines

(quoted diff) +2 -0

+		printk(KERN_INFO "oprofile: AMD IBS detected (0x%08x)\n",
+		       (unsigned)ibs_caps);

that cast looks ugly and unnecessary.

I fixed both in the commit below. (not tested yet)

Thanks,

Ingo

--------------->

Quoted 1 line

From c7c25802b39c443b3745cfa973dc49a97a3491f8 Mon Sep 17 00:00:00 2001

From: Robert Richter <[email protected]>
Date: Mon, 3 Jan 2011 12:15:14 +0100
Subject: [PATCH] arch/x86/oprofile/op_model_amd.c: Perform initialisation on a single CPU

[    7.034377] BUG: using smp_processor_id() in preemptible [00000000] code: modprobe/483
[    7.034385] caller is setup_APIC_eilvt+0x155/0x180
[    7.034389] Pid: 483, comm: modprobe Not tainted 2.6.37-rc1-20101110+ #1
[    7.034392] Call Trace:
[    7.034400]  [<ffffffff812a2b72>] debug_smp_processor_id+0xd2/0xf0
[    7.034404]  [<ffffffff8101e985>] setup_APIC_eilvt+0x155/0x180

[ ... ]

Addresses https://bugzilla.kernel.org/show_bug.cgi?id=22812

Reported-by <[email protected]>

Signed-off-by Robert Richter <[email protected]>

Cc [email protected] <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Dan Carpenter <[email protected]>

Cc Andrew Morton <[email protected]>

Cc <[email protected]> [2.6.37.x]

LKML-Reference: <[email protected]>

[ small cleanups ]

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/oprofile/op_model_amd.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

arch/x86/oprofile/op_model_amd.c +16 -8

index a011bcc..7d90d47 100644
@@ -630,21 +630,29 @@ static int __init_ibs_nmi(void)
 	return 0;
 }
 
-/* initialize the APIC for the IBS interrupts if available */
+/*
+ * check and reserve APIC extended interrupt LVT offset for IBS if
+ * available
+ *
+ * init_ibs() preforms implicitly cpu-local operations, so pin this
+ * thread to its current CPU
+ */
+
 static void init_ibs(void)
 {
-	ibs_caps = get_ibs_caps();
+	preempt_disable();
 
+	ibs_caps = get_ibs_caps();
 	if (!ibs_caps)
-		return;
+		goto out;
 
-	if (__init_ibs_nmi()) {
+	if (__init_ibs_nmi() < 0)
 		ibs_caps = 0;
-		return;
-	}
+	else
+		printk(KERN_INFO "oprofile: AMD IBS detected (0x%08x)\n", ibs_caps);
 
-	printk(KERN_INFO "oprofile: AMD IBS detected (0x%08x)\n",
-	       (unsigned)ibs_caps);
+out:
+	preempt_enable();
 }
 
 static int (*create_arch_files)(struct super_block *sb, struct dentry *root);

Robert Richter

author

2011-01-03 14:44 UTC #6 lore raw

Re: [PATCH] arch/x86/oprofile/op_model_amd.c: perform initialisation on a single CPU

Ingo,

I tested your patch and it fixes the bug too.

Quoted 8 lines · Ingo Molnar

Quoted 3 lines · Robert Richter
static void init_ibs(void)
{
ibs_caps = get_ibs_caps();
this get_ibs_caps() call is percpu too - still it now runs with preempt off.

get_ibs_caps() uses the cpuid instruction and this is percpu. But mixed silicon support does not allow the use of different cpus with different cpuid features in one system, so it should be save to use it with preemption enabled.

Also, since the check uses a combination of boot_cpu_has() and cpuid_eax(), disabling preemption would not help here. We would have to pin the init code to the boot cpu then. Suppose a boot cpu with ibs enabled and a secondary (init) cpu with ibs disabled. This will crash the system when accessing ibs msrs.

Anyway, I am fine with your change.

Quoted 5 lines

Review
Quoted 3 lines
(quoted diff) +2 -0
+		printk(KERN_INFO "oprofile: AMD IBS detected (0x%08x)\n",
+		       (unsigned)ibs_caps);
that cast looks ugly and unnecessary.

Yes, this cast is unnecessary.

Quoted 2 lines

I fixed both in the commit below. (not tested yet)

Thanks for looking at this,

-Robert

Signature · 2 lines

Advanced Micro Devices, Inc.
Operating System Research Center