Re: [RFC] perf, x86: Segregate PMU workaraunds into x86_pmu_quirk_ops structure

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Mon, May 31, 2010 at 03:00:58PM +0200, Robert Richter wrote:
> On 29.05.10 14:24:09, Cyrill Gorcunov wrote:
> > Hi,
> > 
> > I would appreciate comments/complains on the following patch. The idea is to implement
> > PMU quirks with minimal impact. At the moment two quirks are addressed -
> > PEBS disabling on Clovertown and P4 performance counter double write.
> > PEBS disabling already was there only moved to x86_pmu_quirk_ops. Note
> > that I didn't use pointer to the structure intensionally but embed it into
> > x86_pmu, if the structure grow we will need to use a pointer to the structure.
> 
> The quirk functions add additional code and ops structures to the
> already existing model specific code. This quirks would be fine if we
> would could merge model specific code and get unified code. But these
> model specific code cannot be replaced. So I rather prefer to
> implement cpu errata in model specific code.
> 

agreed, but this quirks ops looked as more clear solution for me,
at least in a sake of initiating the 'find something better' dialogue
you know :)

> > @@ -185,6 +185,11 @@ union perf_capabilities {
> >  	u64	capabilities;
> >  };
> >  
> > +struct x86_pmu_quirk_ops {
> > +	void		(*pmu_init)(void);
> 
> This init quirk could be much better handled in the model specific
> init code (intel_pmu_init()/amd_pmu_init()). I don't see a reason for
> adding the quirk first and then immediately calling it. The quirk
> function could be called directly instead.
> 

well, at moment we have only the one caller but if the number of
callers increase _better_ to have it called via function pointer
since it's easier to find out the callers in further and we're allowed
to use such approach since this is a not fast path code. On the other
hands I rather agree with you -- it's overzealous at the moment.
/me: dropping idea on *pmu_init

> > +	void		(*perfctr_write)(unsigned long addr, u64 value);
> 
> This one is difficult to avoid ...
> 

unfortunately

> > @@ -924,7 +930,11 @@ x86_perf_event_set_period(struct perf_ev
> >  	 */
> >  	atomic64_set(&hwc->prev_count, (u64)-left);
> >  
> > -	wrmsrl(hwc->event_base + idx,
> > +	if (x86_pmu.quirks.perfctr_write)
> > +		x86_pmu.quirks.perfctr_write(hwc->event_base + idx,
> > +				(u64)(-left) & x86_pmu.cntval_mask);
> > +	else
> > +		wrmsrl(hwc->event_base + idx,
> 
> ... but it introduces another check in the fast path. There are some
> options to avoid this. First we could see if we rather implement this
> in model specific interrupt handlers (there is p4_pmu_handle_irq()).

no, we can't, I thought about that, this code is called from several
places.

> Or, we implement an optimized check for perf quirks, maybe using
> ALTERNATIVE or jump labels.
> 

yes! Robert, I completely forgot about alternatives. I guess that
is exactly what we need! I'll try to implement.

> I think we can handle both quirks, but if we start using and extending
> it more, it will have a performance impact and code will also more
> complicated. So, I think it is rather inappropriate as a general
> approach.
> 
> -Robert
> 

Thanks a huge for comments, Robert!

> >  			(u64)(-left) & x86_pmu.cntval_mask);
> >  
> >  	perf_event_update_userpage(event);
> 
> -- 
> Advanced Micro Devices, Inc.
> Operating System Research Center
> email: robert.richter@amd.com
> 
	-- Cyrill