Hi Ard
> -----Original Message-----
> From: Ard Biesheuvel <ardb@xxxxxxxxxx>
> Sent: Monday, October 17, 2022 5:27 PM
> To: Justin He <Justin.He@xxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>; Len Brown <lenb@xxxxxxxxxx>; James
> Morse <James.Morse@xxxxxxx>; Tony Luck <tony.luck@xxxxxxxxx>; Mauro
> Carvalho Chehab <mchehab@xxxxxxxxxx>; Peter Zijlstra
> <peterz@xxxxxxxxxxxxx>; Robert Richter <rric@xxxxxxxxxx>; Robert Moore
> <robert.moore@xxxxxxxxx>; Qiuxu Zhuo <qiuxu.zhuo@xxxxxxxxx>; Yazen
> Ghannam <yazen.ghannam@xxxxxxx>; Jan Luebbe <jlu@xxxxxxxxxxxxxx>;
> Khuong Dinh <khuong@xxxxxxxxxxxxxxxxxxxxxx>; Kani Toshi
> <toshi.kani@xxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-edac@xxxxxxxxxxxxxxx; devel@xxxxxxxxxx;
> Rafael J . Wysocki <rafael@xxxxxxxxxx>; Shuai Xue
> <xueshuai@xxxxxxxxxxxxxxxxx>; Jarkko Sakkinen <jarkko@xxxxxxxxxx>;
> linux-efi@xxxxxxxxxxxxxxx; kernel test robot <lkp@xxxxxxxxx>
> Subject: Re: [PATCH v8 6/7] apei/ghes: Use unrcu_pointer for cmpxchg
>
> Hi Justin,
>
> On Mon, 17 Oct 2022 at 10:47, Justin He <Justin.He@xxxxxxx> wrote:
> >
> > Hi Ard
> >
> > > -----Original Message-----
> > > Subject: Re: [PATCH v8 6/7] apei/ghes: Use unrcu_pointer for cmpxchg
> > >
> > > On Fri, 14 Oct 2022 at 17:11, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> > > >
> > > > On Fri, Oct 14, 2022 at 04:31:37PM +0200, Ard Biesheuvel wrote:
> > > > > + if (slot != -1) {
> > > > > + /*
> > > > > + * 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(&rcu_dereference(victim)->rcu,
> > > > > + ghes_estatus_cache_rcu_free);
> > > > }
> > > >
> > > > I think you can use unrcu_pointer() here instead, there should not
> > > > be a data dependency since the ->rcu member itself should be
> > > > otherwise unused (and if it were, we wouldn't care about its previous
> content anyway).
> > > >
> > > > But only Alpha cares about that distinction anyway, so *shrug*.
> > > >
> > >
> > > Ah yeah good point - and we are not actually dereferencing the
> > > pointer at all here, just adding an offset to get at the address of the rcu
> member.
> > >
> > > So we can take this block out of the rcu_read_lock() section as well.
> > >
> > >
> > > > While I much like the xchg() variant; I still don't really fancy
> > > > the verbage the sparse nonsense makes us do.
> > > >
> > > > victim = xchg_release(&ghes_estatus_caches[slot],
> > > new_cache);
> > > > if (victim)
> > > > call_rcu(&victim->rcu,
> > > > ghes_estatus_cache_rcu_free);
> > > >
> > > > is much nicer code.
> > > >
> > > > Over all; I'd simply ignore sparse (I often do).
> > > >
> > >
> > > No disagreement there.
> >
> > What do you think of the updated patch:
> >
> > apei/ghes: Use xchg() for updating new cache slot instead of
> > cmpxchg()
> >
> > 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, and hence just drop the existing item unconditionally.
> > Note that this does not result in loss of error events, it simply
> > means we might cause a false cache miss, and report the same event one
> > additional time in quick succession even if the cache should have prevented
> that.
> >
>
> Please add a line here
>
> Co-developed-by: Jia He <justin.he@xxxxxxx>
>
> > Signed-off-by: Jia He <justin.he@xxxxxxx>
> > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > ---
> > [Justin]: I removed __rcu annotation of victim, removed the
> > RCU_INITIALIZER cast and added the unptr for xchg.
> >
> > drivers/acpi/apei/ghes.c | 44 ++++++++++++++++++++--------------------
> > 1 file changed, 22 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index
> > 27c72b175e4b..5fc8a135450b 100644
> > --- a/drivers/acpi/apei/ghes.c
> > +++ b/drivers/acpi/apei/ghes.c
> > @@ -150,7 +150,7 @@ struct ghes_vendor_record_entry { static struct
> > gen_pool *ghes_estatus_pool; static unsigned long
> > ghes_estatus_pool_size_request;
> >
> > -static struct ghes_estatus_cache
> > *ghes_estatus_caches[GHES_ESTATUS_CACHES_SIZE];
> > +static struct ghes_estatus_cache __rcu
> > +*ghes_estatus_caches[GHES_ESTATUS_CACHES_SIZE];
> > static atomic_t ghes_estatus_cache_alloced;
> >
> > static int ghes_panic_timeout __read_mostly = 30; @@ -785,31 +785,26
> > @@ static struct ghes_estatus_cache *ghes_estatus_cache_alloc(
> > return cache;
> > }
> >
> > -static void ghes_estatus_cache_free(struct ghes_estatus_cache *cache)
> > +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);
> > }
> >
> > -static void ghes_estatus_cache_rcu_free(struct rcu_head *head) -{
> > - struct ghes_estatus_cache *cache;
> > -
> > - cache = container_of(head, struct ghes_estatus_cache, rcu);
> > - ghes_estatus_cache_free(cache);
> > -}
> > -
> > static void ghes_estatus_cache_add(
> > struct acpi_hest_generic *generic,
> > struct acpi_hest_generic_status *estatus) {
> > int i, slot = -1, count;
> > unsigned long long now, duration, period, max_period = 0;
> > - struct ghes_estatus_cache *cache, *slot_cache = NULL,
> *new_cache;
> > + struct ghes_estatus_cache *cache, *new_cache;
> > + struct ghes_estatus_cache *victim;
> >
> > new_cache = ghes_estatus_cache_alloc(generic, estatus);
> > if (new_cache == NULL)
> > @@ -820,13 +815,11 @@ static void ghes_estatus_cache_add(
> > cache = rcu_dereference(ghes_estatus_caches[i]);
> > if (cache == NULL) {
> > slot = i;
> > - slot_cache = NULL;
> > break;
> > }
> > duration = now - cache->time_in;
> > if (duration >= GHES_ESTATUS_IN_CACHE_MAX_NSEC) {
> > slot = i;
> > - slot_cache = cache;
> > break;
> > }
> > count = atomic_read(&cache->count); @@ -835,17
> +828,24
> > @@ static void ghes_estatus_cache_add(
> > if (period > max_period) {
> > max_period = period;
> > slot = i;
> > - slot_cache = cache;
> > }
> > }
> > - /* new_cache must be put into array after its contents are written
> */
> > - smp_wmb();
> > - if (slot != -1 && cmpxchg(ghes_estatus_caches + slot,
> > - slot_cache, new_cache) == slot_cache)
> {
> > - if (slot_cache)
> > - call_rcu(&slot_cache->rcu,
> ghes_estatus_cache_rcu_free);
> > - } else
> > - ghes_estatus_cache_free(new_cache);
> > + if (slot != -1) {
> > + /*
> > + * 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.
>
> Please move the comment back where it was. 'At this point' is now ambiguous
> because victim has not been assigned yet.
>
> > + * 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.
> > + */
> > + victim =
> unrcu_pointer(xchg_release(&ghes_estatus_caches[slot],
> > + new_cache));
>
> Doesn't this still trigger the sparse warning on x86?
Yes,
I will add the RCU_INITIALIZER back if Peter doesn't object.
--
Cheers,
Justin (Jia He)
