This short series of patches improves the event contraint
tables for Intel SNB, IVB and HSW processors.
1/ removes unnecessary constraints on the Load Latency event
The constraint to counter 3 is not needed. The events works
well on any PEBS-capable counter. The artificial constraint on
counter 3, was used to simplify event scheduling because the
event uses an extra MSR which is ahred by all the counters.
But perf_events can managed shared regs without artificial
counter constraints.
2/ remove unecessary constraint on precise store on HSW
On SNB,IVB, the precise store event has to be on counter 3.
On HSW, precise store is not implemented the same way and
can use any counter. Thus we lift the constraint on all
precise store events on HSW.
The advantage of this series is that it allows capturing load
latency and precise store events at the same time without
multiplexing.
Stephane Eranian (2):
perf/x86: update Haswell PEBS event constraints
perf/x86: fix constraint for load latency and precise store event
arch/x86/kernel/cpu/perf_event_intel.c | 2 --
arch/x86/kernel/cpu/perf_event_intel_ds.c | 22 ++++++++--------------
2 files changed, 8 insertions(+), 16 deletions(-)
--
1.7.9.5
The load latency does not have to be constrained to counter 3
on any of SNB, IVB, HSW. It operates fine on any PEBS-capable
counter.
The precise store event for SNB, IVB needs to be on counter 3.
But on Haswell, precise store is implemented differently and
the constraint is not needed anymore, so we remove it.
The artificial constraint on counter 3 was used to ease
scheduling because the load latency events rely on an
extra MSR which is shared for all the counters. But
perf_events has an infrastructure to handle shared_regs
and does not need to constrain the load latency event to
a single counter. It was already using that infrastructure
with the constraint on counter 3. By eliminating the constraint
on load latency, it becomes possible to measure loads and stores
precisely without multiplexing.
Signed-off-by: Stephane Eranian <[email protected]>
---
arch/x86/kernel/cpu/perf_event_intel.c | 2 --
arch/x86/kernel/cpu/perf_event_intel_ds.c | 6 +++---
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index adb02aa..06c6616 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -110,7 +110,6 @@ static struct event_constraint intel_snb_event_constraints[] __read_mostly =
INTEL_UEVENT_CONSTRAINT(0x06a3, 0x4), /* CYCLE_ACTIVITY.STALLS_L1D_PENDING */
INTEL_EVENT_CONSTRAINT(0x48, 0x4), /* L1D_PEND_MISS.PENDING */
INTEL_UEVENT_CONSTRAINT(0x01c0, 0x2), /* INST_RETIRED.PREC_DIST */
- INTEL_EVENT_CONSTRAINT(0xcd, 0x8), /* MEM_TRANS_RETIRED.LOAD_LATENCY */
INTEL_UEVENT_CONSTRAINT(0x04a3, 0xf), /* CYCLE_ACTIVITY.CYCLES_NO_DISPATCH */
INTEL_UEVENT_CONSTRAINT(0x02a3, 0x4), /* CYCLE_ACTIVITY.CYCLES_L1D_PENDING */
EVENT_CONSTRAINT_END
@@ -210,7 +209,6 @@ static struct event_constraint intel_hsw_event_constraints[] = {
FIXED_EVENT_CONSTRAINT(0x0300, 2), /* CPU_CLK_UNHALTED.REF */
INTEL_EVENT_CONSTRAINT(0x48, 0x4), /* L1D_PEND_MISS.* */
INTEL_UEVENT_CONSTRAINT(0x01c0, 0x2), /* INST_RETIRED.PREC_DIST */
- INTEL_EVENT_CONSTRAINT(0xcd, 0x8), /* MEM_TRANS_RETIRED.LOAD_LATENCY */
/* CYCLE_ACTIVITY.CYCLES_L1D_PENDING */
INTEL_EVENT_CONSTRAINT(0x08a3, 0x4),
/* CYCLE_ACTIVITY.STALLS_L1D_PENDING */
diff --git a/arch/x86/kernel/cpu/perf_event_intel_ds.c b/arch/x86/kernel/cpu/perf_event_intel_ds.c
index 46d4def..0685905 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_ds.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c
@@ -628,7 +628,7 @@ struct event_constraint intel_snb_pebs_event_constraints[] = {
INTEL_UEVENT_CONSTRAINT(0x02c2, 0xf), /* UOPS_RETIRED.RETIRE_SLOTS */
INTEL_EVENT_CONSTRAINT(0xc4, 0xf), /* BR_INST_RETIRED.* */
INTEL_EVENT_CONSTRAINT(0xc5, 0xf), /* BR_MISP_RETIRED.* */
- INTEL_PLD_CONSTRAINT(0x01cd, 0x8), /* MEM_TRANS_RETIRED.LAT_ABOVE_THR */
+ INTEL_PLD_CONSTRAINT(0x01cd, 0xf), /* MEM_TRANS_RETIRED.LAT_ABOVE_THR */
INTEL_PST_CONSTRAINT(0x02cd, 0x8), /* MEM_TRANS_RETIRED.PRECISE_STORES */
INTEL_EVENT_CONSTRAINT(0xd0, 0xf), /* MEM_UOP_RETIRED.* */
INTEL_EVENT_CONSTRAINT(0xd1, 0xf), /* MEM_LOAD_UOPS_RETIRED.* */
@@ -644,7 +644,7 @@ struct event_constraint intel_ivb_pebs_event_constraints[] = {
INTEL_UEVENT_CONSTRAINT(0x02c2, 0xf), /* UOPS_RETIRED.RETIRE_SLOTS */
INTEL_EVENT_CONSTRAINT(0xc4, 0xf), /* BR_INST_RETIRED.* */
INTEL_EVENT_CONSTRAINT(0xc5, 0xf), /* BR_MISP_RETIRED.* */
- INTEL_PLD_CONSTRAINT(0x01cd, 0x8), /* MEM_TRANS_RETIRED.LAT_ABOVE_THR */
+ INTEL_PLD_CONSTRAINT(0x01cd, 0xf), /* MEM_TRANS_RETIRED.LAT_ABOVE_THR */
INTEL_PST_CONSTRAINT(0x02cd, 0x8), /* MEM_TRANS_RETIRED.PRECISE_STORES */
INTEL_EVENT_CONSTRAINT(0xd0, 0xf), /* MEM_UOP_RETIRED.* */
INTEL_EVENT_CONSTRAINT(0xd1, 0xf), /* MEM_LOAD_UOPS_RETIRED.* */
@@ -661,7 +661,7 @@ struct event_constraint intel_hsw_pebs_event_constraints[] = {
INTEL_UEVENT_CONSTRAINT(0x01c5, 0xf), /* BR_MISP_RETIRED.CONDITIONAL */
INTEL_UEVENT_CONSTRAINT(0x04c5, 0xf), /* BR_MISP_RETIRED.ALL_BRANCHES */
INTEL_UEVENT_CONSTRAINT(0x20c5, 0xf), /* BR_MISP_RETIRED.NEAR_TAKEN */
- INTEL_PLD_CONSTRAINT(0x01cd, 0x8), /* MEM_TRANS_RETIRED.* */
+ INTEL_PLD_CONSTRAINT(0x01cd, 0xf), /* MEM_TRANS_RETIRED.* */
/* MEM_UOPS_RETIRED.STLB_MISS_LOADS */
INTEL_UEVENT_CONSTRAINT(0x11d0, 0xf),
/* MEM_UOPS_RETIRED.STLB_MISS_STORES */
--
1.7.9.5
The following events support PEBS for all umasks,
thus use INTEL_EVENT_CONSTRAINT() instead of
INTEL_UEVENT_CONSTRAINT():
0xd1 MEM_LOAD_UOPS_RETIRED
0xd2 MEM_LOAD_UOPS_LLC_HIT_RETIRED
0xd3 MEM_LOAD_UOPS_LLC_MISS_RETIRED
For event 0xd0 (MEM_UOPS_RETIRED), the same is true, except
we need to distinguish precise store (umask 0x82) from load
latency events, thus we keep the breakdown per umask. But all
umasks do support PEBS.
Signed-off-by: Stephane Eranian <[email protected]>
---
arch/x86/kernel/cpu/perf_event_intel_ds.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/arch/x86/kernel/cpu/perf_event_intel_ds.c b/arch/x86/kernel/cpu/perf_event_intel_ds.c
index 980970c..46d4def 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_ds.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c
@@ -672,17 +672,11 @@ struct event_constraint intel_hsw_pebs_event_constraints[] = {
INTEL_UEVENT_CONSTRAINT(0x42d0, 0xf),
INTEL_UEVENT_CONSTRAINT(0x81d0, 0xf), /* MEM_UOPS_RETIRED.ALL_LOADS */
INTEL_PST_HSW_CONSTRAINT(0x82d0, 0xf), /* MEM_UOPS_RETIRED.ALL_STORES */
- INTEL_UEVENT_CONSTRAINT(0x01d1, 0xf), /* MEM_LOAD_UOPS_RETIRED.L1_HIT */
- INTEL_UEVENT_CONSTRAINT(0x02d1, 0xf), /* MEM_LOAD_UOPS_RETIRED.L2_HIT */
- INTEL_UEVENT_CONSTRAINT(0x04d1, 0xf), /* MEM_LOAD_UOPS_RETIRED.L3_HIT */
- /* MEM_LOAD_UOPS_RETIRED.HIT_LFB */
- INTEL_UEVENT_CONSTRAINT(0x40d1, 0xf),
- /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.XSNP_MISS */
- INTEL_UEVENT_CONSTRAINT(0x01d2, 0xf),
- /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.XSNP_HIT */
- INTEL_UEVENT_CONSTRAINT(0x02d2, 0xf),
- /* MEM_LOAD_UOPS_LLC_MISS_RETIRED.LOCAL_DRAM */
- INTEL_UEVENT_CONSTRAINT(0x01d3, 0xf),
+ INTEL_EVENT_CONSTRAINT(0xd1, 0xf), /* MEM_LOAD_UOPS_RETIRED.* */
+ /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.* */
+ INTEL_EVENT_CONSTRAINT(0xd2, 0xf),
+ /* MEM_LOAD_UOPS_LLC_MISS_RETIRED.* */
+ INTEL_EVENT_CONSTRAINT(0xd3, 0xf),
INTEL_UEVENT_CONSTRAINT(0x04c8, 0xf), /* HLE_RETIRED.Abort */
INTEL_UEVENT_CONSTRAINT(0x04c9, 0xf), /* RTM_RETIRED.Abort */
--
1.7.9.5
On Thu, Jun 19, 2014 at 05:58:28PM +0200, Stephane Eranian wrote:
> The following events support PEBS for all umasks,
> thus use INTEL_EVENT_CONSTRAINT() instead of
> INTEL_UEVENT_CONSTRAINT():
>
> 0xd1 MEM_LOAD_UOPS_RETIRED
> 0xd2 MEM_LOAD_UOPS_LLC_HIT_RETIRED
> 0xd3 MEM_LOAD_UOPS_LLC_MISS_RETIRED
>
> For event 0xd0 (MEM_UOPS_RETIRED), the same is true, except
> we need to distinguish precise store (umask 0x82) from load
> latency events, thus we keep the breakdown per umask. But all
> umasks do support PEBS.
I sent a similar patch some time ago
http://lkml.iu.edu/hypermail/linux/kernel/1404.2/01509.html
However these days I'm actually thinking of just getting
rid of the detailed table except for PREC_DIST. All the PEBS
controls should be noops if the event does not support PEBS
-Andi
On Thu, Jun 19, 2014 at 8:00 PM, Andi Kleen <[email protected]> wrote:
>
> On Thu, Jun 19, 2014 at 05:58:28PM +0200, Stephane Eranian wrote:
> > The following events support PEBS for all umasks,
> > thus use INTEL_EVENT_CONSTRAINT() instead of
> > INTEL_UEVENT_CONSTRAINT():
> >
> > 0xd1 MEM_LOAD_UOPS_RETIRED
> > 0xd2 MEM_LOAD_UOPS_LLC_HIT_RETIRED
> > 0xd3 MEM_LOAD_UOPS_LLC_MISS_RETIRED
> >
> > For event 0xd0 (MEM_UOPS_RETIRED), the same is true, except
> > we need to distinguish precise store (umask 0x82) from load
> > latency events, thus we keep the breakdown per umask. But all
> > umasks do support PEBS.
>
> I sent a similar patch some time ago
>
> http://lkml.iu.edu/hypermail/linux/kernel/1404.2/01509.html
>
> However these days I'm actually thinking of just getting
> rid of the detailed table except for PREC_DIST. All the PEBS
> controls should be noops if the event does not support PEBS
>
I don't quite understand that.
You need to know which events support PEBS. You need a table
for that. then I think you could drop the counter mask because
with HT on, no mask is needed. When HT is off you need a mask,
but it can be hardcoded for all events, well at least on HSW.
> I don't quite understand that.
> You need to know which events support PEBS. You need a table
We're talking about the kernel allowing things here.
Yes the user still needs to know what supports PEBS, but
that doesn't concern the kernel.
You can just allow it for all, it's a nop if the event doesn't
support it. And also the fields like DataLA are simply 0 when
not supported.
The only thing you need is a rule to limit to 4 counters.
Then only cases that are special (PREC_DIST, extra registers)
would need to be handled explicitely.
-Andi
On Thu, Jun 19, 2014 at 10:18 PM, Andi Kleen <[email protected]> wrote:
>> I don't quite understand that.
>> You need to know which events support PEBS. You need a table
>
> We're talking about the kernel allowing things here.
> Yes the user still needs to know what supports PEBS, but
> that doesn't concern the kernel.
>
Just need to make sure you don't return bogus information.
> You can just allow it for all, it's a nop if the event doesn't
> support it. And also the fields like DataLA are simply 0 when
> not supported.
>
Let's take a example. If I do resource_stalls:pp, the kernel
will let it go through and clear the PMI bit on the config as
is required for PEBS mode. The counter will count normally
and never fire an interrupt, even when it overflows. It would
never execute the PMI handler and thus never look at the
PEBS content. You'd never get any samples.
> The only thing you need is a rule to limit to 4 counters.
>
> Then only cases that are special (PREC_DIST, extra registers)
> would need to be handled explicitely.
>
extra registers do no impose counter constraints. That's the approach
used by Intel tools to simplify scheduling. As I said in my patch, in
Linux we do this differently.
So yes, you'd need this for PREC_DIST and precise store on SNB, IVB.
On Thu, Jun 19, 2014 at 10:31:29PM +0200, Stephane Eranian wrote:
> On Thu, Jun 19, 2014 at 10:18 PM, Andi Kleen <[email protected]> wrote:
> >> I don't quite understand that.
> >> You need to know which events support PEBS. You need a table
> >
> > We're talking about the kernel allowing things here.
> > Yes the user still needs to know what supports PEBS, but
> > that doesn't concern the kernel.
> >
> Just need to make sure you don't return bogus information.
GIGO. We only need to prevent security issues.
> > You can just allow it for all, it's a nop if the event doesn't
> > support it. And also the fields like DataLA are simply 0 when
> > not supported.
> >
>
> Let's take a example. If I do resource_stalls:pp, the kernel
> will let it go through and clear the PMI bit on the config as
> is required for PEBS mode. The counter will count normally
> and never fire an interrupt, even when it overflows. It would
> never execute the PMI handler and thus never look at the
> PEBS content. You'd never get any samples.
Yes if the user specifies a bogus raw event it will not count.
That's fine. The important part is just that nothing ever crashes.
-Andi
On Thu, Jun 19, 2014 at 10:40 PM, Andi Kleen <[email protected]> wrote:
> On Thu, Jun 19, 2014 at 10:31:29PM +0200, Stephane Eranian wrote:
>> On Thu, Jun 19, 2014 at 10:18 PM, Andi Kleen <[email protected]> wrote:
>> >> I don't quite understand that.
>> >> You need to know which events support PEBS. You need a table
>> >
>> > We're talking about the kernel allowing things here.
>> > Yes the user still needs to know what supports PEBS, but
>> > that doesn't concern the kernel.
>> >
>> Just need to make sure you don't return bogus information.
>
> GIGO. We only need to prevent security issues.
>
>> > You can just allow it for all, it's a nop if the event doesn't
>> > support it. And also the fields like DataLA are simply 0 when
>> > not supported.
>> >
>>
>> Let's take a example. If I do resource_stalls:pp, the kernel
>> will let it go through and clear the PMI bit on the config as
>> is required for PEBS mode. The counter will count normally
>> and never fire an interrupt, even when it overflows. It would
>> never execute the PMI handler and thus never look at the
>> PEBS content. You'd never get any samples.
>
> Yes if the user specifies a bogus raw event it will not count.
> That's fine. The important part is just that nothing ever crashes.
>
That would certainly avoid the problem of missing events in pebs table.
I had a problem with that just today. It also speed up scheduling
as well by avoid the table lookups.
Note that I will soon post a patch to speed up scheduling for all x86
processors.
On Thu, Jun 19, 2014 at 05:58:29PM +0200, Stephane Eranian wrote:
> The load latency does not have to be constrained to counter 3
> on any of SNB, IVB, HSW. It operates fine on any PEBS-capable
> counter.
>
> The precise store event for SNB, IVB needs to be on counter 3.
> But on Haswell, precise store is implemented differently and
> the constraint is not needed anymore, so we remove it.
Looks good to me.
-Andi
On Thu, Jun 19, 2014 at 10:45 PM, Stephane Eranian <[email protected]> wrote:
> On Thu, Jun 19, 2014 at 10:40 PM, Andi Kleen <[email protected]> wrote:
>> On Thu, Jun 19, 2014 at 10:31:29PM +0200, Stephane Eranian wrote:
>>> On Thu, Jun 19, 2014 at 10:18 PM, Andi Kleen <[email protected]> wrote:
>>> >> I don't quite understand that.
>>> >> You need to know which events support PEBS. You need a table
>>> >
>>> > We're talking about the kernel allowing things here.
>>> > Yes the user still needs to know what supports PEBS, but
>>> > that doesn't concern the kernel.
>>> >
>>> Just need to make sure you don't return bogus information.
>>
>> GIGO. We only need to prevent security issues.
>>
>>> > You can just allow it for all, it's a nop if the event doesn't
>>> > support it. And also the fields like DataLA are simply 0 when
>>> > not supported.
>>> >
>>>
>>> Let's take a example. If I do resource_stalls:pp, the kernel
>>> will let it go through and clear the PMI bit on the config as
>>> is required for PEBS mode. The counter will count normally
>>> and never fire an interrupt, even when it overflows. It would
>>> never execute the PMI handler and thus never look at the
>>> PEBS content. You'd never get any samples.
>>
>> Yes if the user specifies a bogus raw event it will not count.
>> That's fine. The important part is just that nothing ever crashes.
>>
> That would certainly avoid the problem of missing events in pebs table.
> I had a problem with that just today. It also speed up scheduling
> as well by avoid the table lookups.
>
I can take of writing the patch to do this, if you want.
> Note that I will soon post a patch to speed up scheduling for all x86
> processors.
I'll put that one in too.
On Thu, Jun 19, 2014 at 05:58:28PM +0200, Stephane Eranian wrote:
> + INTEL_EVENT_CONSTRAINT(0xd1, 0xf), /* MEM_LOAD_UOPS_RETIRED.* */
> + /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.* */
> + INTEL_EVENT_CONSTRAINT(0xd2, 0xf),
> + /* MEM_LOAD_UOPS_LLC_MISS_RETIRED.* */
> + INTEL_EVENT_CONSTRAINT(0xd3, 0xf),
> INTEL_UEVENT_CONSTRAINT(0x04c8, 0xf), /* HLE_RETIRED.Abort */
> INTEL_UEVENT_CONSTRAINT(0x04c9, 0xf), /* RTM_RETIRED.Abort */
Please keep the event and comment on the same line, screw 80 chars, this
interleaves stuff is impossible to read.
On Thu, Jun 19, 2014 at 11:00:28AM -0700, Andi Kleen wrote:
> However these days I'm actually thinking of just getting
> rid of the detailed table except for PREC_DIST. All the PEBS
> controls should be noops if the event does not support PEBS
I had something like the below stuck on the 'look more at later' list
and later never really ever happened.
---
arch/x86/kernel/cpu/perf_event.c | 3 +++
arch/x86/kernel/cpu/perf_event.h | 1 +
arch/x86/kernel/cpu/perf_event_intel.c | 19 ++++++++++++++--
arch/x86/kernel/cpu/perf_event_intel_ds.c | 38 +++++++++++++++++--------------
4 files changed, 42 insertions(+), 19 deletions(-)
diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index ae407f7226c8..f42405c9868b 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1541,6 +1541,7 @@ static int __init init_hw_perf_events(void)
pr_cont("%s PMU driver.\n", x86_pmu.name);
x86_pmu.attr_rdpmc = 1; /* enable userspace RDPMC usage by default */
+ x86_pmu.attr_strict_pebs = 1;
for (quirk = x86_pmu.quirks; quirk; quirk = quirk->next)
quirk->func();
@@ -1855,9 +1856,11 @@ static ssize_t set_attr_rdpmc(struct device *cdev,
}
static DEVICE_ATTR(rdpmc, S_IRUSR | S_IWUSR, get_attr_rdpmc, set_attr_rdpmc);
+static DEVICE_BOOL_ATTR(strict_pebs, S_IRUSR | S_IWUSR, x86_pmu.attr_strict_pebs);
static struct attribute *x86_pmu_attrs[] = {
&dev_attr_rdpmc.attr,
+ &dev_attr_strict_pebs.attr.attr,
NULL,
};
diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
index 3b2f9bdd974b..a11eeab9b611 100644
--- a/arch/x86/kernel/cpu/perf_event.h
+++ b/arch/x86/kernel/cpu/perf_event.h
@@ -413,6 +413,7 @@ struct x86_pmu {
*/
int attr_rdpmc_broken;
int attr_rdpmc;
+ bool attr_strict_pebs;
struct attribute **format_attrs;
struct attribute **event_attrs;
diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index aa333d966886..6e68f3dc9a30 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -1729,6 +1729,12 @@ static void intel_pebs_aliases_snb(struct perf_event *event)
}
}
+#define ARCH_PERFMON_STRICT_PEBS \
+ (ARCH_PERFMON_EVENTSEL_ANY | \
+ ARCH_PERFMON_EVENTSEL_CMASK | \
+ ARCH_PERFMON_EVENTSEL_EDGE | \
+ ARCH_PERFMON_EVENTSEL_INV)
+
static int intel_pmu_hw_config(struct perf_event *event)
{
int ret = x86_pmu_hw_config(event);
@@ -1736,8 +1742,17 @@ static int intel_pmu_hw_config(struct perf_event *event)
if (ret)
return ret;
- if (event->attr.precise_ip && x86_pmu.pebs_aliases)
- x86_pmu.pebs_aliases(event);
+ if (event->attr.precise_ip) {
+ if ((event->attr.config & INTEL_ARCH_EVENT_MASK) == 0x0000)
+ return -EINVAL;
+
+ if ((event->attr.config & ARCH_PERFMON_STRICT_PEBS) &&
+ (x86_pmu.attr_strict_pebs || !capable(CAP_SYS_ADMIN)))
+ return -EINVAL;
+
+ if (x86_pmu.pebs_aliases)
+ x86_pmu.pebs_aliases(event);
+ }
if (intel_pmu_needs_lbr_smpl(event)) {
ret = intel_pmu_setup_lbr_filter(event);
diff --git a/arch/x86/kernel/cpu/perf_event_intel_ds.c b/arch/x86/kernel/cpu/perf_event_intel_ds.c
index ae96cfa5eddd..36b1f2afa61c 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_ds.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c
@@ -540,6 +540,7 @@ struct event_constraint intel_core2_pebs_event_constraints[] = {
INTEL_UEVENT_CONSTRAINT(0x00c5, 0x1), /* BR_INST_RETIRED.MISPRED */
INTEL_UEVENT_CONSTRAINT(0x1fc7, 0x1), /* SIMD_INST_RETURED.ANY */
INTEL_EVENT_CONSTRAINT(0xcb, 0x1), /* MEM_LOAD_RETIRED.* */
+ INTEL_UEVENT_CONSTRAINT(0x0000, 0x1), /* generic PEBS mask */
EVENT_CONSTRAINT_END
};
@@ -547,6 +548,7 @@ struct event_constraint intel_atom_pebs_event_constraints[] = {
INTEL_UEVENT_CONSTRAINT(0x00c0, 0x1), /* INST_RETIRED.ANY */
INTEL_UEVENT_CONSTRAINT(0x00c5, 0x1), /* MISPREDICTED_BRANCH_RETIRED */
INTEL_EVENT_CONSTRAINT(0xcb, 0x1), /* MEM_LOAD_RETIRED.* */
+ INTEL_UEVENT_CONSTRAINT(0x0000, 0x1), /* generic PEBS mask */
EVENT_CONSTRAINT_END
};
@@ -573,6 +575,7 @@ struct event_constraint intel_slm_pebs_event_constraints[] = {
INTEL_UEVENT_CONSTRAINT(0xf7c5, 0x1), /* BR_INST_MISP_RETIRED.RETURN_PS */
INTEL_UEVENT_CONSTRAINT(0xfbc5, 0x1), /* BR_INST_MISP_RETIRED.IND_CALL_PS */
INTEL_UEVENT_CONSTRAINT(0xfec5, 0x1), /* BR_INST_MISP_RETIRED.TAKEN_JCC_PS */
+ INTEL_UEVENT_CONSTRAINT(0x0000, 0x1), /* generic PEBS mask */
EVENT_CONSTRAINT_END
};
@@ -588,6 +591,7 @@ struct event_constraint intel_nehalem_pebs_event_constraints[] = {
INTEL_UEVENT_CONSTRAINT(0x20c8, 0xf), /* ITLB_MISS_RETIRED */
INTEL_EVENT_CONSTRAINT(0xcb, 0xf), /* MEM_LOAD_RETIRED.* */
INTEL_EVENT_CONSTRAINT(0xf7, 0xf), /* FP_ASSIST.* */
+ INTEL_UEVENT_CONSTRAINT(0x0000, 0xf), /* generic PEBS mask */
EVENT_CONSTRAINT_END
};
@@ -603,6 +607,7 @@ struct event_constraint intel_westmere_pebs_event_constraints[] = {
INTEL_UEVENT_CONSTRAINT(0x20c8, 0xf), /* ITLB_MISS_RETIRED */
INTEL_EVENT_CONSTRAINT(0xcb, 0xf), /* MEM_LOAD_RETIRED.* */
INTEL_EVENT_CONSTRAINT(0xf7, 0xf), /* FP_ASSIST.* */
+ INTEL_UEVENT_CONSTRAINT(0x0000, 0xf), /* generic PEBS mask */
EVENT_CONSTRAINT_END
};
@@ -619,6 +624,7 @@ struct event_constraint intel_snb_pebs_event_constraints[] = {
INTEL_EVENT_CONSTRAINT(0xd2, 0xf), /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.* */
INTEL_EVENT_CONSTRAINT(0xd3, 0xf), /* MEM_LOAD_UOPS_LLC_MISS_RETIRED.* */
INTEL_UEVENT_CONSTRAINT(0x02d4, 0xf), /* MEM_LOAD_UOPS_MISC_RETIRED.LLC_MISS */
+ INTEL_UEVENT_CONSTRAINT(0x0000, 0xf), /* generic PEBS mask */
EVENT_CONSTRAINT_END
};
@@ -634,42 +640,36 @@ struct event_constraint intel_ivb_pebs_event_constraints[] = {
INTEL_EVENT_CONSTRAINT(0xd1, 0xf), /* MEM_LOAD_UOPS_RETIRED.* */
INTEL_EVENT_CONSTRAINT(0xd2, 0xf), /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.* */
INTEL_EVENT_CONSTRAINT(0xd3, 0xf), /* MEM_LOAD_UOPS_LLC_MISS_RETIRED.* */
+ INTEL_UEVENT_CONSTRAINT(0x0000, 0xf), /* generic PEBS mask */
EVENT_CONSTRAINT_END
};
struct event_constraint intel_hsw_pebs_event_constraints[] = {
INTEL_UEVENT_CONSTRAINT(0x01c0, 0x2), /* INST_RETIRED.PRECDIST */
- INTEL_PST_HSW_CONSTRAINT(0x01c2, 0xf), /* UOPS_RETIRED.ALL */
+ INTEL_PST_HSW_CONSTRAINT(0x01c2, 0xf),/* UOPS_RETIRED.ALL */
INTEL_UEVENT_CONSTRAINT(0x02c2, 0xf), /* UOPS_RETIRED.RETIRE_SLOTS */
INTEL_EVENT_CONSTRAINT(0xc4, 0xf), /* BR_INST_RETIRED.* */
INTEL_UEVENT_CONSTRAINT(0x01c5, 0xf), /* BR_MISP_RETIRED.CONDITIONAL */
INTEL_UEVENT_CONSTRAINT(0x04c5, 0xf), /* BR_MISP_RETIRED.ALL_BRANCHES */
INTEL_UEVENT_CONSTRAINT(0x20c5, 0xf), /* BR_MISP_RETIRED.NEAR_TAKEN */
INTEL_PLD_CONSTRAINT(0x01cd, 0x8), /* MEM_TRANS_RETIRED.* */
- /* MEM_UOPS_RETIRED.STLB_MISS_LOADS */
- INTEL_UEVENT_CONSTRAINT(0x11d0, 0xf),
- /* MEM_UOPS_RETIRED.STLB_MISS_STORES */
- INTEL_UEVENT_CONSTRAINT(0x12d0, 0xf),
+ INTEL_UEVENT_CONSTRAINT(0x11d0, 0xf), /* MEM_UOPS_RETIRED.STLB_MISS_LOADS */
+ INTEL_UEVENT_CONSTRAINT(0x12d0, 0xf), /* MEM_UOPS_RETIRED.STLB_MISS_STORES */
INTEL_UEVENT_CONSTRAINT(0x21d0, 0xf), /* MEM_UOPS_RETIRED.LOCK_LOADS */
INTEL_UEVENT_CONSTRAINT(0x41d0, 0xf), /* MEM_UOPS_RETIRED.SPLIT_LOADS */
- /* MEM_UOPS_RETIRED.SPLIT_STORES */
- INTEL_UEVENT_CONSTRAINT(0x42d0, 0xf),
+ INTEL_UEVENT_CONSTRAINT(0x42d0, 0xf), /* MEM_UOPS_RETIRED.SPLIT_STORES */
INTEL_UEVENT_CONSTRAINT(0x81d0, 0xf), /* MEM_UOPS_RETIRED.ALL_LOADS */
- INTEL_PST_HSW_CONSTRAINT(0x82d0, 0xf), /* MEM_UOPS_RETIRED.ALL_STORES */
+ INTEL_PST_HSW_CONSTRAINT(0x82d0, 0xf),/* MEM_UOPS_RETIRED.ALL_STORES */
INTEL_UEVENT_CONSTRAINT(0x01d1, 0xf), /* MEM_LOAD_UOPS_RETIRED.L1_HIT */
INTEL_UEVENT_CONSTRAINT(0x02d1, 0xf), /* MEM_LOAD_UOPS_RETIRED.L2_HIT */
INTEL_UEVENT_CONSTRAINT(0x04d1, 0xf), /* MEM_LOAD_UOPS_RETIRED.L3_HIT */
- /* MEM_LOAD_UOPS_RETIRED.HIT_LFB */
- INTEL_UEVENT_CONSTRAINT(0x40d1, 0xf),
- /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.XSNP_MISS */
- INTEL_UEVENT_CONSTRAINT(0x01d2, 0xf),
- /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.XSNP_HIT */
- INTEL_UEVENT_CONSTRAINT(0x02d2, 0xf),
- /* MEM_LOAD_UOPS_LLC_MISS_RETIRED.LOCAL_DRAM */
- INTEL_UEVENT_CONSTRAINT(0x01d3, 0xf),
+ INTEL_UEVENT_CONSTRAINT(0x40d1, 0xf), /* MEM_LOAD_UOPS_RETIRED.HIT_LFB */
+ INTEL_UEVENT_CONSTRAINT(0x01d2, 0xf), /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.XSNP_MISS */
+ INTEL_UEVENT_CONSTRAINT(0x02d2, 0xf), /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.XSNP_HIT */
+ INTEL_UEVENT_CONSTRAINT(0x01d3, 0xf), /* MEM_LOAD_UOPS_LLC_MISS_RETIRED.LOCAL_DRAM */
INTEL_UEVENT_CONSTRAINT(0x04c8, 0xf), /* HLE_RETIRED.Abort */
INTEL_UEVENT_CONSTRAINT(0x04c9, 0xf), /* RTM_RETIRED.Abort */
-
+ INTEL_UEVENT_CONSTRAINT(0x0000, 0xf), /* generic PEBS mask */
EVENT_CONSTRAINT_END
};
@@ -686,6 +686,10 @@ struct event_constraint *intel_pebs_constraints(struct perf_event *event)
event->hw.flags |= c->flags;
return c;
}
+ if (!x86_pmu.attr_strict_pebs &&
+ capable(CAP_SYS_ADMIN) &&
+ !c->code)
+ return c;
}
}
On Thu, Jun 19, 2014 at 01:40:41PM -0700, Andi Kleen wrote:
> On Thu, Jun 19, 2014 at 10:31:29PM +0200, Stephane Eranian wrote:
> > On Thu, Jun 19, 2014 at 10:18 PM, Andi Kleen <[email protected]> wrote:
> > >> I don't quite understand that.
> > >> You need to know which events support PEBS. You need a table
> > >
> > > We're talking about the kernel allowing things here.
> > > Yes the user still needs to know what supports PEBS, but
> > > that doesn't concern the kernel.
> > >
> > Just need to make sure you don't return bogus information.
>
> GIGO. We only need to prevent security issues.
> Yes if the user specifies a bogus raw event it will not count.
> That's fine. The important part is just that nothing ever crashes.
Right. But IIRC you were previously arguing that we can in fact crash
the machine with raw PEBS events, as illustrated with the SNB PEBS
cycles 'event'.
Which is where my strict_pebs patch came from; by default only allow the
sanitized known-safe list of events, but allow the system administrator
to disable that test and allow any PEBS event.
On Mon, Jun 23, 2014 at 9:14 AM, Peter Zijlstra <[email protected]> wrote:
> On Thu, Jun 19, 2014 at 11:00:28AM -0700, Andi Kleen wrote:
>> However these days I'm actually thinking of just getting
>> rid of the detailed table except for PREC_DIST. All the PEBS
>> controls should be noops if the event does not support PEBS
>
> I had something like the below stuck on the 'look more at later' list
> and later never really ever happened.
>
> ---
> arch/x86/kernel/cpu/perf_event.c | 3 +++
> arch/x86/kernel/cpu/perf_event.h | 1 +
> arch/x86/kernel/cpu/perf_event_intel.c | 19 ++++++++++++++--
> arch/x86/kernel/cpu/perf_event_intel_ds.c | 38 +++++++++++++++++--------------
> 4 files changed, 42 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index ae407f7226c8..f42405c9868b 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -1541,6 +1541,7 @@ static int __init init_hw_perf_events(void)
> pr_cont("%s PMU driver.\n", x86_pmu.name);
>
> x86_pmu.attr_rdpmc = 1; /* enable userspace RDPMC usage by default */
> + x86_pmu.attr_strict_pebs = 1;
>
> for (quirk = x86_pmu.quirks; quirk; quirk = quirk->next)
> quirk->func();
> @@ -1855,9 +1856,11 @@ static ssize_t set_attr_rdpmc(struct device *cdev,
> }
>
> static DEVICE_ATTR(rdpmc, S_IRUSR | S_IWUSR, get_attr_rdpmc, set_attr_rdpmc);
> +static DEVICE_BOOL_ATTR(strict_pebs, S_IRUSR | S_IWUSR, x86_pmu.attr_strict_pebs);
>
> static struct attribute *x86_pmu_attrs[] = {
> &dev_attr_rdpmc.attr,
> + &dev_attr_strict_pebs.attr.attr,
> NULL,
> };
>
> diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
> index 3b2f9bdd974b..a11eeab9b611 100644
> --- a/arch/x86/kernel/cpu/perf_event.h
> +++ b/arch/x86/kernel/cpu/perf_event.h
> @@ -413,6 +413,7 @@ struct x86_pmu {
> */
> int attr_rdpmc_broken;
> int attr_rdpmc;
> + bool attr_strict_pebs;
> struct attribute **format_attrs;
> struct attribute **event_attrs;
>
> diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
> index aa333d966886..6e68f3dc9a30 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -1729,6 +1729,12 @@ static void intel_pebs_aliases_snb(struct perf_event *event)
> }
> }
>
> +#define ARCH_PERFMON_STRICT_PEBS \
> + (ARCH_PERFMON_EVENTSEL_ANY | \
> + ARCH_PERFMON_EVENTSEL_CMASK | \
> + ARCH_PERFMON_EVENTSEL_EDGE | \
> + ARCH_PERFMON_EVENTSEL_INV)
> +
> static int intel_pmu_hw_config(struct perf_event *event)
> {
> int ret = x86_pmu_hw_config(event);
> @@ -1736,8 +1742,17 @@ static int intel_pmu_hw_config(struct perf_event *event)
> if (ret)
> return ret;
>
> - if (event->attr.precise_ip && x86_pmu.pebs_aliases)
> - x86_pmu.pebs_aliases(event);
> + if (event->attr.precise_ip) {
> + if ((event->attr.config & INTEL_ARCH_EVENT_MASK) == 0x0000)
> + return -EINVAL;
> +
> + if ((event->attr.config & ARCH_PERFMON_STRICT_PEBS) &&
> + (x86_pmu.attr_strict_pebs || !capable(CAP_SYS_ADMIN)))
> + return -EINVAL;
> +
I don't think filters work with any PEBS events. The event captured
does not qualify
for any of the filters (root or non-root).
> + if (x86_pmu.pebs_aliases)
> + x86_pmu.pebs_aliases(event);
> + }
>
> if (intel_pmu_needs_lbr_smpl(event)) {
> ret = intel_pmu_setup_lbr_filter(event);
> diff --git a/arch/x86/kernel/cpu/perf_event_intel_ds.c b/arch/x86/kernel/cpu/perf_event_intel_ds.c
> index ae96cfa5eddd..36b1f2afa61c 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel_ds.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c
> @@ -540,6 +540,7 @@ struct event_constraint intel_core2_pebs_event_constraints[] = {
> INTEL_UEVENT_CONSTRAINT(0x00c5, 0x1), /* BR_INST_RETIRED.MISPRED */
> INTEL_UEVENT_CONSTRAINT(0x1fc7, 0x1), /* SIMD_INST_RETURED.ANY */
> INTEL_EVENT_CONSTRAINT(0xcb, 0x1), /* MEM_LOAD_RETIRED.* */
> + INTEL_UEVENT_CONSTRAINT(0x0000, 0x1), /* generic PEBS mask */
> EVENT_CONSTRAINT_END
> };
>
You probably need to explain that 0x0000 MUST be the last event in
each table, i.e., catch all
event.
> @@ -547,6 +548,7 @@ struct event_constraint intel_atom_pebs_event_constraints[] = {
> INTEL_UEVENT_CONSTRAINT(0x00c0, 0x1), /* INST_RETIRED.ANY */
> INTEL_UEVENT_CONSTRAINT(0x00c5, 0x1), /* MISPREDICTED_BRANCH_RETIRED */
> INTEL_EVENT_CONSTRAINT(0xcb, 0x1), /* MEM_LOAD_RETIRED.* */
> + INTEL_UEVENT_CONSTRAINT(0x0000, 0x1), /* generic PEBS mask */
> EVENT_CONSTRAINT_END
> };
>
> @@ -573,6 +575,7 @@ struct event_constraint intel_slm_pebs_event_constraints[] = {
> INTEL_UEVENT_CONSTRAINT(0xf7c5, 0x1), /* BR_INST_MISP_RETIRED.RETURN_PS */
> INTEL_UEVENT_CONSTRAINT(0xfbc5, 0x1), /* BR_INST_MISP_RETIRED.IND_CALL_PS */
> INTEL_UEVENT_CONSTRAINT(0xfec5, 0x1), /* BR_INST_MISP_RETIRED.TAKEN_JCC_PS */
> + INTEL_UEVENT_CONSTRAINT(0x0000, 0x1), /* generic PEBS mask */
> EVENT_CONSTRAINT_END
> };
>
> @@ -588,6 +591,7 @@ struct event_constraint intel_nehalem_pebs_event_constraints[] = {
> INTEL_UEVENT_CONSTRAINT(0x20c8, 0xf), /* ITLB_MISS_RETIRED */
> INTEL_EVENT_CONSTRAINT(0xcb, 0xf), /* MEM_LOAD_RETIRED.* */
> INTEL_EVENT_CONSTRAINT(0xf7, 0xf), /* FP_ASSIST.* */
> + INTEL_UEVENT_CONSTRAINT(0x0000, 0xf), /* generic PEBS mask */
> EVENT_CONSTRAINT_END
> };
>
> @@ -603,6 +607,7 @@ struct event_constraint intel_westmere_pebs_event_constraints[] = {
> INTEL_UEVENT_CONSTRAINT(0x20c8, 0xf), /* ITLB_MISS_RETIRED */
> INTEL_EVENT_CONSTRAINT(0xcb, 0xf), /* MEM_LOAD_RETIRED.* */
> INTEL_EVENT_CONSTRAINT(0xf7, 0xf), /* FP_ASSIST.* */
> + INTEL_UEVENT_CONSTRAINT(0x0000, 0xf), /* generic PEBS mask */
> EVENT_CONSTRAINT_END
> };
>
> @@ -619,6 +624,7 @@ struct event_constraint intel_snb_pebs_event_constraints[] = {
> INTEL_EVENT_CONSTRAINT(0xd2, 0xf), /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.* */
> INTEL_EVENT_CONSTRAINT(0xd3, 0xf), /* MEM_LOAD_UOPS_LLC_MISS_RETIRED.* */
> INTEL_UEVENT_CONSTRAINT(0x02d4, 0xf), /* MEM_LOAD_UOPS_MISC_RETIRED.LLC_MISS */
> + INTEL_UEVENT_CONSTRAINT(0x0000, 0xf), /* generic PEBS mask */
> EVENT_CONSTRAINT_END
> };
>
> @@ -634,42 +640,36 @@ struct event_constraint intel_ivb_pebs_event_constraints[] = {
> INTEL_EVENT_CONSTRAINT(0xd1, 0xf), /* MEM_LOAD_UOPS_RETIRED.* */
> INTEL_EVENT_CONSTRAINT(0xd2, 0xf), /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.* */
> INTEL_EVENT_CONSTRAINT(0xd3, 0xf), /* MEM_LOAD_UOPS_LLC_MISS_RETIRED.* */
> + INTEL_UEVENT_CONSTRAINT(0x0000, 0xf), /* generic PEBS mask */
> EVENT_CONSTRAINT_END
> };
>
> struct event_constraint intel_hsw_pebs_event_constraints[] = {
> INTEL_UEVENT_CONSTRAINT(0x01c0, 0x2), /* INST_RETIRED.PRECDIST */
> - INTEL_PST_HSW_CONSTRAINT(0x01c2, 0xf), /* UOPS_RETIRED.ALL */
> + INTEL_PST_HSW_CONSTRAINT(0x01c2, 0xf),/* UOPS_RETIRED.ALL */
> INTEL_UEVENT_CONSTRAINT(0x02c2, 0xf), /* UOPS_RETIRED.RETIRE_SLOTS */
> INTEL_EVENT_CONSTRAINT(0xc4, 0xf), /* BR_INST_RETIRED.* */
> INTEL_UEVENT_CONSTRAINT(0x01c5, 0xf), /* BR_MISP_RETIRED.CONDITIONAL */
> INTEL_UEVENT_CONSTRAINT(0x04c5, 0xf), /* BR_MISP_RETIRED.ALL_BRANCHES */
> INTEL_UEVENT_CONSTRAINT(0x20c5, 0xf), /* BR_MISP_RETIRED.NEAR_TAKEN */
> INTEL_PLD_CONSTRAINT(0x01cd, 0x8), /* MEM_TRANS_RETIRED.* */
> - /* MEM_UOPS_RETIRED.STLB_MISS_LOADS */
> - INTEL_UEVENT_CONSTRAINT(0x11d0, 0xf),
> - /* MEM_UOPS_RETIRED.STLB_MISS_STORES */
> - INTEL_UEVENT_CONSTRAINT(0x12d0, 0xf),
> + INTEL_UEVENT_CONSTRAINT(0x11d0, 0xf), /* MEM_UOPS_RETIRED.STLB_MISS_LOADS */
> + INTEL_UEVENT_CONSTRAINT(0x12d0, 0xf), /* MEM_UOPS_RETIRED.STLB_MISS_STORES */
> INTEL_UEVENT_CONSTRAINT(0x21d0, 0xf), /* MEM_UOPS_RETIRED.LOCK_LOADS */
> INTEL_UEVENT_CONSTRAINT(0x41d0, 0xf), /* MEM_UOPS_RETIRED.SPLIT_LOADS */
> - /* MEM_UOPS_RETIRED.SPLIT_STORES */
> - INTEL_UEVENT_CONSTRAINT(0x42d0, 0xf),
> + INTEL_UEVENT_CONSTRAINT(0x42d0, 0xf), /* MEM_UOPS_RETIRED.SPLIT_STORES */
> INTEL_UEVENT_CONSTRAINT(0x81d0, 0xf), /* MEM_UOPS_RETIRED.ALL_LOADS */
> - INTEL_PST_HSW_CONSTRAINT(0x82d0, 0xf), /* MEM_UOPS_RETIRED.ALL_STORES */
> + INTEL_PST_HSW_CONSTRAINT(0x82d0, 0xf),/* MEM_UOPS_RETIRED.ALL_STORES */
> INTEL_UEVENT_CONSTRAINT(0x01d1, 0xf), /* MEM_LOAD_UOPS_RETIRED.L1_HIT */
> INTEL_UEVENT_CONSTRAINT(0x02d1, 0xf), /* MEM_LOAD_UOPS_RETIRED.L2_HIT */
> INTEL_UEVENT_CONSTRAINT(0x04d1, 0xf), /* MEM_LOAD_UOPS_RETIRED.L3_HIT */
> - /* MEM_LOAD_UOPS_RETIRED.HIT_LFB */
> - INTEL_UEVENT_CONSTRAINT(0x40d1, 0xf),
> - /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.XSNP_MISS */
> - INTEL_UEVENT_CONSTRAINT(0x01d2, 0xf),
> - /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.XSNP_HIT */
> - INTEL_UEVENT_CONSTRAINT(0x02d2, 0xf),
> - /* MEM_LOAD_UOPS_LLC_MISS_RETIRED.LOCAL_DRAM */
> - INTEL_UEVENT_CONSTRAINT(0x01d3, 0xf),
> + INTEL_UEVENT_CONSTRAINT(0x40d1, 0xf), /* MEM_LOAD_UOPS_RETIRED.HIT_LFB */
> + INTEL_UEVENT_CONSTRAINT(0x01d2, 0xf), /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.XSNP_MISS */
> + INTEL_UEVENT_CONSTRAINT(0x02d2, 0xf), /* MEM_LOAD_UOPS_LLC_HIT_RETIRED.XSNP_HIT */
> + INTEL_UEVENT_CONSTRAINT(0x01d3, 0xf), /* MEM_LOAD_UOPS_LLC_MISS_RETIRED.LOCAL_DRAM */
> INTEL_UEVENT_CONSTRAINT(0x04c8, 0xf), /* HLE_RETIRED.Abort */
> INTEL_UEVENT_CONSTRAINT(0x04c9, 0xf), /* RTM_RETIRED.Abort */
> -
> + INTEL_UEVENT_CONSTRAINT(0x0000, 0xf), /* generic PEBS mask */
> EVENT_CONSTRAINT_END
> };
>
> @@ -686,6 +686,10 @@ struct event_constraint *intel_pebs_constraints(struct perf_event *event)
> event->hw.flags |= c->flags;
> return c;
> }
> + if (!x86_pmu.attr_strict_pebs &&
> + capable(CAP_SYS_ADMIN) &&
> + !c->code)
> + return c;
> }
> }
>
On Thu, Jun 19, 2014 at 05:58:29PM +0200, Stephane Eranian wrote:
> The load latency does not have to be constrained to counter 3
> on any of SNB, IVB, HSW. It operates fine on any PEBS-capable
> counter.
>
> The precise store event for SNB, IVB needs to be on counter 3.
> But on Haswell, precise store is implemented differently and
> the constraint is not needed anymore, so we remove it.
>
> The artificial constraint on counter 3 was used to ease
> scheduling because the load latency events rely on an
> extra MSR which is shared for all the counters. But
> perf_events has an infrastructure to handle shared_regs
> and does not need to constrain the load latency event to
> a single counter. It was already using that infrastructure
> with the constraint on counter 3. By eliminating the constraint
> on load latency, it becomes possible to measure loads and stores
> precisely without multiplexing.
So that all makes sense, except why did they pick the same constraint to
begin with? If they'd picked cnt2 for ll and cnt3 (as per the hardware
constraint) for st, this would've already been possible right?
Except of course, that the SDM states that no other PEBS event should be
active when using ll; we don't enforce that (although userspace could
request exclusive). What about this constraint? Is the SDM wrong about
this?
Peter,
On Mon, Jun 23, 2014 at 10:42 AM, Peter Zijlstra <[email protected]> wrote:
> On Thu, Jun 19, 2014 at 05:58:29PM +0200, Stephane Eranian wrote:
>> The load latency does not have to be constrained to counter 3
>> on any of SNB, IVB, HSW. It operates fine on any PEBS-capable
>> counter.
>>
>> The precise store event for SNB, IVB needs to be on counter 3.
>> But on Haswell, precise store is implemented differently and
>> the constraint is not needed anymore, so we remove it.
>>
>> The artificial constraint on counter 3 was used to ease
>> scheduling because the load latency events rely on an
>> extra MSR which is shared for all the counters. But
>> perf_events has an infrastructure to handle shared_regs
>> and does not need to constrain the load latency event to
>> a single counter. It was already using that infrastructure
>> with the constraint on counter 3. By eliminating the constraint
>> on load latency, it becomes possible to measure loads and stores
>> precisely without multiplexing.
>
> So that all makes sense, except why did they pick the same constraint to
> begin with? If they'd picked cnt2 for ll and cnt3 (as per the hardware
> constraint) for st, this would've already been possible right?
>
I don't know why they did it this way. I think somehow, it is believe that
ll and st cannot be captured together (and putting both on cnt3 enforces
that). But when it seems to be working fine. If someone from Intel can
confirm this is okay/not okay then we can revisit.
> Except of course, that the SDM states that no other PEBS event should be
> active when using ll; we don't enforce that (although userspace could
> request exclusive). What about this constraint? Is the SDM wrong about
> this?
For LL this is usually the case if you assume a single measurement is
active. But in system-wide on a shared system, it is possible to have
other events active on the same CPU. I have not tried that to see the
impact on ll.
You can say the same with PREC_DIST which up until HSW needs to be
taken alone, i.e., no other event active. We don't enforce that either, it would
cause problems with the NMI watchdog.
On Mon, Jun 23, 2014 at 10:06:43AM +0200, Stephane Eranian wrote:
> > @@ -1736,8 +1742,17 @@ static int intel_pmu_hw_config(struct perf_event *event)
> > if (ret)
> > return ret;
> >
> > - if (event->attr.precise_ip && x86_pmu.pebs_aliases)
> > - x86_pmu.pebs_aliases(event);
> > + if (event->attr.precise_ip) {
> > + if ((event->attr.config & INTEL_ARCH_EVENT_MASK) == 0x0000)
> > + return -EINVAL;
> > +
> > + if ((event->attr.config & ARCH_PERFMON_STRICT_PEBS) &&
> > + (x86_pmu.attr_strict_pebs || !capable(CAP_SYS_ADMIN)))
> > + return -EINVAL;
> > +
> I don't think filters work with any PEBS events. The event captured
> does not qualify
> for any of the filters (root or non-root).
Filters? You mean the inv,cmask etc? Well, we have the Intel provided
'cycle' events that use them, so exposing them makes sense and allows
such experimentation when we need another such alias.
> > + if (x86_pmu.pebs_aliases)
> > + x86_pmu.pebs_aliases(event);
> > + }
> >
> > if (intel_pmu_needs_lbr_smpl(event)) {
> > ret = intel_pmu_setup_lbr_filter(event);
> > diff --git a/arch/x86/kernel/cpu/perf_event_intel_ds.c b/arch/x86/kernel/cpu/perf_event_intel_ds.c
> > index ae96cfa5eddd..36b1f2afa61c 100644
> > --- a/arch/x86/kernel/cpu/perf_event_intel_ds.c
> > +++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c
> > @@ -540,6 +540,7 @@ struct event_constraint intel_core2_pebs_event_constraints[] = {
> > INTEL_UEVENT_CONSTRAINT(0x00c5, 0x1), /* BR_INST_RETIRED.MISPRED */
> > INTEL_UEVENT_CONSTRAINT(0x1fc7, 0x1), /* SIMD_INST_RETURED.ANY */
> > INTEL_EVENT_CONSTRAINT(0xcb, 0x1), /* MEM_LOAD_RETIRED.* */
> > + INTEL_UEVENT_CONSTRAINT(0x0000, 0x1), /* generic PEBS mask */
> > EVENT_CONSTRAINT_END
> > };
> >
> You probably need to explain that 0x0000 MUST be the last event in
> each table, i.e., catch all
> event.
Yeah, probably ;-) Alternatively we could make PEBS_CONSTRAINT_END that
includes it or so.
Like said (possibly in another email) this patch was a very quick draft
and I don't think I've even ran it, it was on the todo pile..
On Mon, Jun 23, 2014 at 11:00:18AM +0200, Stephane Eranian wrote:
> You can say the same with PREC_DIST which up until HSW needs to be
> taken alone, i.e., no other event active. We don't enforce that either, it would
> cause problems with the NMI watchdog.
Yeah, the SDM states no other PEBS event, so the NMI watchdog would be
fine, but we do indeed not have any means of excluding other PEBS
events, only all other events.
On Mon, Jun 23, 2014 at 1:37 PM, Peter Zijlstra <[email protected]> wrote:
> On Mon, Jun 23, 2014 at 10:06:43AM +0200, Stephane Eranian wrote:
>> > @@ -1736,8 +1742,17 @@ static int intel_pmu_hw_config(struct perf_event *event)
>> > if (ret)
>> > return ret;
>> >
>> > - if (event->attr.precise_ip && x86_pmu.pebs_aliases)
>> > - x86_pmu.pebs_aliases(event);
>> > + if (event->attr.precise_ip) {
>> > + if ((event->attr.config & INTEL_ARCH_EVENT_MASK) == 0x0000)
>> > + return -EINVAL;
>> > +
>> > + if ((event->attr.config & ARCH_PERFMON_STRICT_PEBS) &&
>> > + (x86_pmu.attr_strict_pebs || !capable(CAP_SYS_ADMIN)))
>> > + return -EINVAL;
>> > +
>> I don't think filters work with any PEBS events. The event captured
>> does not qualify
>> for any of the filters (root or non-root).
>
> Filters? You mean the inv,cmask etc? Well, we have the Intel provided
> 'cycle' events that use them, so exposing them makes sense and allows
> such experimentation when we need another such alias.
>
Yes, inv, cmask, edge, any. They are ignored for the sampled instruction.
They are used to get to the PEBS trigger, then after it is the plain event/umask
for the next qualifying instruction.
I believe that the reason while cycles:pp somewhat works is because you are
after stalls. cycles:pp = uops_retired:any:cmask=1:inv. The sampled instruction
matches uops_retired:any (filters ignored). Given you are stalled,
nothing retires.
The next uops to retire (forward progress) is captured.
>> > + if (x86_pmu.pebs_aliases)
>> > + x86_pmu.pebs_aliases(event);
>> > + }
>> >
>> > if (intel_pmu_needs_lbr_smpl(event)) {
>> > ret = intel_pmu_setup_lbr_filter(event);
>> > diff --git a/arch/x86/kernel/cpu/perf_event_intel_ds.c b/arch/x86/kernel/cpu/perf_event_intel_ds.c
>> > index ae96cfa5eddd..36b1f2afa61c 100644
>> > --- a/arch/x86/kernel/cpu/perf_event_intel_ds.c
>> > +++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c
>> > @@ -540,6 +540,7 @@ struct event_constraint intel_core2_pebs_event_constraints[] = {
>> > INTEL_UEVENT_CONSTRAINT(0x00c5, 0x1), /* BR_INST_RETIRED.MISPRED */
>> > INTEL_UEVENT_CONSTRAINT(0x1fc7, 0x1), /* SIMD_INST_RETURED.ANY */
>> > INTEL_EVENT_CONSTRAINT(0xcb, 0x1), /* MEM_LOAD_RETIRED.* */
>> > + INTEL_UEVENT_CONSTRAINT(0x0000, 0x1), /* generic PEBS mask */
>> > EVENT_CONSTRAINT_END
>> > };
>> >
>> You probably need to explain that 0x0000 MUST be the last event in
>> each table, i.e., catch all
>> event.
>
> Yeah, probably ;-) Alternatively we could make PEBS_CONSTRAINT_END that
> includes it or so.
You could do that, though it would serve dual purpose: end marker and catch all.
>
> Like said (possibly in another email) this patch was a very quick draft
> and I don't think I've even ran it, it was on the todo pile..
On Mon, Jun 23, 2014 at 09:35:00AM +0200, Peter Zijlstra wrote:
> On Thu, Jun 19, 2014 at 01:40:41PM -0700, Andi Kleen wrote:
> > On Thu, Jun 19, 2014 at 10:31:29PM +0200, Stephane Eranian wrote:
> > > On Thu, Jun 19, 2014 at 10:18 PM, Andi Kleen <[email protected]> wrote:
> > > >> I don't quite understand that.
> > > >> You need to know which events support PEBS. You need a table
> > > >
> > > > We're talking about the kernel allowing things here.
> > > > Yes the user still needs to know what supports PEBS, but
> > > > that doesn't concern the kernel.
> > > >
> > > Just need to make sure you don't return bogus information.
> >
> > GIGO. We only need to prevent security issues.
>
> > Yes if the user specifies a bogus raw event it will not count.
> > That's fine. The important part is just that nothing ever crashes.
>
> Right. But IIRC you were previously arguing that we can in fact crash
> the machine with raw PEBS events, as illustrated with the SNB PEBS
> cycles 'event'.
The potential problem could only happen for a recognized PEBS event/umask,
but with unsupported flag combinations. That is what the SDM warns
about in 18.8.4.
If the event is not recognized as PEBS it will just effectively
disable the event.
> Which is where my strict_pebs patch came from; by default only allow the
> sanitized known-safe list of events, but allow the system administrator
> to disable that test and allow any PEBS event.
I don't think we need to enforce the list of events
(except for the few with special limited counters)
-Andi
--
[email protected] -- Speaking for myself only
> I don't know why they did it this way. I think somehow, it is believe that
> ll and st cannot be captured together (and putting both on cnt3 enforces
> that). But when it seems to be working fine. If someone from Intel can
> confirm this is okay/not okay then we can revisit.
Depending on the CPU, if you have multiple load and store threshold events
active at the same time it may capture junk.
So it's better to keep that constraint too, or enforce it with other
ways (e.g. an extra reg)
> You can say the same with PREC_DIST which up until HSW needs to be
> taken alone, i.e., no other event active. We don't enforce that either, it would
> cause problems with the NMI watchdog.
I believe it's only sandy bridge where it needs to be taken alone.
-Andi
--
[email protected] -- Speaking for myself only
> Filters? You mean the inv,cmask etc? Well, we have the Intel provided
> 'cycle' events that use them, so exposing them makes sense and allows
> such experimentation when we need another such alias.
The SDM explicitely discourages it.
The 'cycle' event is now handled explicitely, but for others it is still
not ok.
-Andi