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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 3 Jan 2011 12:15:14 +0100
From:	Robert Richter <robert.richter@....com>
To:	Ingo Molnar <mingo@...e.hu>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"oprofile-list@...ts.sourceforge.net" 
	<oprofile-list@...ts.sourceforge.net>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Frédéric Weisbecker <fweisbec@...il.com>,
	<atswartz@...il.com>, "Rafael J. Wysocki" <rjw@...k.pl>,
	Dan Carpenter <error27@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>, <stable@...nel.org>
Subject: [PATCH] arch/x86/oprofile/op_model_amd.c: perform initialisation
 on a single CPU

On 30.12.10 12:38:47, Ingo Molnar wrote:
> 
> * Robert Richter <robert.richter@....com> wrote:
> 
> > On 29.12.10 11:37:43, Ingo Molnar wrote:
> > 
> > > 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.
> > 
> > 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.

Ingo,

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

Thanks,

-Robert

--

>From 41659787561d3b384dac0b8458de95e35d8f2a34 Mon Sep 17 00:00:00 2001
From: Robert Richter <robert.richter@....com>
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: <atswartz@...il.com>
Cc: "Rafael J. Wysocki" <rjw@...k.pl>
Cc: Dan Carpenter <error27@...il.com>
Cc: Ingo Molnar <mingo@...e.hu>
Cc: Andrew Morton <akpm@...ux-foundation.org>
Cc: <stable@...nel.org>         [2.6.37.x]
Signed-off-by: Robert Richter <robert.richter@....com>
---
 arch/x86/oprofile/op_model_amd.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/x86/oprofile/op_model_amd.c b/arch/x86/oprofile/op_model_amd.c
index a011bcc..8d7d883 100644
--- a/arch/x86/oprofile/op_model_amd.c
+++ b/arch/x86/oprofile/op_model_amd.c
@@ -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);
-- 
1.7.3.2



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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/