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

[Shiju Jose]

Hi Boris,

Thanks for the feedback.
Apologies for the delay.

>-----Original Message-----
>From: Borislav Petkov [mailto:bp@xxxxxxxxx]
>Sent: 31 December 2020 16:44
>To: Shiju Jose <shiju.jose@xxxxxxxxxx>
>Cc: linux-edac@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; james.morse@xxxxxxx;
>mchehab+huawei@xxxxxxxxxx; tony.luck@xxxxxxxxx; rjw@xxxxxxxxxxxxx;
>lenb@xxxxxxxxxx; rrichter@xxxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>;
>xuwei (O) <xuwei5@xxxxxxxxxx>; Jonathan Cameron
><jonathan.cameron@xxxxxxxxxx>; John Garry <john.garry@xxxxxxxxxx>;
>tanxiaofei <tanxiaofei@xxxxxxxxxx>; Shameerali Kolothum Thodi
><shameerali.kolothum.thodi@xxxxxxxxxx>; Salil Mehta
><salil.mehta@xxxxxxxxxx>
>Subject: Re: [RFC PATCH 1/2] EDAC/ghes: Add EDAC device for the CPU
>caches
>
>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.

L2 cache corrected errors are detected occasionally on few of
our ARM64 hardware boards. Though it is rare, the probability of
the CPU cache errors frequently occurring can't be avoided.
The earlier failure detection by monitoring the cache corrected
errors for the frequent occurrences and taking preventive
action could prevent more serious hardware faults.

On Intel architectures, cache corrected errors are reported and
the affected cores are offline in the architecture specific method.
http://www.mcelog.org/cache.html

However for the firmware-first error reporting, specifically on
ARM64 architectures, there is no provision present for reporting
the cache corrected error count to the user-space and taking
preventive action such as offline the affected cores.
>
>> 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?
CONFIG_EDAC_GHES_CPU_CACHE_ERROR is added to make this
feature optional only for the platforms which need this and supported.

>
>> +	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.
Sure. I will change.

>
>> +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?
Yes. This was the suggestion from James and to offline the affected cores for the shared cache.

>
>> +	}
>> +	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?
Yes. Changed the list to the percpu variable.

>
>> @@ -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().
>
Did you mean calling  ghes_edac_create_cpu_device() in the 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.
>

Thanks,
Shiju