Skip to contentSkip to conversation

perf/x86/intel/uncore: pmu->type->single_fixed question

RFC Jiri Olsa 4 participants 11 messages Nov 30, 2016 – Jan 16, 2017

Subsystems (2)
x86 perf events
lore
Share & tools
lore

Jiri Olsa

author
#0 lore raw

hi,
I'm trying to find out some documentation background for this part of uncore code:

---
static int uncore_pmu_event_init(struct perf_event *event)

{
        ...
        if (event->attr.config == UNCORE_FIXED_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;
        ...

---

that for some uncore types (those with pmu->type->single_fixed) only the first pmu (code_id == 0) will allow to touch the clocktick event

other cores boxes will not allow to open clocktick event, eventhough it's announced via /sys/../events/..

I'm probably missing some HW logic of specific boxes that would explain that, but I can't find it.

thanks for info,
jirka

#1 lore raw
Quoted 22 lines

hi,
I'm trying to find out some documentation background for this part of uncore code:

---

static int uncore_pmu_event_init(struct perf_event *event) {
        ...
        if (event->attr.config == UNCORE_FIXED_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;
        ...

---

Quoted 8 lines

that for some uncore types (those with pmu->type->single_fixed) only the first pmu (code_id == 0) will allow to touch the clocktick event

other cores boxes will not allow to open clocktick event, eventhough it's announced via /sys/../events/..

I'm probably missing some HW logic of specific boxes that would explain that, but I can't find it.

The client uncore has a standalone clocktick fixed counter. It doesn't belong to any boxes, which is different from server uncore.

But client and server uncore share the same uncore_pmu_event_init. So it forces that only the first box can access the fixed counter.

Maybe we should create a clocktick box for client uncore to fix it.

You can find the fixed counter information from 18.11.6 in latest SDM (Order Number: 325384-060US).

There should be a Skylake client uncore document published somewhere. But I cannot find it from Google. Let me ask around.

Thanks,
Kan

#2 lore raw
Quoted 2 lines · Kan

Maybe we should create a clocktick box for client uncore to fix it.

That would break all the user space code that uses the existing format.

-Andi

#3 lore raw
Quoted 8 lines
Quoted 2 lines · Kan

Maybe we should create a clocktick box for client uncore to fix it.

That would break all the user space code that uses the existing format.

OK.
If so, I think we should remove the non-exist events for non-first boxes.

What do you think about the patch as below?

Andi? Jirka?

------

Quoted 1 line

From 844c12b5e6fc1e2c27b2f094c89098ea9405ea41 Mon Sep 17 00:00:00 2001

From: Kan Liang <[email protected]>
Date: Thu, 1 Dec 2016 03:29:29 -0500
Subject: [PATCH] perf/x86/intel/uncore: remove non-exist clockticks events

For client cbox, only the first box can access the clockticks event. Other cboxes will not allow to open clocktick event, eventhough it's announced via /sys/../events/..

The client uncore has a standalone clocktick fixed counter. It doesn't belong to any boxes, which is different from server uncore. To make the code compatible, the client forces that only the first box can access the fixed counter.

The clocktick event should be removed from other boxes. Currently, all the pmus of same type share the same attr_groups, which include all the events.
no_fixed_attr_groups is introduced for other boxes, which remove the non-exist events.

Signed-off-by Kan Liang <[email protected]>
---
 arch/x86/events/intel/uncore.c | 70 ++++++++++++++++++++++++++++++++----------
 arch/x86/events/intel/uncore.h |  1 +
 2 files changed, 55 insertions(+), 16 deletions(-)
arch/x86/events/intel/uncore.c +54 -16
index dbaaf7dc..6d3606e 100644
@@ -739,6 +739,9 @@ static int uncore_pmu_register(struct intel_uncore_pmu *pmu)
 		pmu->pmu.attr_groups = pmu->type->attr_groups;
 	}
 
+	if (pmu->type->single_fixed && pmu->pmu_idx)
+		pmu->pmu.attr_groups = pmu->type->no_fixed_attr_groups;
+
 	if (pmu->type->num_boxes == 1) {
 		if (strlen(pmu->type->name) > 0)
 			sprintf(pmu->name, "uncore_%s", pmu->type->name);

@@ -811,6 +814,8 @@ static void uncore_type_exit(struct intel_uncore_type *type)
 	}
 	kfree(type->events_group);
 	type->events_group = NULL;
+	kfree(type->no_fixed_attr_groups[2]);
+	type->no_fixed_attr_groups[2] = NULL;
 }
 
 static void uncore_types_exit(struct intel_uncore_type **types)

@@ -819,13 +824,45 @@ static void uncore_types_exit(struct intel_uncore_type **types)
 		uncore_type_exit(*types);
 }
 
-static int __init uncore_type_init(struct intel_uncore_type *type, bool setid)
+static struct attribute_group *
+uncore_alloc_event_group(struct intel_uncore_type *type,
+			 bool single_fixed)
 {
-	struct intel_uncore_pmu *pmus;
 	struct attribute_group *attr_group;
 	struct attribute **attrs;
+	char fixed_event_name[11];
+	int i, j, index = 0;
+
+	if (single_fixed)
+		snprintf(fixed_event_name, 11, "event=0x%x", UNCORE_FIXED_EVENT);
+
+	for (i = 0; type->event_descs[i].attr.attr.name; i++)
+		;
+
+	attr_group = kzalloc(sizeof(struct attribute *) * (i + 1) +
+				sizeof(*attr_group), GFP_KERNEL);
+	if (!attr_group)
+		return NULL;
+
+	attrs = (struct attribute **)(attr_group + 1);
+	attr_group->name = "events";
+	attr_group->attrs = attrs;
+
+	for (j = 0; j < i; j++) {
+		if (single_fixed && !strncmp(type->event_descs[j].config, fixed_event_name, 10))
+			continue;
+
+		attrs[index++] = &type->event_descs[j].attr.attr;
+	}
+
+	return attr_group;
+}
+
+static int __init uncore_type_init(struct intel_uncore_type *type, bool setid)
+{
+	struct intel_uncore_pmu *pmus;
 	size_t size;
-	int i, j;
+	int i;
 
 	pmus = kzalloc(sizeof(*pmus) * type->num_boxes, GFP_KERNEL);
 	if (!pmus)

@@ -848,24 +885,25 @@ static int __init uncore_type_init(struct intel_uncore_type *type, bool setid)
 				0, type->num_counters, 0, 0);
 
 	if (type->event_descs) {
-		for (i = 0; type->event_descs[i].attr.attr.name; i++);
-
-		attr_group = kzalloc(sizeof(struct attribute *) * (i + 1) +
-					sizeof(*attr_group), GFP_KERNEL);
-		if (!attr_group)
+		type->events_group = uncore_alloc_event_group(type, false);
+		if (!type->events_group)
 			return -ENOMEM;
+		if (type->single_fixed) {
+			type->no_fixed_attr_groups[2] = uncore_alloc_event_group(type, true);
+			if (!type->no_fixed_attr_groups[2]) {
+				kfree(type->events_group);
+				return -ENOMEM;
+			}
+		}
+	}
 
-		attrs = (struct attribute **)(attr_group + 1);
-		attr_group->name = "events";
-		attr_group->attrs = attrs;
-
-		for (j = 0; j < i; j++)
-			attrs[j] = &type->event_descs[j].attr.attr;
+	type->pmu_group = &uncore_pmu_attr_group;
 
-		type->events_group = attr_group;
+	if (type->single_fixed) {
+		type->no_fixed_attr_groups[0] = type->pmu_group;
+		type->no_fixed_attr_groups[1] = type->format_group;
 	}
 
-	type->pmu_group = &uncore_pmu_attr_group;
 	return 0;
 }
arch/x86/events/intel/uncore.h +1 -0
index ad986c1..23bf3ff 100644
@@ -59,6 +59,7 @@ struct intel_uncore_type {
 	struct intel_uncore_ops *ops;
 	struct uncore_event_desc *event_descs;
 	const struct attribute_group *attr_groups[4];
+	const struct attribute_group *no_fixed_attr_groups[4];
 	struct pmu *pmu; /* for custom pmu ops */
 };
Signature · 1 line
2.5.5
maintainer
#4 lore raw
Quoted 6 lines · Kan
Quoted 6 lines
Quoted 2 lines · Kan

Maybe we should create a clocktick box for client uncore to fix it.

Quoted 8 lines
Quoted 2 lines

That would break all the user space code that uses the existing format.

OK.
If so, I think we should remove the non-exist events for non-first boxes.

What do you think about the patch as below?

Andi? Jirka?

Urgh, more lines for ugly :-(

I really would prefer to move the thing to its own PMU.

#5 lore raw
Quoted 3 lines

I really would prefer to move the thing to its own PMU.

The patch as below creates a new PMU to fix the issue.

Jirka, could you please try the patch on your machine?

Thanks,
Kan
-------

Quoted 1 line

From 2de8b2eda6b54734e08a608b5fc8c367b94369d3 Mon Sep 17 00:00:00 2001

From: Kan Liang <[email protected]>
Date: Mon, 12 Dec 2016 09:03:35 -0500
Subject: [PATCH] perf/x86/intel/uncore: fix nonexistent clockticks event for
client uncore

The clockticks event can only be used by the first Cbox pmu. Other Cboxes don't allow to open clockticks event, eventhough it's announced via /sys/../events/..

For client uncore, there is only one clocktick fixed counter. Current kernel code forces that only the first box can access the fixed counter in uncore_pmu_event_init. But it doesn't take care of the the attr_groups. All the pmus of same type share the same attr_groups. If the clockticks event is set for the first box, user can also observe the event in other boxes.

The clocktick 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 changes with the patch.
clockticks event is removed from Cbox. It will return unsupported, if uncore_cbox_0/clockticks/ is accessed. User may need to change their script to use uncore_clock/clockticks/ to instead.

Signed-off-by Kan Liang <[email protected]>
---
 arch/x86/events/intel/uncore.c     | 13 +++++++------
 arch/x86/events/intel/uncore_snb.c | 38 +++++++++++++++++++++++++++-----------
 2 files changed, 34 insertions(+), 17 deletions(-)
arch/x86/events/intel/uncore.c +7 -6
index dbaaf7dc..03afeca 100644
@@ -652,6 +652,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

@@ -675,12 +679,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;
arch/x86/events/intel/uncore_snb.c +27 -11
index a3dcc12..1b037ea 100644
@@ -117,7 +117,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 */ },
 };
 

@@ -155,17 +155,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 = {

@@ -182,9 +177,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,
 };
 

@@ -229,22 +249,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,
 };
Signature · 1 line
2.4.3
author Tested-by
#6 lore raw
Quoted 33 lines · Kan
Quoted 3 lines

I really would prefer to move the thing to its own PMU.

The patch as below creates a new PMU to fix the issue.

Jirka, could you please try the patch on your machine?

Thanks,
Kan
-------
From 2de8b2eda6b54734e08a608b5fc8c367b94369d3 Mon Sep 17 00:00:00 2001
From: Kan Liang <[email protected]>
Date: Mon, 12 Dec 2016 09:03:35 -0500
Subject: [PATCH] perf/x86/intel/uncore: fix nonexistent clockticks event for
client uncore

The clockticks event can only be used by the first Cbox pmu. Other Cboxes don't allow to open clockticks event, eventhough it's announced via /sys/../events/..

For client uncore, there is only one clocktick fixed counter. Current kernel code forces that only the first box can access the fixed counter in uncore_pmu_event_init. But it doesn't take care of the the attr_groups. All the pmus of same type share the same attr_groups. If the clockticks event is set for the first box, user can also observe the event in other boxes.

The clocktick 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.

Quoted 6 lines

User observable changes with the patch.
clockticks event is removed from Cbox. It will return unsupported, if uncore_cbox_0/clockticks/ is accessed. User may need to change their script to use uncore_clock/clockticks/ to instead.

Signed-off-by Kan Liang <[email protected]>

seems ok

Tested-by Jiri Olsa <[email protected]>

thanks,
jirka

#7 lore raw
Quoted 41 lines · Kan
Quoted 6 lines
Quoted 3 lines

I really would prefer to move the thing to its own PMU.

The patch as below creates a new PMU to fix the issue.

Quoted 7 lines

Jirka, could you please try the patch on your machine?

Thanks,
Kan
-------
From 2de8b2eda6b54734e08a608b5fc8c367b94369d3 Mon Sep 17

00:00:00 2001

Quoted 25 lines

From: Kan Liang <[email protected]>
Date: Mon, 12 Dec 2016 09:03:35 -0500
Subject: [PATCH] perf/x86/intel/uncore: fix nonexistent clockticks event for client uncore

The clockticks event can only be used by the first Cbox pmu. Other Cboxes don't allow to open clockticks event, eventhough it's announced via /sys/../events/..

For client uncore, there is only one clocktick fixed counter. Current kernel code forces that only the first box can access the fixed counter in uncore_pmu_event_init. But it doesn't take care of the the attr_groups. All the pmus of same type share the same attr_groups. If the clockticks event is set for the first box, user can also observe the event in other boxes.

The clocktick 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 changes with the patch.
clockticks event is removed from Cbox. It will return unsupported, if uncore_cbox_0/clockticks/ is accessed. User may need to change their script to use uncore_clock/clockticks/ to instead.

Quoted 6 lines
Quoted 1 line
Signed-off-by Kan Liang <[email protected]>

seems ok

Tested-by Jiri Olsa <[email protected]>

Hi Peter and Ingo,

Any comments for the patch?
Do I need to re-send it?

Thanks,
Kan

#8 lore raw
Quoted 48 lines
Quoted 41 lines · Kan
Quoted 6 lines
Quoted 3 lines

I really would prefer to move the thing to its own PMU.

The patch as below creates a new PMU to fix the issue.

Quoted 7 lines

Jirka, could you please try the patch on your machine?

Thanks,
Kan
-------
From 2de8b2eda6b54734e08a608b5fc8c367b94369d3 Mon Sep 17

00:00:00 2001

Quoted 25 lines

From: Kan Liang <[email protected]>
Date: Mon, 12 Dec 2016 09:03:35 -0500
Subject: [PATCH] perf/x86/intel/uncore: fix nonexistent clockticks event for client uncore

The clockticks event can only be used by the first Cbox pmu. Other Cboxes don't allow to open clockticks event, eventhough it's announced via /sys/../events/..

For client uncore, there is only one clocktick fixed counter. Current kernel code forces that only the first box can access the fixed counter in uncore_pmu_event_init. But it doesn't take care of the the attr_groups. All the pmus of same type share the same attr_groups. If the clockticks event is set for the first box, user can also observe the event in other boxes.

The clocktick 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 changes with the patch.
clockticks event is removed from Cbox. It will return unsupported, if uncore_cbox_0/clockticks/ is accessed. User may need to change their script to use uncore_clock/clockticks/ to instead.

Quoted 6 lines
Quoted 1 line
Signed-off-by Kan Liang <[email protected]>

seems ok

Tested-by Jiri Olsa <[email protected]>
Quoted 7 lines

Hi Peter and Ingo,

Any comments for the patch?
Do I need to re-send it?

Thanks,
Kan

Hi Peter and Ingo,

Ping.
Please let me know if you want me to re-send the patch.

Thanks,
Kan

#9 lore raw
In reply to Jiri Olsa
Quoted 37 lines
Quoted 22 lines

hi,
I'm trying to find out some documentation background for this part of uncore code:

---

static int uncore_pmu_event_init(struct perf_event *event) {
        ...
        if (event->attr.config == UNCORE_FIXED_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;
        ...

---

Quoted 8 lines

that for some uncore types (those with pmu->type->single_fixed) only the first pmu (code_id == 0) will allow to touch the clocktick event

other cores boxes will not allow to open clocktick event, eventhough it's announced via /sys/../events/..

I'm probably missing some HW logic of specific boxes that would explain that, but I can't find it.

The client uncore has a standalone clocktick fixed counter. It doesn't belong to any boxes, which is different from server uncore.

But client and server uncore share the same uncore_pmu_event_init. So it forces that only the first box can access the fixed counter.

Quoted 7 lines

Maybe we should create a clocktick box for client uncore to fix it.

You can find the fixed counter information from 18.11.6 in latest SDM (Order Number: 325384-060US).

There should be a Skylake client uncore document published somewhere. But I cannot find it from Google. Let me ask around.

Here is the published document.
http://www.intel.com/content/www/us/en/processors/core/6th-gen-core-family-uncore-performance-monitoring-manual.html

Quoted 3 lines

Thanks,
Kan

author
#10 lore raw
Quoted 39 lines · Kan
Quoted 39 lines
Quoted 22 lines

hi,
I'm trying to find out some documentation background for this part of uncore code:

---

static int uncore_pmu_event_init(struct perf_event *event) {
        ...
        if (event->attr.config == UNCORE_FIXED_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;
        ...

---

Quoted 8 lines

that for some uncore types (those with pmu->type->single_fixed) only the first pmu (code_id == 0) will allow to touch the clocktick event

other cores boxes will not allow to open clocktick event, eventhough it's announced via /sys/../events/..

I'm probably missing some HW logic of specific boxes that would explain that, but I can't find it.

The client uncore has a standalone clocktick fixed counter. It doesn't belong to any boxes, which is different from server uncore.

But client and server uncore share the same uncore_pmu_event_init. So it forces that only the first box can access the fixed counter.

Maybe we should create a clocktick box for client uncore to fix it.

Quoted 9 lines
Quoted 5 lines

You can find the fixed counter information from 18.11.6 in latest SDM (Order Number: 325384-060US).

There should be a Skylake client uncore document published somewhere. But I cannot find it from Google. Let me ask around.

Here is the published document.
http://www.intel.com/content/www/us/en/processors/core/6th-gen-core-family-uncore-performance-monitoring-manual.html

will check, thanks a lot!

jirka