Re: [PATCH v5 04/10] ACPI: APEI: GHES: move estatus cache helpers

[Jonathan Cameron]

On Fri, 29 May 2026 10:50:44 +0100
Ahmed Tiba <ahmed.tiba@xxxxxxx> wrote:

> Relocate the estatus cache allocation and lookup helpers from ghes.c into
> ghes_cper.c. This code move keeps the logic intact while making the cache
> implementation available to forthcoming users.
> 
> Signed-off-by: Ahmed Tiba <ahmed.tiba@xxxxxxx>

A couple of minor things inline.

With the two I've called out tidied up
Reviewed-by: Jonathan Cameron <jic23@xxxxxxxxxx>

> diff --git a/drivers/acpi/apei/ghes_cper.c b/drivers/acpi/apei/ghes_cper.c
> index 8080e0f76dac..0a117f478afb 100644
> --- a/drivers/acpi/apei/ghes_cper.c
> +++ b/drivers/acpi/apei/ghes_cper.c
> @@ -13,10 +13,14 @@
>   */
>  
>  #include <linux/err.h>
> +#include <linux/genalloc.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
> +#include <linux/math64.h>
>  #include <linux/mm.h>
>  #include <linux/ratelimit.h>
> +#include <linux/rcupdate.h>
> +#include <linux/sched/clock.h>
>  #include <linux/slab.h>

> +static void ghes_estatus_cache_rcu_free(struct rcu_head *head)
> +{
> +	struct ghes_estatus_cache *cache;
> +	u32 len;
> +
> +	cache = container_of(head, struct ghes_estatus_cache, rcu);
> +	len = cper_estatus_len(GHES_ESTATUS_FROM_CACHE(cache));
> +	len = GHES_ESTATUS_CACHE_LEN(len);
> +	gen_pool_free(ghes_estatus_pool, (unsigned long)cache, len);
> +	atomic_dec(&ghes_estatus_cache_alloced);
> +}
> +
> +void
> +ghes_estatus_cache_add(struct acpi_hest_generic *generic,
> +		       struct acpi_hest_generic_status *estatus)

void ghes_estatus_cache_add(struct acpi_hest_generic *generic,
			    struct acpi_hest_generic_status *estatus)

is under 80 chars (and how you have it in the header!)

(RB assumes you fix this - or argue against perhaps because of a change in
 a future patch)

> +{
> +	unsigned long long now, duration, period, max_period = 0;
> +	struct ghes_estatus_cache *cache, *new_cache;
> +	struct ghes_estatus_cache __rcu *victim;
> +	int i, slot = -1, count;
> +
> +	new_cache = ghes_estatus_cache_alloc(generic, estatus);
> +	if (!new_cache)
> +		return;
> +
> +	rcu_read_lock();
> +	now = sched_clock();
> +	for (i = 0; i < GHES_ESTATUS_CACHES_SIZE; i++) {
> +		cache = rcu_dereference(ghes_estatus_caches[i]);
> +		if (cache == NULL) {
> +			slot = i;
> +			break;
> +		}
> +		duration = now - cache->time_in;
> +		if (duration >= GHES_ESTATUS_IN_CACHE_MAX_NSEC) {
> +			slot = i;
> +			break;
> +		}
> +		count = atomic_read(&cache->count);
> +		period = duration;
> +		do_div(period, (count + 1));
> +		if (period > max_period) {
> +			max_period = period;
> +			slot = i;
> +		}
> +	}
> +	rcu_read_unlock();
> +
> +	if (slot != -1) {
If you even end up doing tidy up of this code, would be nicer to flip
the logic here and do an early return.
	if (slot == -1)
		return;
Then the rest is much less indented.

No need to do that in this series though as nothing 'wrong' with the
current code as such.

> +		/*
> +		 * Use release semantics to ensure that ghes_estatus_cached()
> +		 * running on another CPU will see the updated cache fields if
> +		 * it can see the new value of the pointer.
> +		 */
> +		victim = xchg_release(&ghes_estatus_caches[slot],
> +				      RCU_INITIALIZER(new_cache));
> +
> +		/*
> +		 * At this point, victim may point to a cached item different
> +		 * from the one based on which we selected the slot. Instead of
> +		 * going to the loop again to pick another slot, let's just
> +		 * drop the other item anyway: this may cause a false cache
> +		 * miss later on, but that won't cause any problems.
> +		 */
> +		if (victim)
> +			call_rcu(&unrcu_pointer(victim)->rcu,
> +				 ghes_estatus_cache_rcu_free);
> +	}
> +}
> diff --git a/include/acpi/ghes_cper.h b/include/acpi/ghes_cper.h
> index 6b7632cfaf66..1b5dbeca9bb6 100644
> --- a/include/acpi/ghes_cper.h
> +++ b/include/acpi/ghes_cper.h
> @@ -16,6 +16,7 @@
>  #ifndef ACPI_APEI_GHES_CPER_H
>  #define ACPI_APEI_GHES_CPER_H
>  
> +#include <linux/atomic.h>

Why?  Nothing in in the types used in what is added to the header needs
it - maybe I'm suffering Friday syndrome. Seems like it belongs in another
patch or in a c file rather than the header.
(RB assumes this fixed or argued against)


>  #include <linux/workqueue.h>
>  
>  #include <acpi/ghes.h>
> @@ -54,6 +55,8 @@
>  	((struct acpi_hest_generic_data *)                              \
>  	((struct ghes_vendor_record_entry *)(vendor_entry) + 1))
>  
> +extern struct gen_pool *ghes_estatus_pool;
> +
>  static inline bool is_hest_type_generic_v2(struct ghes *ghes)
>  {
>  	return ghes->generic->header.type == ACPI_HEST_TYPE_GENERIC_ERROR_V2;
> @@ -98,5 +101,8 @@ int __ghes_read_estatus(struct acpi_hest_generic_status *estatus,
>  			u64 buf_paddr, enum fixed_addresses fixmap_idx,
>  			size_t buf_len);
>  #endif
> +int ghes_estatus_cached(struct acpi_hest_generic_status *estatus);
> +void ghes_estatus_cache_add(struct acpi_hest_generic *generic,
> +			    struct acpi_hest_generic_status *estatus);
>  
>  #endif /* ACPI_APEI_GHES_CPER_H */
>