linux-kernel - Re: [RFC] perf, x86: Segregate PMU workaraunds into x86_pmu_quirk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 31 May 2010 15:00:58 +0200
From:	Robert Richter <robert.richter@....com>
To:	Cyrill Gorcunov <gorcunov@...il.com>
CC:	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...e.hu>,
	Frédéric Weisbecker <fweisbec@...il.com>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] perf, x86: Segregate PMU workaraunds into
 x86_pmu_quirk_ops structure

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

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