Re: [PATCH 3/4] util/qht: use striped locks under TSAN

From: Emilio Cota <cota@braap.org>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: qemu-devel@nongnu.org, "Alex Bennée" <alex.bennee@linaro.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>
Subject: Re: [PATCH 3/4] util/qht: use striped locks under TSAN
Date: Mon, 9 Jan 2023 17:10:08 -0500	[thread overview]
Message-ID: <Y7yQwHMNArx2tlvV@cota-l14> (raw)
In-Reply-To: <4f9cf406-1a46-8f7c-6a52-fb13d8c27317@linaro.org>

On Sun, Jan 08, 2023 at 11:51:44 -0800, Richard Henderson wrote:
> On 1/8/23 08:39, Emilio Cota wrote:
> > +static inline void qht_bucket_lock_init(const struct qht_map *map,
> > +                                        struct qht_bucket *b)
> > +{
> > +    qht_do_if_first_in_stripe(map, b, qemu_spin_init);
> > +}
> > +
> > +static inline void qht_bucket_lock_destroy(const struct qht_map *map,
> > +                                           struct qht_bucket *b)
> > +{
> > +    qht_do_if_first_in_stripe(map, b, (void (*)(QemuSpin *))qemu_spin_destroy);
> > +}
> 
> I think it's clear the declaration of qemu_spin_destroy is wrong vs const.

Done, added a separate patch for this.

> Why do you create two wrappers for qht_do_if_first_in_stripe, but...
> 
> > +static inline void qht_head_init(struct qht_map *map, struct qht_bucket *b)
> >   {
> >       memset(b, 0, sizeof(*b));
> > -    qemu_spin_init(&b->lock);
> > +    qht_bucket_lock_init(map, b);
> >       seqlock_init(&b->sequence);
> >   }
> > @@ -250,7 +328,7 @@ static void qht_map_lock_buckets(struct qht_map *map)
> >       for (i = 0; i < map->n_buckets; i++) {
> >           struct qht_bucket *b = &map->buckets[i];
> > -        qemu_spin_lock(&b->lock);
> > +        qht_do_if_first_in_stripe(map, b, qemu_spin_lock);
> >       }
> 
> ... not others?

Thought the remaining two cases were obvious enough.

I've now removed the wrappers.

> The actual logic of the stripe seems ok.

Thanks!

		Emilio