Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
From: James Morse <james.morse@arm.com>
To: Shiju Jose <shiju.jose@huawei.com>
Cc: Borislav Petkov <bp@alien8.de>,
	"linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>,
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"tony.luck@intel.com" <tony.luck@intel.com>,
	"rjw@rjwysocki.net" <rjw@rjwysocki.net>,
	"lenb@kernel.org" <lenb@kernel.org>,
	Linuxarm <linuxarm@huawei.com>,
	Jonathan Cameron <jonathan.cameron@huawei.com>
Subject: Re: [RFC PATCH 0/7] RAS/CEC: Extend CEC for errors count check on short time period
Date: Wed, 7 Oct 2020 17:45:15 +0100
Message-ID: <7d15f48a-3380-6b43-04c7-7f9e67564519@arm.com> (raw)
In-Reply-To: <8d826b53a3fc453ba1c468aaf8eb2e75@huawei.com>

Hi Shiju,

On 06/10/2020 17:13, Shiju Jose wrote:

[...]

> Please find following pseudo code we added for the kernel side to make sure
> we correctly understand your suggestions.
> 
> 1. Create edac device and edac device sysfs entries for the online CPU caches.
> /drivers/edac/edac_device.c
> struct edac_device_ctl_info  *edac_device_add_cache(unsigned int id, u8 level, u8 type) {

Eh? Ah, you are adding helpers for devices that are a cache. As far as I can see, edac
only cares about 'devices', I don't think this is needed unless there are multiple users,
or it makes a visible difference to user-space.

Otherwise this could just go into ghes_edac.c


How this looks to user-space probably needs discussing. We should avoid inventing anything
new. I'd expect user-space to see something like the structure described at the top of
edac_device.h... but I can't spot a driver using this.
(its a shame its not the other way up, to avoid duplicating shared caches)

Some archaeology may be needed!

(if there is some existing structure, I agree it should be wrapped up in helpers to ensure
its easiest to be the same. This may be what a edac_device_block is...)


>  }


> /drivers/base/cacheinfo.c
> int cache_create_edac_entries(u64 mpidr, u8 cache_level, u8 cache_type)
> { 
> 	...
> 	/* Get cacheinfo for each online cpus */
> 	for_each_online_cpu(i) {
> 		struct cpu_cacheinfo *cpu_ci = get_cpu_cacheinfo(i);

I agree the structure of the caches should come from cacheinfo, and you spotted it only
works for online CPUs!. This means there is an interaction with cpuhp here)


> 		if (!cpu_ci || !cpu_ci->id)

cpu->id?  0 is a valid id, there is an attribute flag to say this is valid.
This field exists in struct cacheinfo, not struct cpu_cacheinfo.


> 			continue;
>         		... 
> 		/*Add  the edac entry for the CPU cache */
> 		edev_cache = edac_device_add_cache(cpu_ci->id, cpu_ci ->level, cpu_ci ->type)
> 		if (!edev_cache)
> 			break;

This would break all other edac users.
The edac driver for the platform should take care of creating this stuff, not the core
cacheinfo code.

The edac driver for the platform may know that L2 doesn't report RAS errors, so there is
no point exposing it.

For firmware-first, we can't know this until an error shows up, so have to create
everything. This stuff should only be created/exported when ghes_edac.c is determined to
be this platforms edac driver. This code should live in ghes_edac.c.


> 		...
> 	}
> 	...	
> }

> unsigned int cache_get_cache_id(u64 proc_id, u8 cache_level, u8 cache_type)

See get_cpu_cacheinfo_id(int cpu, int level) in next. (something very similar to this
lived in arch/x86, bits of the MPAM tree that moved it got queued for next)


> { 
> 	unsigned int cache_id = 0;
> 	...
> 	/* Walk looking for matching cache node */   
> 	for_each_online_cpu(i) {

(there is an interaction with cpuhp here)


> 		struct cpu_cacheinfo *cpu_ci = get_cpu_cacheinfo(i);
> 		if (!cpu_ci || !cpu_ci->id)
> 			continue;


> 		id = CONV(proc_id);  /* need to check */

No idea what is going on here.

(Deriving an ID from the CPU_s_ that are attached to the cache is arm64 specific. This has
to work for x86 too.
The MPAM out-of-tree code does this as we don't have anything else. Feedback when it was
posted as RFC was that the id values should be compacted, I was hoping we would get
something like an id from the PPTT before needing this value as resctrl ABI for MPAM)


> 		if((id == cpu_ci->id) && (cache_level == cpu_ci->level) && (cache_type == cpu_ci->type))  {
> 			cache_id = cpu_ci->id;
> 			break;
> 		}
> 	}
> 	return cache_id;
> }


> 2. Store CPU CE count in the edac sysfs entry for the CPU cache.
> 
> drivers/edac/ghes_edac.c
> void ghes_edac_report_cpu_error(int cache_id, u8 cache_level, u8 cache_type , uint32 ce_count)
> {
> 	...
> 	/* Check edac entry for cache already present, if not add new entry */       

You can't create devices at runtime! The notification comes in irq context, and
edac_device_add_device() takes a mutex, and you need to allocate memory.

This could be deferred to process context - but I bet its a nuisance for user-space to now
know what counters are available until errors show up.


> 	edev_cache = find_edac_device_cache(cache_id, cache_level, cache_type);
> 	if (!edev_cache) {
> 		/*Add  the edac entry for the cache */
> 		edev_cache = edac_device_add_cache(cache_id, cache_level, cache_type);
> 		if (!edev_cache)
> 			return;
> 	}

Better would be to lookup the device based on the CPER. (It already looks up the DIMM
based on the CPER)


> 	/* Store the ce_count to /sys/devices/system/edac/ cpu/cpu<no>/L<N>cache/ce_count */
> 	edac_device_handle_ce_count(edev_cache, ce_count, ...)
> }
>  
> drivers/acpi/apei/ghes.c
> void ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata) {
>  	...
>  	if (sec_sev != GHES_SEV_CORRECTED)
>  		return;

(Xiaofei Tan is looking at supporting some other of these, so you need to interact with
that work too)


>  	mpidr = cper_sec_proc_arm->mpidr;    

You want an arch-specific call to convert this to the linux CPU number, and then use that
everywhere. This makes the code more re-usable, and less surprising to other readers.

arm64's get_logical_index() looks like it does what you want.


>  	for(i = 0; i < cper_sec_proc_arm->err_info_num; i++) {
>  		if(cper_sec_proc_info->type != CPER_ARM_CACHE_ERROR) 
>  			continue; 
>  		ce_count = cper_arm_err_info->multiple_error + 1;
> 		cache_type = cper_arm_err_info->type;
> 		cache_level = cper_arm_err_info->error_info<24: 22>;  

> 		cache_id = cache_get_cache_id(mpidr, cache_level, cache_type);

This needs to be architecture agnostic, it must take the cpu number.


>  		if (!cache_id)
>  			return;

0 is a valid id.


> 		ghes_edac_report_cpu_error(cache_id, cache_level, cache_type , ce_count);
> 	}
>               ...
> 	return;	
> }


Thanks,

James

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-02 12:22 Shiju Jose
2020-10-02 12:22 ` [RFC PATCH 1/7] RAS/CEC: Replace the macro PFN with ELEM_NO Shiju Jose
2020-10-02 12:22 ` [RFC PATCH 2/7] RAS/CEC: Replace pfns_poisoned with elems_poisoned Shiju Jose
2020-10-02 12:22 ` [RFC PATCH 3/7] RAS/CEC: Move X86 MCE specific code under CONFIG_X86_MCE Shiju Jose
2020-10-02 12:22 ` [RFC PATCH 4/7] RAS/CEC: Modify cec_mod_work() for common use Shiju Jose
2020-10-02 12:22 ` [RFC PATCH 5/7] RAS/CEC: Add support for errors count check on short time period Shiju Jose
2020-10-02 12:22 ` [RFC PATCH 6/7] RAS/CEC: Add CPU Correctable Error Collector to isolate an erroneous CPU core Shiju Jose
2020-10-02 12:22 ` [RFC PATCH 7/7] ACPI / APEI: Add reporting ARM64 CPU correctable errors to the CEC Shiju Jose
2020-10-02 12:43 ` [RFC PATCH 0/7] RAS/CEC: Extend CEC for errors count check on short time period Borislav Petkov
2020-10-02 15:38   ` Shiju Jose
2020-10-02 17:33     ` James Morse
2020-10-02 18:02       ` Borislav Petkov
2020-10-06 16:13       ` Shiju Jose
2020-10-07 16:45         ` James Morse [this message]
2020-10-02 16:04   ` Luck, Tony

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7d15f48a-3380-6b43-04c7-7f9e67564519@arm.com \
    --to=james.morse@arm.com \
    --cc=bp@alien8.de \
    --cc=jonathan.cameron@huawei.com \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=rjw@rjwysocki.net \
    --cc=shiju.jose@huawei.com \
    --cc=tony.luck@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-EDAC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-edac/0 linux-edac/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-edac linux-edac/ https://lore.kernel.org/linux-edac \
		linux-edac@vger.kernel.org
	public-inbox-index linux-edac

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-edac


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git