[RFC PATCH] perf: honoring cpuid for number of fixed counters

[RFC PATCH] perf: honoring cpuid for number of fixed counters

[Imre Palik]

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

perf doesn't seem to honor the number of fixed counters specified by cpuid
leaf 0xa.  It always assume that intel CPUs have at least 3 fixed counters.

So if some of the fixed counters are masked out by the hypervisor, it still
tries to check/set them.  This is good for testing the masking code in the
hypervisor, but not so nice otherwise.

This patch makes perf pehave somewhat nicer when the number of fixed
counters is less than three.

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

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 87848eb..eaa0b5e 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -647,6 +647,8 @@ static void perf_sched_init(struct perf_sched *sched, struct perf_event **events
 	sched->state.event	= idx;		/* start with min weight */
 	sched->state.weight	= wmin;
 	sched->state.unassigned	= num;
+	sched->state.used[0]    =
+		~0UL << (INTEL_PMC_IDX_FIXED + x86_pmu.num_counters_fixed);
 }
 
 static void perf_sched_save_state(struct perf_sched *sched)
diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
index 3998131..60beb98 100644
--- a/arch/x86/kernel/cpu/perf_event_intel.c
+++ b/arch/x86/kernel/cpu/perf_event_intel.c
@@ -3042,13 +3042,6 @@ __init int intel_pmu_init(void)
 
 	x86_pmu.max_pebs_events		= min_t(unsigned, MAX_PEBS_EVENTS, x86_pmu.num_counters);
 
-	/*
-	 * Quirk: v2 perfmon does not report fixed-purpose events, so
-	 * assume at least 3 events:
-	 */
-	if (version > 1)
-		x86_pmu.num_counters_fixed = max((int)edx.split.num_counters_fixed, 3);
-
 	if (boot_cpu_has(X86_FEATURE_PDCM)) {
 		u64 capabilities;
 
-- 
1.7.9.5

Re: [RFC PATCH] perf: honoring cpuid for number of fixed counters

[Peter Zijlstra]

On Wed, Jun 03, 2015 at 10:03:48AM +0200, Imre Palik wrote:
> From: "Palik, Imre" <imrep@amazon.de>
> 
> perf doesn't seem to honor the number of fixed counters specified by cpuid
> leaf 0xa.  It always assume that intel CPUs have at least 3 fixed counters.
> 
> So if some of the fixed counters are masked out by the hypervisor, it still
> tries to check/set them.  This is good for testing the masking code in the
> hypervisor, but not so nice otherwise.
> 
> This patch makes perf pehave somewhat nicer when the number of fixed
> counters is less than three.

> @@ -3042,13 +3042,6 @@ __init int intel_pmu_init(void)
>  
>  	x86_pmu.max_pebs_events		= min_t(unsigned, MAX_PEBS_EVENTS, x86_pmu.num_counters);
>  
> -	/*
> -	 * Quirk: v2 perfmon does not report fixed-purpose events, so
> -	 * assume at least 3 events:
> -	 */
> -	if (version > 1)
> -		x86_pmu.num_counters_fixed = max((int)edx.split.num_counters_fixed, 3);
> -
>  	if (boot_cpu_has(X86_FEATURE_PDCM)) {
>  		u64 capabilities;

So the problem is that there is real hardware out there that gets the
CPUID stuff wrong, and this patch penalizes that by then not using the
fixed counters.

Further, the Intel Arch PerfMon v2 spec actually specifies there to be 3
fixed function counters.

So anything that says it is v2+ and does not have the 3, is non
compliant.

I would suggest you go fix your hypervisor.

Lacking that option; you could probe the MSRs to see if they're really
there using wrmsr_safe() or something like that -- see
check_hw_exists().

Re: [RFC PATCH] perf: honoring cpuid for number of fixed counters

[Imre Palik]

On 06/03/15 10:36, Peter Zijlstra wrote:
> On Wed, Jun 03, 2015 at 10:03:48AM +0200, Imre Palik wrote:
>> From: "Palik, Imre" <imrep@amazon.de>
>>
>> perf doesn't seem to honor the number of fixed counters specified by cpuid
>> leaf 0xa.  It always assume that intel CPUs have at least 3 fixed counters.
>>
>> So if some of the fixed counters are masked out by the hypervisor, it still
>> tries to check/set them.  This is good for testing the masking code in the
>> hypervisor, but not so nice otherwise.
>>
>> This patch makes perf pehave somewhat nicer when the number of fixed
>> counters is less than three.
> 
>> @@ -3042,13 +3042,6 @@ __init int intel_pmu_init(void)
>>  
>>  	x86_pmu.max_pebs_events		= min_t(unsigned, MAX_PEBS_EVENTS, x86_pmu.num_counters);
>>  
>> -	/*
>> -	 * Quirk: v2 perfmon does not report fixed-purpose events, so
>> -	 * assume at least 3 events:
>> -	 */
>> -	if (version > 1)
>> -		x86_pmu.num_counters_fixed = max((int)edx.split.num_counters_fixed, 3);
>> -
>>  	if (boot_cpu_has(X86_FEATURE_PDCM)) {
>>  		u64 capabilities;
> 
> So the problem is that there is real hardware out there that gets the
> CPUID stuff wrong, and this patch penalizes that by then not using the
> fixed counters.

I haven't thought about this.  Thanks.

> Further, the Intel Arch PerfMon v2 spec actually specifies there to be 3
> fixed function counters.
> 
> So anything that says it is v2+ and does not have the 3, is non
> compliant.
>
> I would suggest you go fix your hypervisor.

If I set up the hypervisor to advertise Arch PerfMon v1 (0 fixed counters), then  without my patch, perf still tries to use fixed counters.  So something is clearly broken here.

> Lacking that option; you could probe the MSRs to see if they're really
> there using wrmsr_safe() or something like that -- see
> check_hw_exists().

I'll send something along these lines soon.

Re: [RFC PATCH] perf: honoring cpuid for number of fixed counters

[Peter Zijlstra]

On Thu, Jun 04, 2015 at 12:35:08PM +0200, Imre Palik wrote:
> On 06/03/15 10:36, Peter Zijlstra wrote:
> > Further, the Intel Arch PerfMon v2 spec actually specifies there to be 3
> > fixed function counters.
> > 
> > So anything that says it is v2+ and does not have the 3, is non
> > compliant.
> >
> > I would suggest you go fix your hypervisor.
> 
> If I set up the hypervisor to advertise Arch PerfMon v1 (0 fixed
> counters), then  without my patch, perf still tries to use fixed
> counters.  So something is clearly broken here.

So the code you deleted does if (version > 1), and last I checked that
should return false if version == 1.

So please check what's happening there first.

Re: [RFC PATCH] perf: honoring cpuid for number of fixed counters

[Imre Palik]

On 06/04/15 13:49, Peter Zijlstra wrote:
> On Thu, Jun 04, 2015 at 12:35:08PM +0200, Imre Palik wrote:
>> On 06/03/15 10:36, Peter Zijlstra wrote:
>>> Further, the Intel Arch PerfMon v2 spec actually specifies there to be 3
>>> fixed function counters.
>>>
>>> So anything that says it is v2+ and does not have the 3, is non
>>> compliant.
>>>
>>> I would suggest you go fix your hypervisor.
>>
>> If I set up the hypervisor to advertise Arch PerfMon v1 (0 fixed
>> counters), then  without my patch, perf still tries to use fixed
>> counters.  So something is clearly broken here.
> 
> So the code you deleted does if (version > 1), and last I checked that
> should return false if version == 1.
> 
> So please check what's happening there first.

Checked, before sent my last mail.

The trouble is that the number of fixed counters is not taken into account when scheduling the events,
and the cpu model based event constraints will favour fixed counters.  So perf tries to use them.

Re: [RFC PATCH] perf: honoring cpuid for number of fixed counters

[Peter Zijlstra]

On Thu, Jun 04, 2015 at 02:30:37PM +0200, Imre Palik wrote:
> The trouble is that the number of fixed counters is not taken into
> account when scheduling the events, and the cpu model based event
> constraints will favour fixed counters.  So perf tries to use them.

Ah! so that is what your hunk below does. Tricky, and without comment
that.

---

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 87848eb..eaa0b5e 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -647,6 +647,8 @@ static void perf_sched_init(struct perf_sched *sched, struct perf_event **events
        sched->state.event      = idx;          /* start with min weight */
        sched->state.weight     = wmin;
        sched->state.unassigned = num;
+       sched->state.used[0]    =
+               ~0UL << (INTEL_PMC_IDX_FIXED + x86_pmu.num_counters_fixed);
 }

 static void perf_sched_save_state(struct perf_sched *sched)

---

Please change the FIXED_EVENT constraints init instead; that way
validate_event() will actually work too, otherwise it thinks it can
schedule the fixed function only events.

That is, change the below loop from intel_pmu_init():

	if (x86_pmu.event_constraints) {
		/*
		 * event on fixed counter2 (REF_CYCLES) only works on this
		 * 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;
			}

			c->idxmsk64 |= (1ULL << x86_pmu.num_counters) - 1;
			c->weight += x86_pmu.num_counters;
		}
	}

To clear all counters that are not in fact present, that way we keep the
event constraints correct instead of working around invalid constraints.

Re: [RFC PATCH] perf: honoring cpuid for number of fixed counters

[Imre Palik]

On 06/04/15 15:29, Peter Zijlstra wrote:
> On Thu, Jun 04, 2015 at 02:30:37PM +0200, Imre Palik wrote:
>> The trouble is that the number of fixed counters is not taken into
>> account when scheduling the events, and the cpu model based event
>> constraints will favour fixed counters.  So perf tries to use them.
> 
> Ah! so that is what your hunk below does. Tricky, and without comment
> that.
> 
> ---
> 
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index 87848eb..eaa0b5e 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -647,6 +647,8 @@ static void perf_sched_init(struct perf_sched *sched, struct perf_event **events
>         sched->state.event      = idx;          /* start with min weight */
>         sched->state.weight     = wmin;
>         sched->state.unassigned = num;
> +       sched->state.used[0]    =
> +               ~0UL << (INTEL_PMC_IDX_FIXED + x86_pmu.num_counters_fixed);
>  }
> 
>  static void perf_sched_save_state(struct perf_sched *sched)
> 
> ---
> 
> Please change the FIXED_EVENT constraints init instead; that way
> validate_event() will actually work too, otherwise it thinks it can
> schedule the fixed function only events.
> 
> That is, change the below loop from intel_pmu_init():
> 
> 	if (x86_pmu.event_constraints) {
> 		/*
> 		 * event on fixed counter2 (REF_CYCLES) only works on this
> 		 * 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;
> 			}
> 
> 			c->idxmsk64 |= (1ULL << x86_pmu.num_counters) - 1;
> 			c->weight += x86_pmu.num_counters;
> 		}
> 	}
> 
> To clear all counters that are not in fact present, that way we keep the
> event constraints correct instead of working around invalid constraints.

Thanks.

I knew there should be a better way ...

Will post a new patch soon.