Re: [Qemu-devel] [PATCH v2 7/9] util/qht: atomically set b->hashes

["Emilio G. Cota" <cota@braap.org>]

On Thu, Sep 22, 2016 at 11:13:14 +0100, Alex Bennée wrote:
> ThreadSanitizer detects a possible race between reading/writing the
> hashes. As ordering semantics are already documented for qht we just
> need to ensure a race can't tear the hash value so we can use the
> relaxed atomic_set/read functions.

Just being pedantic, but I think the commit log could be improved.
I think it would be more correct to say we're avoiding being out
of C11's spec by using atomic_read/set, instead of tolerating concurrent
regular loads/stores.

Tearing is not really the issue, in the sense that the seqlock protects
against that. IOW, we're not worried about tearing, we're worried about
being out of spec, as Paolo pointed out:

On Mon, Sep 19, 2016 at 20:37:06 +0200, Paolo Bonzini wrote:
> On 19/09/2016 20:06, Emilio G. Cota wrote:
> > On Mon, Sep 19, 2016 at 16:51:38 +0100, Alex Bennée wrote:
> >> > ThreadSanitizer detects a possible race between reading/writing the
> >> > hashes. As ordering semantics are already documented for qht we just
> >> > need to ensure a race can't tear the hash value so we can use the
> >> > relaxed atomic_set/read functions.
> > This was discussed here:
> >
> > https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg03658.html
> >
> > To reiterate: reading torn hash values is fine, since the retry will
> > happen regardless (and all pointers[] remain valid through the RCU
> > read-critical section).
>
> True, but C11 says data races are undefined, not merely unspecified.
> seqlock-protected data requires a relaxed read and write, because they
> are read concurrently in the read and write sides.

Acknowledging in the commit log the tiny-yet-measurable perf hit would be
good, too (I'd just copy the before/after results I posted).

That said,

  Reviewed-by: Emilio G. Cota <cota@braap.org>

Thanks,

		E.