* [PATCH 1/1] perf/x86: filter branches for PEBS event
@ 2015-03-26 18:13 kan.liang
2015-03-26 19:40 ` Stephane Eranian
2015-03-27 12:17 ` Peter Zijlstra
0 siblings, 2 replies; 8+ messages in thread
From: kan.liang @ 2015-03-26 18:13 UTC (permalink / raw)
To: a.p.zijlstra; +Cc: eranian, andi, linux-kernel, Kan Liang
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;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] perf/x86: filter branches for PEBS event
2015-03-26 18:13 [PATCH 1/1] perf/x86: filter branches for PEBS event kan.liang
@ 2015-03-26 19:40 ` Stephane Eranian
2015-03-26 20:20 ` Liang, Kan
2015-03-27 12:10 ` Peter Zijlstra
2015-03-27 12:17 ` Peter Zijlstra
1 sibling, 2 replies; 8+ messages in thread
From: Stephane Eranian @ 2015-03-26 19:40 UTC (permalink / raw)
To: Liang, Kan; +Cc: Peter Zijlstra, Andi Kleen, LKML
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 1/1] perf/x86: filter branches for PEBS event
2015-03-26 19:40 ` Stephane Eranian
@ 2015-03-26 20:20 ` Liang, Kan
2015-03-26 21:02 ` Stephane Eranian
2015-03-27 12:10 ` Peter Zijlstra
1 sibling, 1 reply; 8+ messages in thread
From: Liang, Kan @ 2015-03-26 20:20 UTC (permalink / raw)
To: Stephane Eranian; +Cc: Peter Zijlstra, Andi Kleen, LKML
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5602 bytes --]
> Subject: Re: [PATCH 1/1] perf/x86: filter branches for PEBS event
>
> 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 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Ç+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] perf/x86: filter branches for PEBS event
2015-03-26 20:20 ` Liang, Kan
@ 2015-03-26 21:02 ` Stephane Eranian
0 siblings, 0 replies; 8+ messages in thread
From: Stephane Eranian @ 2015-03-26 21:02 UTC (permalink / raw)
To: Liang, Kan; +Cc: Peter Zijlstra, Andi Kleen, LKML
On Thu, Mar 26, 2015 at 1:20 PM, Liang, Kan <kan.liang@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.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 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
>> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] perf/x86: filter branches for PEBS event
2015-03-26 19:40 ` Stephane Eranian
2015-03-26 20:20 ` Liang, Kan
@ 2015-03-27 12:10 ` Peter Zijlstra
2015-03-27 15:28 ` Stephane Eranian
1 sibling, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2015-03-27 12:10 UTC (permalink / raw)
To: Stephane Eranian; +Cc: Liang, Kan, Andi Kleen, LKML
On Thu, Mar 26, 2015 at 12:40:14PM -0700, Stephane Eranian wrote:
> 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;
> ...
> }
emptyconstraint != unconstrained.
Note how that function only returns emptyconstraint if its rejecting the
event, otherwise it returns NULL such that we continue calling
x86_get_event_constraint().
> 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().
Which, I think it does.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] perf/x86: filter branches for PEBS event
2015-03-26 18:13 [PATCH 1/1] perf/x86: filter branches for PEBS event kan.liang
2015-03-26 19:40 ` Stephane Eranian
@ 2015-03-27 12:17 ` Peter Zijlstra
2015-03-27 13:09 ` Liang, Kan
1 sibling, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2015-03-27 12:17 UTC (permalink / raw)
To: kan.liang; +Cc: eranian, andi, linux-kernel
On Thu, Mar 26, 2015 at 02:13:23PM -0400, kan.liang@intel.com wrote:
> This patch move intel_shared_regs_constraints for branch_reg ahead of
> intel_pebs_constraints.
Why not all shared regs?
> 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.
-ENOPARSE.
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 1/1] perf/x86: filter branches for PEBS event
2015-03-27 12:17 ` Peter Zijlstra
@ 2015-03-27 13:09 ` Liang, Kan
0 siblings, 0 replies; 8+ messages in thread
From: Liang, Kan @ 2015-03-27 13:09 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: eranian, andi, linux-kernel
> On Thu, Mar 26, 2015 at 02:13:23PM -0400, kan.liang@intel.com wrote:
> > This patch move intel_shared_regs_constraints for branch_reg ahead of
> > intel_pebs_constraints.
>
> Why not all shared regs?
Yes, all shared regs can also be moved ahead.
The patch is named for modifying the branch filter. I didn't want to change
other codes. So I keep them unchanged.
I will do it in next version.
>
> > 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.
>
> -ENOPARSE.
Will remove the description, since it doesn't need in next version.
Thanks,
Kan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] perf/x86: filter branches for PEBS event
2015-03-27 12:10 ` Peter Zijlstra
@ 2015-03-27 15:28 ` Stephane Eranian
0 siblings, 0 replies; 8+ messages in thread
From: Stephane Eranian @ 2015-03-27 15:28 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Liang, Kan, Andi Kleen, LKML
On Fri, Mar 27, 2015 at 5:10 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Mar 26, 2015 at 12:40:14PM -0700, Stephane Eranian wrote:
>> 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;
>> ...
>> }
>
> emptyconstraint != unconstrained.
>
> Note how that function only returns emptyconstraint if its rejecting the
> event, otherwise it returns NULL such that we continue calling
> x86_get_event_constraint().
>
Ah, yes, I had forgotten about that. Then everything is fine.
>> 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().
>
> Which, I think it does.
Indeed.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-03-27 15:28 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-26 18:13 [PATCH 1/1] perf/x86: filter branches for PEBS event kan.liang
2015-03-26 19:40 ` Stephane Eranian
2015-03-26 20:20 ` Liang, Kan
2015-03-26 21:02 ` Stephane Eranian
2015-03-27 12:10 ` Peter Zijlstra
2015-03-27 15:28 ` Stephane Eranian
2015-03-27 12:17 ` Peter Zijlstra
2015-03-27 13:09 ` Liang, Kan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.