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

Groups > linux.kernel > #1540357 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [RFC] perf/x86/intel/uncore: pmu->type->single_fixed question "Liang, Kan" <kan.liang@intel.com> - 2016-12-12 15:50 +0100
    Re: [RFC] perf/x86/intel/uncore: pmu->type->single_fixed question Jiri Olsa <jolsa@redhat.com> - 2016-12-14 13:00 +0100
      RE: [RFC] perf/x86/intel/uncore: pmu->type->single_fixed question "Liang, Kan" <kan.liang@intel.com> - 2016-12-20 19:10 +0100

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

> 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 <kan.liang@intel.com>
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 <kan.liang@intel.com>
---
 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 dbaaf7dc..03afeca 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -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;
diff --git a/arch/x86/events/intel/uncore_snb.c b/arch/x86/events/intel/uncore_snb.c
index a3dcc12..1b037ea 100644
--- a/arch/x86/events/intel/uncore_snb.c
+++ b/arch/x86/events/intel/uncore_snb.c
@@ -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,
 };
 
-- 
2.4.3

[toc] | [next] | [standalone]

#1541869

On Mon, Dec 12, 2016 at 02:49:03PM +0000, Liang, Kan wrote:
> 
> 
> > 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 <kan.liang@intel.com>
> 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 <kan.liang@intel.com>

seems ok

Tested-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

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

#1545339

> On Mon, Dec 12, 2016 at 02:49:03PM +0000, Liang, Kan wrote:
> >
> >
> > > 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 <kan.liang@intel.com>
> > 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 <kan.liang@intel.com>
> 
> seems ok
> 
> Tested-by: Jiri Olsa <jolsa@redhat.com>
> 

Hi Peter and Ingo,

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

Thanks,
Kan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web