[PATCH 0/4] tsan fixes

[PATCH 0/4] tsan fixes

[Emilio Cota]

Hi,

Here are some fixes for tsan issues that I've encountered.

The most important patch is 3/4, which allows us to run tsan for
non-trivial workloads.

Thanks,
		Emilio

[PATCH 1/4] cpu: free cpu->tb_jmp_cache with RCU

[Emilio Cota]

Fixes the appended use-after-free. The root cause is that
during tb invalidation we use CPU_FOREACH, and therefore
to safely free a vCPU we must wait for an RCU grace period
to elapse.

$ x86_64-linux-user/qemu-x86_64 tests/tcg/x86_64-linux-user/munmap-pthread
=================================================================
==1800604==ERROR: AddressSanitizer: heap-use-after-free on address 0x62d0005f7418 at pc 0x5593da6704eb bp 0x7f4961a7ac70 sp 0x7f4961a7ac60
READ of size 8 at 0x62d0005f7418 thread T2
    #0 0x5593da6704ea in tb_jmp_cache_inval_tb ../accel/tcg/tb-maint.c:244
    #1 0x5593da6704ea in do_tb_phys_invalidate ../accel/tcg/tb-maint.c:290
    #2 0x5593da670631 in tb_phys_invalidate__locked ../accel/tcg/tb-maint.c:306
    #3 0x5593da670631 in tb_invalidate_phys_page_range__locked ../accel/tcg/tb-maint.c:542
    #4 0x5593da67106d in tb_invalidate_phys_range ../accel/tcg/tb-maint.c:614
    #5 0x5593da6a64d4 in target_munmap ../linux-user/mmap.c:766
    #6 0x5593da6dba05 in do_syscall1 ../linux-user/syscall.c:10105
    #7 0x5593da6f564c in do_syscall ../linux-user/syscall.c:13329
    #8 0x5593da49e80c in cpu_loop ../linux-user/x86_64/../i386/cpu_loop.c:233
    #9 0x5593da6be28c in clone_func ../linux-user/syscall.c:6633
    #10 0x7f496231cb42 in start_thread nptl/pthread_create.c:442
    #11 0x7f49623ae9ff  (/lib/x86_64-linux-gnu/libc.so.6+0x1269ff)

0x62d0005f7418 is located 28696 bytes inside of 32768-byte region [0x62d0005f0400,0x62d0005f8400)
freed by thread T148 here:
    #0 0x7f49627b6460 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:52
    #1 0x5593da5ac057 in cpu_exec_unrealizefn ../cpu.c:180
    #2 0x5593da81f851  (/home/cota/src/qemu/build/qemu-x86_64+0x484851)

Signed-off-by: Emilio Cota <cota@braap.org>
---
 accel/tcg/cpu-exec.c  | 1 -
 cpu.c                 | 7 +++++++
 include/hw/core/cpu.h | 3 +++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 356fe348de..ca95d21528 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -1070,7 +1070,6 @@ void tcg_exec_unrealizefn(CPUState *cpu)
 #endif /* !CONFIG_USER_ONLY */
 
     tlb_destroy(cpu);
-    g_free(cpu->tb_jmp_cache);
 }
 
 #ifndef CONFIG_USER_ONLY
diff --git a/cpu.c b/cpu.c
index 4a7d865427..564200559f 100644
--- a/cpu.c
+++ b/cpu.c
@@ -164,6 +164,12 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
 #endif /* CONFIG_USER_ONLY */
 }
 
+static void cpu_free_rcu(CPUState *cpu)
+{
+    /* .tb_jmp_cache is NULL except under TCG */
+    g_free(cpu->tb_jmp_cache);
+}
+
 void cpu_exec_unrealizefn(CPUState *cpu)
 {
 #ifndef CONFIG_USER_ONLY
@@ -181,6 +187,7 @@ void cpu_exec_unrealizefn(CPUState *cpu)
     }
 
     cpu_list_remove(cpu);
+    call_rcu(cpu, cpu_free_rcu, rcu);
 }
 
 /*
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 8830546121..9fefad5656 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -266,6 +266,7 @@ struct qemu_work_item;
 
 /**
  * CPUState:
+ * @rcu: Used for safe deferred memory reclamation.
  * @cpu_index: CPU index (informative).
  * @cluster_index: Identifies which cluster this CPU is in.
  *   For boards which don't define clusters or for "loose" CPUs not assigned
@@ -321,6 +322,8 @@ struct qemu_work_item;
  * State of one CPU core or thread.
  */
 struct CPUState {
+    struct rcu_head rcu;
+
     /*< private >*/
     DeviceState parent_obj;
     /* cache to avoid expensive CPU_GET_CLASS */
-- 
2.34.1

Re: [PATCH 1/4] cpu: free cpu->tb_jmp_cache with RCU

[Richard Henderson]

On 1/8/23 08:39, Emilio Cota wrote:
> Fixes the appended use-after-free. The root cause is that
> during tb invalidation we use CPU_FOREACH, and therefore
> to safely free a vCPU we must wait for an RCU grace period
> to elapse.
> 
> $ x86_64-linux-user/qemu-x86_64 tests/tcg/x86_64-linux-user/munmap-pthread
> =================================================================
> ==1800604==ERROR: AddressSanitizer: heap-use-after-free on address 0x62d0005f7418 at pc 0x5593da6704eb bp 0x7f4961a7ac70 sp 0x7f4961a7ac60
> READ of size 8 at 0x62d0005f7418 thread T2
>      #0 0x5593da6704ea in tb_jmp_cache_inval_tb ../accel/tcg/tb-maint.c:244
>      #1 0x5593da6704ea in do_tb_phys_invalidate ../accel/tcg/tb-maint.c:290
>      #2 0x5593da670631 in tb_phys_invalidate__locked ../accel/tcg/tb-maint.c:306
>      #3 0x5593da670631 in tb_invalidate_phys_page_range__locked ../accel/tcg/tb-maint.c:542
>      #4 0x5593da67106d in tb_invalidate_phys_range ../accel/tcg/tb-maint.c:614
>      #5 0x5593da6a64d4 in target_munmap ../linux-user/mmap.c:766
>      #6 0x5593da6dba05 in do_syscall1 ../linux-user/syscall.c:10105
>      #7 0x5593da6f564c in do_syscall ../linux-user/syscall.c:13329
>      #8 0x5593da49e80c in cpu_loop ../linux-user/x86_64/../i386/cpu_loop.c:233
>      #9 0x5593da6be28c in clone_func ../linux-user/syscall.c:6633
>      #10 0x7f496231cb42 in start_thread nptl/pthread_create.c:442
>      #11 0x7f49623ae9ff  (/lib/x86_64-linux-gnu/libc.so.6+0x1269ff)
> 
> 0x62d0005f7418 is located 28696 bytes inside of 32768-byte region [0x62d0005f0400,0x62d0005f8400)
> freed by thread T148 here:
>      #0 0x7f49627b6460 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:52
>      #1 0x5593da5ac057 in cpu_exec_unrealizefn ../cpu.c:180
>      #2 0x5593da81f851  (/home/cota/src/qemu/build/qemu-x86_64+0x484851)
> 
> Signed-off-by: Emilio Cota <cota@braap.org>
> ---
>   accel/tcg/cpu-exec.c  | 1 -
>   cpu.c                 | 7 +++++++
>   include/hw/core/cpu.h | 3 +++
>   3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 356fe348de..ca95d21528 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -1070,7 +1070,6 @@ void tcg_exec_unrealizefn(CPUState *cpu)
>   #endif /* !CONFIG_USER_ONLY */
>   
>       tlb_destroy(cpu);
> -    g_free(cpu->tb_jmp_cache);

Can you simply use g_free_rcu here?

>   }
>   
>   #ifndef CONFIG_USER_ONLY
> diff --git a/cpu.c b/cpu.c
> index 4a7d865427..564200559f 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -164,6 +164,12 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>   #endif /* CONFIG_USER_ONLY */
>   }
>   
> +static void cpu_free_rcu(CPUState *cpu)
> +{
> +    /* .tb_jmp_cache is NULL except under TCG */
> +    g_free(cpu->tb_jmp_cache);
> +}
> +
>   void cpu_exec_unrealizefn(CPUState *cpu)
>   {
>   #ifndef CONFIG_USER_ONLY
> @@ -181,6 +187,7 @@ void cpu_exec_unrealizefn(CPUState *cpu)
>       }
>   
>       cpu_list_remove(cpu);
> +    call_rcu(cpu, cpu_free_rcu, rcu);

Certainly this seems wrong, exposing tb_jmp_cache beyond tcg.


r~

Re: [PATCH 1/4] cpu: free cpu->tb_jmp_cache with RCU

[Emilio Cota]

On Sun, Jan 08, 2023 at 11:19:53 -0800, Richard Henderson wrote:
> On 1/8/23 08:39, Emilio Cota wrote:
(snip)
> > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> > index 356fe348de..ca95d21528 100644
> > --- a/accel/tcg/cpu-exec.c
> > +++ b/accel/tcg/cpu-exec.c
> > @@ -1070,7 +1070,6 @@ void tcg_exec_unrealizefn(CPUState *cpu)
> >   #endif /* !CONFIG_USER_ONLY */
> >       tlb_destroy(cpu);
> > -    g_free(cpu->tb_jmp_cache);
> 
> Can you simply use g_free_rcu here?

Yes, although we must have removed the CPU from the RCU list
before doing so.

> > diff --git a/cpu.c b/cpu.c
> > index 4a7d865427..564200559f 100644
> > --- a/cpu.c
> > +++ b/cpu.c
> > @@ -164,6 +164,12 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
> >   #endif /* CONFIG_USER_ONLY */
> >   }
> > +static void cpu_free_rcu(CPUState *cpu)
> > +{
> > +    /* .tb_jmp_cache is NULL except under TCG */
> > +    g_free(cpu->tb_jmp_cache);
> > +}
> > +
> >   void cpu_exec_unrealizefn(CPUState *cpu)
> >   {
> >   #ifndef CONFIG_USER_ONLY
> > @@ -181,6 +187,7 @@ void cpu_exec_unrealizefn(CPUState *cpu)
> >       }
> >       cpu_list_remove(cpu);
> > +    call_rcu(cpu, cpu_free_rcu, rcu);
> 
> Certainly this seems wrong, exposing tb_jmp_cache beyond tcg.

I've changed this in v2 to call tcg_exec_unrealizefn after
cpu_list_remove.

An alternative would be to call the whole cpu_exec_unrealizefn
after an RCU grace period, but I think that might be more trouble
than it's worth.

Thanks,
		Emilio

[PATCH 2/4] util/qht: add missing atomic_set(hashes[i])

[Emilio Cota]

We forgot to add this one in "a890643958 util/qht: atomically set b->hashes".

Detected with tsan.

Signed-off-by: Emilio Cota <cota@braap.org>
---
 util/qht.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/qht.c b/util/qht.c
index 065fc501f4..15866299e6 100644
--- a/util/qht.c
+++ b/util/qht.c
@@ -688,7 +688,7 @@ static inline void qht_bucket_remove_entry(struct qht_bucket *orig, int pos)
     int i;
 
     if (qht_entry_is_last(orig, pos)) {
-        orig->hashes[pos] = 0;
+        qatomic_set(&orig->hashes[pos], 0);
         qatomic_set(&orig->pointers[pos], NULL);
         return;
     }
-- 
2.34.1

Re: [PATCH 2/4] util/qht: add missing atomic_set(hashes[i])

[Richard Henderson]

On 1/8/23 08:39, Emilio Cota wrote:
> We forgot to add this one in "a890643958 util/qht: atomically set b->hashes".
> 
> Detected with tsan.
> 
> Signed-off-by: Emilio Cota<cota@braap.org>
> ---
>   util/qht.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/4] util/qht: add missing atomic_set(hashes[i])

[Philippe Mathieu-Daudé]

On 8/1/23 17:39, Emilio Cota wrote:
> We forgot to add this one in "a890643958 util/qht: atomically set b->hashes".
> 
> Detected with tsan.
> 
> Signed-off-by: Emilio Cota <cota@braap.org>
> ---
>   util/qht.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 3/4] util/qht: use striped locks under TSAN

[Emilio Cota]

Fixes this tsan crash, easy to reproduce with any large enough program:

$ tests/unit/test-qht
1..2
ThreadSanitizer: CHECK failed: sanitizer_deadlock_detector.h:67 "((n_all_locks_)) < (((sizeof(all_locks_with_contexts_)/sizeof((all_locks_with_contexts_)[0]))))" (0x40, 0x40) (tid=1821568)
    #0 __tsan::CheckUnwind() ../../../../src/libsanitizer/tsan/tsan_rtl.cpp:353 (libtsan.so.2+0x90034)
    #1 __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) ../../../../src/libsanitizer/sanitizer_common/sanitizer_termination.cpp:86 (libtsan.so.2+0xca555)
    #2 __sanitizer::DeadlockDetectorTLS<__sanitizer::TwoLevelBitVector<1ul, __sanitizer::BasicBitVector<unsigned long> > >::addLock(unsigned long, unsigned long, unsigned int) ../../../../src/libsanitizer/sanitizer_common/sanitizer_deadlock_detector.h:67 (libtsan.so.2+0xb3616)
    #3 __sanitizer::DeadlockDetectorTLS<__sanitizer::TwoLevelBitVector<1ul, __sanitizer::BasicBitVector<unsigned long> > >::addLock(unsigned long, unsigned long, unsigned int) ../../../../src/libsanitizer/sanitizer_common/sanitizer_deadlock_detector.h:59 (libtsan.so.2+0xb3616)
    #4 __sanitizer::DeadlockDetector<__sanitizer::TwoLevelBitVector<1ul, __sanitizer::BasicBitVector<unsigned long> > >::onLockAfter(__sanitizer::DeadlockDetectorTLS<__sanitizer::TwoLevelBitVector<1ul, __sanitizer::BasicBitVector<unsigned long> > >*, unsigned long, unsigned int) ../../../../src/libsanitizer/sanitizer_common/sanitizer_deadlock_detector.h:216 (libtsan.so.2+0xb3616)
    #5 __sanitizer::DD::MutexAfterLock(__sanitizer::DDCallback*, __sanitizer::DDMutex*, bool, bool) ../../../../src/libsanitizer/sanitizer_common/sanitizer_deadlock_detector1.cpp:169 (libtsan.so.2+0xb3616)
    #6 __tsan::MutexPostLock(__tsan::ThreadState*, unsigned long, unsigned long, unsigned int, int) ../../../../src/libsanitizer/tsan/tsan_rtl_mutex.cpp:200 (libtsan.so.2+0xa3382)
    #7 __tsan_mutex_post_lock ../../../../src/libsanitizer/tsan/tsan_interface_ann.cpp:384 (libtsan.so.2+0x76bc3)
    #8 qemu_spin_lock /home/cota/src/qemu/include/qemu/thread.h:259 (test-qht+0x44a97)
    #9 qht_map_lock_buckets ../util/qht.c:253 (test-qht+0x44a97)
    #10 do_qht_iter ../util/qht.c:809 (test-qht+0x45f33)
    #11 qht_iter ../util/qht.c:821 (test-qht+0x45f33)
    #12 iter_check ../tests/unit/test-qht.c:121 (test-qht+0xe473)
    #13 qht_do_test ../tests/unit/test-qht.c:202 (test-qht+0xe473)
    #14 qht_test ../tests/unit/test-qht.c:240 (test-qht+0xe7c1)
    #15 test_default ../tests/unit/test-qht.c:246 (test-qht+0xe828)
    #16 <null> <null> (libglib-2.0.so.0+0x7daed)
    #17 <null> <null> (libglib-2.0.so.0+0x7d80a)
    #18 <null> <null> (libglib-2.0.so.0+0x7d80a)
    #19 g_test_run_suite <null> (libglib-2.0.so.0+0x7dfe9)
    #20 g_test_run <null> (libglib-2.0.so.0+0x7e055)
    #21 main ../tests/unit/test-qht.c:259 (test-qht+0xd2c6)
    #22 __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 (libc.so.6+0x29d8f)
    #23 __libc_start_main_impl ../csu/libc-start.c:392 (libc.so.6+0x29e3f)
    #24 _start <null> (test-qht+0xdb44)

Signed-off-by: Emilio Cota <cota@braap.org>
---
 util/qht.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 93 insertions(+), 14 deletions(-)

diff --git a/util/qht.c b/util/qht.c
index 15866299e6..6174533f10 100644
--- a/util/qht.c
+++ b/util/qht.c
@@ -151,6 +151,22 @@ struct qht_bucket {
 
 QEMU_BUILD_BUG_ON(sizeof(struct qht_bucket) > QHT_BUCKET_ALIGN);
 
+/*
+ * Under TSAN, we use striped locks instead of one lock per bucket chain.
+ * This avoids crashing under TSAN, since TSAN aborts the program if more than
+ * 64 locks are held (this is a hardcoded limit in TSAN).
+ * When resizing a QHT we grab all the buckets' locks, which can easily
+ * go over TSAN's limit. By using striped locks, we avoid this problem.
+ *
+ * Note: this number must be a power of two for easy index computation.
+ */
+#define QHT_TSAN_BUCKET_LOCKS_BITS 4
+#define QHT_TSAN_BUCKET_LOCKS (1 << QHT_TSAN_BUCKET_LOCKS_BITS)
+
+struct qht_tsan_lock {
+    QemuSpin lock;
+} QEMU_ALIGNED(QHT_BUCKET_ALIGN);
+
 /**
  * struct qht_map - structure to track an array of buckets
  * @rcu: used by RCU. Keep it as the top field in the struct to help valgrind
@@ -160,6 +176,7 @@ QEMU_BUILD_BUG_ON(sizeof(struct qht_bucket) > QHT_BUCKET_ALIGN);
  * @n_added_buckets: number of added (i.e. "non-head") buckets
  * @n_added_buckets_threshold: threshold to trigger an upward resize once the
  *                             number of added buckets surpasses it.
+ * @tsan_bucket_locks: Array of striped locks to be used only under TSAN.
  *
  * Buckets are tracked in what we call a "map", i.e. this structure.
  */
@@ -169,6 +186,9 @@ struct qht_map {
     size_t n_buckets;
     size_t n_added_buckets;
     size_t n_added_buckets_threshold;
+#ifdef CONFIG_TSAN
+    struct qht_tsan_lock tsan_bucket_locks[QHT_TSAN_BUCKET_LOCKS];
+#endif
 };
 
 /* trigger a resize when n_added_buckets > n_buckets / div */
@@ -229,10 +249,68 @@ static inline size_t qht_elems_to_buckets(size_t n_elems)
     return pow2ceil(n_elems / QHT_BUCKET_ENTRIES);
 }
 
-static inline void qht_head_init(struct qht_bucket *b)
+/*
+ * When using striped locks (i.e. under TSAN), we have to be careful not
+ * to operate on the same lock twice (e.g. when iterating through all buckets).
+ * We achieve this by operating only on each stripe's first matching lock.
+ */
+static inline void qht_do_if_first_in_stripe(const struct qht_map *map,
+                                             struct qht_bucket *b,
+                                             void (*func)(QemuSpin *spin))
+{
+#ifdef CONFIG_TSAN
+    unsigned long bucket_idx = b - map->buckets;
+    bool is_first_in_stripe = (bucket_idx >> QHT_TSAN_BUCKET_LOCKS_BITS) == 0;
+    if (is_first_in_stripe) {
+        unsigned long lock_idx = bucket_idx & (QHT_TSAN_BUCKET_LOCKS - 1);
+        func(&map->tsan_bucket_locks[lock_idx]);
+    }
+#else
+    func(&b->lock);
+#endif
+}
+
+static inline void qht_bucket_lock_init(const struct qht_map *map,
+                                        struct qht_bucket *b)
+{
+    qht_do_if_first_in_stripe(map, b, qemu_spin_init);
+}
+
+static inline void qht_bucket_lock_destroy(const struct qht_map *map,
+                                           struct qht_bucket *b)
+{
+    qht_do_if_first_in_stripe(map, b, (void (*)(QemuSpin *))qemu_spin_destroy);
+}
+
+static inline void qht_bucket_lock_do(const struct qht_map *map,
+                                      struct qht_bucket *b,
+                                      void (*func)(QemuSpin *lock))
+{
+#ifdef CONFIG_TSAN
+    unsigned long bucket_idx = b - map->buckets;
+    unsigned long lock_idx = bucket_idx & (QHT_TSAN_BUCKET_LOCKS - 1);
+    func(&map->tsan_bucket_locks[lock_idx]);
+#else
+    func(&b->lock);
+#endif
+}
+
+static inline void qht_bucket_lock(const struct qht_map *map,
+                                   struct qht_bucket *b)
+{
+    qht_bucket_lock_do(map, b, qemu_spin_lock);
+}
+
+static inline void qht_bucket_unlock(const struct qht_map *map,
+                                     struct qht_bucket *b)
+{
+    qht_bucket_lock_do(map, b, qemu_spin_unlock);
+}
+
+static inline void qht_head_init(struct qht_map *map, struct qht_bucket *b)
 {
     memset(b, 0, sizeof(*b));
-    qemu_spin_init(&b->lock);
+    qht_bucket_lock_init(map, b);
     seqlock_init(&b->sequence);
 }
 
@@ -250,7 +328,7 @@ static void qht_map_lock_buckets(struct qht_map *map)
     for (i = 0; i < map->n_buckets; i++) {
         struct qht_bucket *b = &map->buckets[i];
 
-        qemu_spin_lock(&b->lock);
+        qht_do_if_first_in_stripe(map, b, qemu_spin_lock);
     }
 }
 
@@ -261,7 +339,7 @@ static void qht_map_unlock_buckets(struct qht_map *map)
     for (i = 0; i < map->n_buckets; i++) {
         struct qht_bucket *b = &map->buckets[i];
 
-        qemu_spin_unlock(&b->lock);
+        qht_do_if_first_in_stripe(map, b, qemu_spin_unlock);
     }
 }
 
@@ -308,7 +386,7 @@ void qht_map_lock_buckets__no_stale(struct qht *ht, struct qht_map **pmap)
  * Get a head bucket and lock it, making sure its parent map is not stale.
  * @pmap is filled with a pointer to the bucket's parent map.
  *
- * Unlock with qemu_spin_unlock(&b->lock).
+ * Unlock with qht_bucket_unlock.
  *
  * Note: callers cannot have ht->lock held.
  */
@@ -322,18 +400,18 @@ struct qht_bucket *qht_bucket_lock__no_stale(struct qht *ht, uint32_t hash,
     map = qatomic_rcu_read(&ht->map);
     b = qht_map_to_bucket(map, hash);
 
-    qemu_spin_lock(&b->lock);
+    qht_bucket_lock(map, b);
     if (likely(!qht_map_is_stale__locked(ht, map))) {
         *pmap = map;
         return b;
     }
-    qemu_spin_unlock(&b->lock);
+    qht_bucket_unlock(map, b);
 
     /* we raced with a resize; acquire ht->lock to see the updated ht->map */
     qht_lock(ht);
     map = ht->map;
     b = qht_map_to_bucket(map, hash);
-    qemu_spin_lock(&b->lock);
+    qht_bucket_lock(map, b);
     qht_unlock(ht);
     *pmap = map;
     return b;
@@ -345,12 +423,13 @@ static inline bool qht_map_needs_resize(const struct qht_map *map)
            map->n_added_buckets_threshold;
 }
 
-static inline void qht_chain_destroy(const struct qht_bucket *head)
+static inline void qht_chain_destroy(const struct qht_map *map,
+                                     struct qht_bucket *head)
 {
     struct qht_bucket *curr = head->next;
     struct qht_bucket *prev;
 
-    qemu_spin_destroy(&head->lock);
+    qht_bucket_lock_destroy(map, head);
     while (curr) {
         prev = curr;
         curr = curr->next;
@@ -364,7 +443,7 @@ static void qht_map_destroy(struct qht_map *map)
     size_t i;
 
     for (i = 0; i < map->n_buckets; i++) {
-        qht_chain_destroy(&map->buckets[i]);
+        qht_chain_destroy(map, &map->buckets[i]);
     }
     qemu_vfree(map->buckets);
     g_free(map);
@@ -390,7 +469,7 @@ static struct qht_map *qht_map_create(size_t n_buckets)
     map->buckets = qemu_memalign(QHT_BUCKET_ALIGN,
                                  sizeof(*map->buckets) * n_buckets);
     for (i = 0; i < n_buckets; i++) {
-        qht_head_init(&map->buckets[i]);
+        qht_head_init(map, &map->buckets[i]);
     }
     return map;
 }
@@ -638,7 +717,7 @@ bool qht_insert(struct qht *ht, void *p, uint32_t hash, void **existing)
     b = qht_bucket_lock__no_stale(ht, hash, &map);
     prev = qht_insert__locked(ht, map, b, p, hash, &needs_resize);
     qht_bucket_debug__locked(b);
-    qemu_spin_unlock(&b->lock);
+    qht_bucket_unlock(map, b);
 
     if (unlikely(needs_resize) && ht->mode & QHT_MODE_AUTO_RESIZE) {
         qht_grow_maybe(ht);
@@ -749,7 +828,7 @@ bool qht_remove(struct qht *ht, const void *p, uint32_t hash)
     b = qht_bucket_lock__no_stale(ht, hash, &map);
     ret = qht_remove__locked(b, p, hash);
     qht_bucket_debug__locked(b);
-    qemu_spin_unlock(&b->lock);
+    qht_bucket_unlock(map, b);
     return ret;
 }
 
-- 
2.34.1

Re: [PATCH 3/4] util/qht: use striped locks under TSAN

[Richard Henderson]

On 1/8/23 08:39, Emilio Cota wrote:
> +static inline void qht_bucket_lock_init(const struct qht_map *map,
> +                                        struct qht_bucket *b)
> +{
> +    qht_do_if_first_in_stripe(map, b, qemu_spin_init);
> +}
> +
> +static inline void qht_bucket_lock_destroy(const struct qht_map *map,
> +                                           struct qht_bucket *b)
> +{
> +    qht_do_if_first_in_stripe(map, b, (void (*)(QemuSpin *))qemu_spin_destroy);
> +}

I think it's clear the declaration of qemu_spin_destroy is wrong vs const.

Why do you create two wrappers for qht_do_if_first_in_stripe, but...

> +static inline void qht_head_init(struct qht_map *map, struct qht_bucket *b)
>   {
>       memset(b, 0, sizeof(*b));
> -    qemu_spin_init(&b->lock);
> +    qht_bucket_lock_init(map, b);
>       seqlock_init(&b->sequence);
>   }
>   
> @@ -250,7 +328,7 @@ static void qht_map_lock_buckets(struct qht_map *map)
>       for (i = 0; i < map->n_buckets; i++) {
>           struct qht_bucket *b = &map->buckets[i];
>   
> -        qemu_spin_lock(&b->lock);
> +        qht_do_if_first_in_stripe(map, b, qemu_spin_lock);
>       }

... not others?

The actual logic of the stripe seems ok.


r~

Re: [PATCH 3/4] util/qht: use striped locks under TSAN

[Emilio Cota]

On Sun, Jan 08, 2023 at 11:51:44 -0800, Richard Henderson wrote:
> On 1/8/23 08:39, Emilio Cota wrote:
> > +static inline void qht_bucket_lock_init(const struct qht_map *map,
> > +                                        struct qht_bucket *b)
> > +{
> > +    qht_do_if_first_in_stripe(map, b, qemu_spin_init);
> > +}
> > +
> > +static inline void qht_bucket_lock_destroy(const struct qht_map *map,
> > +                                           struct qht_bucket *b)
> > +{
> > +    qht_do_if_first_in_stripe(map, b, (void (*)(QemuSpin *))qemu_spin_destroy);
> > +}
> 
> I think it's clear the declaration of qemu_spin_destroy is wrong vs const.

Done, added a separate patch for this.

> Why do you create two wrappers for qht_do_if_first_in_stripe, but...
> 
> > +static inline void qht_head_init(struct qht_map *map, struct qht_bucket *b)
> >   {
> >       memset(b, 0, sizeof(*b));
> > -    qemu_spin_init(&b->lock);
> > +    qht_bucket_lock_init(map, b);
> >       seqlock_init(&b->sequence);
> >   }
> > @@ -250,7 +328,7 @@ static void qht_map_lock_buckets(struct qht_map *map)
> >       for (i = 0; i < map->n_buckets; i++) {
> >           struct qht_bucket *b = &map->buckets[i];
> > -        qemu_spin_lock(&b->lock);
> > +        qht_do_if_first_in_stripe(map, b, qemu_spin_lock);
> >       }
> 
> ... not others?

Thought the remaining two cases were obvious enough.

I've now removed the wrappers.

> The actual logic of the stripe seems ok.

Thanks!

		Emilio

[PATCH 4/4] plugins: make qemu_plugin_user_exit's locking order consistent with fork_start's

[Emilio Cota]

To fix potential deadlocks as reported by tsan.

Signed-off-by: Emilio Cota <cota@braap.org>
---
 plugins/core.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/plugins/core.c b/plugins/core.c
index ccb770a485..728bacef95 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -500,10 +500,17 @@ void qemu_plugin_user_exit(void)
     enum qemu_plugin_event ev;
     CPUState *cpu;
 
-    QEMU_LOCK_GUARD(&plugin.lock);
-
+    /*
+     * Locking order: we must acquire locks in an order that is consistent
+     * with the one in fork_start(). That is:
+     * - start_exclusive(), which acquires qemu_cpu_list_lock,
+     *   must be called before acquiring plugin.lock.
+     * - tb_flush(), which acquires mmap_lock(), must be called
+     *   while plugin.lock is not held.
+     */
     start_exclusive();
 
+    qemu_rec_mutex_lock(&plugin.lock);
     /* un-register all callbacks except the final AT_EXIT one */
     for (ev = 0; ev < QEMU_PLUGIN_EV_MAX; ev++) {
         if (ev != QEMU_PLUGIN_EV_ATEXIT) {
@@ -513,13 +520,12 @@ void qemu_plugin_user_exit(void)
             }
         }
     }
-
-    tb_flush(current_cpu);
-
     CPU_FOREACH(cpu) {
         qemu_plugin_disable_mem_helpers(cpu);
     }
+    qemu_rec_mutex_unlock(&plugin.lock);
 
+    tb_flush(current_cpu);
     end_exclusive();
 
     /* now it's safe to handle the exit case */
-- 
2.34.1

Re: [PATCH 4/4] plugins: make qemu_plugin_user_exit's locking order consistent with fork_start's

[Richard Henderson]

On 1/8/23 08:39, Emilio Cota wrote:
> To fix potential deadlocks as reported by tsan.
> 
> Signed-off-by: Emilio Cota<cota@braap.org>
> ---
>   plugins/core.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~