Re: [PATCH v10 6/7] apei/ghes: Use xchg_release() for updating new cache slot instead of cmpxchg()

[Borislav Petkov]

On Tue, Oct 18, 2022 at 08:22:13AM +0000, Jia He wrote:
> From: Ard Biesheuvel <ardb@xxxxxxxxxx>
> 
> From: Ard Biesheuvel <ardb@xxxxxxxxxx>
> 
> ghes_estatus_cache_add() selects a slot, and either succeeds in
> replacing its contents with a pointer to a new cached item, or it just
> gives up and frees the new item again, without attempting to select
> another slot even if one might be available.
> 
> Since only inserting new items is needed, the race can only cause a failure
> if the selected slot was updated with another new item concurrently,
> which means that it is arbitrary which of those two items gets
> dropped. This means the cmpxchg() and the special case are not necessary,

Hmm, are you sure about this?

Looking at this complex code, I *think* the intent of the cache is to
collect already reported errors - the ghes_estatus_cached() checks - and
the adding happens when you report a new one:

        if (!ghes_estatus_cached(estatus)) {
                if (ghes_print_estatus(NULL, ghes->generic, estatus))
                        ghes_estatus_cache_add(ghes->generic, estatus);

Now, the loop in ghes_estatus_cache_add() is trying to pick out the,
well, oldest element in there. Meaning, something which got reported
already but a long while ago. There's even a sentence trying to say what
this does:

/*
 * GHES error status reporting throttle, to report more kinds of
 * errors, instead of just most frequently occurred errors.
 */

And the cmpxchg() is there to make sure when that selected element
slot_cache is removed, it really *is* that element that gets removed and
not one which replaced it in the meantime.

So it is likely I'm missing something here but it sure looks like this
is some sort of a complex, lockless, LRU scheme...

Hmmm.

-- 
Regards/Gruss,
    Boris.

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