From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48152) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bO2Mx-0004pc-54 for qemu-devel@nongnu.org; Fri, 15 Jul 2016 08:37:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bO2Ms-0005wU-4x for qemu-devel@nongnu.org; Fri, 15 Jul 2016 08:37:26 -0400 Received: from mail-lf0-x241.google.com ([2a00:1450:4010:c07::241]:36408) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bO2Mr-0005wP-4w for qemu-devel@nongnu.org; Fri, 15 Jul 2016 08:37:22 -0400 Received: by mail-lf0-x241.google.com with SMTP id 33so6990789lfw.3 for ; Fri, 15 Jul 2016 05:37:20 -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: <5788D8FE.90607@gmail.com> Date: Fri, 15 Jul 2016 15:37:18 +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: > 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. > + * I understand why an implied wmb can be expected after the entry is removed: so that the caller can trash the contents of the object removed. However that would require double-check on lookup side to make sure the entry hadn't been removed after the first lookup (something like seqlock read side). But I have no idea why we might like to promise wmb before the removal. Could you please share your point regarding this? I tempt to remove any promises on qht_remove() because it is not clear for me what would be the natural expectations on memory ordering. As of qht_insert() and qht_lookup(), I agree and this is enough guarantees for the series. Thanks, Sergey > * 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);