Re: [Qemu-devel] [RFC 7/8] util/qht: atomically set b->hashes
On 19/09/2016 21:06, Emilio G. Cota wrote: > Let me then just point out that this comes at a small perf loss. > > Running 'taskset -c 0 tests/qht-bench -n 1 -d 10' (i.e. all lookups) 10 times, > we get: > > before the patch: > $ ./mean.pl 34.04 34.24 34.38 34.25 34.18 34.51 34.46 34.44 34.29 34.08 > 34.287 +- 0.160072900059109 > after: > $ ./mean.pl 33.94 34.00 33.52 33.46 33.55 33.71 34.27 34.06 34.28 34.58 > 33.937 +- 0.374731014640279 > > But hey we can live with that. Hmm it shouldn't. I'll take a look at the generated assembly... Paolo
Re: [Qemu-devel] [RFC 7/8] util/qht: atomically set b->hashes
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. Ah I see. Let me then just point out that this comes at a small perf loss. Running 'taskset -c 0 tests/qht-bench -n 1 -d 10' (i.e. all lookups) 10 times, we get: before the patch: $ ./mean.pl 34.04 34.24 34.38 34.25 34.18 34.51 34.46 34.44 34.29 34.08 34.287 +- 0.160072900059109 after: $ ./mean.pl 33.94 34.00 33.52 33.46 33.55 33.71 34.27 34.06 34.28 34.58 33.937 +- 0.374731014640279 But hey we can live with that. Cheers, E.
Re: [Qemu-devel] [RFC 7/8] util/qht: atomically set b->hashes
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. Paolo
Re: [Qemu-devel] [RFC 7/8] util/qht: atomically set b->hashes
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). Couldn't we just tell tsan to ignore it? Thanks, Emilio
[Qemu-devel] [RFC 7/8] util/qht: atomically set b->hashes
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.
Signed-off-by: Alex Bennée
---
util/qht.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/util/qht.c b/util/qht.c
index 16a8d79..571639d 100644
--- a/util/qht.c
+++ b/util/qht.c
@@ -379,7 +379,7 @@ static void qht_bucket_reset__locked(struct qht_bucket
*head)
if (b->pointers[i] == NULL) {
goto done;
}
-b->hashes[i] = 0;
+atomic_set(&b->hashes[i], 0);
atomic_set(&b->pointers[i], NULL);
}
b = b->next;
@@ -444,7 +444,7 @@ void *qht_do_lookup(struct qht_bucket *head,
qht_lookup_func_t func,
do {
for (i = 0; i < QHT_BUCKET_ENTRIES; i++) {
-if (b->hashes[i] == hash) {
+if (atomic_read(&b->hashes[i]) == hash) {
/* The pointer is dereferenced before seqlock_read_retry,
* so (unlike qht_insert__locked) we need to use
* atomic_rcu_read here.
@@ -538,8 +538,8 @@ static bool qht_insert__locked(struct qht *ht, struct
qht_map *map,
if (new) {
atomic_rcu_set(&prev->next, b);
}
-b->hashes[i] = hash;
/* smp_wmb() implicit in seqlock_write_begin. */
+atomic_set(&b->hashes[i], hash);
atomic_set(&b->pointers[i], p);
seqlock_write_end(&head->sequence);
return true;
@@ -607,10 +607,10 @@ qht_entry_move(struct qht_bucket *to, int i, struct
qht_bucket *from, int j)
qht_debug_assert(to->pointers[i]);
qht_debug_assert(from->pointers[j]);
-to->hashes[i] = from->hashes[j];
+atomic_set(&to->hashes[i], from->hashes[j]);
atomic_set(&to->pointers[i], from->pointers[j]);
-from->hashes[j] = 0;
+atomic_set(&from->hashes[j], 0);
atomic_set(&from->pointers[j], NULL);
}
--
2.9.3
