| Message ID | 1468354426-837-2-git-send-email-sergey.fedorov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 12, 2016 at 23:13:36 +0300, Sergey Fedorov wrote: > From: Sergey Fedorov <serge.fdrv@gmail.com> > It is naturally expected that some memory ordering should be provided > around qht_insert(), qht_remove(), and qht_lookup(). Document these > assumptions in the header file and put some comments in the source to > denote how that memory ordering requirements are fulfilled. I wouldn't put those comments in the source--seqlock callers should know what they're doing, and what barriers seqlocks imply. I'm OK with stating what the implied ordering is in the header file, though. Thanks, Emilio
On 13/07/2016 01:19, Emilio G. Cota wrote: > I wouldn't put those comments in the source--seqlock callers should > know what they're doing, and what barriers seqlocks imply. In general I'd agree with you, however in this case the "begin" calls are what implements QHT's guarantee *for the caller*, so I think it's worth having the comments. In other words, if for any reason you do anything before the read_begin and write_begin you still have to provide barrier semantics. It's not an explanation, it's a protection against future mistakes. There's no need for such comment at read_retry and write_end callsites, though. Also, it's spelled "guarantee". :) Paolo > I'm OK with stating what the implied ordering is in the header > file, though.
On 13/07/16 10:36, Paolo Bonzini wrote: > > On 13/07/2016 01:19, Emilio G. Cota wrote: >> I wouldn't put those comments in the source--seqlock callers should >> know what they're doing, and what barriers seqlocks imply. > In general I'd agree with you, however in this case the "begin" calls > are what implements QHT's guarantee *for the caller*, so I think it's > worth having the comments. In other words, if for any reason you do > anything before the read_begin and write_begin you still have to provide > barrier semantics. It's not an explanation, it's a protection against > future mistakes. Exactly :) > There's no need for such comment at read_retry and write_end callsites, > though. Why? > Also, it's spelled "guarantee". :) Hmm, I can't see where the spelling isn't correct. Thanks, Sergey
On 13/07/2016 19:50, Sergey Fedorov wrote: > On 13/07/16 10:36, Paolo Bonzini wrote: >> >> On 13/07/2016 01:19, Emilio G. Cota wrote: >>> I wouldn't put those comments in the source--seqlock callers should >>> know what they're doing, and what barriers seqlocks imply. >> In general I'd agree with you, however in this case the "begin" calls >> are what implements QHT's guarantee *for the caller*, so I think it's >> worth having the comments. In other words, if for any reason you do >> anything before the read_begin and write_begin you still have to provide >> barrier semantics. It's not an explanation, it's a protection against >> future mistakes. > > Exactly :) > >> There's no need for such comment at read_retry and write_end callsites, >> though. > > Why? > >> Also, it's spelled "guarantee". :) > > Hmm, I can't see where the spelling isn't correct. There are a few "gaurantee"s in the patch. If you decide to go with my own patch (http://article.gmane.org/gmane.comp.emulators.qemu/426431) for v4, please add a Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Thanks, Paolo
On 14/07/16 16:56, Paolo Bonzini wrote: > > On 13/07/2016 19:50, Sergey Fedorov wrote: >> On 13/07/16 10:36, Paolo Bonzini wrote: >>> On 13/07/2016 01:19, Emilio G. Cota wrote: >>>> I wouldn't put those comments in the source--seqlock callers should >>>> know what they're doing, and what barriers seqlocks imply. >>> In general I'd agree with you, however in this case the "begin" calls >>> are what implements QHT's guarantee *for the caller*, so I think it's >>> worth having the comments. In other words, if for any reason you do >>> anything before the read_begin and write_begin you still have to provide >>> barrier semantics. It's not an explanation, it's a protection against >>> future mistakes. >> Exactly :) >> >>> There's no need for such comment at read_retry and write_end callsites, >>> though. >> Why? >> >>> Also, it's spelled "guarantee". :) >> Hmm, I can't see where the spelling isn't correct. > There are a few "gaurantee"s in the patch. Ah, I see this now :) > > If you decide to go with my own patch > (http://article.gmane.org/gmane.comp.emulators.qemu/426431) for v4, > please add a > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Thanks, Sergey
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. + * * Returns the corresponding pointer when a match is found. * Returns NULL otherwise. */ @@ -105,6 +111,9 @@ 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, smp_wmb() is implied after the pointer is + * removed from the hash table. + * * 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 40d6e218f759..0469d068b4de 100644 --- a/util/qht.c +++ b/util/qht.c @@ -466,8 +466,10 @@ void *qht_lookup__slowpath(struct qht_bucket *b, qht_lookup_func_t func, void *ret; do { + /* seqlock_read_begin() also serves as the gaurantee of smp_rmb() */ version = seqlock_read_begin(&b->sequence); ret = qht_do_lookup(b, func, userp, hash); + /* seqlock_read_retry() also serves as the gaurantee of smp_rmb() */ } while (seqlock_read_retry(&b->sequence, version)); return ret; } @@ -483,8 +485,10 @@ void *qht_lookup(struct qht *ht, qht_lookup_func_t func, const void *userp, map = atomic_rcu_read(&ht->map); b = qht_map_to_bucket(map, hash); + /* seqlock_read_begin() also serves as the gaurantee of smp_rmb() */ version = seqlock_read_begin(&b->sequence); ret = qht_do_lookup(b, func, userp, hash); + /* seqlock_read_retry() also serves as the gaurantee of smp_rmb() */ if (likely(!seqlock_read_retry(&b->sequence, version))) { return ret; } @@ -530,6 +534,7 @@ static bool qht_insert__locked(struct qht *ht, struct qht_map *map, found: /* found an empty key: acquire the seqlock and write */ + /* seqlock_write_begin() also serves as the guarantee of smp_wmb() */ seqlock_write_begin(&head->sequence); if (new) { atomic_rcu_set(&prev->next, b); @@ -661,6 +666,9 @@ bool qht_remove__locked(struct qht_map *map, struct qht_bucket *head, qht_debug_assert(b->hashes[i] == hash); seqlock_write_begin(&head->sequence); qht_bucket_remove_entry(b, i); + /* seqlock_write_end() also serves as the guarantee of + * smp_wmb() + */ seqlock_write_end(&head->sequence); return true; }