Re: [RFC PATCH 1/2] EDAC/ghes: Add EDAC device for the CPU caches

[Borislav Petkov]

On Tue, Dec 08, 2020 at 05:29:58PM +0000, Shiju Jose wrote:
> The corrected error count on the CPU caches required
> reporting to the user-space for the predictive failure
> analysis. For this purpose, add an EDAC device and device
> blocks for the CPU caches found.
> The cache's corrected error count would be stored in the
> /sys/devices/system/edac/cpu/cpu*/cache*/ce_count.

This still doesn't begin to explain why the kernel needs this. I had
already asked whether errors in CPU caches are something which happen
often enough so that software should count them but nothing came. So pls
justify first why this wants to be added to the kernel.

> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 7a47680d6f07..c73eeab27ac9 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -74,6 +74,16 @@ config EDAC_GHES
>  
>  	  In doubt, say 'Y'.
>  
> +config EDAC_GHES_CPU_ERROR
> +	bool "EDAC device for reporting firmware-first BIOS detected CPU error count"

Why a separate Kconfig item?

> +	depends on EDAC_GHES
> +	help
> +	  EDAC device for the firmware-first BIOS detected CPU error count reported

Well this is not what it is doing - you're talking about cache errors.
"CPU errors" can be a lot more than just cache errors.

> +static void ghes_edac_create_cpu_device(struct device *dev)
> +{
> +	int cpu;
> +	struct cpu_cacheinfo *this_cpu_ci;
> +
> +	/*
> +	 * Find the maximum number of caches present in the CPU heirarchy
> +	 * among the online CPUs.
> +	 */
> +	for_each_online_cpu(cpu) {
> +		this_cpu_ci = get_cpu_cacheinfo(cpu);
> +		if (!this_cpu_ci)
> +			continue;
> +		if (max_number_of_caches < this_cpu_ci->num_leaves)
> +			max_number_of_caches = this_cpu_ci->num_leaves;

So this is counting the number of cache levels on the system? So you
want to count the errors per cache levels?

> +	}
> +	if (!max_number_of_caches)
> +		return;
> +
> +	/*
> +	 * EDAC device interface only supports creating the CPU cache hierarchy for alls
> +	 * the CPUs together. Thus need to allocate cpu_edac_block_list for the
> +	 * max_number_of_caches among all the CPUs irrespective of the number of caches
> +	 * per CPU might vary.
> +	 */

So this is lumping all the caches together into a single list? What for?
To untangle to the proper ones when the error gets reported?

Have you heard of percpu variables?

> @@ -624,6 +787,10 @@ int ghes_edac_register(struct ghes *ghes, struct device *dev)
>  	ghes_pvt = pvt;
>  	spin_unlock_irqrestore(&ghes_lock, flags);
>  
> +#if defined(CONFIG_EDAC_GHES_CPU_ERROR)
> +	ghes_edac_create_cpu_device(dev);
> +#endif
> +

Init stuff belongs into ghes_scan_system().

...

Ok, I've seen enough. "required reporting to the user-space for the
predictive failure analysis." is not even trying to explain *why* you're
doing this, what *actual* problem it is addressing and why should the
kernel get it.

And without a proper problem definition of what you're trying to solve,
this is not going anywhere.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette