• Ard Biesheuvel's avatar
    apei/ghes: Use xchg_release() for updating new cache slot instead of cmpxchg() · dd3fa54b
    Ard Biesheuvel authored
    Some documentation first, about how this machinery works:
    
    It seems, the intent of the GHES error records cache is to collect
    already reported errors - see the ghes_estatus_cached() checks. 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.
       */
    
    New elements are added to the cache this way:
    
      if (!ghes_estatus_cached(estatus)) {
              if (ghes_print_estatus(NULL, ghes->generic, estatus))
                      ghes_estatus_cache_add(ghes->generic, estatus);
    
    The intent being, once this new error record is reported, it gets cached
    so that it doesn't get reported for a while due to too many, same-type
    error records getting reported in burst-like scenarios. I.e., new,
    unreported error types can have a higher chance of getting reported.
    
    Now, the loop in ghes_estatus_cache_add() is trying to pick out the
    oldest element in there. Meaning, something which got reported already
    but a long while ago, i.e., a LRU-type scheme.
    
    And the cmpxchg() is there presumably 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.
    
    Now, ghes_estatus_cache_add() selects a slot, and either succeeds in
    replacing its contents with a pointer to a newly 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 being done here, 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.
    
    And "dropped" here means, the item doesn't get added to the cache so
    the next time it is seen, it'll get reported again and an insertion
    attempt will be done again. Eventually, it'll get inserted and all those
    times when the insertion fails, the item will get reported although the
    cache is supposed to prevent that and "ratelimit" those repeated error
    records. Not a big deal in any case.
    
    This means the cmpxchg() and the special case are not necessary.
    Therefore, just drop the existing item unconditionally.
    
    Move the xchg_release() and call_rcu() out of rcu_read_lock/unlock
    section since there is no actually dereferencing the pointer at all.
    
      [ bp:
        - Flesh out and summarize what was discussed on the thread now
          that that cache contraption is understood;
        - Touch up code style. ]
    Co-developed-by: default avatarJia He <justin.he@arm.com>
    Signed-off-by: default avatarJia He <justin.he@arm.com>
    Signed-off-by: default avatarArd Biesheuvel <ardb@kernel.org>
    Signed-off-by: default avatarBorislav Petkov <bp@suse.de>
    Link: https://lore.kernel.org/r/20221010023559.69655-7-justin.he@arm.comSigned-off-by: default avatarRafael J. Wysocki <rafael.j.wysocki@intel.com>
    dd3fa54b
ghes.c 39.5 KB