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

[Alex Bennée]

Emilio G. Cota <address@hidden> writes:

> 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.

You are quite right. Having been in the guts of the ThreadSanitizer with
the toolchain guys I'm starting to see this is the only real way to mark
things (it doesn't actually implement stand-along barriers).

>
> 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).

Will do.

>
> That said,
>
>   Reviewed-by: Emilio G. Cota <address@hidden>
>
> Thanks,

Thnaks,


--
Alex Bennée