> 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.
Thanks,
Kan
>
> > --
> > 1.8.3.2
> >
N�����r��y����b�X��ǧv�^�){.n�+����{����zX����ܨ}���Ơz�&j:+v�������zZ+��+zf���h���~����i���z� �w���?�����&�)ߢf��^jǫy�m��@A�a���
0��h���i
