On Thu, Mar 26, 2015 at 1:20 PM, Liang, Kan <kan.li...@intel.com> wrote: > > >> Subject: Re: [PATCH 1/1] perf/x86: filter branches for PEBS event >> >> On Thu, Mar 26, 2015 at 11:13 AM, <kan.li...@intel.com> wrote: >> > From: Kan Liang <kan.li...@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.li...@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 didn’t change __intel_shared_reg_get_constraints and its input. > So using LBR on the L1D_PEND_MISS event, it would return NULL. > /* > * need to call x86_get_event_constraint() > * to check if associated event has constraints > */ > c = NULL; > Since it's not PEBS, intel_pebs_constraints will also return NULL. > So it will not return. It will continue to check extra_reg and finally > check if associated event has constraints. >>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). >> > The patch doesn't change the behavior for non-PEBS event. > I know that. I was commenting in general on the function. I think it still has the problem I mention. And it needs to be fixed.
> Thanks, > Kan >> >> > -- >> > 1.8.3.2 >> > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/