Messages in this thread | | | Date | Thu, 26 Mar 2015 12:40:14 -0700 | Subject | Re: [PATCH 1/1] perf/x86: filter branches for PEBS event | From | Stephane Eranian <> |
| |
On Thu, Mar 26, 2015 at 11:13 AM, <kan.liang@intel.com> wrote: > From: Kan Liang <kan.liang@intel.com> > > For supporting Intel LBR branches filtering, Intel LBR sharing logic > mechanism is introduced from commit b36817e88630 ("perf/x86: Add Intel > LBR sharing logic"). It modifies __intel_shared_reg_get_constraints to > config lbr_sel, which is finally used to set LBR_SELECT. > However, the intel_shared_regs_constraints is called after > intel_pebs_constraints. The PEBS event will return immediately after > intel_pebs_constraints. So it's impossible to filter branches for PEBS > event. > > This patch move intel_shared_regs_constraints for branch_reg ahead of > intel_pebs_constraints. > intel_shared_regs_constraints for branch_reg doesn't modify event->hw, > so it's safe to be called before intel_pebs_constraints. > intel_shared_regs_constraints for branch_reg also special case when it > returns &emptyconstraint. It put constraints for extra_reg. This patch > remove it. Because it will never get constraints for extra_reg if return > is &emptyconstraint. > > Signed-off-by: Kan Liang <kan.liang@intel.com> > --- > arch/x86/kernel/cpu/perf_event_intel.c | 34 ++++++++++++---------------------- > 1 file changed, 12 insertions(+), 22 deletions(-) > > diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c > index 9f1dd18..247780a 100644 > --- a/arch/x86/kernel/cpu/perf_event_intel.c > +++ b/arch/x86/kernel/cpu/perf_event_intel.c > @@ -1587,25 +1587,14 @@ __intel_shared_reg_put_constraints(struct cpu_hw_events *cpuc, > > static struct event_constraint * > intel_shared_regs_constraints(struct cpu_hw_events *cpuc, > - struct perf_event *event) > + struct perf_event *event, > + struct hw_perf_event_extra *reg) > { > - struct event_constraint *c = NULL, *d; > - struct hw_perf_event_extra *xreg, *breg; > + struct event_constraint *c = NULL; > + > + if (reg->idx != EXTRA_REG_NONE) > + c = __intel_shared_reg_get_constraints(cpuc, event, reg); > > - xreg = &event->hw.extra_reg; > - if (xreg->idx != EXTRA_REG_NONE) { > - c = __intel_shared_reg_get_constraints(cpuc, event, xreg); > - if (c == &emptyconstraint) > - return c; > - } > - breg = &event->hw.branch_reg; > - if (breg->idx != EXTRA_REG_NONE) { > - d = __intel_shared_reg_get_constraints(cpuc, event, breg); > - if (d == &emptyconstraint) { > - __intel_shared_reg_put_constraints(cpuc, xreg); > - c = d; > - } > - } > return c; > } > > @@ -1629,17 +1618,18 @@ x86_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event) > static struct event_constraint * > intel_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event) > { > - struct event_constraint *c; > + struct event_constraint *c, *d; > > c = intel_bts_constraints(event); > if (c) > return c; > > - c = intel_pebs_constraints(event); > - if (c) > - return c; > + c = intel_shared_regs_constraints(cpuc, event, &event->hw.branch_reg); > + d = intel_pebs_constraints(event); > + if (d || c) > + return (d) ? (d) : (c); > > - c = intel_shared_regs_constraints(cpuc, event); > + c = intel_shared_regs_constraints(cpuc, event, &event->hw.extra_reg); > if (c) > return c; > You are addressing one of the problems of this routine. But I think there is a more serious issue which is not addressed here. The intel_shared_regs_constraints() assumes that the associated event is necessarily unconstrained:
__intel_shared_reg_get_constraints() { struct event_constraint *c = &emptyconstraint; ... }
This is true for offcore_response, but for LBR this may not always be the case. I may want to use LBR on the L1D_PEND_MISS event and it would need to be on counter 2. But I believe that the current code could place it on counter 0 simply because you return if shared_reg_get_constraint() is successful, but it looks only at the LBR constraint not the event constraint. I think in the presence of LBR, you always need to call share_get_reg() and x86_get_event_constraint(). This is to ensure that both the shared constraint AND the event constraint are satisfied (and of course, in case one fails, the other needs to be released).
> -- > 1.8.3.2 >
| |