Re: [PATCH v2 -tip] perf: x86, add SandyBridge support

[Stephane Eranian]

On Tue, Mar 1, 2011 at 9:45 AM, Lin Ming <ming.m.lin@xxxxxxxxx> wrote:
> On Tue, 2011-03-01 at 15:43 +0800, Stephane Eranian wrote:
>> On Mon, Feb 28, 2011 at 10:15 AM, Peter Zijlstra <a.p.zijlstra@xxxxxxxxx> wrote:
>> > On Mon, 2011-02-28 at 15:22 +0800, Lin Ming wrote:
>> >> This patch adds basic SandyBridge support, including hardware cache
>> >> events and PEBS events support.
>> >>
>> >> LLC-* hareware cache events don't work for now, it depends on the
>> >> offcore patches.
>> >
>> > What's the status of those, Stephane reported some problems last I
>> > remember?
>> >
>> I tried the trick I mentioned and it seems to work.
>>
>> Something like below with hwc->extra_alloc.
>> Could probably find a better name for that field.
>
> Stephane,
>
> I'll integrate below changes to the offcore patches, OK?
>
Let me try one more test on this.
I want to show the case the caused the problem in the first place.

> diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
> index f152930..ac1d100 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -906,7 +906,7 @@ intel_percore_constraints(struct cpu_hw_events *cpuc, struct perf_event *event)
> Â Â Â Âint free_slot;
> Â Â Â Âint found;
>
> - Â Â Â if (!x86_pmu.percore_constraints)
> + Â Â Â if (!x86_pmu.percore_constraints || hwc->extra_alloc)
> Â Â Â Â Â Â Â Âreturn NULL;
>
> Â Â Â Âfor (c = x86_pmu.percore_constraints; c->cmask; c++) {
> @@ -931,6 +931,7 @@ intel_percore_constraints(struct cpu_hw_events *cpuc, struct perf_event *event)
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âif (hwc->extra_config == era->extra_config) {
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âera->ref++;
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âcpuc->percore_used = 1;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â hwc->extra_alloc = 1;
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âc = NULL;
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â}
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â/* else conflict */
> @@ -945,6 +946,7 @@ intel_percore_constraints(struct cpu_hw_events *cpuc, struct perf_event *event)
> Â Â Â Â Â Â Â Â Â Â Â Âera->extra_reg = hwc->extra_reg;
> Â Â Â Â Â Â Â Â Â Â Â Âera->extra_config = hwc->extra_config;
> Â Â Â Â Â Â Â Â Â Â Â Âcpuc->percore_used = 1;
> + Â Â Â Â Â Â Â Â Â Â Â hwc->extra_alloc = 1;
> Â Â Â Â Â Â Â Â Â Â Â Âc = NULL;
> Â Â Â Â Â Â Â Â}
> Â Â Â Â Â Â Â Âraw_spin_unlock(&pc->lock);
> @@ -998,6 +1000,7 @@ static void intel_put_event_constraints(struct cpu_hw_events *cpuc,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Âera->extra_config == hwc->extra_config &&
> Â Â Â Â Â Â Â Â Â Â Â Â Â Âera->extra_reg == er->msr) {
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âera->ref--;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â hwc->extra_alloc = 0;
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âbreak;
> Â Â Â Â Â Â Â Â Â Â Â Â}
> Â Â Â Â Â Â Â Â}
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index f531ce3..dbbf33a 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -546,6 +546,7 @@ struct hw_perf_event {
>            Âint       last_cpu;
>            Âunsigned int  Âextra_reg;
> Â Â Â Â Â Â Â Â Â Â Â Âu64 Â Â Â Â Â Â extra_config;
> +            int       extra_alloc;
> Â Â Â Â Â Â Â Â};
> Â Â Â Â Â Â Â Âstruct { /* software */
> Â Â Â Â Â Â Â Â Â Â Â Âstruct hrtimer Âhrtimer;
>
>
>>
>> static struct event_constraint *
>> intel_percore_constraints(struct cpu_hw_events *cpuc, struct perf_event *event)
>> {
>> Â Â Â Â struct hw_perf_event *hwc = &event->hw;
>> Â Â Â Â unsigned int e = hwc->config & ARCH_PERFMON_EVENTSEL_EVENT;
>> Â Â Â Â struct event_constraint *c;
>> Â Â Â Â struct intel_percore *pc;
>> Â Â Â Â struct er_account *era;
>> Â Â Â Â int i;
>> Â Â Â Â int free_slot;
>> Â Â Â Â int found;
>>
>> Â Â Â Â if (!x86_pmu.percore_constraints)
>> Â Â Â Â Â Â Â Â return NULL;
>>
>> Â Â Â Â if (hwc->extra_alloc)
>> Â Â Â Â Â Â Â Â return NULL;
>>
>> Â Â Â Â for (c = x86_pmu.percore_constraints; c->cmask; c++) {
>> Â Â Â Â Â Â Â Â if (e != c->code)
>> Â Â Â Â Â Â Â Â Â Â Â Â continue;
>>
>> Â Â Â Â Â Â Â Â /*
>> Â Â Â Â Â Â Â Â Â* Allocate resource per core.
>> Â Â Â Â Â Â Â Â Â*/
>> Â Â Â Â Â Â Â Â c = NULL;
>> Â Â Â Â Â Â Â Â pc = cpuc->per_core;
>> Â Â Â Â Â Â Â Â if (!pc)
>> Â Â Â Â Â Â Â Â Â Â Â Â break;
>> Â Â Â Â Â Â Â Â c = &emptyconstraint;
>> Â Â Â Â Â Â Â Â raw_spin_lock(&pc->lock);
>> Â Â Â Â Â Â Â Â free_slot = -1;
>> Â Â Â Â Â Â Â Â found = 0;
>> Â Â Â Â Â Â Â Â for (i = 0; i < MAX_EXTRA_REGS; i++) {
>> Â Â Â Â Â Â Â Â Â Â Â Â era = &pc->regs[i];
>> Â Â Â Â Â Â Â Â Â Â Â Â if (era->ref > 0 && hwc->extra_reg == era->extra_reg) {
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â /* Allow sharing same config */
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (hwc->extra_config == era->extra_config) {
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â era->ref++;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â cpuc->percore_used = 1;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â hwc->extra_alloc = 1;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â c = NULL;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â /* else conflict */
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â found = 1;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
>> Â Â Â Â Â Â Â Â Â Â Â Â } else if (era->ref == 0 && free_slot == -1)
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â free_slot = i;
>> Â Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â Â if (!found && free_slot != -1) {
>> Â Â Â Â Â Â Â Â Â Â Â Â era = &pc->regs[free_slot];
>> Â Â Â Â Â Â Â Â Â Â Â Â era->ref = 1;
>> Â Â Â Â Â Â Â Â Â Â Â Â era->extra_reg = hwc->extra_reg;
>> Â Â Â Â Â Â Â Â Â Â Â Â era->extra_config = hwc->extra_config;
>> Â Â Â Â Â Â Â Â Â Â Â Â cpuc->percore_used = 1;
>> Â Â Â Â Â Â Â Â Â Â Â Â hwc->extra_alloc = 1;
>> Â Â Â Â Â Â Â Â Â Â Â Â c = NULL;
>> Â Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â Â raw_spin_unlock(&pc->lock);
>> Â Â Â Â Â Â Â Â return c;
>> Â Â Â Â }
>>
>> Â Â Â Â return NULL;
>> }
>>
>> static void intel_put_event_constraints(struct cpu_hw_events *cpuc,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct perf_event *event)
>> {
>> Â Â Â Â struct extra_reg *er;
>> Â Â Â Â struct intel_percore *pc;
>> Â Â Â Â struct er_account *era;
>> Â Â Â Â struct hw_perf_event *hwc = &event->hw;
>> Â Â Â Â int i, allref;
>>
>> Â Â Â Â if (!cpuc->percore_used)
>> Â Â Â Â Â Â Â Â return;
>>
>> Â Â Â Â for (er = x86_pmu.extra_regs; er->msr; er++) {
>> Â Â Â Â Â Â Â Â if (er->event != (hwc->config & er->config_mask))
>> Â Â Â Â Â Â Â Â Â Â Â Â continue;
>>
>> Â Â Â Â Â Â Â Â pc = cpuc->per_core;
>> Â Â Â Â Â Â Â Â raw_spin_lock(&pc->lock);
>> Â Â Â Â Â Â Â Â for (i = 0; i < MAX_EXTRA_REGS; i++) {
>> Â Â Â Â Â Â Â Â Â Â Â Â era = &pc->regs[i];
>> Â Â Â Â Â Â Â Â Â Â Â Â if (era->ref > 0 &&
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â era->extra_config == hwc->extra_config &&
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â era->extra_reg == er->msr) {
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â era->ref--;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â hwc->extra_alloc = 0;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
>> Â Â Â Â Â Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â Â allref = 0;
>> Â Â Â Â Â Â Â Â for (i = 0; i < MAX_EXTRA_REGS; i++)
>> Â Â Â Â Â Â Â Â Â Â Â Â allref += pc->regs[i].ref;
>> Â Â Â Â Â Â Â Â if (allref == 0)
>> Â Â Â Â Â Â Â Â Â Â Â Â cpuc->percore_used = 0;
>> Â Â Â Â Â Â Â Â raw_spin_unlock(&pc->lock);
>> Â Â Â Â Â Â Â Â break;
>> Â Â Â Â }
>> }
>>
>> >
>> >> Â#define INTEL_EVENT_CONSTRAINT(c, n) \
>> >> Â Â Â EVENT_CONSTRAINT(c, n, ARCH_PERFMON_EVENTSEL_EVENT)
>> >> +#define INTEL_EVENT_CONSTRAINT2(c, n) Â Â Â Â\
>> >> + Â Â EVENT_CONSTRAINT(c, n, INTEL_ARCH_EVENT_MASK)
>> >
>> > That's a particularly bad name, how about something like
>> >
>> > INTEL_UEVENT_CONSTRAINT or somesuch.
>> >
>> >> @@ -702,7 +738,13 @@ static void intel_ds_init(void)
>> >> Â Â Â Â Â Â Â Â Â Â Â printk(KERN_CONT "PEBS fmt1%c, ", pebs_type);
>> >> Â Â Â Â Â Â Â Â Â Â Â x86_pmu.pebs_record_size = sizeof(struct pebs_record_nhm);
>> >> Â Â Â Â Â Â Â Â Â Â Â x86_pmu.drain_pebs = intel_pmu_drain_pebs_nhm;
>> >> - Â Â Â Â Â Â Â Â Â Â x86_pmu.pebs_constraints = intel_nehalem_pebs_events;
>> >> + Â Â Â Â Â Â Â Â Â Â switch (boot_cpu_data.x86_model) {
>> >> + Â Â Â Â Â Â Â Â Â Â case 42: /* SandyBridge */
>> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â x86_pmu.pebs_constraints = intel_snb_pebs_events;
>> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
>> >> + Â Â Â Â Â Â Â Â Â Â default:
>> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â x86_pmu.pebs_constraints = intel_nehalem_pebs_events;
>> >> + Â Â Â Â Â Â Â Â Â Â }
>> >> Â Â Â Â Â Â Â Â Â Â Â break;
>> >>
>> >> Â Â Â Â Â Â Â default:
>> >
>> > We already have this massive model switch right after this function,
>> > might as well move the pebs constraint assignment there.
>> >
>
>
>
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_