[PATCHv2 1/1] perf/x86/intel: make error messages less confusing

[PATCHv2 1/1] perf/x86/intel: make error messages less confusing

[Eduardo Valentin]

On a system with X86_FEATURE_ARCH_PERFMON disabled
and with a model not known by family PMU drivers,
user gets a kernel message log like the following:
[ 0.100114] Performance Events: unsupported p6 CPU model 85 no PMU driver, software events only.

The "unsupported .. CPU" part may be confusing for some
users leading to wrong understanding that the kernel
does not support the CPU model.

This patch rewords the messages on the failure path to:
[ 0.667154] Performance Events: CPU does not support PMU: no PMU driver, software events only.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Eduardo Valentin <eduval@amazon.com>
Cc: Jia Zhang <qianyue.zj@alibaba-inc.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <eduval@amazon.com>
---

Changes from V1->V2:
- As per initial review, the propose messaging was even
more confusing. Simplified it by only saying that
the CPU does not support PMU.


 arch/x86/events/intel/core.c | 15 +++++++++++----
 arch/x86/events/intel/p4.c   |  5 +----
 arch/x86/events/intel/p6.c   |  1 -
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index 035c37481f57..2ddb97f03f4a 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3889,15 +3889,22 @@ __init int intel_pmu_init(void)
 	char *name;
 
 	if (!cpu_has(&boot_cpu_data, X86_FEATURE_ARCH_PERFMON)) {
+		int ret = -ENODEV;
+
 		switch (boot_cpu_data.x86) {
 		case 0x6:
-			return p6_pmu_init();
+			ret = p6_pmu_init();
+			break;
 		case 0xb:
-			return knc_pmu_init();
+			ret = knc_pmu_init();
+			break;
 		case 0xf:
-			return p4_pmu_init();
+			ret = p4_pmu_init();
+			break;
 		}
-		return -ENODEV;
+		if (ret)
+			pr_cont("CPU does not support PMU: ");
+		return ret;
 	}
 
 	/*
diff --git a/arch/x86/events/intel/p4.c b/arch/x86/events/intel/p4.c
index d32c0eed38ca..fb5e8576d9ac 100644
--- a/arch/x86/events/intel/p4.c
+++ b/arch/x86/events/intel/p4.c
@@ -1345,11 +1345,8 @@ __init int p4_pmu_init(void)
 	BUILD_BUG_ON(ARCH_P4_MAX_CCCR > INTEL_PMC_MAX_GENERIC);
 
 	rdmsr(MSR_IA32_MISC_ENABLE, low, high);
-	if (!(low & (1 << 7))) {
-		pr_cont("unsupported Netburst CPU model %d ",
-			boot_cpu_data.x86_model);
+	if (!(low & (1 << 7)))
 		return -ENODEV;
-	}
 
 	memcpy(hw_cache_event_ids, p4_hw_cache_event_ids,
 		sizeof(hw_cache_event_ids));
diff --git a/arch/x86/events/intel/p6.c b/arch/x86/events/intel/p6.c
index 408879b0c0d4..e8e03e68b22f 100644
--- a/arch/x86/events/intel/p6.c
+++ b/arch/x86/events/intel/p6.c
@@ -269,7 +269,6 @@ __init int p6_pmu_init(void)
 		break;
 
 	default:
-		pr_cont("unsupported p6 CPU model %d ", boot_cpu_data.x86_model);
 		return -ENODEV;
 	}
 
-- 
2.18.0

Re: [PATCHv2 1/1] perf/x86/intel: make error messages less confusing

[Eduardo Valentin]

Hello,

On Thu, Aug 23, 2018 at 08:07:32AM -0700, Eduardo Valentin wrote:
> On a system with X86_FEATURE_ARCH_PERFMON disabled
> and with a model not known by family PMU drivers,
> user gets a kernel message log like the following:
> [ 0.100114] Performance Events: unsupported p6 CPU model 85 no PMU driver, software events only.
> 
> The "unsupported .. CPU" part may be confusing for some
> users leading to wrong understanding that the kernel
> does not support the CPU model.
> 
> This patch rewords the messages on the failure path to:
> [ 0.667154] Performance Events: CPU does not support PMU: no PMU driver, software events only.
> 


Gentle ping on this patch, any further comments?

> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Eduardo Valentin <eduval@amazon.com>
> Cc: Jia Zhang <qianyue.zj@alibaba-inc.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Eduardo Valentin <eduval@amazon.com>
> ---
> 
> Changes from V1->V2:
> - As per initial review, the propose messaging was even
> more confusing. Simplified it by only saying that
> the CPU does not support PMU.
> 
> 
>  arch/x86/events/intel/core.c | 15 +++++++++++----
>  arch/x86/events/intel/p4.c   |  5 +----
>  arch/x86/events/intel/p6.c   |  1 -
>  3 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 035c37481f57..2ddb97f03f4a 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -3889,15 +3889,22 @@ __init int intel_pmu_init(void)
>  	char *name;
>  
>  	if (!cpu_has(&boot_cpu_data, X86_FEATURE_ARCH_PERFMON)) {
> +		int ret = -ENODEV;
> +
>  		switch (boot_cpu_data.x86) {
>  		case 0x6:
> -			return p6_pmu_init();
> +			ret = p6_pmu_init();
> +			break;
>  		case 0xb:
> -			return knc_pmu_init();
> +			ret = knc_pmu_init();
> +			break;
>  		case 0xf:
> -			return p4_pmu_init();
> +			ret = p4_pmu_init();
> +			break;
>  		}
> -		return -ENODEV;
> +		if (ret)
> +			pr_cont("CPU does not support PMU: ");
> +		return ret;
>  	}
>  
>  	/*
> diff --git a/arch/x86/events/intel/p4.c b/arch/x86/events/intel/p4.c
> index d32c0eed38ca..fb5e8576d9ac 100644
> --- a/arch/x86/events/intel/p4.c
> +++ b/arch/x86/events/intel/p4.c
> @@ -1345,11 +1345,8 @@ __init int p4_pmu_init(void)
>  	BUILD_BUG_ON(ARCH_P4_MAX_CCCR > INTEL_PMC_MAX_GENERIC);
>  
>  	rdmsr(MSR_IA32_MISC_ENABLE, low, high);
> -	if (!(low & (1 << 7))) {
> -		pr_cont("unsupported Netburst CPU model %d ",
> -			boot_cpu_data.x86_model);
> +	if (!(low & (1 << 7)))
>  		return -ENODEV;
> -	}
>  
>  	memcpy(hw_cache_event_ids, p4_hw_cache_event_ids,
>  		sizeof(hw_cache_event_ids));
> diff --git a/arch/x86/events/intel/p6.c b/arch/x86/events/intel/p6.c
> index 408879b0c0d4..e8e03e68b22f 100644
> --- a/arch/x86/events/intel/p6.c
> +++ b/arch/x86/events/intel/p6.c
> @@ -269,7 +269,6 @@ __init int p6_pmu_init(void)
>  		break;
>  
>  	default:
> -		pr_cont("unsupported p6 CPU model %d ", boot_cpu_data.x86_model);
>  		return -ENODEV;
>  	}
>  
> -- 
> 2.18.0
> 

-- 
All the best,
Eduardo Valentin

Re: [PATCHv2 1/1] perf/x86/intel: make error messages less confusing

[Andi Kleen]

On Tue, Sep 04, 2018 at 11:23:12AM -0700, Eduardo Valentin wrote:
> Hello,
> 
> On Thu, Aug 23, 2018 at 08:07:32AM -0700, Eduardo Valentin wrote:
> > On a system with X86_FEATURE_ARCH_PERFMON disabled
> > and with a model not known by family PMU drivers,
> > user gets a kernel message log like the following:
> > [ 0.100114] Performance Events: unsupported p6 CPU model 85 no PMU driver, software events only.
> > 
> > The "unsupported .. CPU" part may be confusing for some
> > users leading to wrong understanding that the kernel
> > does not support the CPU model.
> > 
> > This patch rewords the messages on the failure path to:
> > [ 0.667154] Performance Events: CPU does not support PMU: no PMU driver, software events only.
> > 
> 
> 
> Gentle ping on this patch, any further comments?

Reviewed-by: Andi Kleen <ak@linux.intel.com>

-Andi

Re: [PATCHv2 1/1] perf/x86/intel: make error messages less confusing

[Peter Zijlstra]

On Thu, Aug 23, 2018 at 08:07:32AM -0700, Eduardo Valentin wrote:
> On a system with X86_FEATURE_ARCH_PERFMON disabled
> and with a model not known by family PMU drivers,
> user gets a kernel message log like the following:
> [ 0.100114] Performance Events: unsupported p6 CPU model 85 no PMU driver, software events only.
> 
> The "unsupported .. CPU" part may be confusing for some
> users leading to wrong understanding that the kernel
> does not support the CPU model.

Send them back to first grade, such that they might learn to read?

Re: [PATCHv2 1/1] perf/x86/intel: make error messages less confusing

[Eduardo Valentin]

On Wed, Sep 05, 2018 at 10:52:12AM +0200, Peter Zijlstra wrote:
> On Thu, Aug 23, 2018 at 08:07:32AM -0700, Eduardo Valentin wrote:
> > On a system with X86_FEATURE_ARCH_PERFMON disabled
> > and with a model not known by family PMU drivers,
> > user gets a kernel message log like the following:
> > [ 0.100114] Performance Events: unsupported p6 CPU model 85 no PMU driver, software events only.
> > 
> > The "unsupported .. CPU" part may be confusing for some
> > users leading to wrong understanding that the kernel
> > does not support the CPU model.
> 
> Send them back to first grade, such that they might learn to read?
> 

:-)

-- 
All the best,
Eduardo Valentin

Re: [PATCHv2 1/1] perf/x86/intel: make error messages less confusing

[Andi Kleen]

On Wed, Sep 05, 2018 at 08:53:17AM -0700, Eduardo Valentin wrote:
> On Wed, Sep 05, 2018 at 10:52:12AM +0200, Peter Zijlstra wrote:
> > On Thu, Aug 23, 2018 at 08:07:32AM -0700, Eduardo Valentin wrote:
> > > On a system with X86_FEATURE_ARCH_PERFMON disabled
> > > and with a model not known by family PMU drivers,
> > > user gets a kernel message log like the following:
> > > [ 0.100114] Performance Events: unsupported p6 CPU model 85 no PMU driver, software events only.
> > > 
> > > The "unsupported .. CPU" part may be confusing for some
> > > users leading to wrong understanding that the kernel
> > > does not support the CPU model.
> > 
> > Send them back to first grade, such that they might learn to read?
> > 
> 
> :-)

I think it's a valid concern, I guess Eduardo actually has real people
who got confused.

-Andi

Re: [PATCHv2 1/1] perf/x86/intel: make error messages less confusing

[Peter Zijlstra]

On Wed, Sep 05, 2018 at 02:47:07PM -0700, Andi Kleen wrote:
> On Wed, Sep 05, 2018 at 08:53:17AM -0700, Eduardo Valentin wrote:
> > On Wed, Sep 05, 2018 at 10:52:12AM +0200, Peter Zijlstra wrote:
> > > On Thu, Aug 23, 2018 at 08:07:32AM -0700, Eduardo Valentin wrote:
> > > > On a system with X86_FEATURE_ARCH_PERFMON disabled
> > > > and with a model not known by family PMU drivers,
> > > > user gets a kernel message log like the following:
> > > > [ 0.100114] Performance Events: unsupported p6 CPU model 85 no PMU driver, software events only.
> > > > 
> > > > The "unsupported .. CPU" part may be confusing for some
> > > > users leading to wrong understanding that the kernel
> > > > does not support the CPU model.
> > > 
> > > Send them back to first grade, such that they might learn to read?
> > > 
> > 
> > :-)
> 
> I think it's a valid concern, I guess Eduardo actually has real people
> who got confused.

But it is really easy to confuse real people; as real people are mostly
clueless.  There is only so much you can do for the semi illiterate
masses. Should we dumb down everything to baby talk just to cater to
them?

The string is clearly prefixed by the subsystem, if you get confused by
that your reading comprehension really is rock bottom.

  [ 0.100114] Performance Events: unsupported p6 CPU model 85 no PMU driver, software events only.

Heck, it even mentions "no PMU driver", how much clues do you need?
Also, the proposed alternative:

  [ 0.667154] Performance Events: CPU does not support PMU: no PMU driver, software events only.

Looses out information on which CPU family we failed on. Nor does it
mention the most likely reason for this error: virt crap.

I'd not mind a warning like:

  [] Performance Events: Your crappy virt solution is lying about it's CPU model, it doesn't have a (matching) PMU.

Re: [PATCHv2 1/1] perf/x86/intel: make error messages less confusing

[Eduardo Valentin]

Hey Peter,

On Thu, Sep 06, 2018 at 09:21:49AM +0200, Peter Zijlstra wrote:
> On Wed, Sep 05, 2018 at 02:47:07PM -0700, Andi Kleen wrote:
> > On Wed, Sep 05, 2018 at 08:53:17AM -0700, Eduardo Valentin wrote:
> > > On Wed, Sep 05, 2018 at 10:52:12AM +0200, Peter Zijlstra wrote:
> > > > On Thu, Aug 23, 2018 at 08:07:32AM -0700, Eduardo Valentin wrote:
> > > > > On a system with X86_FEATURE_ARCH_PERFMON disabled
> > > > > and with a model not known by family PMU drivers,
> > > > > user gets a kernel message log like the following:
> > > > > [ 0.100114] Performance Events: unsupported p6 CPU model 85 no PMU driver, software events only.
> > > > > 
> > > > > The "unsupported .. CPU" part may be confusing for some
> > > > > users leading to wrong understanding that the kernel
> > > > > does not support the CPU model.
> > > > 
> > > > Send them back to first grade, such that they might learn to read?
> > > > 
> > > 
> > > :-)
> > 
> > I think it's a valid concern, I guess Eduardo actually has real people
> > who got confused.
> 
> But it is really easy to confuse real people; as real people are mostly
> clueless.  There is only so much you can do for the semi illiterate
> masses. Should we dumb down everything to baby talk just to cater to
> them?
> 
> The string is clearly prefixed by the subsystem, if you get confused by
> that your reading comprehension really is rock bottom.
> 
>   [ 0.100114] Performance Events: unsupported p6 CPU model 85 no PMU driver, software events only.
> 

Once again, the confusing part is the "unsupported CPU".

> Heck, it even mentions "no PMU driver", how much clues do you need?
> Also, the proposed alternative:
> 
>   [ 0.667154] Performance Events: CPU does not support PMU: no PMU driver, software events only.
> 
> Looses out information on which CPU family we failed on. Nor does it


Maybe keeping the CPU family and rephrasing the unsupported part?

> mention the most likely reason for this error: virt crap.
> 
> I'd not mind a warning like:
> 
>   [] Performance Events: Your crappy virt solution is lying about it's CPU model, it doesn't have a (matching) PMU.
> 

Well, I would be OK if the kernel spits out a message saying that vPMU is disabled or something.
That would be more accurate than unsupported CPU.

> 
> 

-- 
All the best,
Eduardo Valentin