Prev: [PATCH] altera_uart: Proper section for altera_uart_remove
Next: drivers: remove all i2c_set_clientdata(client, NULL)
From: Robert Richter on 31 May 2010 09:10
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(a)amd.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on 31 May 2010 09:50
On Mon, 2010-05-31 at 15:00 +0200, Robert Richter wrote:
>
> 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.

The reason the existing quirk() function is called after *_pmu_init() is
because of the funny way it all writes to the console.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Zijlstra on 31 May 2010 12:40

On Sat, 2010-05-29 at 22:24 +0400, Cyrill Gorcunov wrote:
> @@ -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,
> (u64)(-left) & x86_pmu.cntval_mask);

This bit is rather ugly,.. not quite sure how to clean it up though.
Anybody got a bright idea?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [PATCH] altera_uart: Proper section for altera_uart_remove
Next: drivers: remove all i2c_set_clientdata(client, NULL)