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

[Robert Richter <robert.richter@amd.com>]

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