lkml.org 
[lkml]   [2012]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] perf: Fix intel shared extra msr allocation
From
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@infradead.org> 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2012-06-05 16:21    [W:0.121 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site