Re: [Qemu-devel] [PATCH v6 00/15] tb hash improvements

[Alex Bennée]

Emilio G. Cota <address@hidden> writes:

> v5: https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg02366.html
>
> v6 applies cleanly on top of tcg-next (8b1fe3f4 "cpu-exec:
> Clean up 'interrupt_request' reloading", tagged "pull-tcg-20160512").

Emilio,

How is v7 going? I only mention it because we are starting to discuss
feature freeze for 2.7 and I'm pretty keen to get QHT merged before
then.

Richard,

How happy are you with this series so far? Are you planning to take in
via your tree when ready?

>
> Changes from v5, mostly from Sergey's review:
>
> - processor.h: use #ifdef #elif throughout the file
>
> - tb_hash_func: use uint32 for 'flags' param
>
> - tb_hash_func5: do 'foo >> 32' instead of 'foo >> 31 >> 1', since foo
>   is a u64.
>
> - thread.h:
>   * qemu_spin_locked: remove acquire semantics; simply use atomic_read().
>   * qemu_spin_trylock: return bool instead of 0 or -EBUSY; this saves
>     a branch.
>   * qemu_spin:
>     + use __sync_lock_test_and_set and __sync_lock_release; drop
>       the patches touching atomic.h.
>     + add unlikely() hint to "while (test_and_set)"; this gives a small
>       speedup under no contention.
>
> - qht:
>   * merge the parallel-writes patch into the QHT patch.
>     [Richard: I dropped your reviewed-by since the patch changed
>      quite a bit.]
>   * drop unneeded #includes from qht.h
>   * document qht.h using kerneldoc.
>   * use unsigned int for storing the seqlock version.
>   * fix a couple of typos in the comments at the top of qht.c.
>   * explain better the "no duplicated pointer" policy: while trying to
>     insert an already-existing hash-pointer pair is OK (insert will
>     just return false), it's not OK to insert different hash-pointer
>     pairs that share the same pointer value, but not the hashes.
>   * Add comment about lookups having to be done in an RCU read-critical
>     section.
>   * remove map->stale; simply check ht->map before and after acquiring
>     a bucket lock.
>   * only use atomic_read/set on bucket pointers, not hashes. Reading
>     partially-updated hashes is OK, since we'll retry anyway thanks
>     to the seqlock. Add a comment regarding this at the top of struct
>     qht_bucket.
>   * s/b->n/b->n_buckets/
>   * define qht_debug_assert, enabled #ifdef QHT_DEBUG. Use it instead of
>     assert(), except in one case (slow path) where g_assert_cmpuint is
>     convenient.
>   * use a mutex for ht->lock instead of a spinlock. This makes the resize
>     code simpler, since holding ht->lock for a bit of time is OK now;
>     other threads won't be busy-waiting. Document that ht->lock needs
>     to be grabbed before b->lock.
>   * use atomic_rcu_read/set instead of open-coding them.
>   * qht_remove: only clear out b->hashes[] and b->pointers[] if they belong
>                 to what was the last entry in the chain.
>   * qht_remove: add debug assert against inserting a NULL pointer.
>
> Thanks,
>
>               Emilio


--
Alex Bennée