Re: [PATCH] perf: Fix intel shared extra msr allocation

[Stephane Eranian]

Looks nicer, still missing this little piece + is_fake definition and
setting is allocate_fakecpuc())

@@ -1210,7 +1210,7 @@ __intel_shared_reg_put_constraints(struct
cpu_hw_events *cpuc,
         * allocated. Also takes care of event which do
         * not use an extra shared reg
         */
-       if (!reg->alloc)
+       if (!reg->alloc || cpuc->is_fake)
                return;


On Tue, Jun 5, 2012 at 3:04 PM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> On Tue, 2012-06-05 at 14:51 +0200, Stephane Eranian wrote:
>> So I think if you say in:
>> __intel_shared_reg_put_constraints()
>>
>> Â Â Âif (!cpuc->is_fake)
>> Â Â Â Â Â Âreg->alloc = 1;
>>
>> That should do it given that:
>>
>> __intel_shared_reg_put_constraints(struct cpu_hw_events *cpuc,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct hw_perf_event_extra *reg)
>> {
>> Â Â Â Â struct er_account *era;
>> Â Â Â Â if (!reg->alloc)
>> Â Â Â Â Â Â Â Â return;
>>
>> }
>>
>> But then, if reg->alloc was already set to 1, you will have a problem
>> if you leave
>> it as is. So I think in __intel_shared_reg_put_constraints(), you still need:
>>
>> __intel_shared_reg_put_constraints(struct cpu_hw_events *cpuc,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct hw_perf_event_extra *reg)
>> {
>> Â Â Â Âif (!reg->alloc || cpuc->is_fake)
>> Â Â Â Â Â return;
>>
>> Or something like that.
>
> Right, and then something slightly less hideous for intel_try_alt_er().
>
> How does something like this look?
>
> ---
> Âarch/x86/kernel/cpu/perf_event_intel.c | Â 43 +++++++++++++++++++++-----------
> Â1 file changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
> index 166546e..a3b7eb3 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -1119,27 +1119,33 @@ intel_bts_constraints(struct perf_event *event)
> Â Â Â Âreturn NULL;
> Â}
>
> -static bool intel_try_alt_er(struct perf_event *event, int orig_idx)
> +static int intel_alt_er(int idx)
> Â{
> Â Â Â Âif (!(x86_pmu.er_flags & ERF_HAS_RSP_1))
> - Â Â Â Â Â Â Â return false;
> + Â Â Â Â Â Â Â return idx;
>
> - Â Â Â if (event->hw.extra_reg.idx == EXTRA_REG_RSP_0) {
> + Â Â Â if (idx == EXTRA_REG_RSP_0)
> + Â Â Â Â Â Â Â return EXTRA_REG_RSP_1;
> +
> + Â Â Â if (idx == EXTRA_REG_RSP_1)
> + Â Â Â Â Â Â Â return EXTRA_REG_RSP_0;
> +
> + Â Â Â return idx;
> +}
> +
> +static void intel_fixup_er(struct perf_event *event, int idx)
> +{
> + Â Â Â if (idx == EXTRA_REG_RSP_0) {
> Â Â Â Â Â Â Â Âevent->hw.config &= ~INTEL_ARCH_EVENT_MASK;
> Â Â Â Â Â Â Â Âevent->hw.config |= 0x01bb;
> Â Â Â Â Â Â Â Âevent->hw.extra_reg.idx = EXTRA_REG_RSP_1;
> Â Â Â Â Â Â Â Âevent->hw.extra_reg.reg = MSR_OFFCORE_RSP_1;
> - Â Â Â } else if (event->hw.extra_reg.idx == EXTRA_REG_RSP_1) {
> + Â Â Â } else if (idx == EXTRA_REG_RSP_1) {
> Â Â Â Â Â Â Â Âevent->hw.config &= ~INTEL_ARCH_EVENT_MASK;
> Â Â Â Â Â Â Â Âevent->hw.config |= 0x01b7;
> Â Â Â Â Â Â Â Âevent->hw.extra_reg.idx = EXTRA_REG_RSP_0;
> Â Â Â Â Â Â Â Âevent->hw.extra_reg.reg = MSR_OFFCORE_RSP_0;
> Â Â Â Â}
> -
> - Â Â Â if (event->hw.extra_reg.idx == orig_idx)
> - Â Â Â Â Â Â Â return false;
> -
> - Â Â Â return true;
> Â}
>
> Â/*
> @@ -1157,14 +1163,14 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
> Â Â Â Âstruct event_constraint *c = &emptyconstraint;
> Â Â Â Âstruct er_account *era;
> Â Â Â Âunsigned long flags;
> - Â Â Â int orig_idx = reg->idx;
> + Â Â Â int idx = reg->idx;
>
> Â Â Â Â/* already allocated shared msr */
> Â Â Â Âif (reg->alloc)
> Â Â Â Â Â Â Â Âreturn NULL; /* call x86_get_event_constraint() */
>
> Âagain:
> - Â Â Â era = &cpuc->shared_regs->regs[reg->idx];
> + Â Â Â era = &cpuc->shared_regs->regs[idx];
> Â Â Â Â/*
> Â Â Â Â * we use spin_lock_irqsave() to avoid lockdep issues when
> Â Â Â Â * passing a fake cpuc
> @@ -1181,16 +1187,23 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
> Â Â Â Â Â Â Â Âatomic_inc(&era->ref);
>
> Â Â Â Â Â Â Â Â/* no need to reallocate during incremental event scheduling */
> - Â Â Â Â Â Â Â reg->alloc = 1;
> + Â Â Â Â Â Â Â if (!cpuc->is_fake) {
> + Â Â Â Â Â Â Â Â Â Â Â if (idx != reg->idx)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â intel_fixup_er(event, idx);
> + Â Â Â Â Â Â Â Â Â Â Â reg->alloc = 1;
> + Â Â Â Â Â Â Â }
>
> Â Â Â Â Â Â Â Â/*
> Â Â Â Â Â Â Â Â * need to call x86_get_event_constraint()
> Â Â Â Â Â Â Â Â * to check if associated event has constraints
> Â Â Â Â Â Â Â Â */
> Â Â Â Â Â Â Â Âc = NULL;
> - Â Â Â } else if (intel_try_alt_er(event, orig_idx)) {
> - Â Â Â Â Â Â Â raw_spin_unlock_irqrestore(&era->lock, flags);
> - Â Â Â Â Â Â Â goto again;
> + Â Â Â } else {
> + Â Â Â Â Â Â Â idx = intel_alt_er(idx);
> + Â Â Â Â Â Â Â if (idx != reg->idx) {
> + Â Â Â Â Â Â Â Â Â Â Â raw_spin_unlock_irqrestore(&era->lock, flags);
> + Â Â Â Â Â Â Â Â Â Â Â goto again;
> + Â Â Â Â Â Â Â }
> Â Â Â Â}
> Â Â Â Âraw_spin_unlock_irqrestore(&era->lock, flags);
>
>
--
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/