Messages in this thread |  | | | Date | Wed, 2 Mar 2011 08:48:19 +0100 | | Subject | Re: [PATCH v2 1/4] perf-events: Add support for supplementary event registers | | From | Stephane Eranian <> |
| |
On Wed, Mar 2, 2011 at 1:37 AM, Lin Ming <ming.m.lin@intel.com> wrote: > On Wed, 2011-03-02 at 01:09 +0800, Stephane Eranian wrote: >> Lin, >> >> See comments below: >> >> On Tue, Mar 1, 2011 at 5:47 PM, Lin Ming <ming.m.lin@intel.com> wrote: >> > From: Andi Kleen <ak@linux.intel.com> >> > 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 || 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; >> >> Not sure I understand this c = NULL, given you hardcoded return NULL >> outside of the >> loop. I forgot to mention this earlier. > > Right, that is redundant. > Remove it. > >> >> > + 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; >> > +} >> > + >> > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h >> > index 8ceb5a6..48d966a 100644 >> > --- a/include/linux/perf_event.h >> > +++ b/include/linux/perf_event.h >> > @@ -225,8 +225,14 @@ struct perf_event_attr { >> > }; >> > >> > __u32 bp_type; >> > - __u64 bp_addr; >> > - __u64 bp_len; >> > + union { >> > + __u64 bp_addr; >> > + __u64 config1; /* extension of config0 */ >> > + }; >> > + union { >> > + __u64 bp_len; >> > + __u64 config2; /* extension of config1 */ >> > + }; >> > }; >> > >> I don't see where those config0 or config1 are coming from. > > config0 means perf_event_attr::config. > yes.
> Peter suggested to extend the attr config space. > http://marc.info/?l=linux-kernel&m=129838808907525&w=2 > Ok, I get it, it is cascading. that's fine.
|  |