Re: [PATCH 12/12] perf, x86: implement the ibs interrupt handler

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

On 19.04.10 14:19:57, Stephane Eranian wrote:
> > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > index bc473ac..a7e4aa5 100644
> > --- a/arch/x86/include/asm/msr-index.h
> > +++ b/arch/x86/include/asm/msr-index.h
> > @@ -113,6 +113,7 @@
> >  #define MSR_AMD64_IBSFETCHCTL          0xc0011030
> >  #define MSR_AMD64_IBSFETCHLINAD                0xc0011031
> >  #define MSR_AMD64_IBSFETCHPHYSAD       0xc0011032
> > +#define MSR_AMD64_IBSFETCH_SIZE                3
> 
> I would use COUNT instead of size given the unit is registers not bytes.

I will change the naming for all macros.

> > +static int amd_pmu_check_ibs(int idx, unsigned int msr, u64 valid,
> > +                            u64 reenable, int size, struct pt_regs *iregs)
> > +{
> > +       struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
> > +       struct perf_event *event = cpuc->events[idx];
> > +       struct perf_sample_data data;
> > +       struct perf_raw_record raw;
> > +       struct pt_regs regs;
> > +       u64 buffer[MSR_AMD64_IBS_SIZE_MAX];
> > +       u64 *buf = buffer;
> > +       int i;
> > +
> > +       if (!test_bit(idx, cpuc->active_mask))
> > +               return 0;
> > +
> > +       rdmsrl(msr++, *buf);
> > +       if (!(*buf++ & valid))
> > +               return 0;
> > +
> > +       perf_sample_data_init(&data, 0);
> > +       if (event->attr.sample_type & PERF_SAMPLE_RAW) {
> > +               for (i = 1; i < size; i++)
> > +                       rdmsrl(msr++, *buf++);
> > +               raw.size = sizeof(u64) * size;
> > +               raw.data = buffer;
> > +               data.raw = &raw;
> > +       }
> > +
> 
> Need to add the padding: raw.size = sizeof(u64) * size + sizeof(u32);

Yes, this triggers a warning. Will change it and add 4 padding bytes
at the buffer start (to be also 8 byte aligned).

> > +static int amd_pmu_handle_irq(struct pt_regs *regs)
> > +{
> > +       int handled, handled2;
> > +
> > +       handled = x86_pmu_handle_irq(regs);
> > +
> > +       if (!x86_pmu.ibs)
> > +               return handled;
> > +
> > +       handled2 = 0;
> > +       handled2 += amd_pmu_check_ibs(X86_PMC_IDX_SPECIAL_IBS_FETCH,
> > +                                     MSR_AMD64_IBSFETCHCTL, IBS_FETCH_VAL,
> > +                                     IBS_FETCH_ENABLE, MSR_AMD64_IBSFETCH_SIZE,
> > +                                     regs);
> > +       handled2 += amd_pmu_check_ibs(X86_PMC_IDX_SPECIAL_IBS_OP,
> > +                                     MSR_AMD64_IBSOPCTL, IBS_OP_VAL,
> > +                                     IBS_OP_ENABLE, MSR_AMD64_IBSOP_SIZE,
> > +                                     regs);
> > +
> > +       if (handled2)
> > +               inc_irq_stat(apic_perf_irqs);
> > +
> 
> If you have both regular counter intr + IBS you will double-count
> apic_perf_irqs.
> I would do: if (handled2 && !handled) inc_irq_stat().
> 

True, will change this.

-Robert

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