From: Kan Liang <[email protected]>
For client uncore, all CBOXes seems have their own clockticks event
uncore_cbox_*/clockticks/. However, only the clockticks event for CBOX_0
can work. Accessing other CBOX clockticks evnet will always return
EINVAL. Here is an example.
$ perf stat -x, -e
uncore_cbox_0/clockticks/,uncore_cbox_1/clockticks/ sleep 1
10660314,,uncore_cbox_0/clockticks/,1001596935,100.00,,,,
<not supported>,,uncore_cbox_1/clockticks/,0,100.00,,,,
There is only one clockticks fixed counter in Hardware.
Current kernel only allow the first CBOX to access the fixed counter.
However, it does not expose the limitation to user space.
User can still see the nonexistent clockticks event.
Simply removing the clockticks event for the rest CBOX does not work.
Because all boxes share the same attr_groups. If it is set for one box,
it will applied to other boxes.
The clockticks fixed counter is a standalone counter. It should be
removed from the CBOX PMUs. A new type of PMU is added which only
supports fixed counter events.
User observable change with the patch.
clockticks event is removed from CBOX. User may need to change their
script to use uncore_clock/clockticks/ instead.
$ perf stat -x, -e uncore_clock/clockticks/ sleep 1
10421811,,uncore_clock/clockticks/,1001581963,100.00,,,,
Reported-by: Jiri Olsa <[email protected]>
Tested-by: Jiri Olsa <[email protected]>
Signed-off-by: Kan Liang <[email protected]>
---
The report and discussion can be found here.
https://lkml.org/lkml/2016/11/30/277
Changes since V2:
- Refine changelog
Changes since V1:
- Refine changelog
- Add Tested-by
arch/x86/events/intel/uncore.c | 13 +++++++------
arch/x86/events/intel/uncore_snb.c | 38 +++++++++++++++++++++++++++-----------
2 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 8106df7db758..26a93f7a6003 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -658,6 +658,10 @@ static int uncore_pmu_event_init(struct perf_event *event)
if (hwc->sample_period)
return -EINVAL;
+ /* Single fixed PMU only has fixed event */
+ if (pmu->type->single_fixed && (event->attr.config != UNCORE_FIXED_EVENT))
+ return -EINVAL;
+
/*
* Place all uncore events for a particular physical package
* onto a single cpu
@@ -681,12 +685,9 @@ static int uncore_pmu_event_init(struct perf_event *event)
/* no fixed counter */
if (!pmu->type->fixed_ctl)
return -EINVAL;
- /*
- * if there is only one fixed counter, only the first pmu
- * can access the fixed counter
- */
- if (pmu->type->single_fixed && pmu->pmu_idx > 0)
- return -EINVAL;
+
+ if (pmu->type->single_fixed)
+ event->hw.idx = UNCORE_PMC_IDX_FIXED;
/* fixed counters have event field hardcoded to zero */
hwc->config = 0ULL;
diff --git a/arch/x86/events/intel/uncore_snb.c b/arch/x86/events/intel/uncore_snb.c
index dda44302207a..4090d872883c 100644
--- a/arch/x86/events/intel/uncore_snb.c
+++ b/arch/x86/events/intel/uncore_snb.c
@@ -122,7 +122,7 @@ static void snb_uncore_msr_exit_box(struct intel_uncore_box *box)
}
static struct uncore_event_desc snb_uncore_events[] = {
- INTEL_UNCORE_EVENT_DESC(clockticks, "event=0xff,umask=0x00"),
+ INTEL_UNCORE_EVENT_DESC(clockticks, "event=0xff"),
{ /* end: all zeroes */ },
};
@@ -160,17 +160,12 @@ static struct intel_uncore_type snb_uncore_cbox = {
.num_counters = 2,
.num_boxes = 4,
.perf_ctr_bits = 44,
- .fixed_ctr_bits = 48,
.perf_ctr = SNB_UNC_CBO_0_PER_CTR0,
.event_ctl = SNB_UNC_CBO_0_PERFEVTSEL0,
- .fixed_ctr = SNB_UNC_FIXED_CTR,
- .fixed_ctl = SNB_UNC_FIXED_CTR_CTRL,
- .single_fixed = 1,
.event_mask = SNB_UNC_RAW_EVENT_MASK,
.msr_offset = SNB_UNC_CBO_MSR_OFFSET,
.ops = &snb_uncore_msr_ops,
.format_group = &snb_uncore_format_group,
- .event_descs = snb_uncore_events,
};
static struct intel_uncore_type snb_uncore_arb = {
@@ -187,9 +182,34 @@ static struct intel_uncore_type snb_uncore_arb = {
.format_group = &snb_uncore_format_group,
};
+static struct attribute *snb_uncore_clock_formats_attr[] = {
+ &format_attr_event.attr,
+ NULL,
+};
+
+static struct attribute_group snb_uncore_clock_format_group = {
+ .name = "format",
+ .attrs = snb_uncore_clock_formats_attr,
+};
+
+static struct intel_uncore_type snb_uncore_clockbox = {
+ .name = "clock",
+ .num_counters = 1,
+ .num_boxes = 1,
+ .fixed_ctr_bits = 48,
+ .fixed_ctr = SNB_UNC_FIXED_CTR,
+ .fixed_ctl = SNB_UNC_FIXED_CTR_CTRL,
+ .single_fixed = 1,
+ .event_mask = SNB_UNC_CTL_EV_SEL_MASK,
+ .format_group = &snb_uncore_clock_format_group,
+ .ops = &snb_uncore_msr_ops,
+ .event_descs = snb_uncore_events,
+};
+
static struct intel_uncore_type *snb_msr_uncores[] = {
&snb_uncore_cbox,
&snb_uncore_arb,
+ &snb_uncore_clockbox,
NULL,
};
@@ -234,22 +254,18 @@ static struct intel_uncore_type skl_uncore_cbox = {
.num_counters = 4,
.num_boxes = 5,
.perf_ctr_bits = 44,
- .fixed_ctr_bits = 48,
.perf_ctr = SNB_UNC_CBO_0_PER_CTR0,
.event_ctl = SNB_UNC_CBO_0_PERFEVTSEL0,
- .fixed_ctr = SNB_UNC_FIXED_CTR,
- .fixed_ctl = SNB_UNC_FIXED_CTR_CTRL,
- .single_fixed = 1,
.event_mask = SNB_UNC_RAW_EVENT_MASK,
.msr_offset = SNB_UNC_CBO_MSR_OFFSET,
.ops = &skl_uncore_msr_ops,
.format_group = &snb_uncore_format_group,
- .event_descs = snb_uncore_events,
};
static struct intel_uncore_type *skl_msr_uncores[] = {
&skl_uncore_cbox,
&snb_uncore_arb,
+ &snb_uncore_clockbox,
NULL,
};
--
2.11.0
> User observable change with the patch.
> clockticks event is removed from CBOX. User may need to change their
> script to use uncore_clock/clockticks/ instead.
I don't think we can do that and break everyone's scripts. Have to keep
the old behavior, even though it is not ideal.
Or you fix the the uncore subsystem to support a separate attribute
array for the first cbox.
-Andi
>
> > User observable change with the patch.
> > clockticks event is removed from CBOX. User may need to change their
> > script to use uncore_clock/clockticks/ instead.
>
> I don't think we can do that and break everyone's scripts. Have to keep the
> old behavior, even though it is not ideal.
>
> Or you fix the the uncore subsystem to support a separate attribute array for
> the first cbox.
>
I once had a patch to only support clockticks for first box.
https://lkml.org/lkml/2016/12/1/477
But Peter think it's better to move it to its own PMU.
https://lkml.org/lkml/2016/12/1/484
Each solution has its own advantage and drawback. I'm OK for either of them.
I'm not sure what others prefer.
Peter? Ingo? What do you think?
Thanks,
Kan