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

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

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.

> @@ -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.

> +	void		(*perfctr_write)(unsigned long addr, u64 value);

This one is difficult to avoid ...

> @@ -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()).
Or, we implement an optimized check for perf quirks, maybe using
ALTERNATIVE or jump labels.

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

>  			(u64)(-left) & x86_pmu.cntval_mask);
>  
>  	perf_event_update_userpage(event);

-- 
Advanced Micro Devices, Inc.
Operating System Research Center
email: robert.richter@amd.com