Re: [PATCH 1/1] perf/x86: filter branches for PEBS event

From: Stephane Eranian
Date: Thu Mar 26 2015 - 17:03:01 EST

On Thu, Mar 26, 2015 at 1:20 PM, Liang, Kan <kan.liang@xxxxxxxxx> wrote:
>
>
>> Subject: Re: [PATCH 1/1] perf/x86: filter branches for PEBS event
>>
>> On Thu, Mar 26, 2015 at 11:13 AM, <kan.liang@xxxxxxxxx> wrote:
>> > From: Kan Liang <kan.liang@xxxxxxxxx>
>> >
>> > 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@xxxxxxxxx>
>> > ---
>> > 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/