From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51539) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNOXd-0005RP-9g for qemu-devel@nongnu.org; Wed, 13 Jul 2016 14:06:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bNOV6-0001xF-Ut for qemu-devel@nongnu.org; Wed, 13 Jul 2016 14:05:49 -0400 Received: from mail-lf0-x241.google.com ([2a00:1450:4010:c07::241]:33636) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNOV6-0001vy-FP for qemu-devel@nongnu.org; Wed, 13 Jul 2016 14:03:12 -0400 Received: by mail-lf0-x241.google.com with SMTP id f93so4198109lfi.0 for ; Wed, 13 Jul 2016 11:03:12 -0700 (PDT) References: <1468354426-837-1-git-send-email-sergey.fedorov@linaro.org> <1468354426-837-2-git-send-email-sergey.fedorov@linaro.org> <1bfb56fa-55dd-455a-e4ec-c34ec7996baa@redhat.com> From: Sergey Fedorov Message-ID: <5786825D.5020009@gmail.com> Date: Wed, 13 Jul 2016 21:03:09 +0300 MIME-Version: 1.0 In-Reply-To: <1bfb56fa-55dd-455a-e4ec-c34ec7996baa@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 01/11] util/qht: Document memory ordering assumptions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , Sergey Fedorov , qemu-devel@nongnu.org, mttcg@listserver.greensocs.com, fred.konrad@greensocs.com, a.rigo@virtualopensystems.com, cota@braap.org, bobby.prani@gmail.com, rth@twiddle.net Cc: peter.maydell@linaro.org, patches@linaro.org, claudio.fontana@huawei.com, mark.burton@greensocs.com, jan.kiszka@siemens.com, =?UTF-8?Q?Alex_Benn=c3=a9e?= On 13/07/16 14:13, Paolo Bonzini wrote: > > On 12/07/2016 22:13, Sergey Fedorov wrote: >> diff --git a/include/qemu/qht.h b/include/qemu/qht.h >> index 70bfc68b8d67..5f633e5d8100 100644 >> --- a/include/qemu/qht.h >> +++ b/include/qemu/qht.h >> @@ -69,6 +69,9 @@ void qht_destroy(struct qht *ht); >> * Attempting to insert a NULL @p is a bug. >> * Inserting the same pointer @p with different @hash values is a bug. >> * >> + * In case of successful operation, smp_wmb() is implied before the pointer is >> + * inserted into the hash table. >> + * >> * Returns true on sucess. >> * Returns false if the @p-@hash pair already exists in the hash table. >> */ >> @@ -86,6 +89,9 @@ bool qht_insert(struct qht *ht, void *p, uint32_t hash); >> * The user-provided @func compares pointers in QHT against @userp. >> * If the function returns true, a match has been found. >> * >> + * smp_rmb() is implied before and after the pointer is looked up and retrieved >> + * from the hash table. > Do we really need to guarantee smp_rmb(), or is smp_read_barrier_depends() > aka atomic_rcu_read() enough? The idea was something like: qht_lookup() can be "paired" with either qht_insert() or qht_remove(). The intention was to guarantee independent tb_jmp_cache lookup performed before qht_lookup() in tb_find_physical(). > > Likewise, perhaps only an implicit smp_wmb() before the insert is > "interesting" to qht_insert__locked callers . I see the idea behind this patch is worthwhile so I spend more time on refining it and I must look at your patch carefully ;) Thanks, Sergey > > Something like: > > diff --git a/include/qemu/qht.h b/include/qemu/qht.h > index 70bfc68..f4f1d55 100644 > --- a/include/qemu/qht.h > +++ b/include/qemu/qht.h > @@ -69,6 +69,9 @@ void qht_destroy(struct qht *ht); > * Attempting to insert a NULL @p is a bug. > * Inserting the same pointer @p with different @hash values is a bug. > * > + * In case of successful operation, smp_wmb() is implied before the pointer is > + * inserted into the hash table. > + * > * Returns true on sucess. > * Returns false if the @p-@hash pair already exists in the hash table. > */ > @@ -83,6 +86,8 @@ bool qht_insert(struct qht *ht, void *p, uint32_t hash); > * > * Needs to be called under an RCU read-critical section. > * > + * smp_read_barrier_depends() is implied before the call to @func. > + * > * The user-provided @func compares pointers in QHT against @userp. > * If the function returns true, a match has been found. > * > @@ -105,6 +110,10 @@ void *qht_lookup(struct qht *ht, qht_lookup_func_t func, const void *userp, > * This guarantees that concurrent lookups will always compare against valid > * data. > * > + * In case of successful operation, a smp_wmb() barrier is implied before and > + * after the pointer is removed from the hash table. In other words, > + * a successful qht_remove acts as a bidirectional write barrier. > + * > * Returns true on success. > * Returns false if the @p-@hash pair was not found. > */ > diff --git a/util/qht.c b/util/qht.c > index 40d6e21..d38948e 100644 > --- a/util/qht.c > +++ b/util/qht.c > @@ -445,7 +445,11 @@ 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) { > - void *p = atomic_read(&b->pointers[i]); > + /* The pointer is dereferenced before seqlock_read_retry, > + * so (unlike qht_insert__locked) we need to use > + * atomic_rcu_read here. > + */ > + void *p = atomic_rcu_read(&b->pointers[i]); > > if (likely(p) && likely(func(p, userp))) { > return p; > @@ -535,6 +539,7 @@ static bool qht_insert__locked(struct qht *ht, struct qht_map *map, > atomic_rcu_set(&prev->next, b); > } > b->hashes[i] = hash; > + /* smp_wmb() implicit in seqlock_write_begin. */ > atomic_set(&b->pointers[i], p); > seqlock_write_end(&head->sequence); > return true; > @@ -659,6 +664,9 @@ bool qht_remove__locked(struct qht_map *map, struct qht_bucket *head, > } > if (q == p) { > qht_debug_assert(b->hashes[i] == hash); > + /* seqlock_write_begin and seqlock_write_end provide write > + * memory barrier semantics to callers of qht_remove. > + */ > seqlock_write_begin(&head->sequence); > qht_bucket_remove_entry(b, i); > seqlock_write_end(&head->sequence);