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

From: Eduardo Valentin
Date: Tue Sep 04 2018 - 14:33:17 EST


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@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> Cc: x86@xxxxxxxxxx
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> Cc: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Cc: Eduardo Valentin <eduval@xxxxxxxxxx>
> Cc: Jia Zhang <qianyue.zj@xxxxxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: Eduardo Valentin <eduval@xxxxxxxxxx>
> ---
>
> 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