Skip to content

Commit

Permalink
util/qht: atomically set b->hashes
Browse files Browse the repository at this point in the history
ThreadSanitizer detects a possible race between reading/writing the
hashes. The ordering semantics are already documented for QHT however
for true C11 compliance we should use relaxed atomic primitives for
accesses that are done across threads. On x86 this slightly changes to
the code to not do a load/compare in a single instruction leading to a
slight performance degradation.

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

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Emilio G. Cota <cota@braap.org>
Message-Id: <20160930213106.20186-10-alex.bennee@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
  • Loading branch information
stsquad authored and bonzini committed Oct 4, 2016
1 parent 027d9a7 commit a890643
Showing 1 changed file with 5 additions and 5 deletions.
10 changes: 5 additions & 5 deletions util/qht.c
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand Down

0 comments on commit a890643

Please sign in to comment.