Re: [GIT PULL] oprofile fixes for v2.6.37

[Ingo Molnar]

* Robert Richter <robert.richter@xxxxxxx> 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.

Thanks,

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