Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1533220 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC] perf/x86/intel/uncore: pmu->type->single_fixed question Jiri Olsa <jolsa@redhat.com> - 2016-11-30 12:00 +0100
    RE: [RFC] perf/x86/intel/uncore: pmu->type->single_fixed question "Liang, Kan" <kan.liang@intel.com> - 2016-11-30 15:50 +0100
      Re: [RFC] perf/x86/intel/uncore: pmu->type->single_fixed question Andi Kleen <andi@firstfloor.org> - 2016-11-30 19:00 +0100
        Re: [RFC] perf/x86/intel/uncore: pmu->type->single_fixed question Peter Zijlstra <peterz@infradead.org> - 2016-12-01 17:40 +0100
        RE: [RFC] perf/x86/intel/uncore: pmu->type->single_fixed question "Liang, Kan" <kan.liang@intel.com> - 2016-12-01 17:40 +0100
    RE: [RFC] perf/x86/intel/uncore: pmu->type->single_fixed question "Liang, Kan" <kan.liang@intel.com> - 2016-11-30 16:30 +0100
      Re: [RFC] perf/x86/intel/uncore: pmu->type->single_fixed question Jiri Olsa <jolsa@redhat.com> - 2016-11-30 16:40 +0100

#1533220 — [RFC] perf/x86/intel/uncore: pmu->type->single_fixed question

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

[toc] | [next] | [standalone]

#1533362

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

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

[toc] | [prev] | [next] | [standalone]

#1533474

"Liang, Kan" <kan.liang@intel.com> writes:
>
> 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

[toc] | [prev] | [next] | [standalone]

#1534265

On Thu, Dec 01, 2016 at 04:34:43PM +0000, Liang, Kan wrote:
> 
> > 
> > "Liang, Kan" <kan.liang@intel.com> writes:
> > >
> > > 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?

Urgh, more lines for ugly :-(

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

[toc] | [prev] | [next] | [standalone]

#1534275

> 
> "Liang, Kan" <kan.liang@intel.com> writes:
> >
> > 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?

------
From 844c12b5e6fc1e2c27b2f094c89098ea9405ea41 Mon Sep 17 00:00:00 2001
From: Kan Liang <kan.liang@intel.com>
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 <kan.liang@intel.com>
---
 arch/x86/events/intel/uncore.c | 70 ++++++++++++++++++++++++++++++++----------
 arch/x86/events/intel/uncore.h |  1 +
 2 files changed, 55 insertions(+), 16 deletions(-)

diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index dbaaf7dc..6d3606e 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -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;
 }
 
diff --git a/arch/x86/events/intel/uncore.h b/arch/x86/events/intel/uncore.h
index ad986c1..23bf3ff 100644
--- a/arch/x86/events/intel/uncore.h
+++ b/arch/x86/events/intel/uncore.h
@@ -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 */
 };
 
-- 
2.5.5

[toc] | [prev] | [next] | [standalone]

#1533384

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

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



> 
> Thanks,
> Kan

[toc] | [prev] | [next] | [standalone]

#1533389

On Wed, Nov 30, 2016 at 03:27:52PM +0000, Liang, Kan wrote:
> 
> > 
> > > 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.
> > 
> > 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.
> 
> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web