Re: [Qemu-devel] [PATCH] qht: support resetting an uninitialized qht

[Igor Mammedov]

On Wed, 10 Aug 2016 15:25:49 -0400
"Emilio G. Cota" <address@hidden> wrote:

Emilio,

Pls also see
 https://www.mail-archive.com/address@hidden/msg391669.html
which fixed issue for me, I've CCed you.


> tb_flush() is called when debugging the guest (under both KVM
> and TCG accelerators) with gdb. tb_flush() resets TCG's qht, which
> segfaults if we're using KVM due to the qht not being initialized.
> 
> Fix this adding a magic number field to struct qht to track whether a qht
> has been initialized with qht_init(). Then, explicitly allow
> passing uninitialized qht's to qht_reset() and qht_reset_size(),
> just like we do with qht_statistics_init().
> 
> Reported-by: Brent Baccala <address@hidden>
> Reported-by: Igor Mammedov <address@hidden>
> Signed-off-by: Emilio G. Cota <address@hidden>
> ---
>  include/qemu/qht.h |  7 +++++++
>  tests/test-qht.c   |  3 +++
>  util/qht.c         | 20 +++++++++++++++++---
>  3 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/include/qemu/qht.h b/include/qemu/qht.h
> index 311139b..39dd5e8 100644
> --- a/include/qemu/qht.h
> +++ b/include/qemu/qht.h
> @@ -15,6 +15,7 @@ struct qht {
>      struct qht_map *map;
>      QemuMutex lock; /* serializes setters of ht->map */
>      unsigned int mode;
> +    unsigned int magic;
>  };
>  
>  /**
> @@ -124,6 +125,8 @@ bool qht_remove(struct qht *ht, const void *p, uint32_t 
> hash);
>   * If concurrent readers may exist, the objects pointed to by the hash table
>   * must remain valid for the existing RCU grace period -- see qht_remove().
>   * See also: qht_reset_size()
> + *
> + * Note: it is OK to pass an uninitialized @ht.
>   */
>  void qht_reset(struct qht *ht);
>  
> @@ -138,6 +141,8 @@ void qht_reset(struct qht *ht);
>   * If concurrent readers may exist, the objects pointed to by the hash table
>   * must remain valid for the existing RCU grace period -- see qht_remove().
>   * See also: qht_reset(), qht_resize().
> + *
> + * Note: it is OK to pass an uninitialized @ht.
>   */
>  bool qht_reset_size(struct qht *ht, size_t n_elems);
>  
> @@ -173,6 +178,8 @@ void qht_iter(struct qht *ht, qht_iter_func_t func, void 
> *userp);
>   *
>   * When done with @stats, pass the struct to qht_statistics_destroy().
>   * Failing to do this will leak memory.
> + *
> + * Note: it is OK to pass an uninitialized @ht.
>   */
>  void qht_statistics_init(struct qht *ht, struct qht_stats *stats);
>  
> diff --git a/tests/test-qht.c b/tests/test-qht.c
> index 46a64b6..a923b2e 100644
> --- a/tests/test-qht.c
> +++ b/tests/test-qht.c
> @@ -97,6 +97,9 @@ static void qht_do_test(unsigned int mode, size_t 
> init_entries)
>  {
>      /* under KVM we might fetch stats from an uninitialized qht */
>      check_n(0);
> +    /* resetting an uninitialized qht can happen as well, e.g. KVM + gdb */
> +    qht_reset(&ht);
> +    qht_reset_size(&ht, 0);
>  
>      qht_init(&ht, 0, mode);
>  
> diff --git a/util/qht.c b/util/qht.c
> index 16a8d79..e4c90d6 100644
> --- a/util/qht.c
> +++ b/util/qht.c
> @@ -89,6 +89,8 @@
>  #define QHT_BUCKET_ENTRIES 4
>  #endif
>  
> +#define QHT_MAGIC 0xbebec4fe
> +
>  /*
>   * Note: reading partially-updated pointers in @pointers could lead to
>   * segfaults. We thus access them with atomic_read/set; this guarantees
> @@ -182,6 +184,11 @@ static inline void qht_map_debug__all_locked(struct 
> qht_map *map)
>  { }
>  #endif /* QHT_DEBUG */
>  
> +static inline bool qht_inited(const struct qht *ht)
> +{
> +    return ht->magic == QHT_MAGIC;
> +}
> +
>  static inline size_t qht_elems_to_buckets(size_t n_elems)
>  {
>      return pow2ceil(n_elems / QHT_BUCKET_ENTRIES);
> @@ -356,6 +363,7 @@ void qht_init(struct qht *ht, size_t n_elems, unsigned 
> int mode)
>      size_t n_buckets = qht_elems_to_buckets(n_elems);
>  
>      ht->mode = mode;
> +    ht->magic = QHT_MAGIC;
>      qemu_mutex_init(&ht->lock);
>      map = qht_map_create(n_buckets);
>      atomic_rcu_set(&ht->map, map);
> @@ -403,6 +411,10 @@ void qht_reset(struct qht *ht)
>  {
>      struct qht_map *map;
>  
> +    if (unlikely(!qht_inited(ht))) {
> +        return;
> +    }
> +
>      qht_map_lock_buckets__no_stale(ht, &map);
>      qht_map_reset__all_locked(map);
>      qht_map_unlock_buckets(map);
> @@ -415,6 +427,9 @@ bool qht_reset_size(struct qht *ht, size_t n_elems)
>      size_t n_buckets;
>      bool resize = false;
>  
> +    if (unlikely(!qht_inited(ht))) {
> +        return false;
> +    }
>      n_buckets = qht_elems_to_buckets(n_elems);
>  
>      qemu_mutex_lock(&ht->lock);
> @@ -787,17 +802,16 @@ void qht_statistics_init(struct qht *ht, struct 
> qht_stats *stats)
>      struct qht_map *map;
>      int i;
>  
> -    map = atomic_rcu_read(&ht->map);
> -
>      stats->used_head_buckets = 0;
>      stats->entries = 0;
>      qdist_init(&stats->chain);
>      qdist_init(&stats->occupancy);
>      /* bail out if the qht has not yet been initialized */
> -    if (unlikely(map == NULL)) {
> +    if (unlikely(!qht_inited(ht))) {
>          stats->head_buckets = 0;
>          return;
>      }
> +    map = atomic_rcu_read(&ht->map);
>      stats->head_buckets = map->n_buckets;
>  
>      for (i = 0; i < map->n_buckets; i++) {