[PATCH v2] perf: honoring the architectural performance monitoring version

[PATCH v2] perf: honoring the architectural performance monitoring version

[Imre Palik]

From: "Palik, Imre" <imrep@amazon.de>

Architectural performance monitoring version 1 doesn't support fixed counters.
Currently, even if a hypervisor advertises support for architectural
performance monitoring version 1, perf may still tries to use the fixed
counters, as the constraints are set up based on the CPU model.

This patch ensures that perf honors the architectural performance monitoring
version returned by CPUID, and it only uses the fixed counters for version two
and above.

Some of the ideas in this patch are coming from Peter Zijlstra.

Signed-off-by: Imre Palik <imrep@amazon.de>
Cc: Anthony Liguori <aliguori@amazon.com>
---
 arch/x86/kernel/cpu/perf_event_intel.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index d4c0a0e..2917248 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -2604,13 +2604,13 @@ __init int intel_pmu_init(void)
 		 * counter, so do not extend mask to generic counters
 		 */
 		for_each_event_constraint(c, x86_pmu.event_constraints) {
-			if (c->cmask != FIXED_EVENT_FLAGS
-			    || c->idxmsk64 == INTEL_PMC_MSK_FIXED_REF_CYCLES) {
-				continue;
+			if (c->cmask == FIXED_EVENT_FLAGS
+			    && c->idxmsk64 != INTEL_PMC_MSK_FIXED_REF_CYCLES) {
+				c->idxmsk64 |= (1ULL << x86_pmu.num_counters) - 1;
 			}
-
-			c->idxmsk64 |= (1ULL << x86_pmu.num_counters) - 1;
-			c->weight += x86_pmu.num_counters;
+			c->idxmsk64 &=
+				~(~0UL << (INTEL_PMC_IDX_FIXED + x86_pmu.num_counters_fixed));
+			c->weight = hweight64(c->idxmsk64);
 		}
 	}
 
-- 
1.7.9.5

[tip:perf/urgent] perf/x86: Honor the architectural performance monitoring version

[tip-bot for Palik, Imre]

Commit-ID:  2c33645d366d13b969d936b68b9f4875b1fdddea
Gitweb:     http://git.kernel.org/tip/2c33645d366d13b969d936b68b9f4875b1fdddea
Author:     Palik, Imre <imrep@amazon.de>
AuthorDate: Mon, 8 Jun 2015 14:46:49 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 19 Jun 2015 09:38:48 +0200

perf/x86: Honor the architectural performance monitoring version

Architectural performance monitoring, version 1, doesn't support fixed counters.

Currently, even if a hypervisor advertises support for architectural
performance monitoring version 1, perf may still try to use the fixed
counters, as the constraints are set up based on the CPU model.

This patch ensures that perf honors the architectural performance monitoring
version returned by CPUID, and it only uses the fixed counters for version 2
and above.

(Some of the ideas in this patch came from Peter Zijlstra.)

Signed-off-by: Imre Palik <imrep@amazon.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Anthony Liguori <aliguori@amazon.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1433767609-1039-1-git-send-email-imrep.amz@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/perf_event_intel.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index afdd7c0..2813ea0 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -3324,13 +3324,13 @@ __init int intel_pmu_init(void)
 		 * counter, so do not extend mask to generic counters
 		 */
 		for_each_event_constraint(c, x86_pmu.event_constraints) {
-			if (c->cmask != FIXED_EVENT_FLAGS
-			    || c->idxmsk64 == INTEL_PMC_MSK_FIXED_REF_CYCLES) {
-				continue;
+			if (c->cmask == FIXED_EVENT_FLAGS
+			    && c->idxmsk64 != INTEL_PMC_MSK_FIXED_REF_CYCLES) {
+				c->idxmsk64 |= (1ULL << x86_pmu.num_counters) - 1;
 			}
-
-			c->idxmsk64 |= (1ULL << x86_pmu.num_counters) - 1;
-			c->weight += x86_pmu.num_counters;
+			c->idxmsk64 &=
+				~(~0UL << (INTEL_PMC_IDX_FIXED + x86_pmu.num_counters_fixed));
+			c->weight = hweight64(c->idxmsk64);
 		}
 	}
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] perf: honoring the architectural performance monitoring version

[Imre Palik]

From: "Palik, Imre" <imrep@amazon.de>

Architectural performance monitoring version 1 doesn't support fixed
counters.  Currently, even if a hypervisor advertises support for
architectural performance monitoring version 1, perf may still tries to use
the fixed counters, as the constraints are set up based on the CPU model.

This patch ensures that perf honors the architectural performance
monitoring version returned by CPUID, and it only uses the fixed counters
for version two and above.

Some of the ideas in this patch are coming from Peter Zijlstra.

Signed-off-by: Imre Palik <imrep@amazon.de>
Cc: Anthony Liguori <aliguori@amazon.com>
---
 arch/x86/kernel/cpu/perf_event_intel.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index 3998131..bde66aa 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -1870,7 +1870,7 @@ x86_get_event_constraints(struct cpu_hw_events *cpuc, int idx,
 		for_each_event_constraint(c, x86_pmu.event_constraints) {
 			if ((event->hw.config & c->cmask) == c->code) {
 				event->hw.flags |= c->flags;
-				return c;
+				return  c->idxmsk64 ? c : NULL;
 			}
 		}
 	}
@@ -3341,9 +3341,12 @@ __init int intel_pmu_init(void)
 		for_each_event_constraint(c, x86_pmu.event_constraints) {
 			if (c->cmask != FIXED_EVENT_FLAGS
 			    || c->idxmsk64 == INTEL_PMC_MSK_FIXED_REF_CYCLES) {
+				c->idxmsk64 &=
+					~(~0UL << (INTEL_PMC_IDX_FIXED + x86_pmu.num_counters_fixed));
 				continue;
 			}
-
+			c->idxmsk64 &=
+				~(~0UL << (INTEL_PMC_IDX_FIXED + x86_pmu.num_counters_fixed));
 			c->idxmsk64 |= (1ULL << x86_pmu.num_counters) - 1;
 			c->weight += x86_pmu.num_counters;
 		}
-- 
1.7.9.5

Re: [PATCH v2] perf: honoring the architectural performance monitoring version

[Peter Zijlstra]

On Fri, Jun 05, 2015 at 05:28:45PM +0200, Imre Palik wrote:
> From: "Palik, Imre" <imrep@amazon.de>
> 
> Architectural performance monitoring version 1 doesn't support fixed
> counters.  Currently, even if a hypervisor advertises support for
> architectural performance monitoring version 1, perf may still tries to use
> the fixed counters, as the constraints are set up based on the CPU model.
> 
> This patch ensures that perf honors the architectural performance
> monitoring version returned by CPUID, and it only uses the fixed counters
> for version two and above.
> 
> Some of the ideas in this patch are coming from Peter Zijlstra.
> 
> Signed-off-by: Imre Palik <imrep@amazon.de>
> Cc: Anthony Liguori <aliguori@amazon.com>
> ---
>  arch/x86/kernel/cpu/perf_event_intel.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
> index 3998131..bde66aa 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -1870,7 +1870,7 @@ x86_get_event_constraints(struct cpu_hw_events *cpuc, int idx,
>  		for_each_event_constraint(c, x86_pmu.event_constraints) {
>  			if ((event->hw.config & c->cmask) == c->code) {
>  				event->hw.flags |= c->flags;
> -				return c;
> +				return  c->idxmsk64 ? c : NULL;

One too many spaces there :-) Returning c as found, even with empty
idxmsk is fine.

Also, I think this is broken, I think we hard assume
x86_get_event_constraints() returns a valid constraint, see for example:

	x86_schedule_event():

		c = x86_pmu.get_event_constraints()
			= intel_get_event_constraints()
				 = __intel_get_event_constraints()
					 = x86_get_event_constraints();

		cpuc->event_constraint[i] = c;

		...

		c = cpuc->event_constraint[i];

		if (!test_bit(hwc->idx, c->idxmask)) <-- *boom*


> @@ -3341,9 +3341,12 @@ __init int intel_pmu_init(void)
>  		for_each_event_constraint(c, x86_pmu.event_constraints) {
>  			if (c->cmask != FIXED_EVENT_FLAGS
>  			    || c->idxmsk64 == INTEL_PMC_MSK_FIXED_REF_CYCLES) {
> +				c->idxmsk64 &=
> +					~(~0UL << (INTEL_PMC_IDX_FIXED + x86_pmu.num_counters_fixed));

If you change idxmsk64 you also need to update weight.

>  				continue;
>  			}
> -
> +			c->idxmsk64 &=
> +				~(~0UL << (INTEL_PMC_IDX_FIXED + x86_pmu.num_counters_fixed));
>  			c->idxmsk64 |= (1ULL << x86_pmu.num_counters) - 1;
>  			c->weight += x86_pmu.num_counters;

And since we're now not unconditionally adding num_counters bits, that
weight update is broken.

For both sites, something like:

		c->weight = hweight64(c->idxmsk64);

Will recompute the weight.

Thanks!