Messages in this thread | | | Date | Mon, 3 Jan 2011 15:44:20 +0100 | From | Robert Richter <> | Subject | 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.
On 03.01.11 07:03:10, Ingo Molnar wrote: > > * Robert Richter <robert.richter@amd.com> wrote: > > > 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.
> > > + printk(KERN_INFO "oprofile: AMD IBS detected (0x%08x)\n", > > + (unsigned)ibs_caps); > > that cast looks ugly and unnecessary.
Yes, this cast is unnecessary.
> > I fixed both in the commit below. (not tested yet)
Thanks for looking at this,
-Robert
-- Advanced Micro Devices, Inc. Operating System Research Center
| |